From mboxrd@z Thu Jan 1 00:00:00 1970 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on inbox.vuxu.org X-Spam-Level: X-Spam-Status: No, score=0.2 required=5.0 tests=DKIM_INVALID,DKIM_SIGNED, T_SCC_BODY_TEXT_LINE autolearn=no autolearn_force=no version=3.4.4 Received: (qmail 3843 invoked from network); 16 Aug 2022 20:45:53 -0000 Received: from 9front.inri.net (168.235.81.73) by inbox.vuxu.org with ESMTPUTF8; 16 Aug 2022 20:45:53 -0000 Received: from wopr.sciops.net ([216.126.196.60]) by 9front; Tue Aug 16 16:44:41 -0400 2022 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sciops.net; s=20210706; t=1660682619; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to; bh=ql/Sa3rVRZq/0ePclbJi6R1q8jIoVRIFmRx55FKJZqU=; b=RWF07AC1rgHLQe0ferbTWlzu87dxVQ9S/fdtmnEcas72FKUf9Y4Yq03EYQXXXT0EZO94KI R4ir+djmk7Yc8EiD3OMrfZbbEcX+fzEIEoCG1V9YIpJzjJFh+8WgvZeYxSnwbLiwRvJGlX cl//MXz9qpgOH1S0OeaIdUv4vCjZcQk= Received: by wopr.sciops.net (OpenSMTPD) with ESMTPSA id 81bb0ea2 (TLSv1.2:ECDHE-RSA-CHACHA20-POLY1305:256:NO) for <9front@9front.org>; Tue, 16 Aug 2022 13:43:38 -0700 (PDT) Message-ID: Date: Tue, 16 Aug 2022 22:44:33 +0200 From: qwx@sciops.net To: 9front@9front.org In-Reply-To: <559C4D36E88CDC22C329548E7BBFDF3F@wopr.sciops.net> MIME-Version: 1.0 Content-Type: text/plain; charset="US-ASCII" Content-Transfer-Encoding: 7bit List-ID: <9front.9front.org> List-Help: X-Glyph: ➈ X-Bullshit: webscale overflow-preventing HTTP over SSL service-aware STM generator Subject: Re: [9front] fix race condition on writes to #v/vgactl Reply-To: 9front@9front.org Precedence: bulk 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(); }