Patchwork EFI-pstore, sleeping from back context after fault

login
register
mail settings
Submitter Sebastian Andrzej Siewior
Date Nov. 26, 2018, 5:04 p.m.
Message ID <20181126170450.ef2xxoevwwrmqh5m@linutronix.de>
Download mbox | patch
Permalink /patch/665229/
State New
Headers show

Comments

Sebastian Andrzej Siewior - Nov. 26, 2018, 5:04 p.m.
So I triggered a bug in x86 code. First the "okay" backtrace:
|BUG: GPF in non-whitelisted uaccess (non-canonical address?)
|general protection fault: 0000 [#1] PREEMPT SMP NOPTI
|CPU: 26 PID: 2236 Comm: sig-xstate-bum Not tainted 4.20.0-rc3 #45
|RIP: 0010:__fpu__restore_sig+0x1c1/0x540
|Call Trace:
| fpu__restore_sig+0x28/0x40
| restore_sigcontext+0x13a/0x180
| __ia32_sys_rt_sigreturn+0xae/0x100
| do_syscall_64+0x4f/0x100
| entry_SYSCALL_64_after_hwframe+0x44/0xa9
|RIP: 0033:0x7f9b06aea227
|---[ end trace a45ac23b593e9ab0 ]---

and now the not okay part:

|BUG: sleeping function called from invalid context at kernel/sched/completion.c:99
|in_atomic(): 1, irqs_disabled(): 1, pid: 2236, name: sig-xstate-bum
|Preemption disabled at:
|[<ffffffff99d60512>] pstore_dump+0x72/0x330
|CPU: 26 PID: 2236 Comm: sig-xstate-bum Tainted: G      D           4.20.0-rc3 #45
|Call Trace:
| dump_stack+0x4f/0x6a
| ___might_sleep.cold.91+0xd3/0xe4
| __might_sleep+0x50/0x90
| wait_for_completion+0x32/0x130
| virt_efi_query_variable_info+0x14e/0x160
| efi_query_variable_store+0x51/0x1a0
| efivar_entry_set_safe+0xa3/0x1b0
| efi_pstore_write+0x109/0x140
| pstore_dump+0x11c/0x330
| kmsg_dump+0xa4/0xd0
| oops_exit+0x22/0x30
| oops_end+0x67/0xd0
| die+0x41/0x4a
| do_general_protection+0xc1/0x150
| general_protection+0x1e/0x30
|RIP: 0010:__fpu__restore_sig+0x1c1/0x540
| fpu__restore_sig+0x28/0x40
| restore_sigcontext+0x13a/0x180
| __ia32_sys_rt_sigreturn+0xae/0x100
| do_syscall_64+0x4f/0x100
| entry_SYSCALL_64_after_hwframe+0x44/0xa9
|RIP: 0033:0x7f9b06aea227
… [ moar backtrace ]

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:


Sebastian
Kees Cook - Nov. 29, 2018, 9:43 p.m.
On Mon, Nov 26, 2018 at 9:04 AM Sebastian Andrzej Siewior
<bigeasy@linutronix.de> wrote:
>
> So I triggered a bug in x86 code. First the "okay" backtrace:
> |BUG: GPF in non-whitelisted uaccess (non-canonical address?)
> |general protection fault: 0000 [#1] PREEMPT SMP NOPTI
> |CPU: 26 PID: 2236 Comm: sig-xstate-bum Not tainted 4.20.0-rc3 #45
> |RIP: 0010:__fpu__restore_sig+0x1c1/0x540
> |Call Trace:
> | fpu__restore_sig+0x28/0x40
> | restore_sigcontext+0x13a/0x180
> | __ia32_sys_rt_sigreturn+0xae/0x100
> | do_syscall_64+0x4f/0x100
> | entry_SYSCALL_64_after_hwframe+0x44/0xa9
> |RIP: 0033:0x7f9b06aea227
> |---[ end trace a45ac23b593e9ab0 ]---

This bug got handled by Jann Horn, yes? (I remember seeing a related
thread go by...)

>
> and now the not okay part:
>
> |BUG: sleeping function called from invalid context at kernel/sched/completion.c:99
> |in_atomic(): 1, irqs_disabled(): 1, pid: 2236, name: sig-xstate-bum
> |Preemption disabled at:
> |[<ffffffff99d60512>] pstore_dump+0x72/0x330
> |CPU: 26 PID: 2236 Comm: sig-xstate-bum Tainted: G      D           4.20.0-rc3 #45
> |Call Trace:
> | dump_stack+0x4f/0x6a
> | ___might_sleep.cold.91+0xd3/0xe4
> | __might_sleep+0x50/0x90
> | wait_for_completion+0x32/0x130
> | virt_efi_query_variable_info+0x14e/0x160
> | efi_query_variable_store+0x51/0x1a0
> | efivar_entry_set_safe+0xa3/0x1b0
> | efi_pstore_write+0x109/0x140

Eww. :)

> | pstore_dump+0x11c/0x330
> | kmsg_dump+0xa4/0xd0
> | oops_exit+0x22/0x30
> | oops_end+0x67/0xd0
> | die+0x41/0x4a
> | do_general_protection+0xc1/0x150
> | general_protection+0x1e/0x30
> |RIP: 0010:__fpu__restore_sig+0x1c1/0x540
> | fpu__restore_sig+0x28/0x40
> | restore_sigcontext+0x13a/0x180
> | __ia32_sys_rt_sigreturn+0xae/0x100
> | do_syscall_64+0x4f/0x100
> | entry_SYSCALL_64_after_hwframe+0x44/0xa9
> |RIP: 0033:0x7f9b06aea227
> … [ moar backtrace ]
>
> 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?

>         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?

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.

-Kees
Sebastian Andrzej Siewior - Nov. 30, 2018, 11:43 a.m.
On 2018-11-29 13:43:58 [-0800], Kees Cook wrote:
> On Mon, Nov 26, 2018 at 9:04 AM Sebastian Andrzej Siewior
> <bigeasy@linutronix.de> 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
Kees Cook - Nov. 30, 2018, 9:20 p.m.
On Fri, Nov 30, 2018 at 3:43 AM Sebastian Andrzej Siewior
<bigeasy@linutronix.de> wrote:
>
> On 2018-11-29 13:43:58 [-0800], Kees Cook wrote:
> > On Mon, Nov 26, 2018 at 9:04 AM Sebastian Andrzej Siewior
> > <bigeasy@linutronix.de> 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.

Here's the code I see:

        if (!block && ops->set_variable_nonblocking)
                return efivar_entry_set_nonblocking(name, vendor, attributes,
                                                    size, data);

        if (!block) {
                if (down_trylock(&efivars_lock))
                        return -EBUSY;
        } else {
                if (down_interruptible(&efivars_lock))
                        return -EINTR;
        }

        status = check_var_size(attributes, size + ucs2_strsize(name, 1024));
        if (status != EFI_SUCCESS) {
                up(&efivars_lock);
                return -ENOSPC;
        }

        status = ops->set_variable(name, &vendor, attributes, size, data);

        up(&efivars_lock);

In the !block case, this will already take the
efivar_entry_set_nonblocking() path. Isn't that sufficient? (i.e. I'm
not sure why I see a change needed in the EFI code, just the
pstore_cannot_block_path() path?

>
> > >         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.

preempt.h:#define in_nmi()              (preempt_count() & NMI_MASK)
preempt.h:#define in_atomic()   (preempt_count() != 0)

NMI is a special case of preempt_count() test. in_atomic() is a
non-zero preempt_count() test. It looks like we want to use
preemptible()?

#define preemptible() (preempt_count() == 0 && !irqs_disabled())

So, perhaps:

-       if (in_nmi())
+       if (!preemptible())

What do you think?
Kees Cook - Nov. 30, 2018, 10:32 p.m.
On Fri, Nov 30, 2018 at 1:20 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Fri, Nov 30, 2018 at 3:43 AM Sebastian Andrzej Siewior
> <bigeasy@linutronix.de> wrote:
> >
> > On 2018-11-29 13:43:58 [-0800], Kees Cook wrote:
> > > On Mon, Nov 26, 2018 at 9:04 AM Sebastian Andrzej Siewior
> > > <bigeasy@linutronix.de> 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.
>
> Here's the code I see:
>
>         if (!block && ops->set_variable_nonblocking)
>                 return efivar_entry_set_nonblocking(name, vendor, attributes,
>                                                     size, data);
>
>         if (!block) {
>                 if (down_trylock(&efivars_lock))
>                         return -EBUSY;
>         } else {
>                 if (down_interruptible(&efivars_lock))
>                         return -EINTR;
>         }
>
>         status = check_var_size(attributes, size + ucs2_strsize(name, 1024));
>         if (status != EFI_SUCCESS) {
>                 up(&efivars_lock);
>                 return -ENOSPC;
>         }
>
>         status = ops->set_variable(name, &vendor, attributes, size, data);
>
>         up(&efivars_lock);
>
> In the !block case, this will already take the
> efivar_entry_set_nonblocking() path. Isn't that sufficient? (i.e. I'm
> not sure why I see a change needed in the EFI code, just the
> pstore_cannot_block_path() path?
>
> >
> > > >         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.
>
> preempt.h:#define in_nmi()              (preempt_count() & NMI_MASK)
> preempt.h:#define in_atomic()   (preempt_count() != 0)
>
> NMI is a special case of preempt_count() test. in_atomic() is a
> non-zero preempt_count() test. It looks like we want to use
> preemptible()?
>
> #define preemptible() (preempt_count() == 0 && !irqs_disabled())
>
> So, perhaps:
>
> -       if (in_nmi())
> +       if (!preemptible())
>
> What do you think?

I spent some more time looking at this, and I think the best fix would
be to switch pstore also to a semaphore, fix up the logic, and then
replace the efi call to !pstore_cannot_block_path() with a
preemptible() call:

        ret = efivar_entry_set_safe(efi_name, vendor, PSTORE_EFI_ATTRIBUTES,
-                             !pstore_cannot_block_path(record->reason),
-                             record->size, record->psi->buf);
+                             preemptible(), record->size, record->psi->buf);

Patch

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);
 	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) {