[PATCH] powerpc/xmon: Dont register sysrq key when kernel param xmon=off

[PATCH] powerpc/xmon: Dont register sysrq key when kernel param xmon=off

[Vaibhav Jain]

Presently sysrq key for xmon('x') is registered during kernel init
irrespective of the value of kernel param 'xmon'. Thus xmon is enabled
even if 'xmon=off' is passed on the kernel command line.

This minor patch updates setup_xmon_sysrq() to register
'sysrq_xmon_op' only when variable 'xmon_on' is set.

Signed-off-by: Vaibhav Jain <vaibhav@linux.vnet.ibm.com>
---
 arch/powerpc/xmon/xmon.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
index 82e1a3ee6e0f..3b995474b102 100644
--- a/arch/powerpc/xmon/xmon.c
+++ b/arch/powerpc/xmon/xmon.c
@@ -3642,8 +3642,7 @@ static struct sysrq_key_op sysrq_xmon_op = {
 
 static int __init setup_xmon_sysrq(void)
 {
-	register_sysrq_key('x', &sysrq_xmon_op);
-	return 0;
+	return xmon_on ? register_sysrq_key('x', &sysrq_xmon_op) : 0;
 }
 device_initcall(setup_xmon_sysrq);
 #endif /* CONFIG_MAGIC_SYSRQ */
-- 
2.14.3

Re: [PATCH] powerpc/xmon: Dont register sysrq key when kernel param xmon=off

[Balbir Singh]

On Mon, Feb 12, 2018 at 7:59 PM, Vaibhav Jain
<vaibhav@linux.vnet.ibm.com> wrote:
> Presently sysrq key for xmon('x') is registered during kernel init
> irrespective of the value of kernel param 'xmon'. Thus xmon is enabled
> even if 'xmon=off' is passed on the kernel command line.
>
> This minor patch updates setup_xmon_sysrq() to register
> 'sysrq_xmon_op' only when variable 'xmon_on' is set.
>
> Signed-off-by: Vaibhav Jain <vaibhav@linux.vnet.ibm.com>
> ---

Any specific issue you've run into without this patch? I presume
running xmon=off indicates we don't want xmon to take over in case of
panic/die/oops, why are we tying this to sysrq?

Balbir Singh.

Re: [PATCH] powerpc/xmon: Dont register sysrq key when kernel param xmon=off

[Vaibhav Jain]

Thanks for reviewing this patch Balbir

Balbir Singh <bsingharora@gmail.com> writes:

> Any specific issue you've run into without this patch? 
Without this patch since xmon is still accessible via sysrq and there is
no indication/warning on the xmon console mentioning that its is not
fully functional. Specifically xmon-console would still allow user to
set instruction/data breakpoint eventhough they wont work and will
result in a kernel-oops.

Below is command log illustrating this problem on one of my test system
where I tried setting an instruction breakpoint on cmdline_proc_show()
with xmon=off:

~# cat /proc/cmdline 
root=UUID=248ad10e-a272-4187-8672-5b25f701e8b9 ro xmon=off

~# echo 'x' > /proc/sysrq-trigger                                                                                                                                     
[  458.904802] sysrq: SysRq : Entering xmon

[ snip ]

78:mon> ls cmdline_proc_show
cmdline_proc_show: c0000000004196e0
78:mon> bi c0000000004196e0
78:mon> x

~# cat /proc/cmdline
[  505.618702] Oops: Exception in kernel mode, sig: 5 [#1]
[ snip ]
[  505.620082] NIP [c0000000004196e4] cmdline_proc_show+0x4/0x60
[  505.620136] LR [c0000000003b1db0] seq_read+0x130/0x5e0
[  505.620177] Call Trace:
[  505.620202] [c000200e5078fc00] [c0000000003b1d74] seq_read+0xf4/0x5e0 (unreliable)
[  505.620267] [c000200e5078fca0] [c00000000040cae0] proc_reg_read+0xb0/0x110
[  505.620322] [c000200e5078fcf0] [c00000000037687c] __vfs_read+0x6c/0x1b0
[  505.620376] [c000200e5078fd90] [c000000000376a7c] vfs_read+0xbc/0x1b0
[  505.620430] [c000200e5078fde0] [c00000000037724c] SyS_read+0x6c/0x110
[  505.620485] [c000200e5078fe30] [c00000000000b320] system_call+0x58/0x6c
[  505.620536] Instruction dump:
[  505.620570] 3c82ff2a 7fe3fb78 38a00000 3884dee0 4bf98c05 60000000 38210030 e8010010 
[  505.620656] ebe1fff8 7c0803a6 4e800020 3c4c00d6 <38422120> 7c0802a6 f8010010 f821ff91 
[  505.620728] ---[ end trace eaf583921860b3de ]---
[  506.629019] 
Trace/breakpoint trap
~#


> I presume running xmon=off indicates we don't want xmon to take over in case of
> panic/die/oops, 
I believe that when xmon console is available it should be fully
functional rather than partially, otherwise it gets really confusing to
the user as to why Instruction/Data break points arent working.

> why are we tying this to sysrq?
With xmon=off sysrq seems to be the only way to enter xmon console.

-- 
Vaibhav Jain <vaibhav@linux.vnet.ibm.com>
Linux Technology Center, IBM India Pvt. Ltd.

Re: [PATCH] powerpc/xmon: Dont register sysrq key when kernel param xmon=off

[Balbir Singh]

On Mon, Feb 12, 2018 at 11:35 PM, Vaibhav Jain
<vaibhav@linux.vnet.ibm.com> wrote:
> Thanks for reviewing this patch Balbir
>
> Balbir Singh <bsingharora@gmail.com> writes:
>
>> Any specific issue you've run into without this patch?
> Without this patch since xmon is still accessible via sysrq and there is
> no indication/warning on the xmon console mentioning that its is not
> fully functional. Specifically xmon-console would still allow user to
> set instruction/data breakpoint eventhough they wont work and will
> result in a kernel-oops.
>
> Below is command log illustrating this problem on one of my test system
> where I tried setting an instruction breakpoint on cmdline_proc_show()
> with xmon=off:
>
> ~# cat /proc/cmdline
> root=UUID=248ad10e-a272-4187-8672-5b25f701e8b9 ro xmon=off
>
> ~# echo 'x' > /proc/sysrq-trigger
> [  458.904802] sysrq: SysRq : Entering xmon
>
> [ snip ]
>
> 78:mon> ls cmdline_proc_show
> cmdline_proc_show: c0000000004196e0
> 78:mon> bi c0000000004196e0
> 78:mon> x
>
> ~# cat /proc/cmdline
> [  505.618702] Oops: Exception in kernel mode, sig: 5 [#1]
> [ snip ]
> [  505.620082] NIP [c0000000004196e4] cmdline_proc_show+0x4/0x60
> [  505.620136] LR [c0000000003b1db0] seq_read+0x130/0x5e0
> [  505.620177] Call Trace:
> [  505.620202] [c000200e5078fc00] [c0000000003b1d74] seq_read+0xf4/0x5e0 (unreliable)
> [  505.620267] [c000200e5078fca0] [c00000000040cae0] proc_reg_read+0xb0/0x110
> [  505.620322] [c000200e5078fcf0] [c00000000037687c] __vfs_read+0x6c/0x1b0
> [  505.620376] [c000200e5078fd90] [c000000000376a7c] vfs_read+0xbc/0x1b0
> [  505.620430] [c000200e5078fde0] [c00000000037724c] SyS_read+0x6c/0x110
> [  505.620485] [c000200e5078fe30] [c00000000000b320] system_call+0x58/0x6c
> [  505.620536] Instruction dump:
> [  505.620570] 3c82ff2a 7fe3fb78 38a00000 3884dee0 4bf98c05 60000000 38210030 e8010010
> [  505.620656] ebe1fff8 7c0803a6 4e800020 3c4c00d6 <38422120> 7c0802a6 f8010010 f821ff91
> [  505.620728] ---[ end trace eaf583921860b3de ]---
> [  506.629019]
> Trace/breakpoint trap
> ~#
>
>
>> I presume running xmon=off indicates we don't want xmon to take over in case of
>> panic/die/oops,
> I believe that when xmon console is available it should be fully
> functional rather than partially, otherwise it gets really confusing to
> the user as to why Instruction/Data break points arent working.
>

OK, so kernel breakpoints are broken with xmon=off and lead to oops as
opposed to passing them on to a kprobe handler perhaps?

Balbir Singh.

Re: [PATCH] powerpc/xmon: Dont register sysrq key when kernel param xmon=off

[Michael Ellerman]

Vaibhav Jain <vaibhav@linux.vnet.ibm.com> writes:

> Thanks for reviewing this patch Balbir
>
> Balbir Singh <bsingharora@gmail.com> writes:
>
>> Any specific issue you've run into without this patch? 
> Without this patch since xmon is still accessible via sysrq and there is
> no indication/warning on the xmon console mentioning that its is not
> fully functional. Specifically xmon-console would still allow user to
> set instruction/data breakpoint eventhough they wont work and will
> result in a kernel-oops.
>
> Below is command log illustrating this problem on one of my test system
> where I tried setting an instruction breakpoint on cmdline_proc_show()
> with xmon=off:

<snip>

But the same crash happens with XMON_DEFAULT=n and nothing on the
command line.

The problem is not xmon=off on the command line.

The problem is that when xmon_on = false and we enter xmon via sysrq and
then set breakpoints, we need to enable xmon_on before leaving xmon.

So this is a bug introduced by:

  3b5bf42b81d5 ("powerpc/xmon: Fix an unexpected xmon on/off state change")


How to fix it is not entirely clear. In general I like the behaviour we
have since the above commit, ie. quickly dropping into xmon and
inspecting something doesn't leave xmon enabled, which then causes the
system not to kdump/reboot later.

What would be nice is if we keep that behaviour, but any action you take
in xmon that requires xmon to remain resident, ie. setting a breakpoint,
calls a function which makes sure xmon_on = true and if it wasn't prints
a nice message saying "Turning xmon on due to breakpoint insertion" or
something.

cheers

Re: [PATCH] powerpc/xmon: Dont register sysrq key when kernel param xmon=off

[Vaibhav Jain]

Thanks for looking into this patch Mpe.

Michael Ellerman <mpe@ellerman.id.au> writes:
> <snip>
>
> But the same crash happens with XMON_DEFAULT=n and nothing on the
> command line.
Yes, XMON_DEFAULT=n and empty boot command line implies xmon=off hence
you will see the same issue and this patch should fix that issue too.

>
> The problem is not xmon=off on the command line.
>
> The problem is that when xmon_on = false and we enter xmon via sysrq and
> then set breakpoints, we need to enable xmon_on before leaving xmon.
>
Agree on both the points made.

> So this is a bug introduced by:
>
>   3b5bf42b81d5 ("powerpc/xmon: Fix an unexpected xmon on/off state change")
>
>
> How to fix it is not entirely clear. In general I like the behaviour we
> have since the above commit, ie. quickly dropping into xmon and
> inspecting something doesn't leave xmon enabled, which then causes the
> system not to kdump/reboot later.
Agree on the convenience factor of leaving the xmon console
enabled. However we still need a way to disable xmon completely at
kernel-boot time. Leaving xmon enabled even if 'xmon=off' is provided at
command line is counter intuitive.

>
> What would be nice is if we keep that behaviour, but any action you take
> in xmon that requires xmon to remain resident, ie. setting a breakpoint,
> calls a function which makes sure xmon_on = true and if it wasn't prints
> a nice message saying "Turning xmon on due to breakpoint insertion" or
> something.
That makes sense to me and sounds workable. However we already have a
debugfs interface to enable/disable xmon debugger hook. I can also tweak
this interface to also register the sysrq key when xmon is enabled. This
should provide the user the ability to still use xmon if they want to
after the system has booted with xmon=off.

>
> cheers
>

-- 
Vaibhav Jain <vaibhav@linux.vnet.ibm.com>
Linux Technology Center, IBM India Pvt. Ltd.

Re: [PATCH] powerpc/xmon: Dont register sysrq key when kernel param xmon=off

[Michael Ellerman]

Vaibhav Jain <vaibhav@linux.vnet.ibm.com> writes:
> Michael Ellerman <mpe@ellerman.id.au> writes:
>> <snip>
<snip>
>>
>> What would be nice is if we keep that behaviour, but any action you take
>> in xmon that requires xmon to remain resident, ie. setting a breakpoint,
>> calls a function which makes sure xmon_on = true and if it wasn't prints
>> a nice message saying "Turning xmon on due to breakpoint insertion" or
>> something.

> That makes sense to me and sounds workable. However we already have a
> debugfs interface to enable/disable xmon debugger hook. I can also tweak
> this interface to also register the sysrq key when xmon is enabled. This
> should provide the user the ability to still use xmon if they want to
> after the system has booted with xmon=off.

I agree that sounds sensible, but it has one fatal flaw IMO.

Currently you can boot a box with XMON_DEFAULT=n, and it will crash dump
and so on, but if the box gets stuck you can jump on the console and
drop into xmon with sysrq-x.

If we additionally require xmon to be enabled via debugfs every boot
that will break the above use case, and I don't want to do that.

So in short I think I like my idea better :)

cheers

Re: [PATCH] powerpc/xmon: Dont register sysrq key when kernel param xmon=off

[Vaibhav Jain]

Thanks for the feedback Mpe,

I have sent-out a new patch incorporating your review comments at
http://patchwork.ozlabs.org/patch/877792/

-- 
Vaibhav Jain <vaibhav@linux.vnet.ibm.com>
Linux Technology Center, IBM India Pvt. Ltd.