Currently the gsmi driver registers a panic notifier as well as
reboot and die notifiers. The callbacks registered are called in
atomic and very limited context - for instance, panic disables
preemption and local IRQs, also all secondary CPUs (not executing
the panic path) are shutdown.
With that said, taking a spinlock in this scenario is a dangerous
invitation for lockup scenarios. So, fix that by checking if the
spinlock is free to acquire in the panic notifier callback - if not,
bail-out and avoid a potential hang.
Fixes: 74c5b31c6618 ("driver: Google EFI SMI")
Cc: Ard Biesheuvel <[email protected]>
Cc: David Gow <[email protected]>
Cc: Evan Green <[email protected]>
Cc: Julius Werner <[email protected]>
Signed-off-by: Guilherme G. Piccoli <[email protected]>
---
V2:
- do not use spin_trylock anymore, to avoid messing with
non-panic paths; now we just check the spinlock state in
the panic notifier before taking it. Thanks Evan for the
review/idea!
drivers/firmware/google/gsmi.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/drivers/firmware/google/gsmi.c b/drivers/firmware/google/gsmi.c
index adaa492c3d2d..3ef5f3c0b4e4 100644
--- a/drivers/firmware/google/gsmi.c
+++ b/drivers/firmware/google/gsmi.c
@@ -681,6 +681,14 @@ static struct notifier_block gsmi_die_notifier = {
static int gsmi_panic_callback(struct notifier_block *nb,
unsigned long reason, void *arg)
{
+ /*
+ * Perform the lock check before effectively trying
+ * to acquire it on gsmi_shutdown_reason() to avoid
+ * potential lockups in atomic context.
+ */
+ if (spin_is_locked(&gsmi_dev.lock))
+ return NOTIFY_DONE;
+
gsmi_shutdown_reason(GSMI_SHUTDOWN_PANIC);
return NOTIFY_DONE;
}
--
2.37.1
On 19/07/2022 16:53, Guilherme G. Piccoli wrote:
> Currently the gsmi driver registers a panic notifier as well as
> reboot and die notifiers. The callbacks registered are called in
> atomic and very limited context - for instance, panic disables
> preemption and local IRQs, also all secondary CPUs (not executing
> the panic path) are shutdown.
>
> With that said, taking a spinlock in this scenario is a dangerous
> invitation for lockup scenarios. So, fix that by checking if the
> spinlock is free to acquire in the panic notifier callback - if not,
> bail-out and avoid a potential hang.
>
> Fixes: 74c5b31c6618 ("driver: Google EFI SMI")
> Cc: Ard Biesheuvel <[email protected]>
> Cc: David Gow <[email protected]>
> Cc: Evan Green <[email protected]>
> Cc: Julius Werner <[email protected]>
> Signed-off-by: Guilherme G. Piccoli <[email protected]>
>
> ---
>
> V2:
> - do not use spin_trylock anymore, to avoid messing with
> non-panic paths; now we just check the spinlock state in
> the panic notifier before taking it. Thanks Evan for the
> review/idea!
>
> drivers/firmware/google/gsmi.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
> [...]
Hi Evan, do you think this one is good now, based on your previous review?
Appreciate any feedback!
Cheers,
Guilherme
On Tue, Jul 19, 2022 at 12:55 PM Guilherme G. Piccoli
<[email protected]> wrote:
>
> Currently the gsmi driver registers a panic notifier as well as
> reboot and die notifiers. The callbacks registered are called in
> atomic and very limited context - for instance, panic disables
> preemption and local IRQs, also all secondary CPUs (not executing
> the panic path) are shutdown.
>
> With that said, taking a spinlock in this scenario is a dangerous
> invitation for lockup scenarios. So, fix that by checking if the
> spinlock is free to acquire in the panic notifier callback - if not,
> bail-out and avoid a potential hang.
>
> Fixes: 74c5b31c6618 ("driver: Google EFI SMI")
> Cc: Ard Biesheuvel <[email protected]>
> Cc: David Gow <[email protected]>
> Cc: Evan Green <[email protected]>
> Cc: Julius Werner <[email protected]>
> Signed-off-by: Guilherme G. Piccoli <[email protected]>
Reviewed-by: Evan Green <[email protected]>
On 08/08/2022 02:07, Evan Green wrote:
> On Tue, Jul 19, 2022 at 12:55 PM Guilherme G. Piccoli
> <[email protected]> wrote:
>>
>> Currently the gsmi driver registers a panic notifier as well as
>> reboot and die notifiers. The callbacks registered are called in
>> atomic and very limited context - for instance, panic disables
>> preemption and local IRQs, also all secondary CPUs (not executing
>> the panic path) are shutdown.
>>
>> With that said, taking a spinlock in this scenario is a dangerous
>> invitation for lockup scenarios. So, fix that by checking if the
>> spinlock is free to acquire in the panic notifier callback - if not,
>> bail-out and avoid a potential hang.
>>
>> Fixes: 74c5b31c6618 ("driver: Google EFI SMI")
>> Cc: Ard Biesheuvel <[email protected]>
>> Cc: David Gow <[email protected]>
>> Cc: Evan Green <[email protected]>
>> Cc: Julius Werner <[email protected]>
>> Signed-off-by: Guilherme G. Piccoli <[email protected]>
>
> Reviewed-by: Evan Green <[email protected]>
Thanks a bunch Evan!
Ard / Greg, do you think you could get this patch through your -next (or
-fixes) trees? Not sure which tree is the most common for picking GSMI
stuff.
I'm trying to get these fixes merged individually in their trees to not
stall the whole series and increase the burden of re-submitting.
Thanks in advance,
Guilherme
On Mon, Aug 08, 2022 at 12:14:30PM -0300, Guilherme G. Piccoli wrote:
> On 08/08/2022 02:07, Evan Green wrote:
> > On Tue, Jul 19, 2022 at 12:55 PM Guilherme G. Piccoli
> > <[email protected]> wrote:
> >>
> >> Currently the gsmi driver registers a panic notifier as well as
> >> reboot and die notifiers. The callbacks registered are called in
> >> atomic and very limited context - for instance, panic disables
> >> preemption and local IRQs, also all secondary CPUs (not executing
> >> the panic path) are shutdown.
> >>
> >> With that said, taking a spinlock in this scenario is a dangerous
> >> invitation for lockup scenarios. So, fix that by checking if the
> >> spinlock is free to acquire in the panic notifier callback - if not,
> >> bail-out and avoid a potential hang.
> >>
> >> Fixes: 74c5b31c6618 ("driver: Google EFI SMI")
> >> Cc: Ard Biesheuvel <[email protected]>
> >> Cc: David Gow <[email protected]>
> >> Cc: Evan Green <[email protected]>
> >> Cc: Julius Werner <[email protected]>
> >> Signed-off-by: Guilherme G. Piccoli <[email protected]>
> >
> > Reviewed-by: Evan Green <[email protected]>
>
> Thanks a bunch Evan!
>
> Ard / Greg, do you think you could get this patch through your -next (or
> -fixes) trees? Not sure which tree is the most common for picking GSMI
> stuff.
Picking out an individual patch from a series with as many responses and
threads like this one is quite difficult.
Just resend this as a stand-alone patch if you want it applied
stand-alone as our tools want to apply a whole patch series at once.
> I'm trying to get these fixes merged individually in their trees to not
> stall the whole series and increase the burden of re-submitting.
The burden is on the submitter, not the maintainer as we have more
submitters than reviewers/maintainers.
thanks,
greg k-h
On 08/08/2022 12:26, Greg Kroah-Hartman wrote:
> [...]
>>
>> Ard / Greg, do you think you could get this patch through your -next (or
>> -fixes) trees? Not sure which tree is the most common for picking GSMI
>> stuff.
>
> Picking out an individual patch from a series with as many responses and
> threads like this one is quite difficult.
>
> Just resend this as a stand-alone patch if you want it applied
> stand-alone as our tools want to apply a whole patch series at once.
>
>> I'm trying to get these fixes merged individually in their trees to not
>> stall the whole series and increase the burden of re-submitting.
>
> The burden is on the submitter, not the maintainer as we have more
> submitters than reviewers/maintainers.
>
I understand, thanks for letting me know!
Let me clarify / ask something: this series, for example, is composed as
a bunch of patches "centered" around the same idea, panic notifiers
improvements/fixes. But its patches belong to completely different
subsystems, like EFI/misc, architectures (alpha, parisc, arm), core
kernel code, etc.
What is the best way of getting this merged?
(a) Re-send individual patches with the respective Review/ACK tags to
the proper subsystem, or;
(b) Wait until the whole series is ACKed/Reviewed, and a single
maintainer (like you or Andrew, for example) would pick the whole series
and apply at once, even if it spans across multiple parts of the kernel?
Let me know what is the general preference of the kernel maintainers,
and I'll gladly follow that =)
Thanks,
Guilherme
On Mon, Aug 08, 2022 at 12:37:46PM -0300, Guilherme G. Piccoli wrote:
> Let me clarify / ask something: this series, for example, is composed as
> a bunch of patches "centered" around the same idea, panic notifiers
> improvements/fixes. But its patches belong to completely different
> subsystems, like EFI/misc, architectures (alpha, parisc, arm), core
> kernel code, etc.
>
> What is the best way of getting this merged?
> (a) Re-send individual patches with the respective Review/ACK tags to
> the proper subsystem, or;
Yes.
> (b) Wait until the whole series is ACKed/Reviewed, and a single
> maintainer (like you or Andrew, for example) would pick the whole series
> and apply at once, even if it spans across multiple parts of the kernel?
No, only do this after a kernel release cycle happens and there are
straggler patches that did not get picked up by the relevant subsystem
maintainers.
thanks,
greg k-h