| Message ID | 20211110205555.945026-1-sstabellini@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,for-4.16] xen/arm: allocate_bank_memory: don't create memory banks of size zero | expand |
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;
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.
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; >
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;