shithub: riscv

Download patch

ref: bdd385803dcbccdf2750f7259a1784744541dec4
parent: b96573b481440455ea92c67a0bc629b88901b319
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);