| Message ID | 20180212085956.12016-1-vaibhav@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | powerpc/xmon: Dont register sysrq key when kernel param xmon=off | expand |
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.
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.
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.
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
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> 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
Thanks for the feedback Mpe, I have sent-out a new patch incorporating your review comments at http://patchwork.ozlabs.org/patch/877792/
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 */
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(-)