Hi Stefano,
On 10/11/2021 20:55, Stefano Stabellini wrote:
From: Stefano Stabellini <[email protected]>
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 <[email protected]>
Reviewed-by: Julien Grall <[email protected]>
---
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
