| Message ID | 20250211125445.451805-1-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [for-4.20,v2] ARM32/traps: Fix do_trap_undefined_instruction()'s detection of kernel text | expand |
Hi Andrew, On 11/02/2025 12:54, Andrew Cooper wrote: > While fixing some common/arch boundaries for UBSAN support on other > architectures, the following debugging patch: > > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c > index c1f2d1b89d43..58d1d048d339 100644 > --- a/xen/arch/arm/setup.c > +++ b/xen/arch/arm/setup.c > @@ -504,6 +504,8 @@ void asmlinkage __init start_xen(unsigned long fdt_paddr) > > system_state = SYS_STATE_active; > > + dump_execution_state(); > + > for_each_domain( d ) > domain_unpause_by_systemcontroller(d); > > failed with: > > (XEN) *** Serial input to DOM0 (type 'CTRL-a' three times to switch input) > (XEN) CPU0: Unexpected Trap: Undefined Instruction > (XEN) ----[ Xen-4.20-rc arm32 debug=n Not tainted ]---- > (XEN) CPU: 0 > <snip> > (XEN) > (XEN) **************************************** > (XEN) Panic on CPU 0: > (XEN) CPU0: Unexpected Trap: Undefined Instruction > (XEN) **************************************** > > This is because the condition for init text is wrong. While there's nothing > interesting from that point onwards in start_xen(), it's also wrong for > livepatches too. > > Use is_active_kernel_text() which is the correct test for this purpose, and is > aware of init and livepatch regions as well as their lifetimes. > > Fixes: 3e802c6ca1fb ("xen/arm: Correctly support WARN_ON") > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Reviewed-by: Julien Grall <jgrall@amazon.com> Cheers,
On 2/11/25 1:54 PM, Andrew Cooper wrote: > While fixing some common/arch boundaries for UBSAN support on other > architectures, the following debugging patch: > > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c > index c1f2d1b89d43..58d1d048d339 100644 > --- a/xen/arch/arm/setup.c > +++ b/xen/arch/arm/setup.c > @@ -504,6 +504,8 @@ void asmlinkage __init start_xen(unsigned long fdt_paddr) > > system_state = SYS_STATE_active; > > + dump_execution_state(); > + > for_each_domain( d ) > domain_unpause_by_systemcontroller(d); > > failed with: > > (XEN) *** Serial input to DOM0 (type 'CTRL-a' three times to switch input) > (XEN) CPU0: Unexpected Trap: Undefined Instruction > (XEN) ----[ Xen-4.20-rc arm32 debug=n Not tainted ]---- > (XEN) CPU: 0 > <snip> > (XEN) > (XEN) **************************************** > (XEN) Panic on CPU 0: > (XEN) CPU0: Unexpected Trap: Undefined Instruction > (XEN) **************************************** > > This is because the condition for init text is wrong. While there's nothing > interesting from that point onwards in start_xen(), it's also wrong for > livepatches too. > > Use is_active_kernel_text() which is the correct test for this purpose, and is > aware of init and livepatch regions as well as their lifetimes. > > Fixes: 3e802c6ca1fb ("xen/arm: Correctly support WARN_ON") > Signed-off-by: Andrew Cooper<andrew.cooper3@citrix.com> Release-Acked-by:Oleksii Kurochko <oleksii.kurochko@gmail.com> ~ Oleksii > --- > CC: Stefano Stabellini<sstabellini@kernel.org> > CC: Julien Grall<julien@xen.org> > CC: Volodymyr Babchuk<Volodymyr_Babchuk@epam.com> > CC: Bertrand Marquis<bertrand.marquis@arm.com> > CC: Michal Orzel<michal.orzel@amd.com> > CC: Oleksii Kurochko<oleksii.kurochko@gmail.com> > > v2: > * Split out change to dump_execution_state() > > Sample run going wrong: > https://gitlab.com/xen-project/people/andyhhp/xen/-/jobs/9078570105 > > Sample run with dump_execution_state() working: > https://gitlab.com/xen-project/people/andyhhp/xen/-/jobs/9079185111 > --- > xen/arch/arm/arm32/traps.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/xen/arch/arm/arm32/traps.c b/xen/arch/arm/arm32/traps.c > index a2fc1c22cbc9..b88d41811b49 100644 > --- a/xen/arch/arm/arm32/traps.c > +++ b/xen/arch/arm/arm32/traps.c > @@ -36,8 +36,7 @@ void do_trap_undefined_instruction(struct cpu_user_regs *regs) > uint32_t pc = regs->pc; > uint32_t instr; > > - if ( !is_kernel_text(pc) && > - (system_state >= SYS_STATE_active || !is_kernel_inittext(pc)) ) > + if ( !is_active_kernel_text(pc) ) > goto die; > > /* PC should be always a multiple of 4, as Xen is using ARM instruction set */
diff --git a/xen/arch/arm/arm32/traps.c b/xen/arch/arm/arm32/traps.c index a2fc1c22cbc9..b88d41811b49 100644 --- a/xen/arch/arm/arm32/traps.c +++ b/xen/arch/arm/arm32/traps.c @@ -36,8 +36,7 @@ void do_trap_undefined_instruction(struct cpu_user_regs *regs) uint32_t pc = regs->pc; uint32_t instr; - if ( !is_kernel_text(pc) && - (system_state >= SYS_STATE_active || !is_kernel_inittext(pc)) ) + if ( !is_active_kernel_text(pc) ) goto die; /* PC should be always a multiple of 4, as Xen is using ARM instruction set */
While fixing some common/arch boundaries for UBSAN support on other architectures, the following debugging patch: diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c index c1f2d1b89d43..58d1d048d339 100644 --- a/xen/arch/arm/setup.c +++ b/xen/arch/arm/setup.c @@ -504,6 +504,8 @@ void asmlinkage __init start_xen(unsigned long fdt_paddr) system_state = SYS_STATE_active; + dump_execution_state(); + for_each_domain( d ) domain_unpause_by_systemcontroller(d); failed with: (XEN) *** Serial input to DOM0 (type 'CTRL-a' three times to switch input) (XEN) CPU0: Unexpected Trap: Undefined Instruction (XEN) ----[ Xen-4.20-rc arm32 debug=n Not tainted ]---- (XEN) CPU: 0 <snip> (XEN) (XEN) **************************************** (XEN) Panic on CPU 0: (XEN) CPU0: Unexpected Trap: Undefined Instruction (XEN) **************************************** This is because the condition for init text is wrong. While there's nothing interesting from that point onwards in start_xen(), it's also wrong for livepatches too. Use is_active_kernel_text() which is the correct test for this purpose, and is aware of init and livepatch regions as well as their lifetimes. Fixes: 3e802c6ca1fb ("xen/arm: Correctly support WARN_ON") Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Stefano Stabellini <sstabellini@kernel.org> CC: Julien Grall <julien@xen.org> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> CC: Bertrand Marquis <bertrand.marquis@arm.com> CC: Michal Orzel <michal.orzel@amd.com> CC: Oleksii Kurochko <oleksii.kurochko@gmail.com> v2: * Split out change to dump_execution_state() Sample run going wrong: https://gitlab.com/xen-project/people/andyhhp/xen/-/jobs/9078570105 Sample run with dump_execution_state() working: https://gitlab.com/xen-project/people/andyhhp/xen/-/jobs/9079185111 --- xen/arch/arm/arm32/traps.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)