Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1730138 > unrolled thread

[PATCH] sysrq : fix Show Regs call trace on ARM

Back to article view | Back to linux.kernel

Contents

  [PATCH] sysrq : fix Show Regs call trace on ARM Jibin Xu <jibin.xu@windriver.com> - 2017-09-11 05:20 +0200
    Re: [PATCH] sysrq : fix Show Regs call trace on ARM Jiri Slaby <jslaby@suse.cz> - 2017-09-11 07:30 +0200
      Re: [PATCH] sysrq : fix Show Regs call trace on ARM Jibin Xu <jibin.xu@windriver.com> - 2017-09-11 12:00 +0200
        Re: Re: [PATCH] sysrq : fix Show Regs call trace on ARM Jiri Slaby <jslaby@suse.cz> - 2017-09-13 08:10 +0200
          Re: [PATCH] sysrq : fix Show Regs call trace on ARM Jiri Slaby <jslaby@suse.cz> - 2017-09-13 08:20 +0200

#1730138 — [PATCH] sysrq : fix Show Regs call trace on ARM

When kernel configuration SMP,PREEMPT and DEBUG_PREEMPT are enabled,
echo 1 >/proc/sys/kernel/sysrq
echo p >/proc/sysrq-trigger
kernel will print call trace as below:

sysrq: SysRq : Show Regs
BUG: using __this_cpu_read() in preemptible [00000000] code: sh/435
caller is __this_cpu_preempt_check+0x18/0x20
Call trace:
[<ffffff8008088e80>] dump_backtrace+0x0/0x1d0
[<ffffff8008089074>] show_stack+0x24/0x30
[<ffffff8008447970>] dump_stack+0x90/0xb0
[<ffffff8008463950>] check_preemption_disabled+0x100/0x108
[<ffffff8008463998>] __this_cpu_preempt_check+0x18/0x20
[<ffffff80084c9194>] sysrq_handle_showregs+0x1c/0x40
[<ffffff80084c9c7c>] __handle_sysrq+0x12c/0x1a0
[<ffffff80084ca140>] write_sysrq_trigger+0x60/0x70
[<ffffff8008251e00>] proc_reg_write+0x90/0xd0
[<ffffff80081f1788>] __vfs_write+0x48/0x90
[<ffffff80081f241c>] vfs_write+0xa4/0x190
[<ffffff80081f3354>] SyS_write+0x54/0xb0
[<ffffff80080833f0>] el0_svc_naked+0x24/0x28

This can be seen on a common board like an r-pi3.
This happens because when echo p >/proc/sysrq-trigger,
get_irq_regs() is called outside of IRQ context,
if preemption is enabled in this situation,kernel will
print the call trace. Since many prior discussions on
the mailing lists have made it clear that get_irq_regs
either just returns NULL or stale data when used outside
of IRQ context,we simply avoid calling it outside of
IRQ context.

Signed-off-by: Jibin Xu <jibin.xu@windriver.com>
---
 drivers/tty/sysrq.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/sysrq.c b/drivers/tty/sysrq.c
