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

Reply via email to