kern/71792: Wrong/missing 'goto' target label in contigmalloc1()

[Uwe Doering]

>Number:         71792
>Category:       kern
>Synopsis:       Wrong/missing 'goto' target label in contigmalloc1()
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    freebsd-bugs
>State:          open
>Quarter:        
>Keywords:       
>Date-Required:
>Class:          sw-bug
>Submitter-Id:   current-users
>Arrival-Date:   Thu Sep 16 14:00:48 GMT 2004
>Closed-Date:
>Last-Modified:
>Originator:     Uwe Doering
>Release:        FreeBSD 4.10-STABLE i386
>Organization:
EscapeBox - Managed On-Demand UNIX Servers
http://www.escapebox.net

>Environment:
System: FreeBSD geminix.org 4.10-STABLE FreeBSD 4.10-STABLE #1: Tue Sep 14 20:12:02 GMT 2004 root at localhost:/RELENG_4_Enhanced i386

>Description:
There are two loops in contigmalloc1() (src/sys/vm/vm_page.c) that
try to make room in the memory arena if there was no page available
on first try that would fit the applying constraints.  The first
loop scans PQ_INACTIVE, the second one PQ_ACTIVE.

Now, the second loop apparently has come into existence by means of
a cut-and-paste operation.  Unfortunately, the author of the code
segment forgot to give that loop its own 'goto' target label, to be
used in start-over situations.  Instead, the 'goto' statements jump
back into the PQ_INACTIVE loop, possibly over and over again.

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

>Fix:
Give the PQ_ACTIVE loop its own start-over target label.  Please
consider adopting the patch below.

--- vm_page.c.diff begins here ---
--- src/sys/vm/vm_page.c.orig	Wed Jan 28 22:24:01 2004
+++ src/sys/vm/vm_page.c	Sat May  8 20:48:22 2004
@@ -1892,6 +1894,7 @@
 					vm_page_cache(m);
 			}
 
+again2:
 			for (m = TAILQ_FIRST(&vm_page_queues[PQ_ACTIVE].pl);
 				m != NULL;
 				m = next) {
@@ -1901,18 +1904,18 @@
 
 				next = TAILQ_NEXT(m, pageq);
 				if (vm_page_sleep_busy(m, TRUE, "vpctw1"))
-					goto again1;
+					goto again2;
 				vm_page_test_dirty(m);
 				if (m->dirty) {
 					if (m->object->type == OBJT_VNODE) {
 						vn_lock(m->object->handle, LK_EXCLUSIVE | LK_RETRY, curproc);
 						vm_object_page_clean(m->object, 0, 0, OBJPC_SYNC);
 						VOP_UNLOCK(m->object->handle, 0, curproc);
-						goto again1;
+						goto again2;
 					} else if (m->object->type == OBJT_SWAP ||
 								m->object->type == OBJT_DEFAULT) {
 						vm_pageout_flush(&m, 1, 0);
-						goto again1;
+						goto again2;
 					}
 				}
 				if ((m->dirty == 0) && (m->busy == 0) && (m->hold_count == 0))
--- vm_page.c.diff ends here ---
>Release-Note:
>Audit-Trail:
>Unformatted: