new warning on sysrq kernel crash trigger

new warning on sysrq kernel crash trigger

[Ani Sinha]

Hi guys

I am noticing a new warning in linux 3.18 which we did not see before
in linux 3.4 :

bash-4.1# echo c > /proc/sysrq-trigger
[  978.807185] BUG: sleeping function called from invalid context at
../arch/x86/mm/fault.c:1187
[  978.909816] in_atomic(): 0, irqs_disabled(): 0, pid: 4706, name: bash
[  978.987358] Preemption disabled at:[<ffffffff81484339>] printk+0x48/0x4a


I have bisected this to the following change :

commit 984d74a72076a12b400339973e8c98fd2fcd90e5
Author: Rik van Riel <riel@redhat.com>
Date:   Fri Jun 6 14:38:13 2014 -0700

    sysrq: rcu-ify __handle_sysrq


the rcu_read_lock() in handle_sysrq() bumps up
current->rcu_read_lock_nesting. Hence, in __do_page_fault() when it
calls might_sleep() in x86/mm/fault.c line 1191,
preempt_count_equals(0) returns false and hence the warning is
printed.

One way to handle this would be to do something like this:

diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
index eef44d9..d4dbe22 100644
--- a/arch/x86/mm/fault.c
+++ b/arch/x86/mm/fault.c
@@ -1132,7 +1132,7 @@ __do_page_fault(struct pt_regs *regs, unsigned
long error_code,
  * If we're in an interrupt, have no user context or are running
  * in a region with pagefaults disabled then we must not take the fault
  */
- if (unlikely(faulthandler_disabled() || !mm)) {
+ if (unlikely(faulthandler_disabled() || rcu_preempt_depth() || !mm)) {
  bad_area_nosemaphore(regs, error_code, address);
  return;
  }

I am wondering if this would be the right approach. I have tested that
this patch does indeed suppress the warning. If you guys agree, I will
send a patch. It's true that this is a trivial issue since we are
intentionally crashing the kernel but in our case, this additional
complaint from the kernel is confusing our test scripts and they are
generating false positives.

Re: new warning on sysrq kernel crash trigger

[Paul E. McKenney]

On Thu, Dec 10, 2015 at 03:57:09PM -0800, Ani Sinha wrote:
> Hi guys
> 
> I am noticing a new warning in linux 3.18 which we did not see before
> in linux 3.4 :
> 
> bash-4.1# echo c > /proc/sysrq-trigger
> [  978.807185] BUG: sleeping function called from invalid context at
> ../arch/x86/mm/fault.c:1187
> [  978.909816] in_atomic(): 0, irqs_disabled(): 0, pid: 4706, name: bash
> [  978.987358] Preemption disabled at:[<ffffffff81484339>] printk+0x48/0x4a
> 
> 
> I have bisected this to the following change :
> 
> commit 984d74a72076a12b400339973e8c98fd2fcd90e5
> Author: Rik van Riel <riel@redhat.com>
> Date:   Fri Jun 6 14:38:13 2014 -0700
> 
>     sysrq: rcu-ify __handle_sysrq
> 
> 
> the rcu_read_lock() in handle_sysrq() bumps up
> current->rcu_read_lock_nesting. Hence, in __do_page_fault() when it
> calls might_sleep() in x86/mm/fault.c line 1191,
> preempt_count_equals(0) returns false and hence the warning is
> printed.
> 
> One way to handle this would be to do something like this:
> 
> diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
> index eef44d9..d4dbe22 100644
> --- a/arch/x86/mm/fault.c
> +++ b/arch/x86/mm/fault.c
> @@ -1132,7 +1132,7 @@ __do_page_fault(struct pt_regs *regs, unsigned
> long error_code,
>   * If we're in an interrupt, have no user context or are running
>   * in a region with pagefaults disabled then we must not take the fault
>   */
> - if (unlikely(faulthandler_disabled() || !mm)) {
> + if (unlikely(faulthandler_disabled() || rcu_preempt_depth() || !mm)) {

This works if CONFIG_PREEMPT=y, but if CONFIG_PREEMPT=n, then
rcu_preempt_depth() unconditionally returns zero.  And if
CONFIG_PREEMPT_COUNT=y && CONFIG_PREEMPT=n, you would still see
the might_sleep() splat.

Maybe use SRCU instead of RCU for this purpose?

							Thanx, Paul

>   bad_area_nosemaphore(regs, error_code, address);
>   return;
>   }
> 
> I am wondering if this would be the right approach. I have tested that
> this patch does indeed suppress the warning. If you guys agree, I will
> send a patch. It's true that this is a trivial issue since we are
> intentionally crashing the kernel but in our case, this additional
> complaint from the kernel is confusing our test scripts and they are
> generating false positives.
> 

Re: new warning on sysrq kernel crash trigger

[Ani Sinha]

Well I can certainly send a patch but I wonder if simply using SRCU
for this one instance in Rik's original patch will not break anything
else. Rik, please provide your thoughts.

On Thu, Dec 10, 2015 at 9:26 PM, Paul E. McKenney
<paulmck@linux.vnet.ibm.com> wrote:
> On Thu, Dec 10, 2015 at 03:57:09PM -0800, Ani Sinha wrote:
>> Hi guys
>>
>> I am noticing a new warning in linux 3.18 which we did not see before
>> in linux 3.4 :
>>
>> bash-4.1# echo c > /proc/sysrq-trigger
>> [  978.807185] BUG: sleeping function called from invalid context at
>> ../arch/x86/mm/fault.c:1187
>> [  978.909816] in_atomic(): 0, irqs_disabled(): 0, pid: 4706, name: bash
>> [  978.987358] Preemption disabled at:[<ffffffff81484339>] printk+0x48/0x4a
>>
>>
>> I have bisected this to the following change :
>>
>> commit 984d74a72076a12b400339973e8c98fd2fcd90e5
>> Author: Rik van Riel <riel@redhat.com>
>> Date:   Fri Jun 6 14:38:13 2014 -0700
>>
>>     sysrq: rcu-ify __handle_sysrq
>>
>>
>> the rcu_read_lock() in handle_sysrq() bumps up
>> current->rcu_read_lock_nesting. Hence, in __do_page_fault() when it
>> calls might_sleep() in x86/mm/fault.c line 1191,
>> preempt_count_equals(0) returns false and hence the warning is
>> printed.
>>
>> One way to handle this would be to do something like this:
>>
>> diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
>> index eef44d9..d4dbe22 100644
>> --- a/arch/x86/mm/fault.c
>> +++ b/arch/x86/mm/fault.c
>> @@ -1132,7 +1132,7 @@ __do_page_fault(struct pt_regs *regs, unsigned
>> long error_code,
>>   * If we're in an interrupt, have no user context or are running
>>   * in a region with pagefaults disabled then we must not take the fault
>>   */
>> - if (unlikely(faulthandler_disabled() || !mm)) {
>> + if (unlikely(faulthandler_disabled() || rcu_preempt_depth() || !mm)) {
>
> This works if CONFIG_PREEMPT=y, but if CONFIG_PREEMPT=n, then
> rcu_preempt_depth() unconditionally returns zero.  And if
> CONFIG_PREEMPT_COUNT=y && CONFIG_PREEMPT=n, you would still see
> the might_sleep() splat.
>
> Maybe use SRCU instead of RCU for this purpose?
>
>                                                         Thanx, Paul
>
>>   bad_area_nosemaphore(regs, error_code, address);
>>   return;
>>   }
>>
>> I am wondering if this would be the right approach. I have tested that
>> this patch does indeed suppress the warning. If you guys agree, I will
>> send a patch. It's true that this is a trivial issue since we are
>> intentionally crashing the kernel but in our case, this additional
>> complaint from the kernel is confusing our test scripts and they are
>> generating false positives.
>>
>

Re: new warning on sysrq kernel crash trigger

[Ani Sinha]

On Thu, 10 Dec 2015, Paul E. McKenney wrote:

> On Thu, Dec 10, 2015 at 03:57:09PM -0800, Ani Sinha wrote:
> > Hi guys
> > 
> > I am noticing a new warning in linux 3.18 which we did not see before
> > in linux 3.4 :
> > 
> > bash-4.1# echo c > /proc/sysrq-trigger
> > [  978.807185] BUG: sleeping function called from invalid context at
> > ../arch/x86/mm/fault.c:1187
> > [  978.909816] in_atomic(): 0, irqs_disabled(): 0, pid: 4706, name: bash
> > [  978.987358] Preemption disabled at:[<ffffffff81484339>] printk+0x48/0x4a
> > 
> > 
> > I have bisected this to the following change :
> > 
> > commit 984d74a72076a12b400339973e8c98fd2fcd90e5
> > Author: Rik van Riel <riel@redhat.com>
> > Date:   Fri Jun 6 14:38:13 2014 -0700
> > 
> >     sysrq: rcu-ify __handle_sysrq
> > 
> > 
> > the rcu_read_lock() in handle_sysrq() bumps up
> > current->rcu_read_lock_nesting. Hence, in __do_page_fault() when it
> > calls might_sleep() in x86/mm/fault.c line 1191,
> > preempt_count_equals(0) returns false and hence the warning is
> > printed.
> > 
> > One way to handle this would be to do something like this:
> > 
> > diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
> > index eef44d9..d4dbe22 100644
> > --- a/arch/x86/mm/fault.c
> > +++ b/arch/x86/mm/fault.c
> > @@ -1132,7 +1132,7 @@ __do_page_fault(struct pt_regs *regs, unsigned
> > long error_code,
> >   * If we're in an interrupt, have no user context or are running
> >   * in a region with pagefaults disabled then we must not take the fault
> >   */
> > - if (unlikely(faulthandler_disabled() || !mm)) {
> > + if (unlikely(faulthandler_disabled() || rcu_preempt_depth() || !mm)) {
> 
> This works if CONFIG_PREEMPT=y, but if CONFIG_PREEMPT=n, then
> rcu_preempt_depth() unconditionally returns zero.  And if
> CONFIG_PREEMPT_COUNT=y && CONFIG_PREEMPT=n, you would still see
> the might_sleep() splat.
> 
> Maybe use SRCU instead of RCU for this purpose?
> 

>From ae232ce3fb167b2ad363bfac7aab69001bc55a50 Mon Sep 17 00:00:00 2001
From: Ani Sinha <ani@arista.com>
Date: Fri, 11 Dec 2015 12:07:42 -0800
Subject: [PATCH 1/1] Fix 'sleeping function called from invalid context'
 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, replace RCU call in handle_sysrq() to use SRCU.

Tested this patch on linux 3.18 by booting off one of our boards.

Fixes: 984d74a72076a1 ("sysrq: rcu-ify __handle_sysrq")

Signed-off-by: Ani Sinha <ani@arista.com>
---
 drivers/tty/sysrq.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/sysrq.c b/drivers/tty/sysrq.c
index 5381a72..904865f 100644
--- a/drivers/tty/sysrq.c
+++ b/drivers/tty/sysrq.c
@@ -519,10 +519,12 @@ void __handle_sysrq(int key, bool check_mask)
 {
 	struct sysrq_key_op *op_p;
 	int orig_log_level;
-	int i;
+	int i, idx;
+	struct srcu_struct sysrq_rcu;
 
+	init_srcu_struct(&sysrq_rcu);
 	rcu_sysrq_start();
-	rcu_read_lock();
+	idx = srcu_read_lock(&sysrq_rcu);
 	/*
 	 * Raise the apparent loglevel to maximum so that the sysrq header
 	 * is shown to provide the user with positive feedback.  We do not
@@ -564,7 +566,7 @@ void __handle_sysrq(int key, bool check_mask)
 		pr_cont("\n");
 		console_loglevel = orig_log_level;
 	}
-	rcu_read_unlock();
+	srcu_read_unlock(&sysrq_rcu, idx);
 	rcu_sysrq_end();
 }
 
-- 
1.8.1.4

Re: new warning on sysrq kernel crash trigger

[Paul E. McKenney]

On Fri, Dec 11, 2015 at 12:44:13PM -0800, Ani Sinha wrote:
> 
> 
> On Thu, 10 Dec 2015, Paul E. McKenney wrote:
> 
> > On Thu, Dec 10, 2015 at 03:57:09PM -0800, Ani Sinha wrote:
> > > Hi guys
> > > 
> > > I am noticing a new warning in linux 3.18 which we did not see before
> > > in linux 3.4 :
> > > 
> > > bash-4.1# echo c > /proc/sysrq-trigger
> > > [  978.807185] BUG: sleeping function called from invalid context at
> > > ../arch/x86/mm/fault.c:1187
> > > [  978.909816] in_atomic(): 0, irqs_disabled(): 0, pid: 4706, name: bash
> > > [  978.987358] Preemption disabled at:[<ffffffff81484339>] printk+0x48/0x4a
> > > 
> > > 
> > > I have bisected this to the following change :
> > > 
> > > commit 984d74a72076a12b400339973e8c98fd2fcd90e5
> > > Author: Rik van Riel <riel@redhat.com>
> > > Date:   Fri Jun 6 14:38:13 2014 -0700
> > > 
> > >     sysrq: rcu-ify __handle_sysrq
> > > 
> > > 
> > > the rcu_read_lock() in handle_sysrq() bumps up
> > > current->rcu_read_lock_nesting. Hence, in __do_page_fault() when it
> > > calls might_sleep() in x86/mm/fault.c line 1191,
> > > preempt_count_equals(0) returns false and hence the warning is
> > > printed.
> > > 
> > > One way to handle this would be to do something like this:
> > > 
> > > diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
> > > index eef44d9..d4dbe22 100644
> > > --- a/arch/x86/mm/fault.c
> > > +++ b/arch/x86/mm/fault.c
> > > @@ -1132,7 +1132,7 @@ __do_page_fault(struct pt_regs *regs, unsigned
> > > long error_code,
> > >   * If we're in an interrupt, have no user context or are running
> > >   * in a region with pagefaults disabled then we must not take the fault
> > >   */
> > > - if (unlikely(faulthandler_disabled() || !mm)) {
> > > + if (unlikely(faulthandler_disabled() || rcu_preempt_depth() || !mm)) {
> > 
> > This works if CONFIG_PREEMPT=y, but if CONFIG_PREEMPT=n, then
> > rcu_preempt_depth() unconditionally returns zero.  And if
> > CONFIG_PREEMPT_COUNT=y && CONFIG_PREEMPT=n, you would still see
> > the might_sleep() splat.
> > 
> > Maybe use SRCU instead of RCU for this purpose?
> > 
> 
> >From ae232ce3fb167b2ad363bfac7aab69001bc55a50 Mon Sep 17 00:00:00 2001
> From: Ani Sinha <ani@arista.com>
> Date: Fri, 11 Dec 2015 12:07:42 -0800
> Subject: [PATCH 1/1] Fix 'sleeping function called from invalid context'
>  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, replace RCU call in handle_sysrq() to use SRCU.
> 
> Tested this patch on linux 3.18 by booting off one of our boards.
> 
> Fixes: 984d74a72076a1 ("sysrq: rcu-ify __handle_sysrq")
> 
> Signed-off-by: Ani Sinha <ani@arista.com>

Hello, Ani,

This patch looks incomplete.  The synchronize_rcu() that Rik added in
__sysrq_swap_key_ops() needs to become synchronize_srcu().  Which
means that it needs to use the sysrq_rcu structure, which means
that this structure cannot be local to __handle_sysrq().

Please see below...

> ---
>  drivers/tty/sysrq.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/tty/sysrq.c b/drivers/tty/sysrq.c
> index 5381a72..904865f 100644
> --- a/drivers/tty/sysrq.c
> +++ b/drivers/tty/sysrq.c
> @@ -519,10 +519,12 @@ void __handle_sysrq(int key, bool check_mask)
>  {
>  	struct sysrq_key_op *op_p;
>  	int orig_log_level;
> -	int i;
> +	int i, idx;
> +	struct srcu_struct sysrq_rcu;
> 
> +	init_srcu_struct(&sysrq_rcu);

Use DEFINE_STATIC_SRCU() to define sysrq_rcu at the global level,
and then get rid of the two lines above.

							Thanx, Paul

>  	rcu_sysrq_start();
> -	rcu_read_lock();
> +	idx = srcu_read_lock(&sysrq_rcu);
>  	/*
>  	 * Raise the apparent loglevel to maximum so that the sysrq header
>  	 * is shown to provide the user with positive feedback.  We do not
> @@ -564,7 +566,7 @@ void __handle_sysrq(int key, bool check_mask)
>  		pr_cont("\n");
>  		console_loglevel = orig_log_level;
>  	}
> -	rcu_read_unlock();
> +	srcu_read_unlock(&sysrq_rcu, idx);
>  	rcu_sysrq_end();
>  }
> 
> -- 
> 1.8.1.4
> 

Re: new warning on sysrq kernel crash trigger

[Rik van Riel]

On 12/11/2015 03:44 PM, Ani Sinha wrote:
> 
> 
> On Thu, 10 Dec 2015, Paul E. McKenney wrote:
> 
>> On Thu, Dec 10, 2015 at 03:57:09PM -0800, Ani Sinha wrote:
>>> Hi guys
>>>
>>> I am noticing a new warning in linux 3.18 which we did not see before
>>> in linux 3.4 :
>>>
>>> bash-4.1# echo c > /proc/sysrq-trigger
>>> [  978.807185] BUG: sleeping function called from invalid context at
>>> ../arch/x86/mm/fault.c:1187
>>> [  978.909816] in_atomic(): 0, irqs_disabled(): 0, pid: 4706, name: bash
>>> [  978.987358] Preemption disabled at:[<ffffffff81484339>] printk+0x48/0x4a
>>>
>>>
>>> I have bisected this to the following change :
>>>
>>> commit 984d74a72076a12b400339973e8c98fd2fcd90e5
>>> Author: Rik van Riel <riel@redhat.com>
>>> Date:   Fri Jun 6 14:38:13 2014 -0700
>>>
>>>     sysrq: rcu-ify __handle_sysrq
>>>
>>>
>>> the rcu_read_lock() in handle_sysrq() bumps up
>>> current->rcu_read_lock_nesting. Hence, in __do_page_fault() when it
>>> calls might_sleep() in x86/mm/fault.c line 1191,
>>> preempt_count_equals(0) returns false and hence the warning is
>>> printed.
>>>
>>> One way to handle this would be to do something like this:
>>>
>>> diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
>>> index eef44d9..d4dbe22 100644
>>> --- a/arch/x86/mm/fault.c
>>> +++ b/arch/x86/mm/fault.c
>>> @@ -1132,7 +1132,7 @@ __do_page_fault(struct pt_regs *regs, unsigned
>>> long error_code,
>>>   * If we're in an interrupt, have no user context or are running
>>>   * in a region with pagefaults disabled then we must not take the fault
>>>   */
>>> - if (unlikely(faulthandler_disabled() || !mm)) {
>>> + if (unlikely(faulthandler_disabled() || rcu_preempt_depth() || !mm)) {
>>
>> This works if CONFIG_PREEMPT=y, but if CONFIG_PREEMPT=n, then
>> rcu_preempt_depth() unconditionally returns zero.  And if
>> CONFIG_PREEMPT_COUNT=y && CONFIG_PREEMPT=n, you would still see
>> the might_sleep() splat.
>>
>> Maybe use SRCU instead of RCU for this purpose?
>>
> 
> From ae232ce3fb167b2ad363bfac7aab69001bc55a50 Mon Sep 17 00:00:00 2001
> From: Ani Sinha <ani@arista.com>
> Date: Fri, 11 Dec 2015 12:07:42 -0800
> Subject: [PATCH 1/1] Fix 'sleeping function called from invalid context'
>  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, replace RCU call in handle_sysrq() to use SRCU.

The sysrq code can be called from irq context.

Trying to use SRCU from an irq context sounds like it could
be a bad idea, though admittedly I do not know enough about
SRCU to know for sure :)


-- 
All rights reversed

Re: new warning on sysrq kernel crash trigger

[Paul E. McKenney]

On Fri, Dec 11, 2015 at 05:10:43PM -0500, Rik van Riel wrote:
> On 12/11/2015 03:44 PM, Ani Sinha wrote:
> > 
> > 
> > On Thu, 10 Dec 2015, Paul E. McKenney wrote:
> > 
> >> On Thu, Dec 10, 2015 at 03:57:09PM -0800, Ani Sinha wrote:
> >>> Hi guys
> >>>
> >>> I am noticing a new warning in linux 3.18 which we did not see before
> >>> in linux 3.4 :
> >>>
> >>> bash-4.1# echo c > /proc/sysrq-trigger
> >>> [  978.807185] BUG: sleeping function called from invalid context at
> >>> ../arch/x86/mm/fault.c:1187
> >>> [  978.909816] in_atomic(): 0, irqs_disabled(): 0, pid: 4706, name: bash
> >>> [  978.987358] Preemption disabled at:[<ffffffff81484339>] printk+0x48/0x4a
> >>>
> >>>
> >>> I have bisected this to the following change :
> >>>
> >>> commit 984d74a72076a12b400339973e8c98fd2fcd90e5
> >>> Author: Rik van Riel <riel@redhat.com>
> >>> Date:   Fri Jun 6 14:38:13 2014 -0700
> >>>
> >>>     sysrq: rcu-ify __handle_sysrq
> >>>
> >>>
> >>> the rcu_read_lock() in handle_sysrq() bumps up
> >>> current->rcu_read_lock_nesting. Hence, in __do_page_fault() when it
> >>> calls might_sleep() in x86/mm/fault.c line 1191,
> >>> preempt_count_equals(0) returns false and hence the warning is
> >>> printed.
> >>>
> >>> One way to handle this would be to do something like this:
> >>>
> >>> diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
> >>> index eef44d9..d4dbe22 100644
> >>> --- a/arch/x86/mm/fault.c
> >>> +++ b/arch/x86/mm/fault.c
> >>> @@ -1132,7 +1132,7 @@ __do_page_fault(struct pt_regs *regs, unsigned
> >>> long error_code,
> >>>   * If we're in an interrupt, have no user context or are running
> >>>   * in a region with pagefaults disabled then we must not take the fault
> >>>   */
> >>> - if (unlikely(faulthandler_disabled() || !mm)) {
> >>> + if (unlikely(faulthandler_disabled() || rcu_preempt_depth() || !mm)) {
> >>
> >> This works if CONFIG_PREEMPT=y, but if CONFIG_PREEMPT=n, then
> >> rcu_preempt_depth() unconditionally returns zero.  And if
> >> CONFIG_PREEMPT_COUNT=y && CONFIG_PREEMPT=n, you would still see
> >> the might_sleep() splat.
> >>
> >> Maybe use SRCU instead of RCU for this purpose?
> >>
> > 
> > From ae232ce3fb167b2ad363bfac7aab69001bc55a50 Mon Sep 17 00:00:00 2001
> > From: Ani Sinha <ani@arista.com>
> > Date: Fri, 11 Dec 2015 12:07:42 -0800
> > Subject: [PATCH 1/1] Fix 'sleeping function called from invalid context'
> >  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, replace RCU call in handle_sysrq() to use SRCU.
> 
> The sysrq code can be called from irq context.
> 
> Trying to use SRCU from an irq context sounds like it could
> be a bad idea, though admittedly I do not know enough about
> SRCU to know for sure :)

Indeed, not the best idea!  ;-)

I could imagine something like this:

	if (in_irq())
		rcu_read_lock();
	else
		idx = srcu_read_lock(&sysrq_rcu);

And ditto for unlock.  Then, for the update:

	synchronize_rcu_mult(call_rcu, call_sysrq_srcu);

Where:

	static void call_sysrq_srcu(struct rcu_head *head, rcu_callback_t func)
	{
		call_srcu(&sysrq_rcu, head, func);
	}

Here I presume that the page-fault code avoids the might_sleep if invoked
from irq context.

Thoughts?

							Thanx, Paul

Re: new warning on sysrq kernel crash trigger

[Ani Sinha]

On Fri, Dec 11, 2015 at 2:27 PM, Paul E. McKenney
<paulmck@linux.vnet.ibm.com> wrote:
> On Fri, Dec 11, 2015 at 05:10:43PM -0500, Rik van Riel wrote:
>> On 12/11/2015 03:44 PM, Ani Sinha wrote:
>> >
>> >
>> > On Thu, 10 Dec 2015, Paul E. McKenney wrote:
>> >
>> >> On Thu, Dec 10, 2015 at 03:57:09PM -0800, Ani Sinha wrote:
>> >>> Hi guys
>> >>>
>> >>> I am noticing a new warning in linux 3.18 which we did not see before
>> >>> in linux 3.4 :
>> >>>
>> >>> bash-4.1# echo c > /proc/sysrq-trigger
>> >>> [  978.807185] BUG: sleeping function called from invalid context at
>> >>> ../arch/x86/mm/fault.c:1187
>> >>> [  978.909816] in_atomic(): 0, irqs_disabled(): 0, pid: 4706, name: bash
>> >>> [  978.987358] Preemption disabled at:[<ffffffff81484339>] printk+0x48/0x4a
>> >>>
>> >>>
>> >>> I have bisected this to the following change :
>> >>>
>> >>> commit 984d74a72076a12b400339973e8c98fd2fcd90e5
>> >>> Author: Rik van Riel <riel@redhat.com>
>> >>> Date:   Fri Jun 6 14:38:13 2014 -0700
>> >>>
>> >>>     sysrq: rcu-ify __handle_sysrq
>> >>>
>> >>>
>> >>> the rcu_read_lock() in handle_sysrq() bumps up
>> >>> current->rcu_read_lock_nesting. Hence, in __do_page_fault() when it
>> >>> calls might_sleep() in x86/mm/fault.c line 1191,
>> >>> preempt_count_equals(0) returns false and hence the warning is
>> >>> printed.
>> >>>
>> >>> One way to handle this would be to do something like this:
>> >>>
>> >>> diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
>> >>> index eef44d9..d4dbe22 100644
>> >>> --- a/arch/x86/mm/fault.c
>> >>> +++ b/arch/x86/mm/fault.c
>> >>> @@ -1132,7 +1132,7 @@ __do_page_fault(struct pt_regs *regs, unsigned
>> >>> long error_code,
>> >>>   * If we're in an interrupt, have no user context or are running
>> >>>   * in a region with pagefaults disabled then we must not take the fault
>> >>>   */
>> >>> - if (unlikely(faulthandler_disabled() || !mm)) {
>> >>> + if (unlikely(faulthandler_disabled() || rcu_preempt_depth() || !mm)) {
>> >>
>> >> This works if CONFIG_PREEMPT=y, but if CONFIG_PREEMPT=n, then
>> >> rcu_preempt_depth() unconditionally returns zero.  And if
>> >> CONFIG_PREEMPT_COUNT=y && CONFIG_PREEMPT=n, you would still see
>> >> the might_sleep() splat.
>> >>
>> >> Maybe use SRCU instead of RCU for this purpose?
>> >>
>> >
>> > From ae232ce3fb167b2ad363bfac7aab69001bc55a50 Mon Sep 17 00:00:00 2001
>> > From: Ani Sinha <ani@arista.com>
>> > Date: Fri, 11 Dec 2015 12:07:42 -0800
>> > Subject: [PATCH 1/1] Fix 'sleeping function called from invalid context'
>> >  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, replace RCU call in handle_sysrq() to use SRCU.
>>
>> The sysrq code can be called from irq context.
>>
>> Trying to use SRCU from an irq context sounds like it could
>> be a bad idea, though admittedly I do not know enough about
>> SRCU to know for sure :)
>
> Indeed, not the best idea!  ;-)
>
> I could imagine something like this:
>
>         if (in_irq())
>                 rcu_read_lock();
>         else
>                 idx = srcu_read_lock(&sysrq_rcu);
>
> And ditto for unlock.  Then, for the update:
>
>         synchronize_rcu_mult(call_rcu, call_sysrq_srcu);

This won't work on 3.18 as this api was introduced in linux 4.3.

>
> Where:
>
>         static void call_sysrq_srcu(struct rcu_head *head, rcu_callback_t func)
>         {
>                 call_srcu(&sysrq_rcu, head, func);
>         }
>
> Here I presume that the page-fault code avoids the might_sleep if invoked
> from irq context.

Quick look at the code seems to indicate that this is true.

>
> Thoughts?
>
>                                                         Thanx, Paul
>

Re: new warning on sysrq kernel crash trigger

[Paul E. McKenney]

On Fri, Dec 11, 2015 at 03:41:04PM -0800, Ani Sinha wrote:
> On Fri, Dec 11, 2015 at 2:27 PM, Paul E. McKenney
> <paulmck@linux.vnet.ibm.com> wrote:
> > On Fri, Dec 11, 2015 at 05:10:43PM -0500, Rik van Riel wrote:
> >> On 12/11/2015 03:44 PM, Ani Sinha wrote:
> >> >
> >> >
> >> > On Thu, 10 Dec 2015, Paul E. McKenney wrote:
> >> >
> >> >> On Thu, Dec 10, 2015 at 03:57:09PM -0800, Ani Sinha wrote:
> >> >>> Hi guys
> >> >>>
> >> >>> I am noticing a new warning in linux 3.18 which we did not see before
> >> >>> in linux 3.4 :
> >> >>>
> >> >>> bash-4.1# echo c > /proc/sysrq-trigger
> >> >>> [  978.807185] BUG: sleeping function called from invalid context at
> >> >>> ../arch/x86/mm/fault.c:1187
> >> >>> [  978.909816] in_atomic(): 0, irqs_disabled(): 0, pid: 4706, name: bash
> >> >>> [  978.987358] Preemption disabled at:[<ffffffff81484339>] printk+0x48/0x4a
> >> >>>
> >> >>>
> >> >>> I have bisected this to the following change :
> >> >>>
> >> >>> commit 984d74a72076a12b400339973e8c98fd2fcd90e5
> >> >>> Author: Rik van Riel <riel@redhat.com>
> >> >>> Date:   Fri Jun 6 14:38:13 2014 -0700
> >> >>>
> >> >>>     sysrq: rcu-ify __handle_sysrq
> >> >>>
> >> >>>
> >> >>> the rcu_read_lock() in handle_sysrq() bumps up
> >> >>> current->rcu_read_lock_nesting. Hence, in __do_page_fault() when it
> >> >>> calls might_sleep() in x86/mm/fault.c line 1191,
> >> >>> preempt_count_equals(0) returns false and hence the warning is
> >> >>> printed.
> >> >>>
> >> >>> One way to handle this would be to do something like this:
> >> >>>
> >> >>> diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
> >> >>> index eef44d9..d4dbe22 100644
> >> >>> --- a/arch/x86/mm/fault.c
> >> >>> +++ b/arch/x86/mm/fault.c
> >> >>> @@ -1132,7 +1132,7 @@ __do_page_fault(struct pt_regs *regs, unsigned
> >> >>> long error_code,
> >> >>>   * If we're in an interrupt, have no user context or are running
> >> >>>   * in a region with pagefaults disabled then we must not take the fault
> >> >>>   */
> >> >>> - if (unlikely(faulthandler_disabled() || !mm)) {
> >> >>> + if (unlikely(faulthandler_disabled() || rcu_preempt_depth() || !mm)) {
> >> >>
> >> >> This works if CONFIG_PREEMPT=y, but if CONFIG_PREEMPT=n, then
> >> >> rcu_preempt_depth() unconditionally returns zero.  And if
> >> >> CONFIG_PREEMPT_COUNT=y && CONFIG_PREEMPT=n, you would still see
> >> >> the might_sleep() splat.
> >> >>
> >> >> Maybe use SRCU instead of RCU for this purpose?
> >> >>
> >> >
> >> > From ae232ce3fb167b2ad363bfac7aab69001bc55a50 Mon Sep 17 00:00:00 2001
> >> > From: Ani Sinha <ani@arista.com>
> >> > Date: Fri, 11 Dec 2015 12:07:42 -0800
> >> > Subject: [PATCH 1/1] Fix 'sleeping function called from invalid context'
> >> >  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, replace RCU call in handle_sysrq() to use SRCU.
> >>
> >> The sysrq code can be called from irq context.
> >>
> >> Trying to use SRCU from an irq context sounds like it could
> >> be a bad idea, though admittedly I do not know enough about
> >> SRCU to know for sure :)
> >
> > Indeed, not the best idea!  ;-)
> >
> > I could imagine something like this:
> >
> >         if (in_irq())
> >                 rcu_read_lock();
> >         else
> >                 idx = srcu_read_lock(&sysrq_rcu);
> >
> > And ditto for unlock.  Then, for the update:
> >
> >         synchronize_rcu_mult(call_rcu, call_sysrq_srcu);
> 
> This won't work on 3.18 as this api was introduced in linux 4.3.

Then do this:

	synchronize_rcu();
	synchronize_srcu(&sysrq_rcu);

> > Where:
> >
> >         static void call_sysrq_srcu(struct rcu_head *head, rcu_callback_t func)
> >         {
> >                 call_srcu(&sysrq_rcu, head, func);
> >         }
> >
> > Here I presume that the page-fault code avoids the might_sleep if invoked
> > from irq context.
> 
> Quick look at the code seems to indicate that this is true.

Good!  ;-)
	
							Thanx, Paul

Re: new warning on sysrq kernel crash trigger

[Ani Sinha]

I backported your

ee376dbdf277 ("rcu: Consolidate rcu_synchronize and wakeme_after_rcu()" &
ec90a194ae2cb8b8e("rcu: Create a synchronize_rcu_mult()")

and tested this on our 3.18 kernel running on our board. The sysrq
kernel crash seems to have been fixed (behavior as per our old 3.4
kernel). I will send in a patch as per your former suggestion ...


On Fri, Dec 11, 2015 at 4:02 PM, Paul E. McKenney
<paulmck@linux.vnet.ibm.com> wrote:
> On Fri, Dec 11, 2015 at 03:41:04PM -0800, Ani Sinha wrote:
>> On Fri, Dec 11, 2015 at 2:27 PM, Paul E. McKenney
>> <paulmck@linux.vnet.ibm.com> wrote:
>> > On Fri, Dec 11, 2015 at 05:10:43PM -0500, Rik van Riel wrote:
>> >> On 12/11/2015 03:44 PM, Ani Sinha wrote:
>> >> >
>> >> >
>> >> > On Thu, 10 Dec 2015, Paul E. McKenney wrote:
>> >> >
>> >> >> On Thu, Dec 10, 2015 at 03:57:09PM -0800, Ani Sinha wrote:
>> >> >>> Hi guys
>> >> >>>
>> >> >>> I am noticing a new warning in linux 3.18 which we did not see before
>> >> >>> in linux 3.4 :
>> >> >>>
>> >> >>> bash-4.1# echo c > /proc/sysrq-trigger
>> >> >>> [  978.807185] BUG: sleeping function called from invalid context at
>> >> >>> ../arch/x86/mm/fault.c:1187
>> >> >>> [  978.909816] in_atomic(): 0, irqs_disabled(): 0, pid: 4706, name: bash
>> >> >>> [  978.987358] Preemption disabled at:[<ffffffff81484339>] printk+0x48/0x4a
>> >> >>>
>> >> >>>
>> >> >>> I have bisected this to the following change :
>> >> >>>
>> >> >>> commit 984d74a72076a12b400339973e8c98fd2fcd90e5
>> >> >>> Author: Rik van Riel <riel@redhat.com>
>> >> >>> Date:   Fri Jun 6 14:38:13 2014 -0700
>> >> >>>
>> >> >>>     sysrq: rcu-ify __handle_sysrq
>> >> >>>
>> >> >>>
>> >> >>> the rcu_read_lock() in handle_sysrq() bumps up
>> >> >>> current->rcu_read_lock_nesting. Hence, in __do_page_fault() when it
>> >> >>> calls might_sleep() in x86/mm/fault.c line 1191,
>> >> >>> preempt_count_equals(0) returns false and hence the warning is
>> >> >>> printed.
>> >> >>>
>> >> >>> One way to handle this would be to do something like this:
>> >> >>>
>> >> >>> diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
>> >> >>> index eef44d9..d4dbe22 100644
>> >> >>> --- a/arch/x86/mm/fault.c
>> >> >>> +++ b/arch/x86/mm/fault.c
>> >> >>> @@ -1132,7 +1132,7 @@ __do_page_fault(struct pt_regs *regs, unsigned
>> >> >>> long error_code,
>> >> >>>   * If we're in an interrupt, have no user context or are running
>> >> >>>   * in a region with pagefaults disabled then we must not take the fault
>> >> >>>   */
>> >> >>> - if (unlikely(faulthandler_disabled() || !mm)) {
>> >> >>> + if (unlikely(faulthandler_disabled() || rcu_preempt_depth() || !mm)) {
>> >> >>
>> >> >> This works if CONFIG_PREEMPT=y, but if CONFIG_PREEMPT=n, then
>> >> >> rcu_preempt_depth() unconditionally returns zero.  And if
>> >> >> CONFIG_PREEMPT_COUNT=y && CONFIG_PREEMPT=n, you would still see
>> >> >> the might_sleep() splat.
>> >> >>
>> >> >> Maybe use SRCU instead of RCU for this purpose?
>> >> >>
>> >> >
>> >> > From ae232ce3fb167b2ad363bfac7aab69001bc55a50 Mon Sep 17 00:00:00 2001
>> >> > From: Ani Sinha <ani@arista.com>
>> >> > Date: Fri, 11 Dec 2015 12:07:42 -0800
>> >> > Subject: [PATCH 1/1] Fix 'sleeping function called from invalid context'
>> >> >  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, replace RCU call in handle_sysrq() to use SRCU.
>> >>
>> >> The sysrq code can be called from irq context.
>> >>
>> >> Trying to use SRCU from an irq context sounds like it could
>> >> be a bad idea, though admittedly I do not know enough about
>> >> SRCU to know for sure :)
>> >
>> > Indeed, not the best idea!  ;-)
>> >
>> > I could imagine something like this:
>> >
>> >         if (in_irq())
>> >                 rcu_read_lock();
>> >         else
>> >                 idx = srcu_read_lock(&sysrq_rcu);
>> >
>> > And ditto for unlock.  Then, for the update:
>> >
>> >         synchronize_rcu_mult(call_rcu, call_sysrq_srcu);
>>
>> This won't work on 3.18 as this api was introduced in linux 4.3.
>
> Then do this:
>
>         synchronize_rcu();
>         synchronize_srcu(&sysrq_rcu);
>
>> > Where:
>> >
>> >         static void call_sysrq_srcu(struct rcu_head *head, rcu_callback_t func)
>> >         {
>> >                 call_srcu(&sysrq_rcu, head, func);
>> >         }
>> >
>> > Here I presume that the page-fault code avoids the might_sleep if invoked
>> > from irq context.
>>
>> Quick look at the code seems to indicate that this is true.
>
> Good!  ;-)
>
>                                                         Thanx, Paul
>

Re: new warning on sysrq kernel crash trigger

[Ani Sinha]

On Fri, 11 Dec 2015, Paul E. McKenney wrote:

> On Fri, Dec 11, 2015 at 05:10:43PM -0500, Rik van Riel wrote:
> > On 12/11/2015 03:44 PM, Ani Sinha wrote:
> > > 
> > > 
> > > On Thu, 10 Dec 2015, Paul E. McKenney wrote:
> > > 
> > >> On Thu, Dec 10, 2015 at 03:57:09PM -0800, Ani Sinha wrote:
> > >>> Hi guys
> > >>>
> > >>> I am noticing a new warning in linux 3.18 which we did not see before
> > >>> in linux 3.4 :
> > >>>
> > >>> bash-4.1# echo c > /proc/sysrq-trigger
> > >>> [  978.807185] BUG: sleeping function called from invalid context at
> > >>> ../arch/x86/mm/fault.c:1187
> > >>> [  978.909816] in_atomic(): 0, irqs_disabled(): 0, pid: 4706, name: bash
> > >>> [  978.987358] Preemption disabled at:[<ffffffff81484339>] printk+0x48/0x4a
> > >>>
> > >>>
> > >>> I have bisected this to the following change :
> > >>>
> > >>> commit 984d74a72076a12b400339973e8c98fd2fcd90e5
> > >>> Author: Rik van Riel <riel@redhat.com>
> > >>> Date:   Fri Jun 6 14:38:13 2014 -0700
> > >>>
> > >>>     sysrq: rcu-ify __handle_sysrq
> > >>>
> > >>>
> > >>> the rcu_read_lock() in handle_sysrq() bumps up
> > >>> current->rcu_read_lock_nesting. Hence, in __do_page_fault() when it
> > >>> calls might_sleep() in x86/mm/fault.c line 1191,
> > >>> preempt_count_equals(0) returns false and hence the warning is
> > >>> printed.
> > >>>
> > >>> One way to handle this would be to do something like this:
> > >>>
> > >>> diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
> > >>> index eef44d9..d4dbe22 100644
> > >>> --- a/arch/x86/mm/fault.c
> > >>> +++ b/arch/x86/mm/fault.c
> > >>> @@ -1132,7 +1132,7 @@ __do_page_fault(struct pt_regs *regs, unsigned
> > >>> long error_code,
> > >>>   * If we're in an interrupt, have no user context or are running
> > >>>   * in a region with pagefaults disabled then we must not take the fault
> > >>>   */
> > >>> - if (unlikely(faulthandler_disabled() || !mm)) {
> > >>> + if (unlikely(faulthandler_disabled() || rcu_preempt_depth() || !mm)) {
> > >>
> > >> This works if CONFIG_PREEMPT=y, but if CONFIG_PREEMPT=n, then
> > >> rcu_preempt_depth() unconditionally returns zero.  And if
> > >> CONFIG_PREEMPT_COUNT=y && CONFIG_PREEMPT=n, you would still see
> > >> the might_sleep() splat.
> > >>
> > >> Maybe use SRCU instead of RCU for this purpose?
> > >>
> > > 
> > > From ae232ce3fb167b2ad363bfac7aab69001bc55a50 Mon Sep 17 00:00:00 2001
> > > From: Ani Sinha <ani@arista.com>
> > > Date: Fri, 11 Dec 2015 12:07:42 -0800
> > > Subject: [PATCH 1/1] Fix 'sleeping function called from invalid context'
> > >  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, replace RCU call in handle_sysrq() to use SRCU.
> > 
> > The sysrq code can be called from irq context.
> > 
> > Trying to use SRCU from an irq context sounds like it could
> > be a bad idea, though admittedly I do not know enough about
> > SRCU to know for sure :)
> 
> Indeed, not the best idea!  ;-)
> 
> I could imagine something like this:
> 
> 	if (in_irq())
> 		rcu_read_lock();
> 	else
> 		idx = srcu_read_lock(&sysrq_rcu);
> 
> And ditto for unlock.  Then, for the update:
> 
> 	synchronize_rcu_mult(call_rcu, call_sysrq_srcu);
> 
> Where:
> 
> 	static void call_sysrq_srcu(struct rcu_head *head, rcu_callback_t func)
> 	{
> 		call_srcu(&sysrq_rcu, head, func);
> 	}
> 

>From ae232ce3fb167b2ad363bfac7aab69001bc55a50 Mon Sep 17 00:00:00 2001
From: Ani Sinha <ani@arista.com>
Date: Fri, 11 Dec 2015 12:07:42 -0800
Subject: [PATCH 1/1] Fix 'sleeping function called from invalid context'
 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, replace RCU call in handle_sysrq() to use SRCU
in non-irq context.

Tested this patch on linux 3.18 by booting off one of our boards.

Fixes: 984d74a72076a1 ("sysrq: rcu-ify __handle_sysrq")

Signed-off-by: Ani Sinha <ani@arista.com>
---
diff --git a/drivers/tty/sysrq.c b/drivers/tty/sysrq.c
index 5381a72..df7d747 100644
--- a/drivers/tty/sysrq.c
+++ b/drivers/tty/sysrq.c
@@ -54,6 +54,7 @@
 /* Whether we react on sysrq keys or just ignore them */
 static int __read_mostly sysrq_enabled = CONFIG_MAGIC_SYSRQ_DEFAULT_ENABLE;
 static bool __read_mostly sysrq_always_enabled;
+DEFINE_STATIC_SRCU(sysrq_rcu);
 
 static bool sysrq_on(void)
 {
@@ -519,10 +520,13 @@ void __handle_sysrq(int key, bool check_mask)
 {
 	struct sysrq_key_op *op_p;
 	int orig_log_level;
-	int i;
+	int i, idx;
 
 	rcu_sysrq_start();
-	rcu_read_lock();
+	if (in_irq())
+		rcu_read_lock();
+	else
+		idx = srcu_read_lock(&sysrq_rcu);
 	/*
 	 * Raise the apparent loglevel to maximum so that the sysrq header
 	 * is shown to provide the user with positive feedback.  We do not
@@ -564,7 +568,10 @@ void __handle_sysrq(int key, bool check_mask)
 		pr_cont("\n");
 		console_loglevel = orig_log_level;
 	}
-	rcu_read_unlock();
+	if (in_irq())
+		rcu_read_unlock();
+	else
+		srcu_read_unlock(&sysrq_rcu, idx);
 	rcu_sysrq_end();
 }
 
@@ -1040,6 +1047,11 @@ int sysrq_toggle_support(int enable_mask)
 	return 0;
 }
 
+static void call_sysrq_srcu(struct rcu_head *head, rcu_callback_t func)
+{
+	call_srcu(&sysrq_rcu, head, func);
+}
+
 static int __sysrq_swap_key_ops(int key, struct sysrq_key_op *insert_op_p,
                                 struct sysrq_key_op *remove_op_p)
 {
@@ -1059,7 +1071,7 @@ static int __sysrq_swap_key_ops(int key, struct sysrq_key_op *insert_op_p,
 	 * Wait for it to go away before returning, so the code for an old
 	 * op is not freed (eg. on module unload) while it is in use.
 	 */
-	synchronize_rcu();
+	synchronize_rcu_mult(call_rcu, call_sysrq_srcu);
 
 	return retval;
 }

Re: new warning on sysrq kernel crash trigger

[Paul E. McKenney]

On Fri, Dec 11, 2015 at 04:16:37PM -0800, Ani Sinha wrote:
> On Fri, 11 Dec 2015, Paul E. McKenney wrote:
> > On Fri, Dec 11, 2015 at 05:10:43PM -0500, Rik van Riel wrote:
> > > On 12/11/2015 03:44 PM, Ani Sinha wrote:
> > > > 
> > > > 
> > > > On Thu, 10 Dec 2015, Paul E. McKenney wrote:
> > > > 
> > > >> On Thu, Dec 10, 2015 at 03:57:09PM -0800, Ani Sinha wrote:
> > > >>> Hi guys
> > > >>>
> > > >>> I am noticing a new warning in linux 3.18 which we did not see before
> > > >>> in linux 3.4 :
> > > >>>
> > > >>> bash-4.1# echo c > /proc/sysrq-trigger
> > > >>> [  978.807185] BUG: sleeping function called from invalid context at
> > > >>> ../arch/x86/mm/fault.c:1187
> > > >>> [  978.909816] in_atomic(): 0, irqs_disabled(): 0, pid: 4706, name: bash
> > > >>> [  978.987358] Preemption disabled at:[<ffffffff81484339>] printk+0x48/0x4a
> > > >>>
> > > >>>
> > > >>> I have bisected this to the following change :
> > > >>>
> > > >>> commit 984d74a72076a12b400339973e8c98fd2fcd90e5
> > > >>> Author: Rik van Riel <riel@redhat.com>
> > > >>> Date:   Fri Jun 6 14:38:13 2014 -0700
> > > >>>
> > > >>>     sysrq: rcu-ify __handle_sysrq
> > > >>>
> > > >>>
> > > >>> the rcu_read_lock() in handle_sysrq() bumps up
> > > >>> current->rcu_read_lock_nesting. Hence, in __do_page_fault() when it
> > > >>> calls might_sleep() in x86/mm/fault.c line 1191,
> > > >>> preempt_count_equals(0) returns false and hence the warning is
> > > >>> printed.
> > > >>>
> > > >>> One way to handle this would be to do something like this:
> > > >>>
> > > >>> diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
> > > >>> index eef44d9..d4dbe22 100644
> > > >>> --- a/arch/x86/mm/fault.c
> > > >>> +++ b/arch/x86/mm/fault.c
> > > >>> @@ -1132,7 +1132,7 @@ __do_page_fault(struct pt_regs *regs, unsigned
> > > >>> long error_code,
> > > >>>   * If we're in an interrupt, have no user context or are running
> > > >>>   * in a region with pagefaults disabled then we must not take the fault
> > > >>>   */
> > > >>> - if (unlikely(faulthandler_disabled() || !mm)) {
> > > >>> + if (unlikely(faulthandler_disabled() || rcu_preempt_depth() || !mm)) {
> > > >>
> > > >> This works if CONFIG_PREEMPT=y, but if CONFIG_PREEMPT=n, then
> > > >> rcu_preempt_depth() unconditionally returns zero.  And if
> > > >> CONFIG_PREEMPT_COUNT=y && CONFIG_PREEMPT=n, you would still see
> > > >> the might_sleep() splat.
> > > >>
> > > >> Maybe use SRCU instead of RCU for this purpose?
> > > >>
> > > > 
> > > > From ae232ce3fb167b2ad363bfac7aab69001bc55a50 Mon Sep 17 00:00:00 2001
> > > > From: Ani Sinha <ani@arista.com>
> > > > Date: Fri, 11 Dec 2015 12:07:42 -0800
> > > > Subject: [PATCH 1/1] Fix 'sleeping function called from invalid context'
> > > >  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, replace RCU call in handle_sysrq() to use SRCU.
> > > 
> > > The sysrq code can be called from irq context.
> > > 
> > > Trying to use SRCU from an irq context sounds like it could
> > > be a bad idea, though admittedly I do not know enough about
> > > SRCU to know for sure :)
> > 
> > Indeed, not the best idea!  ;-)
> > 
> > I could imagine something like this:
> > 
> > 	if (in_irq())
> > 		rcu_read_lock();
> > 	else
> > 		idx = srcu_read_lock(&sysrq_rcu);
> > 
> > And ditto for unlock.  Then, for the update:
> > 
> > 	synchronize_rcu_mult(call_rcu, call_sysrq_srcu);
> > 
> > Where:
> > 
> > 	static void call_sysrq_srcu(struct rcu_head *head, rcu_callback_t func)
> > 	{
> > 		call_srcu(&sysrq_rcu, head, func);
> > 	}
> > 
> 
> >From ae232ce3fb167b2ad363bfac7aab69001bc55a50 Mon Sep 17 00:00:00 2001
> From: Ani Sinha <ani@arista.com>
> Date: Fri, 11 Dec 2015 12:07:42 -0800
> Subject: [PATCH 1/1] Fix 'sleeping function called from invalid context'
>  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, replace RCU call in handle_sysrq() to use SRCU
> in non-irq context.
> 
> Tested this patch on linux 3.18 by booting off one of our boards.
> 
> Fixes: 984d74a72076a1 ("sysrq: rcu-ify __handle_sysrq")
> 
> Signed-off-by: Ani Sinha <ani@arista.com>

>From an RCU perspective:

Acked-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

But I must defer to Rik from an sysrq perspective.

							Thanx, Paul

> ---
> diff --git a/drivers/tty/sysrq.c b/drivers/tty/sysrq.c
> index 5381a72..df7d747 100644
> --- a/drivers/tty/sysrq.c
> +++ b/drivers/tty/sysrq.c
> @@ -54,6 +54,7 @@
>  /* Whether we react on sysrq keys or just ignore them */
>  static int __read_mostly sysrq_enabled = CONFIG_MAGIC_SYSRQ_DEFAULT_ENABLE;
>  static bool __read_mostly sysrq_always_enabled;
> +DEFINE_STATIC_SRCU(sysrq_rcu);
> 
>  static bool sysrq_on(void)
>  {
> @@ -519,10 +520,13 @@ void __handle_sysrq(int key, bool check_mask)
>  {
>  	struct sysrq_key_op *op_p;
>  	int orig_log_level;
> -	int i;
> +	int i, idx;
> 
>  	rcu_sysrq_start();
> -	rcu_read_lock();
> +	if (in_irq())
> +		rcu_read_lock();
> +	else
> +		idx = srcu_read_lock(&sysrq_rcu);
>  	/*
>  	 * Raise the apparent loglevel to maximum so that the sysrq header
>  	 * is shown to provide the user with positive feedback.  We do not
> @@ -564,7 +568,10 @@ void __handle_sysrq(int key, bool check_mask)
>  		pr_cont("\n");
>  		console_loglevel = orig_log_level;
>  	}
> -	rcu_read_unlock();
> +	if (in_irq())
> +		rcu_read_unlock();
> +	else
> +		srcu_read_unlock(&sysrq_rcu, idx);
>  	rcu_sysrq_end();
>  }
> 
> @@ -1040,6 +1047,11 @@ int sysrq_toggle_support(int enable_mask)
>  	return 0;
>  }
> 
> +static void call_sysrq_srcu(struct rcu_head *head, rcu_callback_t func)
> +{
> +	call_srcu(&sysrq_rcu, head, func);
> +}
> +
>  static int __sysrq_swap_key_ops(int key, struct sysrq_key_op *insert_op_p,
>                                  struct sysrq_key_op *remove_op_p)
>  {
> @@ -1059,7 +1071,7 @@ static int __sysrq_swap_key_ops(int key, struct sysrq_key_op *insert_op_p,
>  	 * Wait for it to go away before returning, so the code for an old
>  	 * op is not freed (eg. on module unload) while it is in use.
>  	 */
> -	synchronize_rcu();
> +	synchronize_rcu_mult(call_rcu, call_sysrq_srcu);
> 
>  	return retval;
>  }
> 

Re: new warning on sysrq kernel crash trigger

[Ani Sinha]

Rik, any comments?

On Sat, Dec 12, 2015 at 6:33 AM, Paul E. McKenney
<paulmck@linux.vnet.ibm.com> wrote:
> On Fri, Dec 11, 2015 at 04:16:37PM -0800, Ani Sinha wrote:
>> On Fri, 11 Dec 2015, Paul E. McKenney wrote:
>> > On Fri, Dec 11, 2015 at 05:10:43PM -0500, Rik van Riel wrote:
>> > > On 12/11/2015 03:44 PM, Ani Sinha wrote:
>> > > >
>> > > >
>> > > > On Thu, 10 Dec 2015, Paul E. McKenney wrote:
>> > > >
>> > > >> On Thu, Dec 10, 2015 at 03:57:09PM -0800, Ani Sinha wrote:
>> > > >>> Hi guys
>> > > >>>
>> > > >>> I am noticing a new warning in linux 3.18 which we did not see before
>> > > >>> in linux 3.4 :
>> > > >>>
>> > > >>> bash-4.1# echo c > /proc/sysrq-trigger
>> > > >>> [  978.807185] BUG: sleeping function called from invalid context at
>> > > >>> ../arch/x86/mm/fault.c:1187
>> > > >>> [  978.909816] in_atomic(): 0, irqs_disabled(): 0, pid: 4706, name: bash
>> > > >>> [  978.987358] Preemption disabled at:[<ffffffff81484339>] printk+0x48/0x4a
>> > > >>>
>> > > >>>
>> > > >>> I have bisected this to the following change :
>> > > >>>
>> > > >>> commit 984d74a72076a12b400339973e8c98fd2fcd90e5
>> > > >>> Author: Rik van Riel <riel@redhat.com>
>> > > >>> Date:   Fri Jun 6 14:38:13 2014 -0700
>> > > >>>
>> > > >>>     sysrq: rcu-ify __handle_sysrq
>> > > >>>
>> > > >>>
>> > > >>> the rcu_read_lock() in handle_sysrq() bumps up
>> > > >>> current->rcu_read_lock_nesting. Hence, in __do_page_fault() when it
>> > > >>> calls might_sleep() in x86/mm/fault.c line 1191,
>> > > >>> preempt_count_equals(0) returns false and hence the warning is
>> > > >>> printed.
>> > > >>>
>> > > >>> One way to handle this would be to do something like this:
>> > > >>>
>> > > >>> diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
>> > > >>> index eef44d9..d4dbe22 100644
>> > > >>> --- a/arch/x86/mm/fault.c
>> > > >>> +++ b/arch/x86/mm/fault.c
>> > > >>> @@ -1132,7 +1132,7 @@ __do_page_fault(struct pt_regs *regs, unsigned
>> > > >>> long error_code,
>> > > >>>   * If we're in an interrupt, have no user context or are running
>> > > >>>   * in a region with pagefaults disabled then we must not take the fault
>> > > >>>   */
>> > > >>> - if (unlikely(faulthandler_disabled() || !mm)) {
>> > > >>> + if (unlikely(faulthandler_disabled() || rcu_preempt_depth() || !mm)) {
>> > > >>
>> > > >> This works if CONFIG_PREEMPT=y, but if CONFIG_PREEMPT=n, then
>> > > >> rcu_preempt_depth() unconditionally returns zero.  And if
>> > > >> CONFIG_PREEMPT_COUNT=y && CONFIG_PREEMPT=n, you would still see
>> > > >> the might_sleep() splat.
>> > > >>
>> > > >> Maybe use SRCU instead of RCU for this purpose?
>> > > >>
>> > > >
>> > > > From ae232ce3fb167b2ad363bfac7aab69001bc55a50 Mon Sep 17 00:00:00 2001
>> > > > From: Ani Sinha <ani@arista.com>
>> > > > Date: Fri, 11 Dec 2015 12:07:42 -0800
>> > > > Subject: [PATCH 1/1] Fix 'sleeping function called from invalid context'
>> > > >  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, replace RCU call in handle_sysrq() to use SRCU.
>> > >
>> > > The sysrq code can be called from irq context.
>> > >
>> > > Trying to use SRCU from an irq context sounds like it could
>> > > be a bad idea, though admittedly I do not know enough about
>> > > SRCU to know for sure :)
>> >
>> > Indeed, not the best idea!  ;-)
>> >
>> > I could imagine something like this:
>> >
>> >     if (in_irq())
>> >             rcu_read_lock();
>> >     else
>> >             idx = srcu_read_lock(&sysrq_rcu);
>> >
>> > And ditto for unlock.  Then, for the update:
>> >
>> >     synchronize_rcu_mult(call_rcu, call_sysrq_srcu);
>> >
>> > Where:
>> >
>> >     static void call_sysrq_srcu(struct rcu_head *head, rcu_callback_t func)
>> >     {
>> >             call_srcu(&sysrq_rcu, head, func);
>> >     }
>> >
>>
>> >From ae232ce3fb167b2ad363bfac7aab69001bc55a50 Mon Sep 17 00:00:00 2001
>> From: Ani Sinha <ani@arista.com>
>> Date: Fri, 11 Dec 2015 12:07:42 -0800
>> Subject: [PATCH 1/1] Fix 'sleeping function called from invalid context'
>>  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, replace RCU call in handle_sysrq() to use SRCU
>> in non-irq context.
>>
>> Tested this patch on linux 3.18 by booting off one of our boards.
>>
>> Fixes: 984d74a72076a1 ("sysrq: rcu-ify __handle_sysrq")
>>
>> Signed-off-by: Ani Sinha <ani@arista.com>
>
> From an RCU perspective:
>
> Acked-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
>
> But I must defer to Rik from an sysrq perspective.
>
>                                                         Thanx, Paul
>
>> ---
>> diff --git a/drivers/tty/sysrq.c b/drivers/tty/sysrq.c
>> index 5381a72..df7d747 100644
>> --- a/drivers/tty/sysrq.c
>> +++ b/drivers/tty/sysrq.c
>> @@ -54,6 +54,7 @@
>>  /* Whether we react on sysrq keys or just ignore them */
>>  static int __read_mostly sysrq_enabled = CONFIG_MAGIC_SYSRQ_DEFAULT_ENABLE;
>>  static bool __read_mostly sysrq_always_enabled;
>> +DEFINE_STATIC_SRCU(sysrq_rcu);
>>
>>  static bool sysrq_on(void)
>>  {
>> @@ -519,10 +520,13 @@ void __handle_sysrq(int key, bool check_mask)
>>  {
>>       struct sysrq_key_op *op_p;
>>       int orig_log_level;
>> -     int i;
>> +     int i, idx;
>>
>>       rcu_sysrq_start();
>> -     rcu_read_lock();
>> +     if (in_irq())
>> +             rcu_read_lock();
>> +     else
>> +             idx = srcu_read_lock(&sysrq_rcu);
>>       /*
>>        * Raise the apparent loglevel to maximum so that the sysrq header
>>        * is shown to provide the user with positive feedback.  We do not
>> @@ -564,7 +568,10 @@ void __handle_sysrq(int key, bool check_mask)
>>               pr_cont("\n");
>>               console_loglevel = orig_log_level;
>>       }
>> -     rcu_read_unlock();
>> +     if (in_irq())
>> +             rcu_read_unlock();
>> +     else
>> +             srcu_read_unlock(&sysrq_rcu, idx);
>>       rcu_sysrq_end();
>>  }
>>
>> @@ -1040,6 +1047,11 @@ int sysrq_toggle_support(int enable_mask)
>>       return 0;
>>  }
>>
>> +static void call_sysrq_srcu(struct rcu_head *head, rcu_callback_t func)
>> +{
>> +     call_srcu(&sysrq_rcu, head, func);
>> +}
>> +
>>  static int __sysrq_swap_key_ops(int key, struct sysrq_key_op *insert_op_p,
>>                                  struct sysrq_key_op *remove_op_p)
>>  {
>> @@ -1059,7 +1071,7 @@ static int __sysrq_swap_key_ops(int key, struct sysrq_key_op *insert_op_p,
>>        * Wait for it to go away before returning, so the code for an old
>>        * op is not freed (eg. on module unload) while it is in use.
>>        */
>> -     synchronize_rcu();
>> +     synchronize_rcu_mult(call_rcu, call_sysrq_srcu);
>>
>>       return retval;
>>  }
>>
>

Re: new warning on sysrq kernel crash trigger

[Rik van Riel]

On 12/14/2015 11:24 AM, Ani Sinha wrote:
> Rik, any comments?

Another good option is to simply ignore this warning, or drop
the rcu_read_lock before doing the alt-syrsq-c action.

After all, alt-sysrq-c is "crash the system, take a crash dump",
which is not an action the system ever returns from.

static struct sysrq_key_op sysrq_crash_op = {
        .handler        = sysrq_handle_crash,
        .help_msg       = "crash(c)",
        .action_msg     = "Trigger a crash",
        .enable_mask    = SYSRQ_ENABLE_DUMP,
};

-- 
All rights reversed

Re: new warning on sysrq kernel crash trigger

[Anirban Sinha]

On Mon, 14 Dec 2015, Rik van Riel wrote:

> On 12/14/2015 11:24 AM, Ani Sinha wrote:
> > Rik, any comments?
> 
> Another good option is to simply ignore this warning, or drop
> the rcu_read_lock before doing the alt-syrsq-c action.
> 
> After all, alt-sysrq-c is "crash the system, take a crash dump",
> which is not an action the system ever returns from.
> 

Yea I thought about this idea previously but then discarded it thinking it 
would be too hacky. Here's the cooked up patch. I hope this can be 
approved for mainline soon (I'm on vacation and working just on this issue 
remotely) :

>From 105ff3ffce380650b3d58b3594a9be47bd604b28 Mon Sep 17 00:00:00 2001
From: Ani Sinha <ani@arista.com>
Date: Mon, 14 Dec 2015 14:55:08 -0800
Subject: [PATCH 1/1] Fix 'sleeping function called from invalid context' 
 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.

Fixes: 984d74a72076a1 ("sysrq: rcu-ify __handle_sysrq")

Signed-off-by: Ani Sinha <ani@arista.com>
---
 drivers/tty/sysrq.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/tty/sysrq.c b/drivers/tty/sysrq.c
index 5381a72..08987ad 100644
--- a/drivers/tty/sysrq.c
+++ b/drivers/tty/sysrq.c
@@ -133,6 +133,12 @@ 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.
+	*/
+	rcu_read_unlock();
 	panic_on_oops = 1;	/* force panic */
 	wmb();
 	*killer = 1;
-- 
1.8.1.4

Re: new warning on sysrq kernel crash trigger

[Ani Sinha]

Rik, should I send a separate email with the patch or you are OK with
what I sent in the email? Are you queueing up my patch for applying
upstream?

On Tue, Dec 15, 2015 at 5:44 AM, Anirban Sinha <ani@arista.com> wrote:
>
>
> On Mon, 14 Dec 2015, Rik van Riel wrote:
>
>> On 12/14/2015 11:24 AM, Ani Sinha wrote:
>> > Rik, any comments?
>>
>> Another good option is to simply ignore this warning, or drop
>> the rcu_read_lock before doing the alt-syrsq-c action.
>>
>> After all, alt-sysrq-c is "crash the system, take a crash dump",
>> which is not an action the system ever returns from.
>>
>
> Yea I thought about this idea previously but then discarded it thinking it
> would be too hacky. Here's the cooked up patch. I hope this can be
> approved for mainline soon (I'm on vacation and working just on this issue
> remotely) :
>
> From 105ff3ffce380650b3d58b3594a9be47bd604b28 Mon Sep 17 00:00:00 2001
> From: Ani Sinha <ani@arista.com>
> Date: Mon, 14 Dec 2015 14:55:08 -0800
> Subject: [PATCH 1/1] Fix 'sleeping function called from invalid context'
>  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.
>
> Fixes: 984d74a72076a1 ("sysrq: rcu-ify __handle_sysrq")
>
> Signed-off-by: Ani Sinha <ani@arista.com>
> ---
>  drivers/tty/sysrq.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/drivers/tty/sysrq.c b/drivers/tty/sysrq.c
> index 5381a72..08987ad 100644
> --- a/drivers/tty/sysrq.c
> +++ b/drivers/tty/sysrq.c
> @@ -133,6 +133,12 @@ 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.
> +       */
> +       rcu_read_unlock();
>         panic_on_oops = 1;      /* force panic */
>         wmb();
>         *killer = 1;
> --
> 1.8.1.4
>

Re: new warning on sysrq kernel crash trigger

[Rik van Riel]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

On 12/14/2015 07:14 PM, Anirban Sinha wrote:
> 
> 
> On Mon, 14 Dec 2015, Rik van Riel wrote:
> 
>> On 12/14/2015 11:24 AM, Ani Sinha wrote:
>>> Rik, any comments?
>> 
>> Another good option is to simply ignore this warning, or drop the
>> rcu_read_lock before doing the alt-syrsq-c action.
>> 
>> After all, alt-sysrq-c is "crash the system, take a crash dump", 
>> which is not an action the system ever returns from.
>> 
> 
> Yea I thought about this idea previously but then discarded it
> thinking it would be too hacky. Here's the cooked up patch. I hope
> this can be approved for mainline soon (I'm on vacation and working
> just on this issue remotely) :
> 
> From 105ff3ffce380650b3d58b3594a9be47bd604b28 Mon Sep 17 00:00:00
> 2001 From: Ani Sinha <ani@arista.com> Date: Mon, 14 Dec 2015
> 14:55:08 -0800 Subject: [PATCH 1/1] Fix 'sleeping function called
> from invalid context' 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.
> 
> Fixes: 984d74a72076a1 ("sysrq: rcu-ify __handle_sysrq")
> 
> Signed-off-by: Ani Sinha <ani@arista.com>

Reviewed-by: Rik van Riel <riel@redhat.com>

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQEcBAEBCAAGBQJWcY++AAoJEM553pKExN6DMsEIAIgRI2dlnimHDR30BWhAhj1m
rPlG3zEKsilR5/MjD3y/LZqIqG2PmMEpIGajeTOu5O9cZhIyon/6snHTST36kN2Y
2CMCdUYNTQtDLpg8RoFsu8cvL4gBdi4J+o/U4E8gFXn6MqNsk3U0Dow/BJl1dPAm
V2/aN2K6od3+HU0q3ZJGfcnc4SipkAnA3nmrh5OntXLtZBfye6ge7UONxLzBI2vR
+7sGTd3ebKd9AZlYevZQxnSaeJbikGJoCwreqMVTueX8fbhvvReo/f6OfnXF6HaF
vDK6lle/BFuHYb11/cWonSuKcphpAOfvX+n90BtbBMedUKNlGvLBBH55feIbOpw=
=BP+x
-----END PGP SIGNATURE-----

Re: new warning on sysrq kernel crash trigger

[Rik van Riel]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

On 12/15/2015 07:52 PM, Ani Sinha wrote:
> Rik, should I send a separate email with the patch or you are OK
> with what I sent in the email? Are you queueing up my patch for
> applying upstream?

I don't have a git tree for people to pull from, and
it looks like the tty & sysrq maintainers are Greg KH
and Jiri Slaby.

Greg, Jiri, where do you prefer Ani sends the patch
for inclusion, or should it go in through Paul's tree?
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQEcBAEBCAAGBQJWcZBxAAoJEM553pKExN6DsFAH/jl6UdCd4vj6ovzCHDr9lWZL
C/I0DwCDRx5VxvyyiiiQz49yWjSSZue7ZAeis42YoJ89apHh3jwYGqUc8WrHz0j1
DVwPMk6DjiInTK2dIsyVVeMxCSr6wk6NDvC8/KwownBK9OvcI20bEfBLdjRUj4Y0
ySe92VStk3n9GIez9M2XAfPV9ADWcUbN6KNkqbKYf9h0qgl3h+9ZhvsiQHPOEdnG
+dsD/FVwnVYDQOdwWroZHi0UmorHS6gQbEHHO851xIkKIztMGY00CnvJOehdJWW8
BNc1pTAUpWiPvDddzjhmGdwEx5kSp/y3JwLu5BJTfVuNHc2Ss9KdZCYNmbpggHk=
=dSvj
-----END PGP SIGNATURE-----

Re: new warning on sysrq kernel crash trigger

[Greg KH]

On Wed, Dec 16, 2015 at 11:25:21AM -0500, Rik van Riel wrote:
> On 12/15/2015 07:52 PM, Ani Sinha wrote:
> > Rik, should I send a separate email with the patch or you are OK
> > with what I sent in the email? Are you queueing up my patch for
> > applying upstream?
> 
> I don't have a git tree for people to pull from, and
> it looks like the tty & sysrq maintainers are Greg KH
> and Jiri Slaby.
> 
> Greg, Jiri, where do you prefer Ani sends the patch
> for inclusion, or should it go in through Paul's tree?

I don't care which, either is fine for me.

Re: new warning on sysrq kernel crash trigger

[Ani Sinha]

On Thu, Dec 17, 2015 at 9:28 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Wed, Dec 16, 2015 at 11:25:21AM -0500, Rik van Riel wrote:
>> On 12/15/2015 07:52 PM, Ani Sinha wrote:
>> > Rik, should I send a separate email with the patch or you are OK
>> > with what I sent in the email? Are you queueing up my patch for
>> > applying upstream?
>>
>> I don't have a git tree for people to pull from, and
>> it looks like the tty & sysrq maintainers are Greg KH
>> and Jiri Slaby.
>>
>> Greg, Jiri, where do you prefer Ani sends the patch
>> for inclusion, or should it go in through Paul's tree?
>
> I don't care which, either is fine for me.

I have sent just the patch to the relevant folks again with CC
linux-kernel@. Hope this is now all set for pulling upstream. Let me
know if I need to do anything else.

thanks
ani