Nicer than the garble... It would be nice if we could see all the panics. Could we also have a per-cpu panic buffer, and then adapt ddb to show them all?
Scott Cheloha <scottchel...@gmail.com> wrote: > 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? > > 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 */ >
