Re: [PATCH v10 6/6] alloc_tag: Defer /proc/allocinfo removal to a workqueue

[Suren Baghdasaryan]

On Thu, Sep 17, 2026 at 6:38 PM Hao Ge <hao.ge@xxxxxxxxx> wrote:
>
> Hi Suren
>
>
> On 2026/9/18 09:09, Suren Baghdasaryan wrote:
> > On Mon, Sep 14, 2026 at 11:59 PM Hao Ge <hao.ge@xxxxxxxxx> wrote:
> >>
> >> shutdown_mem_profiling() calls remove_proc_entry() from
> >> reserve_module_tags(), which runs under mod_lock held for write.
> >> remove_proc_entry() waits for readers, and a reader takes mod_lock for
> >> read in allocinfo_start():
> >>
> >>   CPU0 (insmod)                      CPU1 (read /proc/allocinfo)
> >>   ----------------                   ----------------------------
> >>   reserve_module_tags()
> >>     down_write(&mod_lock)  [held]
> >>                                      use_pde()            [in_use++]
> >>                                      allocinfo_start()
> >>                                        down_read(&mod_lock)  <- blocks
> >>     shutdown_mem_profiling()
> >>       remove_proc_entry()
> >>         wait for in_use == 0         <- blocks
> >>
> >> Move remove_proc_entry() to a workqueue.
> >>
> >> The file creation is moved to the end of alloc_tag_init() as well.
> >> If alloc_tag_init() fails with alloc_tag_cttype still NULL or an
> >> error pointer, a concurrent reader of the leftover file would
> >> dereference it in allocinfo_start() and panic.
> >>
> >> Reported-by: Sashiko <sashiko-bot@xxxxxxxxxx>
> >> Fixes: 4835f747d3ed ("alloc_tag: support for page allocation tag compression")
> >> Cc: stable@xxxxxxxxxxxxxxx
> >> Signed-off-by: Hao Ge <hao.ge@xxxxxxxxx>
> >> ---
> >>  mm/alloc_tag.c | 26 +++++++++++++++++---------
> >>  1 file changed, 17 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/mm/alloc_tag.c b/mm/alloc_tag.c
> >> index 1ca0409b492b..cfa0fc84b68f 100644
> >> --- a/mm/alloc_tag.c
> >> +++ b/mm/alloc_tag.c
> >> @@ -15,6 +15,7 @@
> >>  #include <linux/seq_file.h>
> >>  #include <linux/string_choices.h>
> >>  #include <linux/vmalloc.h>
> >> +#include <linux/workqueue.h>
> >>  #include <linux/kmemleak.h>
> >>  #include <uapi/linux/alloc_tag.h>
> >>
> >> @@ -591,6 +592,13 @@ void pgalloc_tag_swap(struct folio *new, struct folio *old)
> >>         put_page_tag_ref(handle_new);
> >>  }
> >>
> >> +static void remove_allocinfo_file(struct work_struct *work)
> >> +{
> >> +       remove_proc_entry(ALLOCINFO_FILE_NAME, NULL);
> >> +}
> >> +
> >> +static DECLARE_WORK(remove_allocinfo_work, remove_allocinfo_file);
> >> +
> >>  static void shutdown_mem_profiling(bool remove_file)
> >>  {
> >>         if (mem_alloc_profiling_enabled())
> >> @@ -600,7 +608,7 @@ static void shutdown_mem_profiling(bool remove_file)
> >>                 return;
> >>
> >>         if (remove_file)
> >> -               remove_proc_entry(ALLOCINFO_FILE_NAME, NULL);
> >> +               schedule_work(&remove_allocinfo_work);
> >>         mem_profiling_support = false;
> >>  }
> >>
> >> @@ -1358,16 +1366,10 @@ static int __init alloc_tag_init(void)
> >>                 return 0;
> >>         }
> >>
> >> -       if (!proc_create(ALLOCINFO_FILE_NAME, 0400, NULL, &allocinfo_proc_ops)) {
> >> -               pr_err("Failed to create %s file\n", ALLOCINFO_FILE_NAME);
> >> -               shutdown_mem_profiling(false);
> >> -               return -ENOMEM;
> >> -       }
> >> -
> >>         res = alloc_mod_tags_mem();
> >>         if (res) {
> >>                 pr_err("Failed to reserve address space for module tags, errno = %d\n", res);
> >> -               shutdown_mem_profiling(true);
> >> +               shutdown_mem_profiling(false);
> >>                 return res;
> >>         }
> >>
> >> @@ -1375,10 +1377,16 @@ static int __init alloc_tag_init(void)
> >>         if (IS_ERR(alloc_tag_cttype)) {
> >>                 pr_err("Allocation tags registration failed, errno = %pe\n", alloc_tag_cttype);
> >>                 free_mod_tags_mem();
> >> -               shutdown_mem_profiling(true);
> >> +               shutdown_mem_profiling(false);
> >>                 return PTR_ERR(alloc_tag_cttype);
> >>         }
> >>
> >> +       if (!proc_create(ALLOCINFO_FILE_NAME, 0400, NULL, &allocinfo_proc_ops)) {
> >> +               pr_err("Failed to create %s file\n", ALLOCINFO_FILE_NAME);
> >> +               shutdown_mem_profiling(false);
> >
> > You need free_mod_tags_mem() here.
> >
>
> Right. Another problem is exposed here: moving proc_create() to the end
> implies successful return from codetag_register_type(),
> so alloc_tag is already added into codetag_types.
> That looks a bit odd to me. Because all places inside codetag that access this
> linked list will access this incompletely‑initialized codetag_type.
> There is currently no matching unregister interface to tear it down.
> So I drafted one previously:
> void codetag_unregister_type(struct codetag_type *cttype)
> {
>         struct codetag_module *cmod;
>         unsigned long id, tmp;
>
>         mutex_lock(&codetag_lock);
>         list_del(&cttype->link);
>         mutex_unlock(&codetag_lock);
>
>         codetag_lock_module_list(cttype);
>         idr_for_each_entry_ul(&cttype->mod_idr, cmod, tmp, id)
>                 kfree(cmod);
>         idr_destroy(&cttype->mod_idr);
>         codetag_unlock_module_list(cttype);
>
>         kfree(cttype);
> }
>
> But looking back, do we really need to do this? I'm not so sure.

I think having codetag_unregister_type() would be a good idea. For now
it's used only in this failure case, so we can make it an __init
function and not waste any memory at all.

> I previously thought the issue reported by Sashiko was a false positive,
> and I laid out my thoughts back then:
> https://lore.kernel.org/all/afa606df-3d5b-47c2-9972-f3e0c2e13c12@xxxxxxxxx/
> and I thought the change would be straightforward, and defensive programming
> felt acceptable to me, but it turns out to be a little more complex than I expected.
>
> Suren, could you help me analyze this? Thank you very much for your valuable feedback
>
> Thanks
> Best Regards
> Hao
>
> >> +               return -ENOMEM;
> >> +       }
> >> +
> >>         return 0;
> >>  }
> >>  module_init(alloc_tag_init);
> >> --
> >> 2.25.1
> >>