kern/71109: Possible race conditions in pmap.c

[Uwe Doering]

>Number:         71109
>Category:       kern
>Synopsis:       Possible race conditions in pmap.c
>Confidential:   no
>Severity:       serious
>Priority:       high
>Responsible:    freebsd-bugs
>State:          open
>Quarter:        
>Keywords:       
>Date-Required:
>Class:          sw-bug
>Submitter-Id:   current-users
>Arrival-Date:   Sun Aug 29 17:10:28 GMT 2004
>Closed-Date:
>Last-Modified:
>Originator:     Uwe Doering
>Release:        FreeBSD 4.5-RELEASE i386
>Organization:
EscapeBox - Managed On-Demand UNIX Servers
http://www.escapebox.net

>Environment:
System: FreeBSD geminix.org 4.5-RELEASE FreeBSD 4.5-RELEASE #3: Sun Aug 29 09:13:05 GMT 2004 root at localhost:/RELENG_4_Enhanced i386


>Description:
pmap_allocpte() and pmap_enter_quick() in 'src/sys/i386/i386/pmap.c'
both call pmap_page_lookup() and _pmap_allocpte().  The latter two
functions may return NULL, which can be an indicator that the
respective function has been blocking.  The caller is then supposed
to start over again since the current operation at this level can
no longer be considered atomic.

Now, in pmap_allocpte() the return value of pmap_page_lookup()
isn't checked for NULL, and in pmap_enter_quick() the same is
true for _pmap_allocpte().  These flaws cause race conditions
that can result in a kernel panic.

>How-To-Repeat:
The problem becomes apparent when looking at the relevant source
code.

>Fix:
Please consider adopting the patch below.  It applies cleanly to
RELENG_4 as of today, and, as far as I can tell, also applies to
the Alpha architecture (possibly with minor tweaks).

Apart from adding the missing NULL checking we also optimize the
other, already existing NULL check in pmap_enter_quick() in that
we do it only after pmap_page_lookup() has been called, because
'mpte' cannot be NULL in case the first part of the if-else clause
gets executed.

This patch also removes a superfluous call of VM_WAIT in
_pmap_allocpte().  The preceding function vm_page_grab() already
does the appropriate sleeping before returning NULL, even if called
without the VM_ALLOC_RETRY flag.  So we can return from
_pmap_allocpte() right away.


--- pmap.c.diff begins here ---
--- src/sys/i386/i386/pmap.c.orig	Thu May  6 20:56:50 2004
+++ src/sys/i386/i386/pmap.c	Mon May 31 13:03:52 2004
@@ -1228,7 +1228,6 @@
 	m = vm_page_grab(pmap->pm_pteobj, ptepindex,
 			VM_ALLOC_ZERO);
 	if (m == NULL) {
-		VM_WAIT;
 		/*
 		 * Indicate the need to retry.  While waiting, the page table
 		 * page may have been allocated.
@@ -1316,6 +1315,8 @@
 		} else {
 			m = pmap_page_lookup(pmap->pm_pteobj, ptepindex);
 			pmap->pm_ptphint = m;
+			if (m == NULL)
+				goto retry;
 		}
 		m->hold_count++;
 	} else {
@@ -2105,12 +2106,14 @@
 				} else {
 					mpte = pmap_page_lookup(pmap->pm_pteobj, ptepindex);
 					pmap->pm_ptphint = mpte;
+					if (mpte == NULL)
+						goto retry;
 				}
-				if (mpte == NULL)
-					goto retry;
 				mpte->hold_count++;
 			} else {
 				mpte = _pmap_allocpte(pmap, ptepindex);
+				if (mpte == NULL)
+					goto retry;
 			}
 		}
 	} else {
--- pmap.c.diff ends here ---
>Release-Note:
>Audit-Trail:
>Unformatted: