Re: [PATCH for-4.20? 1/4] ARM32/traps: Fix do_trap_undefined_instruction()'s detection of kernel text

["Orzel, Michal" <michal.orzel@xxxxxxx>]

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@xxxxxxxxxx>
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@xxxxxxxxx/T/#mc75e1b1ff6ccf4b0c7e10f55eedb7cacffca1c3d

With this handled:
Reviewed-by: Michal Orzel <michal.orzel@xxxxxxx>

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@xxxxxxxxxx>
> CC: Julien Grall <julien@xxxxxxx>
> CC: Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>
> CC: Bertrand Marquis <bertrand.marquis@xxxxxxx>
> CC: Michal Orzel <michal.orzel@xxxxxxx>
> CC: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> 
> 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
>