code: plan9front

Info  •  Files  •  Log

kernel: fix addbroken() race

There is a unlikely race condition when a
process does addbroken(). Due to the
state being set *after* qunlock().

If the process gets preempted just after qunlock(),
it gets put back in the runqueue in Ready state.

Another process can then unbreak() or
release the process, calling ready()
again on the process (causing a warning
for double-ready) and removing the process
from the broken.p list.

Now the original process resumes and
puts itself in Broken state. Now it is out
of the broken list but in Broken state.

So becomes un-killable by unbreak().

The fix is to mark the process as Broken
before unlocking and changing the QLock
into a Lock which inhibits preemption.

--- a/sys/src/9/port/proc.c

+++ b/sys/src/9/port/proc.c

@@ -1155,32 +1155,25 @@

 	freenote(n);

 }

-/*

- * weird thing: keep at most NBROKEN around

- */

-#define	NBROKEN 4

-struct

-{

-	QLock;

+/* keep some broken processes around */

+static struct {

+	Lock;

 	int	n;

-	Proc	*p[NBROKEN];

-}broken;

+	Proc	*p[4];

+} broken;

 static void

 addbroken(void)

 {

-	qlock(&broken);

-	if(broken.n == NBROKEN) {

+	lock(&broken);

+	if(broken.n == nelem(broken.p)) {

 		ready(broken.p[0]);

-		memmove(&broken.p[0], &broken.p[1], sizeof(Proc*)*(NBROKEN-1));

-		--broken.n;

+		memmove(&broken.p[0], &broken.p[1], sizeof(Proc*)*(--broken.n));

 	}

 	broken.p[broken.n++] = up;

-	qunlock(&broken);

-

-	edfstop(up);

 	up->state = Broken;

 	up->psstate = nil;

+	unlock(&broken);

 	sched();

 }

@@ -1187,18 +1180,18 @@

 void

 unbreak(Proc *p)

 {

-	int b;

+	int i;

-	qlock(&broken);

-	for(b=0; b < broken.n; b++)

-		if(broken.p[b] == p) {

-			broken.n--;

-			memmove(&broken.p[b], &broken.p[b+1],

-					sizeof(Proc*)*(NBROKEN-(b+1)));

+	lock(&broken);

+	for(i=0; i < broken.n; i++){

+		if(broken.p[i] == p) {

+			memmove(&broken.p[i], &broken.p[i+1], sizeof(Proc*)*(broken.n-(i+1)));

+			broken.p[--broken.n] = nil;

 			ready(p);

 			break;

 		}

-	qunlock(&broken);

+	}

+	unlock(&broken);

 }

 int

@@ -1206,14 +1199,15 @@

 {

 	int i, n;

-	qlock(&broken);

+	lock(&broken);

 	n = broken.n;

-	for(i=0; i<n; i++) {

-		ready(broken.p[i]);

+	broken.n = 0;

+	for(i=0; i<n; i++){

+		Proc *p = broken.p[i];

 		broken.p[i] = nil;

+		ready(p);

 	}

-	broken.n = 0;

-	qunlock(&broken);

+	unlock(&broken);

 	return n;

 }

@@ -1327,8 +1321,10 @@

 			free(wq);

 	}

-	if(!freemem)

+	if(!freemem){

+		edfstop(up);

 		addbroken();

+	}

 	qlock(&up->debug);