On Wed, May 12 2021, Scott Cheloha <scottchel...@gmail.com> wrote: > Hi, > > In a separate mail thread, bluhm@ mentioned that panic(9) does not > cleanly handle multiple CPUs entering it simultaneously: > > https://marc.info/?l=openbsd-tech&m=161908805925325&w=2 > > I'm unsure which part of panic(9) is causing the problem he mentions, > but one obvious issue I see is that panicstr is not set atomically, > so two CPUs entering panic(9) simultaneously may clobber panicbuf. > > If we set panicstr atomically only one CPU will write panicbuf. > > Thoughts?
My understanding is that to set panicstr atomically and avoid clobbering, you only need atomic_cas_ptr. Your diff makes panicstr point to volatile data. If intended, what's the rationale? I don't see how it helps avoid races when one cpu "acquires" panicstr and then writes to panicbuf, and another CPU also enters panic but just tries to print panicbuf. If the goal was to mark panicstr itself as volatile, then you're probably trying to solve another problem (making writes to panicstr more instantly/widely visible?) and I *do not know* if that's the right approach (or even needed), but volatile comes with a cost. Take all of this with a grain of salt, I'm not in familiar territory here. > Index: kern/subr_prf.c > =================================================================== > RCS file: /cvs/src/sys/kern/subr_prf.c,v > retrieving revision 1.102 > diff -u -p -r1.102 subr_prf.c > --- kern/subr_prf.c 28 Nov 2020 17:53:05 -0000 1.102 > +++ kern/subr_prf.c 13 May 2021 00:04:28 -0000 > @@ -97,7 +97,7 @@ struct mutex kprintf_mutex = > */ > > extern int log_open; /* subr_log: is /dev/klog open? */ > -const char *panicstr; /* arg to first call to panic (used as a flag > +volatile const char *panicstr; /* arg to first call to panic (used as a flag > to indicate that panic has already been called). */ > const char *faultstr; /* page fault string */ > #ifdef DDB > @@ -195,12 +195,10 @@ panic(const char *fmt, ...) > > bootopt = RB_AUTOBOOT | RB_DUMP; > va_start(ap, fmt); > - if (panicstr) > + if (atomic_cas_ptr(&panicstr, NULL, panicbuf) != NULL) > bootopt |= RB_NOSYNC; > - else { > + else > vsnprintf(panicbuf, sizeof panicbuf, fmt, ap); > - panicstr = panicbuf; > - } > va_end(ap); > > printf("panic: "); > Index: sys/systm.h > =================================================================== > RCS file: /cvs/src/sys/sys/systm.h,v > retrieving revision 1.153 > diff -u -p -r1.153 systm.h > --- sys/systm.h 28 Apr 2021 09:42:04 -0000 1.153 > +++ sys/systm.h 13 May 2021 00:04:28 -0000 > @@ -71,7 +71,7 @@ > * patched by a stalking hacker. > */ > extern int securelevel; /* system security level */ > -extern const char *panicstr; /* panic message */ > +extern volatile const char *panicstr; /* panic message */ > extern const char *faultstr; /* fault message */ > extern const char version[]; /* system version */ > extern const char copyright[]; /* system copyright */ > -- jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE