Re: [9front] fix race condition on writes to #v/vgactl

[qwx@sciops.net]

On Tue Aug 16 08:12:41 +0200 2022, qwx@sciops.net wrote:
> Hello,
> 
> I've been experiencing a seemingly impossible kernel panic when
> messing with vgactl.  Occasionally, writing multiple commands to
> /dev/vgactl rapidly may cause an assert to fail
> (/sys/src/9/port/devdraw.c:/assert).  The panic is perfectly
> reproducible by sending `softscreen off' commands quickly (tested on
> x61t):
> 
> 	; for(i in `{seq 10}) echo softscreen off >/dev/vgactl
> 
> The panic occurs within the first few iterations.  Moving some locks
> out of individual functions to hold them for the entire step fixes
> this.  The patch below works, but qunlock may be called without the
> drawlock being held, I need to recheck and clean up before commiting
> anything.  Credit to moody for suggesting looking in this direction!

Here's the final patch, mostly the same.  I'll commit this later
unless there are any objections.

Thanks,
qwx


diff 3839ad4d3c9b26b8a5d564d1d8f09490799e4cd7 uncommitted
--- a//sys/src/9/pc/devvga.c
+++ b//sys/src/9/pc/devvga.c
@@ -296,7 +296,8 @@
 			error(Ebadarg);
 		if(r.max.x > scr->width || r.max.y > scr->height)
 			error("physical screen bigger than virtual");
-		deletescreenimage();
+		qlock(&drawlock);
+		_deletescreenimage();
 		setactualsize(scr, r);
 		goto Resized;
 	
@@ -324,14 +325,16 @@
 		}
 		if(scr->gscreen == nil)
 			return;
+		qlock(&drawlock);
 		r = actualscreensize(scr);
 		chan = scr->gscreen->chan;
 		z = scr->gscreen->depth;
-		deletescreenimage();
+		_deletescreenimage();
 		setscreensize(scr, scr->width, scr->height, z, chan, tilt);
 		setactualsize(scr, r);
 		/* no break */
 	case CMdrawinit:
+		canqlock(&drawlock);
 		if(scr->gscreen == nil)
 			error(Enoscreen);
 		if(scr->dev && scr->dev->drawinit)
@@ -339,8 +342,9 @@
 		hwblank = scr->blank != nil;
 		hwaccel = scr->fill != nil || scr->scroll != nil;
 	Resized:
-		vgascreenwin(scr);
-		resetscreenimage();
+		_vgascreenwin(scr);
+		_resetscreenimage();
+		qunlock(&drawlock);
 		return;
 
 	case CMlinear:
--- a//sys/src/9/pc/screen.c
+++ b//sys/src/9/pc/screen.c
@@ -64,17 +64,11 @@
 void
 setactualsize(VGAscr *scr, Rectangle r)
 {
-	qlock(&drawlock);
-
 	r.min = ZP;
 	r.max = tiltsize(scr->tilt, r.max);
-	if(rectclip(&r, scr->gscreen->r) == 0){
-		qunlock(&drawlock);
+	if(rectclip(&r, scr->gscreen->r) == 0)
 		return;
-	}
 	scr->gscreen->clipr = r;
-
-	qunlock(&drawlock);
 }
 
 static char*
@@ -136,11 +130,8 @@
 {
 	char *err;
 
-	qlock(&drawlock);
-	if(waserror()){
-		qunlock(&drawlock);
+	if(waserror())
 		nexterror();
-	}
 
 	if(memimageinit() < 0)
 		error("memimageinit failed");
@@ -169,7 +160,6 @@
 		cursoron();
 	}
 
-	qunlock(&drawlock);
 	poperror();
 }
 
--- a//sys/src/9/pc/screen.h
+++ b//sys/src/9/pc/screen.h
@@ -151,6 +151,8 @@
 /* devdraw.c */
 extern void	deletescreenimage(void);
 extern void	resetscreenimage(void);
+extern void	_deletescreenimage(void);
+extern void	_resetscreenimage(void);
 extern void	setscreenimageclipr(Rectangle);
 extern void	drawflush(void);
 extern QLock	drawlock;
@@ -157,6 +159,7 @@
 
 /* vga.c */
 extern void	vgascreenwin(VGAscr*);
+extern void	_vgascreenwin(VGAscr*);
 extern void	vgaimageinit(ulong);
 extern void	vgalinearpci(VGAscr*);
 extern void	vgalinearaddr(VGAscr*, uvlong, vlong);
--- a//sys/src/9/pc/vga.c
+++ b//sys/src/9/pc/vga.c
@@ -176,7 +176,7 @@
 }
 
 void
-vgascreenwin(VGAscr* scr)
+_vgascreenwin(VGAscr* scr)
 {
 	Memimage *i;
 	Rectangle r;
@@ -183,8 +183,6 @@
 	Point p;
 	int h;
 
-	qlock(&drawlock);
-	
 	h = scr->memdefont->height;
 	r = scr->gscreen->r;
 
@@ -215,12 +213,16 @@
 	curpos = window.min;
 
 	flushmemscreen(r);
-
-	qunlock(&drawlock);
-
 	vgascreenputs(kmesg.buf, kmesg.n);
-
 	screenputs = vgascreenputs;
+}
+
+void
+vgascreenwin(VGAscr* scr)
+{
+	qlock(&drawlock);
+	_vgascreenwin(scr);
+	qunlock(&drawlock);
 }
 
 /*
--- a//sys/src/9/port/devdraw.c
+++ b//sys/src/9/port/devdraw.c
@@ -947,9 +947,8 @@
 }
 
 void
-deletescreenimage(void)
+_deletescreenimage(void)
 {
-	dlock();
 	if(screenimage){
 		/* will be freed via screendimage; disable */
 		screenimage->clipr = ZR;
@@ -959,14 +958,27 @@
 		drawfreedimage(screendimage);
 		screendimage = nil;
 	}
+}
+
+void
+deletescreenimage(void)
+{
+	dlock();
+	_deletescreenimage();
 	dunlock();
 }
 
 void
+_resetscreenimage(void)
+{
+	initscreenimage();
+}
+
+void
 resetscreenimage(void)
 {
 	dlock();
-	initscreenimage();
+	_resetscreenimage();
 	dunlock();
 }