[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.