On Fri, Oct 24 2025, Pasha Tatashin wrote: > On Fri, Oct 24, 2025 at 11:52 AM Pratyush Yadav <[email protected]> wrote: >> >> On Fri, Oct 24 2025, Pratyush Yadav wrote: >> >> > On Fri, Oct 24 2025, Pasha Tatashin wrote: >> > >> >>> > -int kho_add_subtree(struct kho_serialization *ser, const char *name, >> >>> > void *fdt) >> >>> > +int kho_add_subtree(const char *name, void *fdt) >> >>> > { >> >>> > - int err = 0; >> >>> > - u64 phys = (u64)virt_to_phys(fdt); >> >>> > - void *root = page_to_virt(ser->fdt); >> >>> > + struct kho_sub_fdt *sub_fdt; >> >>> > + int err; >> >>> > >> >>> > - err |= fdt_begin_node(root, name); >> >>> > - err |= fdt_property(root, PROP_SUB_FDT, &phys, sizeof(phys)); >> >>> > - err |= fdt_end_node(root); >> >>> > + sub_fdt = kmalloc(sizeof(*sub_fdt), GFP_KERNEL); >> >>> > + if (!sub_fdt) >> >>> > + return -ENOMEM; >> >>> > >> >>> > - if (err) >> >>> > - return err; >> >>> > + INIT_LIST_HEAD(&sub_fdt->l); >> >>> > + sub_fdt->name = name; >> >>> > + sub_fdt->fdt = fdt; >> >>> > >> >>> > - return kho_debugfs_fdt_add(&kho_out.dbg, name, fdt, false); >> >>> > + mutex_lock(&kho_out.fdts_lock); >> >>> > + list_add_tail(&sub_fdt->l, &kho_out.sub_fdts); >> >>> > + err = kho_debugfs_fdt_add(&kho_out.dbg, name, fdt, false); >> >>> >> >>> I think you should remove sub_fdt from the list and kfree() it on error >> >>> here. Otherwise we signal an error to the caller and they might free >> >>> sub_fdt->fdt, which will later result in a use-after-free at >> >>> __kho_finalize(). >> >> >> >> I think, it is better to simply do: >> >> WARN_ON_ONCE(kho_debugfs_fdt_add(...)); >> >> Now debugfs is optional, and there is no reason to return an error to >> >> a caller if kho_debugfs_fdt_add() fails >> > >> > Yeah, that works too. >> >> On a second thought, maybe pr_warn() instead of WARN_ON()? This isn't an >> assertion since the debugfs creation can fail for many reasons. It isn't >> expected to always succeed. So a full WARN_ON() splat seems overkill. > > I sent it with WARN_ON_ONCE(), I can change it to pr_warn_once() if > there is another revision, otherwise we can just send a separate patch > to make the change it is not that important.
Yep, makes sense. -- Regards, Pratyush Yadav
