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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <aP8YADj4ha3trjJn@kernel.org>
Date: Mon, 27 Oct 2025 08:58:08 +0200
From: Mike Rapoport <rppt@...nel.org>
To: Pasha Tatashin <pasha.tatashin@...een.com>
Cc: akpm@...ux-foundation.org, brauner@...nel.org, corbet@....net,
	graf@...zon.com, jgg@...pe.ca, linux-kernel@...r.kernel.org,
	linux-kselftest@...r.kernel.org, linux-mm@...ck.org,
	masahiroy@...nel.org, ojeda@...nel.org, pratyush@...nel.org,
	rdunlap@...radead.org, tj@...nel.org
Subject: Re: [PATCH v8 8/8] memblock: Unpreserve memory in case of error

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@...nel.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.