* [PATCH v2 for-4.16] xen/arm: allocate_bank_memory: don't create memory banks of size zero
@ 2021-11-10 20:55 Stefano Stabellini
2021-11-10 21:14 ` Oleksandr
` (2 more replies)
0 siblings, 3 replies; 4+ messages in thread
From: Stefano Stabellini @ 2021-11-10 20:55 UTC (permalink / raw)
To: julien
Cc: sstabellini, xen-devel, oleksandr_tyshchenko, iwj,
Bertrand.Marquis, Volodymyr_Babchuk, Stefano Stabellini
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v2 for-4.16] xen/arm: allocate_bank_memory: don't create memory banks of size zero
2021-11-10 20:55 [PATCH v2 for-4.16] xen/arm: allocate_bank_memory: don't create memory banks of size zero Stefano Stabellini
@ 2021-11-10 21:14 ` Oleksandr
2021-11-11 10:55 ` Ian Jackson
2021-11-11 11:43 ` Julien Grall
2 siblings, 0 replies; 4+ messages in thread
From: Oleksandr @ 2021-11-10 21:14 UTC (permalink / raw)
To: Stefano Stabellini
Cc: julien, xen-devel, oleksandr_tyshchenko, iwj, Bertrand.Marquis,
Volodymyr_Babchuk, Stefano Stabellini
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2 for-4.16] xen/arm: allocate_bank_memory: don't create memory banks of size zero
2021-11-10 20:55 [PATCH v2 for-4.16] xen/arm: allocate_bank_memory: don't create memory banks of size zero Stefano Stabellini
2021-11-10 21:14 ` Oleksandr
@ 2021-11-11 10:55 ` Ian Jackson
2021-11-11 11:43 ` Julien Grall
2 siblings, 0 replies; 4+ messages in thread
From: Ian Jackson @ 2021-11-11 10:55 UTC (permalink / raw)
To: Stefano Stabellini
Cc: julien, xen-devel, oleksandr_tyshchenko, Bertrand.Marquis,
Volodymyr_Babchuk, Stefano Stabellini
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.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2 for-4.16] xen/arm: allocate_bank_memory: don't create memory banks of size zero
2021-11-10 20:55 [PATCH v2 for-4.16] xen/arm: allocate_bank_memory: don't create memory banks of size zero Stefano Stabellini
2021-11-10 21:14 ` Oleksandr
2021-11-11 10:55 ` Ian Jackson
@ 2021-11-11 11:43 ` Julien Grall
2 siblings, 0 replies; 4+ messages in thread
From: Julien Grall @ 2021-11-11 11:43 UTC (permalink / raw)
To: Stefano Stabellini
Cc: xen-devel, oleksandr_tyshchenko, iwj, Bertrand.Marquis,
Volodymyr_Babchuk, Stefano Stabellini
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-11-11 11:43 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-10 20:55 [PATCH v2 for-4.16] xen/arm: allocate_bank_memory: don't create memory banks of size zero Stefano Stabellini
2021-11-10 21:14 ` Oleksandr
2021-11-11 10:55 ` Ian Jackson
2021-11-11 11:43 ` Julien Grall
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.