linux-kernel - Re: [PATCH v2 10/13] kho: Update FDT dynamically for subtree addition/removal

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CA+CK2bBnEKk_mQOCfDhnqbUCovLCKhFw-p54Q4_Sutk4oieOVA@mail.gmail.com>
Date: Sat, 15 Nov 2025 09:51:07 -0500
From: Pasha Tatashin <pasha.tatashin@...een.com>
To: Mike Rapoport <rppt@...nel.org>
Cc: akpm@...ux-foundation.org, bhe@...hat.com, jasonmiu@...gle.com, 
	arnd@...db.de, coxu@...hat.com, dave@...ilevsky.ca, ebiggers@...gle.com, 
	graf@...zon.com, kees@...nel.org, linux-kernel@...r.kernel.org, 
	kexec@...ts.infradead.org, linux-mm@...ck.org
Subject: Re: [PATCH v2 10/13] kho: Update FDT dynamically for subtree addition/removal

On Sat, Nov 15, 2025 at 4:40 AM Mike Rapoport <rppt@...nel.org> wrote:
>
> On Fri, Nov 14, 2025 at 01:59:59PM -0500, Pasha Tatashin wrote:
> > -     struct kho_sub_fdt *sub_fdt;
> > +     phys_addr_t phys = virt_to_phys(fdt);
> > +     void *root_fdt = kho_out.fdt;
> > +     int err = -ENOMEM;
> > +     int off, fdt_err;
> >
> > -     sub_fdt = kmalloc(sizeof(*sub_fdt), GFP_KERNEL);
> > -     if (!sub_fdt)
> > -             return -ENOMEM;
> > +     guard(mutex)(&kho_out.lock);
> > +
> > +     fdt_err = fdt_open_into(root_fdt, root_fdt, PAGE_SIZE);
> > +     if (fdt_err < 0)
> > +             return err;
> >
> > -     INIT_LIST_HEAD(&sub_fdt->l);
> > -     sub_fdt->name = name;
> > -     sub_fdt->fdt = fdt;
> > +     off = fdt_add_subnode(root_fdt, 0, name);
>
> Why not
>         fdt_err = fdt_add_subnode()
>
> as I asked in v1 review?
>

Oh, I missed that, there is a slight difference between the two:
'fdt_err' only contains FDT return value, i.e. error if negative. The
'off' on the other hand in the happy path contains subnode offset, and
contains error only in the unhappy path. This is why I think it is a
little cleaner to keep different name, however, if you still prefer
re-using a single local variable for both, this is fix-up patch:

diff --git a/kernel/liveupdate/kexec_handover.c
b/kernel/liveupdate/kexec_handover.c
index 224bdf5becb6..81f60ccb2dc7 100644
--- a/kernel/liveupdate/kexec_handover.c
+++ b/kernel/liveupdate/kexec_handover.c
@@ -713,7 +713,7 @@ int kho_add_subtree(const char *name, void *fdt)
        phys_addr_t phys = virt_to_phys(fdt);
        void *root_fdt = kho_out.fdt;
        int err = -ENOMEM;
-       int off, fdt_err;
+       int fdt_err;

        guard(mutex)(&kho_out.lock);

@@ -721,14 +721,14 @@ int kho_add_subtree(const char *name, void *fdt)
        if (fdt_err < 0)
                return err;

-       off = fdt_add_subnode(root_fdt, 0, name);
-       if (off < 0) {
-               if (off == -FDT_ERR_EXISTS)
+       fdt_err = fdt_add_subnode(root_fdt, 0, name);
+       if (fdt_err < 0) {
+               if (fdt_err == -FDT_ERR_EXISTS)
                        err = -EEXIST;
                goto out_pack;
        }

-       err = fdt_setprop(root_fdt, off, PROP_SUB_FDT, &phys, sizeof(phys));
+       err = fdt_setprop(root_fdt, fdt_err, PROP_SUB_FDT, &phys, sizeof(phys));
        if (err < 0)
                goto out_pack;