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

[Nick Desaulniers]

On Thu, Sep 20, 2018 at 4:31 AM Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
>
> 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@xxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxx>
> > > Signed-off-by: Matthias Kaehlcke <mka@xxxxxxxxxxxx>
> > > ---
> > > 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@xxxxxxxxxx>
> > > 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.
> >
> > >         rcu_read_unlock();
> > > -       panic_on_oops = 1;      /* force panic */
> > > -       wmb();
> > > -       *killer = 1;
> > > +
> > > +       panic("sysrq triggered crash\n");
> >
> > Otherwise this part looks good. Maybe GKH can apply just this part
> > rather than a v2
> > (if we even care about git blame on comments)?

Greg, if you don't care about the git blame of these comments, the
patch is good to go.

> > Reviewed-by: Nick Desaulniers <ndesaulniers@xxxxxxxxxx>
>
> I can't pick and choose parts of a patch to apply, sorry.   Please fix
> this up properly and resend it in the format that it should be applied
> in.
>
> thanks,
>
> greg k-h



-- 
Thanks,
~Nick Desaulniers