If there is an error half way through KHO memory preservation, we should
rollback and unpreserve everything that is partially preserved.
Signed-off-by: Pasha Tatashin <pasha.tatashin@soleen.com>
Suggested-by: Pratyush Yadav <pratyush@kernel.org>
---
mm/memblock.c | 26 ++++++++++++++++++++++----
1 file changed, 22 insertions(+), 4 deletions(-)
diff --git a/mm/memblock.c b/mm/memblock.c
index e3bef9b35d63..5ceaa02af7d6 100644
--- a/mm/memblock.c
+++ b/mm/memblock.c
@@ -2447,6 +2447,7 @@ int reserve_mem_release_by_name(const char *name)
static int __init prepare_kho_fdt(void)
{
+ bool fdt_folio_preserved = false;
int err = 0, i;
struct page *fdt_page;
void *fdt;
@@ -2462,12 +2463,14 @@ static int __init prepare_kho_fdt(void)
err |= fdt_begin_node(fdt, "");
err |= fdt_property_string(fdt, "compatible", MEMBLOCK_KHO_NODE_COMPATIBLE);
- for (i = 0; i < reserved_mem_count; i++) {
+ for (i = 0; !err && i < reserved_mem_count; i++) {
struct reserve_mem_table *map = &reserved_mem_table[i];
struct page *page = phys_to_page(map->start);
unsigned int nr_pages = map->size >> PAGE_SHIFT;
- err |= kho_preserve_pages(page, nr_pages);
+ err = kho_preserve_pages(page, nr_pages);
+ if (err)
+ break;
err |= fdt_begin_node(fdt, map->name);
err |= fdt_property_string(fdt, "compatible", RESERVE_MEM_KHO_NODE_COMPATIBLE);
err |= fdt_property(fdt, "start", &map->start, sizeof(map->start));
@@ -2477,12 +2480,27 @@ static int __init prepare_kho_fdt(void)
err |= fdt_end_node(fdt);
err |= fdt_finish(fdt);
- err |= kho_preserve_folio(page_folio(fdt_page));
-
if (!err)
+ err = kho_preserve_folio(page_folio(fdt_page));
+
+ if (!err) {
+ fdt_folio_preserved = true;
err = kho_add_subtree(MEMBLOCK_KHO_FDT, fdt);
+ }
if (err) {
+ int nr_reserve_map_preserved = i;
+
+ for (i = 0; i < nr_reserve_map_preserved; i++) {
+ struct reserve_mem_table *map = &reserved_mem_table[i];
+ struct page *page = phys_to_page(map->start);
+ unsigned int nr_pages = map->size >> PAGE_SHIFT;
+
+ kho_unpreserve_pages(page, nr_pages);
+ }
+ if (fdt_folio_preserved)
+ kho_unpreserve_folio(page_folio(fdt_page));
+
pr_err("failed to prepare memblock FDT for KHO: %d\n", err);
put_page(fdt_page);
}
--
2.51.1.821.gb6fe4d2222-googOn Fri, Oct 24, 2025 at 12:10:02PM -0400, Pasha Tatashin wrote: > err |= fdt_end_node(fdt); > err |= fdt_finish(fdt); > > - err |= kho_preserve_folio(page_folio(fdt_page)); > - > if (!err) > + err = kho_preserve_folio(page_folio(fdt_page)); Since we are anyway changing the code here, lets use kho_preserve_pages() -- Sincerely yours, Mike.
On Fri, Oct 24, 2025 at 12:10:02PM -0400, Pasha Tatashin wrote:
> If there is an error half way through KHO memory preservation, we should
> rollback and unpreserve everything that is partially preserved.
>
> Signed-off-by: Pasha Tatashin <pasha.tatashin@soleen.com>
> Suggested-by: Pratyush Yadav <pratyush@kernel.org>
> ---
> mm/memblock.c | 26 ++++++++++++++++++++++----
> 1 file changed, 22 insertions(+), 4 deletions(-)
>
> diff --git a/mm/memblock.c b/mm/memblock.c
> index e3bef9b35d63..5ceaa02af7d6 100644
> --- a/mm/memblock.c
> +++ b/mm/memblock.c
> @@ -2447,6 +2447,7 @@ int reserve_mem_release_by_name(const char *name)
>
> static int __init prepare_kho_fdt(void)
> {
> + bool fdt_folio_preserved = false;
fdt_preserved is enough IMHO.
> int err = 0, i;
> struct page *fdt_page;
> void *fdt;
> @@ -2462,12 +2463,14 @@ static int __init prepare_kho_fdt(void)
>
> err |= fdt_begin_node(fdt, "");
> err |= fdt_property_string(fdt, "compatible", MEMBLOCK_KHO_NODE_COMPATIBLE);
> - for (i = 0; i < reserved_mem_count; i++) {
> + for (i = 0; !err && i < reserved_mem_count; i++) {
> struct reserve_mem_table *map = &reserved_mem_table[i];
> struct page *page = phys_to_page(map->start);
> unsigned int nr_pages = map->size >> PAGE_SHIFT;
>
> - err |= kho_preserve_pages(page, nr_pages);
> + err = kho_preserve_pages(page, nr_pages);
> + if (err)
> + break;
Please
goto err_unpreserve;
> err |= fdt_begin_node(fdt, map->name);
> err |= fdt_property_string(fdt, "compatible", RESERVE_MEM_KHO_NODE_COMPATIBLE);
> err |= fdt_property(fdt, "start", &map->start, sizeof(map->start));
if (err)
goto err_unpreserve;
and drop !err from the loop condition.
> @@ -2477,12 +2480,27 @@ static int __init prepare_kho_fdt(void)
> err |= fdt_end_node(fdt);
> err |= fdt_finish(fdt);
>
> - err |= kho_preserve_folio(page_folio(fdt_page));
> -
> if (!err)
> + err = kho_preserve_folio(page_folio(fdt_page));
> +
> + if (!err) {
> + fdt_folio_preserved = true;
> err = kho_add_subtree(MEMBLOCK_KHO_FDT, fdt);
> + }
>
> if (err) {
> + int nr_reserve_map_preserved = i;
nr_preserved is clear enough.
Also let's declare it before the preservation loop and count it there. Than
we can make loop variable local which makes it safer against certain side
channel attacks. I.e the loop that preserves the memory would be
for (unsigned int i = 0; i < reserve_mem_count; i++ nr_preserved++)
> +
> + for (i = 0; i < nr_reserve_map_preserved; i++) {
> + struct reserve_mem_table *map = &reserved_mem_table[i];
> + struct page *page = phys_to_page(map->start);
> + unsigned int nr_pages = map->size >> PAGE_SHIFT;
> +
> + kho_unpreserve_pages(page, nr_pages);
> + }
> + if (fdt_folio_preserved)
> + kho_unpreserve_folio(page_folio(fdt_page));
> +
> pr_err("failed to prepare memblock FDT for KHO: %d\n", err);
> put_page(fdt_page);
> }
> --
> 2.51.1.821.gb6fe4d2222-goog
>
--
Sincerely yours,
Mike.
On Sun, Oct 26, 2025 at 12:29 PM Mike Rapoport <rppt@kernel.org> wrote:
>
> On Fri, Oct 24, 2025 at 12:10:02PM -0400, Pasha Tatashin wrote:
> > If there is an error half way through KHO memory preservation, we should
> > rollback and unpreserve everything that is partially preserved.
> >
> > Signed-off-by: Pasha Tatashin <pasha.tatashin@soleen.com>
> > Suggested-by: Pratyush Yadav <pratyush@kernel.org>
> > ---
> > mm/memblock.c | 26 ++++++++++++++++++++++----
> > 1 file changed, 22 insertions(+), 4 deletions(-)
> >
> > diff --git a/mm/memblock.c b/mm/memblock.c
> > index e3bef9b35d63..5ceaa02af7d6 100644
> > --- a/mm/memblock.c
> > +++ b/mm/memblock.c
> > @@ -2447,6 +2447,7 @@ int reserve_mem_release_by_name(const char *name)
> >
> > static int __init prepare_kho_fdt(void)
> > {
> > + bool fdt_folio_preserved = false;
>
> fdt_preserved is enough IMHO.
>
> > int err = 0, i;
> > struct page *fdt_page;
> > void *fdt;
> > @@ -2462,12 +2463,14 @@ static int __init prepare_kho_fdt(void)
> >
> > err |= fdt_begin_node(fdt, "");
> > err |= fdt_property_string(fdt, "compatible", MEMBLOCK_KHO_NODE_COMPATIBLE);
> > - for (i = 0; i < reserved_mem_count; i++) {
> > + for (i = 0; !err && i < reserved_mem_count; i++) {
> > struct reserve_mem_table *map = &reserved_mem_table[i];
> > struct page *page = phys_to_page(map->start);
> > unsigned int nr_pages = map->size >> PAGE_SHIFT;
> >
> > - err |= kho_preserve_pages(page, nr_pages);
> > + err = kho_preserve_pages(page, nr_pages);
> > + if (err)
> > + break;
>
> Please
>
> goto err_unpreserve;
While we can do that, we loose some symmetry of not performing
fdt_end_node() and fdt_finish() if fdt lib ever adds some debugging
facility to make sure that open nodes/trees are properly clodes, this
is going to flag that. I prefer my current implementation.
>
> > err |= fdt_begin_node(fdt, map->name);
> > err |= fdt_property_string(fdt, "compatible", RESERVE_MEM_KHO_NODE_COMPATIBLE);
> > err |= fdt_property(fdt, "start", &map->start, sizeof(map->start));
>
> if (err)
> goto err_unpreserve;
>
> and drop !err from the loop condition.
That is going to miss one 'nr_preserved++' . We cannot do that, we
could move it to the beginning of the loop, but I prefer keeping err
right in the condition.
>
> > @@ -2477,12 +2480,27 @@ static int __init prepare_kho_fdt(void)
> > err |= fdt_end_node(fdt);
> > err |= fdt_finish(fdt);
> >
> > - err |= kho_preserve_folio(page_folio(fdt_page));
> > -
> > if (!err)
> > + err = kho_preserve_folio(page_folio(fdt_page));
> > +
> > + if (!err) {
> > + fdt_folio_preserved = true;
> > err = kho_add_subtree(MEMBLOCK_KHO_FDT, fdt);
> > + }
> >
> > if (err) {
> > + int nr_reserve_map_preserved = i;
>
> nr_preserved is clear enough.
Sure.
> Also let's declare it before the preservation loop and count it there. Than
> we can make loop variable local which makes it safer against certain side
> channel attacks. I.e the loop that preserves the memory would be
Sure.
>
> for (unsigned int i = 0; i < reserve_mem_count; i++ nr_preserved++)
>
> > +
> > + for (i = 0; i < nr_reserve_map_preserved; i++) {
> > + struct reserve_mem_table *map = &reserved_mem_table[i];
> > + struct page *page = phys_to_page(map->start);
> > + unsigned int nr_pages = map->size >> PAGE_SHIFT;
> > +
> > + kho_unpreserve_pages(page, nr_pages);
> > + }
> > + if (fdt_folio_preserved)
> > + kho_unpreserve_folio(page_folio(fdt_page));
> > +
> > pr_err("failed to prepare memblock FDT for KHO: %d\n", err);
> > put_page(fdt_page);
> > }
> > --
> > 2.51.1.821.gb6fe4d2222-goog
> >
>
> --
> Sincerely yours,
> Mike.
On Sun, Oct 26, 2025 at 01:41:30PM -0400, Pasha Tatashin wrote:
> On Sun, Oct 26, 2025 at 12:29 PM Mike Rapoport <rppt@kernel.org> wrote:
> >
> > > @@ -2462,12 +2463,14 @@ static int __init prepare_kho_fdt(void)
> > >
> > > err |= fdt_begin_node(fdt, "");
> > > err |= fdt_property_string(fdt, "compatible", MEMBLOCK_KHO_NODE_COMPATIBLE);
> > > - for (i = 0; i < reserved_mem_count; i++) {
> > > + for (i = 0; !err && i < reserved_mem_count; i++) {
> > > struct reserve_mem_table *map = &reserved_mem_table[i];
> > > struct page *page = phys_to_page(map->start);
> > > unsigned int nr_pages = map->size >> PAGE_SHIFT;
> > >
> > > - err |= kho_preserve_pages(page, nr_pages);
> > > + err = kho_preserve_pages(page, nr_pages);
> > > + if (err)
> > > + break;
> >
> > Please
> >
> > goto err_unpreserve;
>
> While we can do that, we loose some symmetry of not performing
> fdt_end_node() and fdt_finish() if fdt lib ever adds some debugging
> facility to make sure that open nodes/trees are properly clodes, this
> is going to flag that. I prefer my current implementation.
Why do we care about fdt that we are never going to use and that's freed a
few lines below?
> > > err |= fdt_begin_node(fdt, map->name);
> > > err |= fdt_property_string(fdt, "compatible", RESERVE_MEM_KHO_NODE_COMPATIBLE);
> > > err |= fdt_property(fdt, "start", &map->start, sizeof(map->start));
> >
> > if (err)
> > goto err_unpreserve;
> >
> > and drop !err from the loop condition.
>
> That is going to miss one 'nr_preserved++' . We cannot do that, we
> could move it to the beginning of the loop, but I prefer keeping err
> right in the condition.
I very much dislike the error handling after this patch. From a single
if (err)
put_page()
it grew into a complex beast with special variables just for the sake of
it.
What I'd like to see is something like
err_unpreserve_fdt:
kho_unpreserve_folio(page_folio(fdt_page));
err_unpreserve_mems:
for (unsigned int i = 0; i < nr_preserved; i++) {
/* unpreserve mem[i] */
}
err_free_fdt:
put_page(fdt_page);
return err;
--
Sincerely yours,
Mike.
On Fri, Oct 24 2025, Pasha Tatashin wrote: > If there is an error half way through KHO memory preservation, we should > rollback and unpreserve everything that is partially preserved. > > Signed-off-by: Pasha Tatashin <pasha.tatashin@soleen.com> > Suggested-by: Pratyush Yadav <pratyush@kernel.org> Reviewed-by: Pratyush Yadav <pratyush@kernel.org> [...] -- Regards, Pratyush Yadav
© 2016 - 2026 Red Hat, Inc.