Re: [PATCH] sysrq: Use panic() to force a crash

[Matthias Kaehlcke <mka@chromium.org>]

On Wed, Sep 19, 2018 at 10:59:51AM -0700, Nick Desaulniers wrote:
> On Tue, Sep 18, 2018 at 5:32 PM Matthias Kaehlcke <mka@chromium.org> wrote:
> >
> > sysrq_handle_crash() currently forces a crash by dereferencing a
> > NULL pointer, which is undefined behavior in C. Just call panic()
> > instead, which is simpler and doesn't depend on compiler specific
> > handling of the undefined behavior.
> >
> > Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> > ---
> > Not sure if it is strictly needed to release the RCU read lock now
> > that panic() is invoked directly (I couldn't repro the warning
> > without rcu_read_unlock()), but since this is a forced crash it
> > seems good practice to keep doing it.
> >
> > The commit that added rcu_read_unlock() and the comment is:
> >
> > commit 984cf355aeaa8f2eda3861b50d0e8d3e3f77e83b
> > Author: Ani Sinha <ani@arista.com>
> > Date:   Thu Dec 17 17:15:10 2015 -0800
> >
> >     sysrq: Fix warning in sysrq generated crash.
> >
> >     Commit 984d74a72076a1 ("sysrq: rcu-ify __handle_sysrq") replaced
> >     spin_lock_irqsave() calls with rcu_read_lock() calls in sysrq. Since
> >     rcu_read_lock() does not disable preemption, faulthandler_disabled() in
> >     __do_page_fault() in x86/fault.c returns false. When the code later calls
> >     might_sleep() in the pagefault handler, we get the following warning:
> >
> >     BUG: sleeping function called from invalid context at ../arch/x86/mm/fault.c:1187
> >     in_atomic(): 0, irqs_disabled(): 0, pid: 4706, name: bash
> >     Preemption disabled at:[<ffffffff81484339>] printk+0x48/0x4a
> >
> >     To fix this, we release the RCU read lock before we crash.
> >
> >     Tested this patch on linux 3.18 by booting off one of our boards.
> > ---
> >  drivers/tty/sysrq.c | 13 +++----------
> >  1 file changed, 3 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/tty/sysrq.c b/drivers/tty/sysrq.c
> > index 06ed20dd01ba..d779a51499a0 100644
> > --- a/drivers/tty/sysrq.c
> > +++ b/drivers/tty/sysrq.c
> > @@ -134,17 +134,10 @@ static struct sysrq_key_op sysrq_unraw_op = {
> >
> >  static void sysrq_handle_crash(int key)
> >  {
> > -       char *killer = NULL;
> > -
> > -       /* we need to release the RCU read lock here,
> > -        * otherwise we get an annoying
> > -        * 'BUG: sleeping function called from invalid context'
> > -        * complaint from the kernel before the panic.
> > -        */
> > +       /* release the RCU read lock before crashing */
> 
> The comment probably could have stayed as is; folks will have to get
> context from git blame on the line immediately below now; while you
> added context in the patch file, it's below the line so wont be part
> of the commit message.

I changed the comment since I have doubts whether it is still correct
with this change. Previously panic() was invoked through the exception
handler, now it is called directly. On the x86 system on which I
tested the patch the warning is not shown when the unlock() is
commented out.

I'm happy to respin if folks think the warning might still be raised
without releasing the lock.