index 3ffc1ce29023..6ed8c47312a8 100644
--- a/drivers/tty/sysrq.c
+++ b/drivers/tty/sysrq.c
@@ -245,8 +245,10 @@ static void sysrq_handle_showallcpus(int key)
 	 * architecture has no support for it:
 	 */
 	if (!trigger_all_cpu_backtrace()) {
-		struct pt_regs *regs = get_irq_regs();
+		struct pt_regs *regs = NULL;
 
+		if (in_irq())
+			regs = get_irq_regs();
 		if (regs) {
 			pr_info("CPU%d:\n", smp_processor_id());
 			show_regs(regs);
@@ -265,7 +267,10 @@ static struct sysrq_key_op sysrq_showallcpus_op = {
 
 static void sysrq_handle_showregs(int key)
 {
-	struct pt_regs *regs = get_irq_regs();
+	struct pt_regs *regs = NULL;
+
+	if (in_irq())
+		regs = get_irq_regs();
 	if (regs)
 		show_regs(regs);
 	perf_event_print_debug();
-- 
2.13.0

[toc] | [next] | [standalone]

#1730151

On 09/11/2017, 05:11 AM, Jibin Xu wrote:
...
> --- a/drivers/tty/sysrq.c
> +++ b/drivers/tty/sysrq.c
> @@ -245,8 +245,10 @@ static void sysrq_handle_showallcpus(int key)
>  	 * architecture has no support for it:
>  	 */
>  	if (!trigger_all_cpu_backtrace()) {
> -		struct pt_regs *regs = get_irq_regs();
> +		struct pt_regs *regs = NULL;
>  
> +		if (in_irq())
> +			regs = get_irq_regs();

Maybe a stupid question: how does get_irq_regs() behave in the softirq
context? I.e. what about s/in_irq/in_interrupt/?

thanks,
-- 
js
suse labs

[toc] | [prev] | [next] | [standalone]

#1730250

Hi,Jiri:

get_irq_regs() in the softirq context return NULL.

thanks,
Jibin Xu

On 2017年09月11日 13:24, Jiri Slaby wrote:
> On 09/11/2017, 05:11 AM, Jibin Xu wrote:
> ...
>> --- a/drivers/tty/sysrq.c
>> +++ b/drivers/tty/sysrq.c
>> @@ -245,8 +245,10 @@ static void sysrq_handle_showallcpus(int key)
>>   	 * architecture has no support for it:
>>   	 */
>>   	if (!trigger_all_cpu_backtrace()) {
>> -		struct pt_regs *regs = get_irq_regs();
>> +		struct pt_regs *regs = NULL;
>>   
>> +		if (in_irq())
>> +			regs = get_irq_regs();
> Maybe a stupid question: how does get_irq_regs() behave in the softirq
> context? I.e. what about s/in_irq/in_interrupt/?
>
> thanks,

[toc] | [prev] | [next] | [standalone]

#1731389

On 09/11/2017, 01:22 PM, Jibin Xu wrote:
> Hi,Jiri: 
> 
> I tested get_irq_regs() behave in the softirq context,
> I called get_irq_regs() by a tasklet, It returns NULL.My understanding is get_irq_regs() can return the right result
> only in hardware IRQ,otherwise it returns ＮＵＬL.
> So I think in_irq() would be better.

Hi,

tasklets are run in the process context (in a kthread). But what about
timers? HARDIRQ is decremented, SOFTIRQ remains set and sysrq handlers
are called in such conditions (in_interrupt() is true, in_irq() is
false). At that moment, irq_regs are still set and valid IMO.

But I would believe for now that sysrq handlers are not invoked from
softirq context. AFAIK they are called only from hardirq (serial port or
keyboard IRQ handler) or process context (write to /proc/sysrq-trigger).
So this change *should* be safe unless someone else objects there are
some kgdb special cases or something.

> thanks,
> Jibin Xu
> 
> On 2017年09月11日 17:55, Jibin Xu wrote:
> 
>> Hi,Jiri:
>> get_irq_regs() in the softirq context return NULL.
>> thanks,
>> Jibin Xu
>> On 2017年09月11日 13:24, Jiri Slaby wrote:
>>> On 09/11/2017, 05:11 AM, Jibin Xu wrote:
>>> ...
>>>> --- a/drivers/tty/sysrq.c
>>>> +++ b/drivers/tty/sysrq.c
>>>> @@ -245,8 +245,10 @@ static void sysrq_handle_showallcpus(int key)
>>>>        * architecture has no support for it:
>>>>        */
>>>>       if (!trigger_all_cpu_backtrace()) {
>>>> -        struct pt_regs *regs = get_irq_regs();
>>>> +        struct pt_regs *regs = NULL;
>>>>   
>>>> +        if (in_irq())
>>>> +            regs = get_irq_regs();
>>> Maybe a stupid question: how does get_irq_regs() behave in the softirq
>>> context? I.e. what about s/in_irq/in_interrupt/?
>>> thanks,
>>
> 


-- 
js
suse labs

[toc] | [prev] | [next] | [standalone]

#1731391

On 09/13/2017, 08:04 AM, Jiri Slaby wrote:
> On 09/11/2017, 01:22 PM, Jibin Xu wrote:
>> Hi,Jiri: 
>>
>> I tested get_irq_regs() behave in the softirq context,
>> I called get_irq_regs() by a tasklet, It returns NULL.My understanding is get_irq_regs() can return the right result
>> only in hardware IRQ,otherwise it returns ＮＵＬL.
>> So I think in_irq() would be better.
> 
> Hi,
> 
> tasklets are run in the process context (in a kthread). But what about
> timers? HARDIRQ is decremented, SOFTIRQ remains set and sysrq handlers
> are called in such conditions (in_interrupt() is true, in_irq() is
> false). At that moment, irq_regs are still set and valid IMO.
> 
> But I would believe for now that sysrq handlers are not invoked from
> softirq context. AFAIK they are called only from hardirq (serial port or
> keyboard IRQ handler) or process context (write to /proc/sysrq-trigger).
> So this change *should* be safe unless someone else objects there are
> some kgdb special cases or something.

Oops, serial8250_timeout actually can do so. So yes, sysrq handlers can
be invoked from softirq (timer) context and they still should print the
registers at best.

I have to leave now, will look into it later if someone (you :)) doesn't
beat me to it.

thanks,
-- 
js
suse labs

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web