Message-ID: <CA+CK2bDrrg0UoJXpeN+Au2-sreYrZ+DHVcEUidzPw2Qk60orgg@mail.gmail.com>
Date: Fri, 24 Oct 2025 09:11:16 -0400
From: Pasha Tatashin <pasha.tatashin@...een.com>
To: Pratyush Yadav <pratyush@...nel.org>
Cc: akpm@...ux-foundation.org, brauner@...nel.org, corbet@....net, 
	graf@...zon.com, jgg@...pe.ca, linux-kernel@...r.kernel.org, 
	linux-kselftest@...r.kernel.org, linux-mm@...ck.org, masahiroy@...nel.org, 
	ojeda@...nel.org, rdunlap@...radead.org, rppt@...nel.org, tj@...nel.org
Subject: Re: [PATCHv7 3/7] kho: drop notifiers

> > -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

>
> > +     mutex_unlock(&kho_out.fdts_lock);
> > +
> > +     return err;
> >  }
> >  EXPORT_SYMBOL_GPL(kho_add_subtree);
> >
> > -int register_kho_notifier(struct notifier_block *nb)
> > +void kho_remove_subtree(void *fdt)
> >  {
> > -     return blocking_notifier_chain_register(&kho_out.chain_head, nb);
> > -}
> > -EXPORT_SYMBOL_GPL(register_kho_notifier);
> > +     struct kho_sub_fdt *sub_fdt;
> > +
> > +     mutex_lock(&kho_out.fdts_lock);
> > +     list_for_each_entry(sub_fdt, &kho_out.sub_fdts, l) {
>
> list_for_each_entry_safe() here since we delete.

Not needed, we are breaking from the iterator when deleting.


> >  bool kho_finalized(void)
> > @@ -1232,15 +1248,17 @@ static __init int kho_init(void)
> >  {
> >       int err = 0;
> >       const void *fdt = kho_get_fdt();
> > +     struct page *fdt_page;
> >
> >       if (!kho_enable)
> >               return 0;
> >
> > -     kho_out.ser.fdt = alloc_page(GFP_KERNEL);
> > -     if (!kho_out.ser.fdt) {
> > +     fdt_page = alloc_page(GFP_KERNEL);
> > +     if (!fdt_page) {
> >               err = -ENOMEM;
> >               goto err_free_scratch;
> >       }
> > +     kho_out.fdt = page_to_virt(fdt_page);
> >
> >       err = kho_debugfs_init();
> >       if (err)
> > @@ -1268,8 +1286,8 @@ static __init int kho_init(void)
> >       return 0;
> >
> >  err_free_fdt:
> > -     put_page(kho_out.ser.fdt);
> > -     kho_out.ser.fdt = NULL;
> > +     put_page(fdt_page);
> > +     kho_out.fdt = NULL;
> >  err_free_scratch:
> >       for (int i = 0; i < kho_scratch_cnt; i++) {
> >               void *start = __va(kho_scratch[i].addr);
> > @@ -1280,7 +1298,7 @@ static __init int kho_init(void)
> >       kho_enable = false;
> >       return err;
> >  }
> > -late_initcall(kho_init);
> > +fs_initcall(kho_init);
>
> Is this change related to this patch? Also, why fs_initcall?
>
> >
> >  static void __init kho_release_scratch(void)
> >  {
> > @@ -1416,7 +1434,7 @@ int kho_fill_kimage(struct kimage *image)
> >       if (!kho_out.finalized)
> >               return 0;
> >
> > -     image->kho.fdt = page_to_phys(kho_out.ser.fdt);
> > +     image->kho.fdt = virt_to_phys(kho_out.fdt);
> >
> >       scratch_size = sizeof(*kho_scratch) * kho_scratch_cnt;
> >       scratch = (struct kexec_buf){
> > diff --git a/kernel/kexec_handover_debugfs.c b/kernel/kexec_handover_debugfs.c
> > index a91b279f1b23..46e9e6c0791f 100644
> > --- a/kernel/kexec_handover_debugfs.c
> > +++ b/kernel/kexec_handover_debugfs.c
> > @@ -61,14 +61,17 @@ int kho_debugfs_fdt_add(struct kho_debugfs *dbg, const char *name,
> >       return __kho_debugfs_fdt_add(&dbg->fdt_list, dir, name, fdt);
> >  }
> >
> > -void kho_debugfs_cleanup(struct kho_debugfs *dbg)
> > +void kho_debugfs_fdt_remove(struct kho_debugfs *dbg, void *fdt)
> >  {
> > -     struct fdt_debugfs *ff, *tmp;
> > -
> > -     list_for_each_entry_safe(ff, tmp, &dbg->fdt_list, list) {
> > -             debugfs_remove(ff->file);
> > -             list_del(&ff->list);
> > -             kfree(ff);
> > +     struct fdt_debugfs *ff;
> > +
> > +     list_for_each_entry(ff, &dbg->fdt_list, list) {
>
> list_for_each_entry_safe() here too.

Not needed, we are breaking out on delete/free.


> >  static int kho_test_save_data(struct kho_test_state *state, void *fdt)
> >  {
> >       phys_addr_t *folios_info __free(kvfree) = NULL;
> > @@ -102,6 +86,9 @@ static int kho_test_save_data(struct kho_test_state *state, void *fdt)
> >       if (!err)
> >               state->folios_info = no_free_ptr(folios_info);
> >
> > +     if (!err)
> > +             err = kho_test();
>
> This name is very undescriptive. Also, not the right place to add the
> subtree since the FDT isn't finished yet. I think it should be done from
> kho_test_save() instead. This patch is also missing removing the subtree
> at exit, and that can cause a UAF.
>
> I sent you a patch earlier with my take on how it should be done. I
> still think that is the way to go:
> https://lore.kernel.org/all/mafs0347woui2.fsf@kernel.org/

Sure, I updated to use your suggested changes.

Powered by blists - more mailing lists