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

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

[Matthias Kaehlcke]

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 */
 	rcu_read_unlock();
-	panic_on_oops = 1;	/* force panic */
-	wmb();
-	*killer = 1;
+
+	panic("sysrq triggered crash\n");
 }
 static struct sysrq_key_op sysrq_crash_op = {
 	.handler	= sysrq_handle_crash,
-- 
2.19.0.397.gdd90340f6a-goog

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

[Nick Desaulniers]

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.

>         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)?
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

>  }
>  static struct sysrq_key_op sysrq_crash_op = {
>         .handler        = sysrq_handle_crash,
> --
> 2.19.0.397.gdd90340f6a-goog
>


-- 
Thanks,
~Nick Desaulniers

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

[Matthias Kaehlcke]

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.

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

[Greg KH]

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.
> 
> >         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)?
> Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

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

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

[Nick Desaulniers]

On Thu, Sep 20, 2018 at 4:31 AM Greg KH <gregkh@linuxfoundation.org> 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@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.
> >
> > >         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@google.com>
>
> 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