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