Re: panic upon starting X in recent -CURRENTs (intel driver)

On Mon, Feb 18, 2008 at 11:35:31PM +0100, Michiel Boland wrote:
> >Hi. After recent upgrade (from 21 dec to today's src) the kernel crashes 
> >when starting X with
> >
> >panic: pmap_remove_all: page 0xc56e07f8 is fictitious
> 
> FWIW below is a trivial program to re-create a similar crash. Needs root, 
> obviously. But still shouldn't cause a panic though. Note that the trick 
> in the program is that we mmap two pages, then munmap only half of them.
> 
> #include <sys/types.h>
> #include <sys/mman.h>
> #include <fcntl.h>
> #include <stdio.h>
> #include <unistd.h>
> 
> static const off_t map_address = 0xa0000;
> static const size_t map_size = 0x1000;
> 
> static int testit(int fd)
> {
> 	void *p;
> 	int rv;
> 
> 	p = mmap(NULL, 2 * map_size, PROT_READ | PROT_WRITE, MAP_SHARED, fd,
> 		  map_address);
> 	if (p == MAP_FAILED) {
> 		perror("mmap");
> 		return -1;
> 	}
> 	rv = *(char *) p;
> 	if (munmap(p, map_size) == -1) {
> 		perror("munmap");
> 		return -1;
> 	}
> 	return rv;
> }
> 
> int main(void)
> {
> 	int fd, rv;
> 
> 	fd = open("/dev/mem", O_RDWR);
> 	if (fd == -1) {
> 		perror("open");
> 		return 1;
> 	}
> 	rv = testit(fd);
> 	close(fd);
> 	return rv;
> }

What happen there is that munmap() do the split for the /dev/mem mapping.
This caused the OBJT_DEVICE ref_count to be bumped, and vm_map_entry_delete()
called vm_object_page_remove(). The later called pmap_remove_all()
unconditionally.

pmap_remove_all has the KASSERT that fails exactly when supplied
fictitious page. It becomes KASSERT in the rev. 1.106 of i386/pmap.c,
committed 2008/01/08, it was under the PMAP_DIAGNOSTIC before.

Since such page has md.pv_list empty anyway, this KASSERT seems to be
only the statement of intent. The change below would prevent the panic
by not calling pmap_remove_all from vm_object_page_remove for such pages.

Alan, do you have objections ? [Alternative seems to be a removal of the
assertions from all pmap implementations, that also weaken the invariants
for other callers that do skip fictitious pages].

diff --git a/sys/vm/vm_object.c b/sys/vm/vm_object.c
index 21c0ac6..21ee10d 100644
--- a/sys/vm/vm_object.c
+++ b/sys/vm/vm_object.c
_at__at_ -1884,7 +1884,8 _at__at_ again:
 		 */
 		if ((wirings = p->wire_count) != 0 &&
 		    (wirings = pmap_page_wired_mappings(p)) != p->wire_count) {
-			pmap_remove_all(p);
+			if ((p->flags & PG_FICTITIOUS) == 0)
+				pmap_remove_all(p);
 			/* Account for removal of managed, wired mappings. */
 			p->wire_count -= wirings;
 			if (!clean_only)
_at__at_ -1898,7 +1899,8 _at__at_ again:
 			if (p->valid & p->dirty)
 				continue;
 		}
-		pmap_remove_all(p);
+		if ((p->flags & PG_FICTITIOUS) == 0)
+			pmap_remove_all(p);
 		/* Account for removal of managed, wired mappings. */
 		if (wirings != 0)
 			p->wire_count -= wirings;