Re: [PATCH] xen/arm: alternative: Don't call vmap() within stop_machine_run()

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Tue, 26 Apr 2022, Julien Grall wrote:
> From: Julien Grall <jgrall@xxxxxxxxxx>
> 
> Commit 88a037e2cfe1 "page_alloc: assert IRQs are enabled in heap
> alloc/free" extended the checks in the buddy allocator to catch
> any use of the helpers from context with interrupts disabled.
> 
> Unfortunately, the rule is not followed in the alternative code and
> this will result to crash at boot with debug enabled:
> 
> (XEN) Xen call trace:
> (XEN)    [<0022a510>] alloc_xenheap_pages+0x120/0x150 (PC)
> (XEN)    [<00000000>] 00000000 (LR)
> (XEN)    [<002736ac>] arch/arm/mm.c#xen_pt_update+0x144/0x6e4
> (XEN)    [<002740d4>] map_pages_to_xen+0x10/0x20
> (XEN)    [<00236864>] __vmap+0x400/0x4a4
> (XEN)    [<0026aee8>] 
> arch/arm/alternative.c#__apply_alternatives_multi_stop+0x144/0x1ec
> (XEN)    [<0022fe40>] stop_machine_run+0x23c/0x300
> (XEN)    [<002c40c4>] apply_alternatives_all+0x34/0x5c
> (XEN)    [<002ce3e8>] start_xen+0xcb8/0x1024
> (XEN)    [<00200068>] arch/arm/arm32/head.o#primary_switched+0xc/0x1c
> 
> The interrupts will be disabled by the state machine in stop_machine_run(),
> hence why the ASSERT is hit.
> 
> For now the patch extending the checks has been reverted, but it would
> be good to re-introduce it (allocation with interrupts disabled is not
> desirable).
> 
> So move the re-mapping of Xen to the caller of stop_machine_run().
> 
> Signed-off-by: Julien Grall <jgrall@xxxxxxxxxx>
> Cc: David Vrabel <dvrabel@xxxxxxxxxxxx>

Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>


> ---
> 
> I managed to successfully boot Xen with this patch and dropping the
> revert.
> ---
>  xen/arch/arm/alternative.c | 34 +++++++++++++++++-----------------
>  1 file changed, 17 insertions(+), 17 deletions(-)
> 
> diff --git a/xen/arch/arm/alternative.c b/xen/arch/arm/alternative.c
> index 237c4e564209..f03cd943c636 100644
> --- a/xen/arch/arm/alternative.c
> +++ b/xen/arch/arm/alternative.c
> @@ -170,7 +170,7 @@ static int __apply_alternatives(const struct alt_region 
> *region,
>   * We might be patching the stop_machine state machine, so implement a
>   * really simple polling protocol here.
>   */
> -static int __apply_alternatives_multi_stop(void *unused)
> +static int __apply_alternatives_multi_stop(void *xenmap)
>  {
>      static int patched = 0;
>  
> @@ -185,22 +185,9 @@ static int __apply_alternatives_multi_stop(void *unused)
>      {
>          int ret;
>          struct alt_region region;
> -        mfn_t xen_mfn = virt_to_mfn(_start);
> -        paddr_t xen_size = _end - _start;
> -        unsigned int xen_order = get_order_from_bytes(xen_size);
> -        void *xenmap;
>  
>          BUG_ON(patched);
>  
> -        /*
> -         * The text and inittext section are read-only. So re-map Xen to
> -         * be able to patch the code.
> -         */
> -        xenmap = __vmap(&xen_mfn, 1U << xen_order, 1, 1, PAGE_HYPERVISOR,
> -                        VMAP_DEFAULT);
> -        /* Re-mapping Xen is not expected to fail during boot. */
> -        BUG_ON(!xenmap);
> -
>          region.begin = __alt_instructions;
>          region.end = __alt_instructions_end;
>  
> @@ -208,8 +195,6 @@ static int __apply_alternatives_multi_stop(void *unused)
>          /* The patching is not expected to fail during boot. */
>          BUG_ON(ret != 0);
>  
> -        vunmap(xenmap);
> -
>          /* Barriers provided by the cache flushing */
>          write_atomic(&patched, 1);
>      }
> @@ -224,14 +209,29 @@ static int __apply_alternatives_multi_stop(void *unused)
>  void __init apply_alternatives_all(void)
>  {
>      int ret;
> +    mfn_t xen_mfn = virt_to_mfn(_start);
> +    paddr_t xen_size = _end - _start;
> +    unsigned int xen_order = get_order_from_bytes(xen_size);
> +    void *xenmap;
>  
>      ASSERT(system_state != SYS_STATE_active);
>  
> +    /*
> +     * The text and inittext section are read-only. So re-map Xen to
> +     * be able to patch the code.
> +     */
> +    xenmap = __vmap(&xen_mfn, 1U << xen_order, 1, 1, PAGE_HYPERVISOR,
> +                    VMAP_DEFAULT);
> +    /* Re-mapping Xen is not expected to fail during boot. */
> +    BUG_ON(!xenmap);
> +
>       /* better not try code patching on a live SMP system */
> -    ret = stop_machine_run(__apply_alternatives_multi_stop, NULL, NR_CPUS);
> +    ret = stop_machine_run(__apply_alternatives_multi_stop, xenmap, NR_CPUS);
>  
>      /* stop_machine_run should never fail at this stage of the boot */
>      BUG_ON(ret);
> +
> +    vunmap(xenmap);
>  }
>  
>  int apply_alternatives(const struct alt_instr *start, const struct alt_instr 
> *end)
> -- 
> 2.32.0
>