git: 9front

Download patch

ref: 559d6a6e7a670c12c7e9df2dda03b0370d553dc0
parent: 3ddb2e652a782498d7e567645ff50a8fc56177a4
author: cinap_lenrek <cinap_lenrek@felloff.net>
date: Thu Mar 28 18:27:14 EDT 2024 

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);
 
-- 
⑨