| Message ID | 20250208000256.431883-2-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Other fixes from UBSAN enablement | expand |
On 08/02/2025 01:02, 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); > > fails 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 any > livepatch which brings in an adjusted BUG_FRAME(). > > Use is_active_kernel_text() which is the correct test for this purpose, and is > aware of init and livepatch regions too. > > Commit c8d4b6304a5e ("xen/arm: add support for run_in_exception_handler()"), > made run_in_exception_handler() work, but didn't complete the TODO left in > commit 3e802c6ca1fb ("xen/arm: Correctly support WARN_ON"). Do so, to make > ARM consistent with other architectures. > > Fixes: 3e802c6ca1fb ("xen/arm: Correctly support WARN_ON") > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> You should have mentioned that this patch requires [1] as a prerequisite. Otherwise this patch fails to build on both arm64 and arm32 with UBSAN enabled. [1] https://lore.kernel.org/xen-devel/359347d3-9a5f-4672-98d6-4c497d960059@gmail.com/T/#mc75e1b1ff6ccf4b0c7e10f55eedb7cacffca1c3d With this handled: Reviewed-by: Michal Orzel <michal.orzel@amd.com> As for taking this patch into 4.20, I don't think this qualifies as a serious bug. At the same time I don't see how it could cause issues, so I'd be ok to take it in. That said, at least one more Arm maintainer should take a vote. ~Michal > --- > 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> > > 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 +-- > xen/arch/arm/include/asm/processor.h | 3 +-- > 2 files changed, 2 insertions(+), 4 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/include/asm/processor.h b/xen/arch/arm/include/asm/processor.h > index 60b587db697f..d80d44aeaa8f 100644 > --- a/xen/arch/arm/include/asm/processor.h > +++ b/xen/arch/arm/include/asm/processor.h > @@ -577,8 +577,7 @@ void panic_PAR(uint64_t par); > void show_registers(const struct cpu_user_regs *regs); > void show_stack(const struct cpu_user_regs *regs); > > -//#define dump_execution_state() run_in_exception_handler(show_execution_state) > -#define dump_execution_state() WARN() > +#define dump_execution_state() run_in_exception_handler(show_execution_state) > > #define cpu_relax() barrier() /* Could yield? */ > > -- > 2.39.5 >
Hi Andrew, On 08/02/2025 00:02, 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); > > fails 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 any > livepatch which brings in an adjusted BUG_FRAME(). > > Use is_active_kernel_text() which is the correct test for this purpose, and is > aware of init and livepatch regions too. > > Commit c8d4b6304a5e ("xen/arm: add support for run_in_exception_handler()"), > made run_in_exception_handler() work, but didn't complete the TODO left in > commit 3e802c6ca1fb ("xen/arm: Correctly support WARN_ON"). Do so, to make > ARM consistent with other architectures. This was done on purpose. If you look at the current implementation of run_in_exception_handler(), it will clobber some registers. With your patch #2, the function should only clobber one. It is a bit better, but it still not great. So I think we need to stick with WARN() on Arm (+ maybe a comment explaning why it is implemented differently). Cheers,
On 10/02/2025 9:23 pm, Julien Grall wrote: > Hi Andrew, > > On 08/02/2025 00:02, 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); >> >> fails 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 any >> livepatch which brings in an adjusted BUG_FRAME(). >> >> Use is_active_kernel_text() which is the correct test for this >> purpose, and is >> aware of init and livepatch regions too. >> >> Commit c8d4b6304a5e ("xen/arm: add support for >> run_in_exception_handler()"), >> made run_in_exception_handler() work, but didn't complete the TODO >> left in >> commit 3e802c6ca1fb ("xen/arm: Correctly support WARN_ON"). Do so, >> to make >> ARM consistent with other architectures. > > This was done on purpose. If you look at the current implementation of > run_in_exception_handler(), it will clobber some registers. > > With your patch #2, the function should only clobber one. It is a bit > better, but it still not great. So I think we need to stick with > WARN() on Arm (+ maybe a comment explaning why it is implemented > differently). I'm sorry but I don't follow. run_in_exception_handler() only uses 1 register (after patch 2), but it's fully described to the invoking context, so nothing is clobbered from the compilers point of view. Are you concerned about losing r0/x0 in the resulting trace? I can certainly split the patch in half. The do_trap_undefined_instruction() change isn't related, although the second hunk is needed for patch 3 to consolidate dump_execution_state() across architectures. ~Andrew
On 10/02/2025 10:13 am, Orzel, Michal wrote: > > On 08/02/2025 01:02, 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); >> >> fails 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 any >> livepatch which brings in an adjusted BUG_FRAME(). >> >> Use is_active_kernel_text() which is the correct test for this purpose, and is >> aware of init and livepatch regions too. >> >> Commit c8d4b6304a5e ("xen/arm: add support for run_in_exception_handler()"), >> made run_in_exception_handler() work, but didn't complete the TODO left in >> commit 3e802c6ca1fb ("xen/arm: Correctly support WARN_ON"). Do so, to make >> ARM consistent with other architectures. >> >> Fixes: 3e802c6ca1fb ("xen/arm: Correctly support WARN_ON") >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > You should have mentioned that this patch requires [1] as a prerequisite. > Otherwise this patch fails to build on both arm64 and arm32 with UBSAN enabled. > > [1] > https://lore.kernel.org/xen-devel/359347d3-9a5f-4672-98d6-4c497d960059@gmail.com/T/#mc75e1b1ff6ccf4b0c7e10f55eedb7cacffca1c3d That is unintentional. I'm going to split this patch in half, because it's clear that the run_in_exception_handler() problems are more complicated than I expected. The fix in do_trap_undefined_instruction() genuinely is entirely independent of UBSAN. ~Andrew
Hi Andrew, On 10/02/2025 22:23, Andrew Cooper wrote: > On 10/02/2025 9:23 pm, Julien Grall wrote: >> Hi Andrew, >> >> On 08/02/2025 00:02, 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); >>> >>> fails 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 any >>> livepatch which brings in an adjusted BUG_FRAME(). >>> >>> Use is_active_kernel_text() which is the correct test for this >>> purpose, and is >>> aware of init and livepatch regions too. >>> >>> Commit c8d4b6304a5e ("xen/arm: add support for >>> run_in_exception_handler()"), >>> made run_in_exception_handler() work, but didn't complete the TODO >>> left in >>> commit 3e802c6ca1fb ("xen/arm: Correctly support WARN_ON"). Do so, >>> to make >>> ARM consistent with other architectures. >> >> This was done on purpose. If you look at the current implementation of >> run_in_exception_handler(), it will clobber some registers. >> >> With your patch #2, the function should only clobber one. It is a bit >> better, but it still not great. So I think we need to stick with >> WARN() on Arm (+ maybe a comment explaning why it is implemented >> differently). > > I'm sorry but I don't follow. > > run_in_exception_handler() only uses 1 register (after patch 2), but > it's fully described to the invoking context, so nothing is clobbered > from the compilers point of view. Maybe "clobbered" was the wrong word. I was comparing the existing implementation (WARN()) with your proposed one. You don't mention in the commit message that r0/x0 will be missing. It wasn't clear to me whether this was intended. > > Are you concerned about losing r0/x0 in the resulting trace? I think the consolidation is not a strong enough reason to end up losing some registers in the dump. See below a proposal. > > I can certainly split the patch in half. The > do_trap_undefined_instruction() change isn't related, although the > second hunk is needed for patch 3 to consolidate dump_execution_state() > across architectures. What about checking if the arch is already providing a dump_execution_state() helper? This should allow the consolidation until we managed to convert Arm over to the generic bug infrastructure. Cheers,
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/include/asm/processor.h b/xen/arch/arm/include/asm/processor.h index 60b587db697f..d80d44aeaa8f 100644 --- a/xen/arch/arm/include/asm/processor.h +++ b/xen/arch/arm/include/asm/processor.h @@ -577,8 +577,7 @@ void panic_PAR(uint64_t par); void show_registers(const struct cpu_user_regs *regs); void show_stack(const struct cpu_user_regs *regs); -//#define dump_execution_state() run_in_exception_handler(show_execution_state) -#define dump_execution_state() WARN() +#define dump_execution_state() run_in_exception_handler(show_execution_state) #define cpu_relax() barrier() /* Could yield? */
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); fails 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 any livepatch which brings in an adjusted BUG_FRAME(). Use is_active_kernel_text() which is the correct test for this purpose, and is aware of init and livepatch regions too. Commit c8d4b6304a5e ("xen/arm: add support for run_in_exception_handler()"), made run_in_exception_handler() work, but didn't complete the TODO left in commit 3e802c6ca1fb ("xen/arm: Correctly support WARN_ON"). Do so, to make ARM consistent with other architectures. 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> 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 +-- xen/arch/arm/include/asm/processor.h | 3 +-- 2 files changed, 2 insertions(+), 4 deletions(-)