Re: [PATCH v8 8/8] memblock: Unpreserve memory in case of error

[Pasha Tatashin <pasha.tatashin@soleen.com>]

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.