On Thu, May 13 2021, Jeremie Courreges-Anglas <j...@wxcvbn.org> wrote:
> 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.
Maybe I should stress more that your diff looks like an improvement
overall and it solves a real world problem. Even if I don't understand
the volatile addition, it probably can't hurt as is, so feel free to go
with other folks' oks. :)
> 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
