On 2018-11-29 13:43:58 [-0800], Kees Cook wrote:
> On Mon, Nov 26, 2018 at 9:04 AM Sebastian Andrzej Siewior
> <bigeasy@xxxxxxxxxxxxx> wrote:
> This bug got handled by Jann Horn, yes? (I remember seeing a related
> thread go by...)
Correct, fix sits in the tip tree. Nevertheless it was useful to spot
the other thing.
> > This looks like it comes from commit 21b3ddd39fee ("efi: Don't use
> > spinlocks for efi vars") because it replaced a spin_lock() with
> > down_interruptible() in this case. In this case, since pstore_dump()
> > holds psinfo->buf_lock with disabled interrupts we can't block…
> >
> > I have this as a workaround:
>
> I'm not sure this is correct...
>
> > diff --git a/drivers/firmware/efi/vars.c b/drivers/firmware/efi/vars.c
> > index 9336ffdf6e2c..d4dcfe46eb2e 100644
> > --- a/drivers/firmware/efi/vars.c
> > +++ b/drivers/firmware/efi/vars.c
> > @@ -755,7 +755,9 @@ int efivar_entry_set_safe(efi_char16_t *name, efi_guid_t vendor, u32 attributes,
> > return -EINTR;
> > }
> >
> > - status = check_var_size(attributes, size + ucs2_strsize(name, 1024));
> > + status = ops->query_variable_store(attributes,
> > + size + ucs2_strsize(name, 1024),
> > + block);
>
> Why does this change help?
check_var_size() uses false as the argument for `block'.
check_var_size_nonblocking uses true as the argument for `block'.
Doing this ops->… allows to pass the `block' argument as received from
caller. And the functions above already use the `block' argument. Plus
the next hunk makes sure that `block' is set properly.
> > if (status != EFI_SUCCESS) {
> > up(&efivars_lock);
> > return -ENOSPC;
> > diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
> > index b821054ca3ed..9aa27a2c8d36 100644
> > --- a/fs/pstore/platform.c
> > +++ b/fs/pstore/platform.c
> > @@ -127,10 +127,10 @@ static const char *get_reason_str(enum kmsg_dump_reason reason)
> > bool pstore_cannot_block_path(enum kmsg_dump_reason reason)
> > {
> > /*
> > - * In case of NMI path, pstore shouldn't be blocked
> > - * regardless of reason.
> > + * In case of disabled preemption / interrupts we can't block on any
> > + * lock regardless of reason.
> > */
> > - if (in_nmi())
> > + if (in_atomic() || irqs_disabled())
> > return true;
> >
> > switch (reason) {
>
> I'd like to avoid any case where pstore might "miss" a crash report,
> though... this seems to make it easier for it to fail to record a
> crash, yes?
Well, if the lock is occupied, yes. But waiting for the completion with
disabled interrupts isn't much better :)
pstore_cannot_block_path() is used in two places. One caller is taking a
spin_lock(). So that one could keep using the "old" function.
The other caller uses it before acquiring the semaphore. The non-try
version can't be used with preemption or interrupts disabled.
You could split those two cases and let the sempaphire user use the
"(in_atomic() || irqs_disabled())" from above.
Be aware that once you hold the spinlock you can't block and must do the
try_lock for the semaphore. Or switch it back to the spinlock and spin…
> I have some pending clean-ups in this area (buf_lock may not be needed
> at all, actually), so I'll try to work through those too.
Okay. But could we please fix this for stable somehow, too?
> -Kees
Sebastian
