Re: [PATCHv7 3/7] kho: drop notifiers

[Pratyush Yadav <pratyush@kernel.org>]

On Fri, Oct 24 2025, Pasha Tatashin wrote:

> On Fri, Oct 24, 2025 at 11:52 AM Pratyush Yadav <pratyush@kernel.org> 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