Re: [PATCH v2 for-4.16] xen/arm: allocate_bank_memory: don't create memory banks of size zero

[Julien Grall <julien@xxxxxxx>]

Hi Stefano,

On 10/11/2021 20:55, Stefano Stabellini wrote:
> From: Stefano Stabellini <stefano.stabellini@xxxxxxxxxx>
>
> allocate_bank_memory can be called with a tot_size of zero, as an
> example see the implementation of allocate_memory which can call
> allocate_bank_memory with a tot_size of zero for the second memory bank.
>
> If tot_size == 0, don't create an empty memory bank, just return
> immediately without error. Otherwise a zero-size memory bank will be
> added to the domain device tree.
>
> Note that Linux is known to be able to cope with zero-size memory banks,
> and Xen more recently gained the ability to do so as well (5a37207df520
> "xen/arm: bootfdt: Ignore empty memory bank"). However, there might be
> other non-Linux OSes that are not able to cope with empty memory banks
> as well as Linux (and now Xen). It would be more robust to avoid
> zero-size memory banks unless required.
>
> Moreover, the code to find empty address regions in make_hypervisor_node
> in Xen is not able to cope with empty memory banks today and would
> result in a Xen crash. This is only a latent bug because
> make_hypervisor_node is only called for Dom0 at present and
> allocate_memory is only called for DomU at the moment. (But if
> make_hypervisor_node was to be called for a DomU, then the Xen crash
> would become manifest.)
As also mentionned by Oleksandr, I don't think make_hypervisor_node() 
could work as-is for DomU because we are not re-using the host memory 
layout (yet). Instead, we would need a logic similar to the one we use 
in libxl.
That said, it makes easier to reason if all the memory banks are non-zero.

> Fixes: f2931b4233ec ("xen/arm: introduce allocate_memory")
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxx>
Reviewed-by: Julien Grall <jgrall@xxxxxxxxxx>

> ---
> Changes in v2:
> - improve commit message
> - add in-code comment
>
> In regards to inclusion in 4.16.
>
> If we don't fix this issue in 4.16, default usage of Xen+Linux won't be
> affected.
>
> However:
> - Non-Linux OSes that cannot cope with zero-size memory banks could
>    error out. I am not aware of any but there are so many out there in
>    embedded it is impossible to tell.
I agree this is the main concern. Although, this not a new bug has been 
present for 3 years now.
> - downstream Xen calling make_hypervisor_node for DomUs will crash
For this and ...

> - future Xen calling make_hypervisor_node for DomUs will have to make
>    sure to fix this anyway
... this see above.

> If we commit the patch in 4.16, we fix these issue. This patch is only 2
> lines of code and very easy to review. The risk is extremely low. >
> Difficult to say what mistakes could have been made in analysis and
> preparation because it is a trivial if-zero-return patch, which is
> likely to fix latent bugs rather than introducing instability.
>
> ---
>   xen/arch/arm/domain_build.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 9e92b640cd..fe38a7c73c 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -395,6 +395,14 @@ static bool __init allocate_bank_memory(struct domain *d,
>       struct membank *bank;
>       unsigned int max_order = ~0;
>   
> +    /*
> +     * allocate_bank_memory can be called with a tot_size of zero for
> +     * the second memory bank. It is not an error and we can safely
> +     * avoid creating a zero-size memory bank.
> +     */
> +    if ( tot_size == 0 )
> +        return true;
> +
>       bank = &kinfo->mem.bank[kinfo->mem.nr_banks];
>       bank->start = gfn_to_gaddr(sgfn);
>       bank->size = tot_size;
--
Julien Grall