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

Stefano Stabellini posted 1 patch 2 years, 4 months ago
Test gitlab-ci failed
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/20211110205555.945026-1-sstabellini@kernel.org
xen/arch/arm/domain_build.c | 8 ++++++++
1 file changed, 8 insertions(+)
[PATCH v2 for-4.16] xen/arm: allocate_bank_memory: don't create memory banks of size zero
Posted by Stefano Stabellini 2 years, 4 months ago
From: Stefano Stabellini <stefano.stabellini@xilinx.com>

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.)

Fixes: f2931b4233ec ("xen/arm: introduce allocate_memory")
Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
---
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.
- downstream Xen calling make_hypervisor_node for DomUs will crash
- future Xen calling make_hypervisor_node for DomUs will have to make
  sure to fix this anyway

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;
-- 
2.25.1


Re: [PATCH v2 for-4.16] xen/arm: allocate_bank_memory: don't create memory banks of size zero
Posted by Oleksandr 2 years, 4 months ago
On 10.11.21 22:55, Stefano Stabellini wrote:

Hi Stefano

> From: Stefano Stabellini <stefano.stabellini@xilinx.com>
>
> 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.)
>
> Fixes: f2931b4233ec ("xen/arm: introduce allocate_memory")
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> ---
> 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.
> - downstream Xen calling make_hypervisor_node for DomUs will crash
> - future Xen calling make_hypervisor_node for DomUs will have to make
>    sure to fix this anyway
Regarding calling make_hypervisor_node() for DomU. I am wondering 
whether algorithms (unallocated_memory and memory_holes)
to find extended regions called from make_hypervisor_node() are also 
suitable for DomU? Anyway, this is not something which is directly 
related to this patch.


Reviewed-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>


>
> 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;

-- 
Regards,

Oleksandr Tyshchenko


Re: [PATCH v2 for-4.16] xen/arm: allocate_bank_memory: don't create memory banks of size zero
Posted by Julien Grall 2 years, 4 months ago
Hi Stefano,

On 10/11/2021 20:55, Stefano Stabellini wrote:
> From: Stefano Stabellini <stefano.stabellini@xilinx.com>
> 
> 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@xilinx.com>
Reviewed-by: Julien Grall <jgrall@amazon.com>

> ---
> 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

Re: [PATCH v2 for-4.16] xen/arm: allocate_bank_memory: don't create memory banks of size zero
Posted by Ian Jackson 2 years, 4 months ago
To the maiontainer who will review this: could you
please consider these comments as part of your review:

Stefano Stabellini writes ("[PATCH v2 for-4.16] xen/arm: allocate_bank_memory: don't create memory banks of size zero"):
> From: Stefano Stabellini <stefano.stabellini@xilinx.com>
> 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.
> - downstream Xen calling make_hypervisor_node for DomUs will crash
> - future Xen calling make_hypervisor_node for DomUs will have to make
>   sure to fix this anyway
> 
> 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.

Then, subject as usual to satisfactory maintainer review,

Release-Acked-by: Ian Jackson <iwj@xenproject.org>

Thanks,
Ian.