Messages in this thread | | | From | Nick Desaulniers <> | Date | Wed, 19 Sep 2018 10:59:51 -0700 | Subject | Re: [PATCH] sysrq: Use panic() to force a crash |
| |
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
| |