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. Pasha > > [...] > > -- > Regards, > Pratyush Yadav
