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

[Hao Ge <hao.ge@linux.dev>]

Hi Suren

On 2026/9/23 10:41, Suren Baghdasaryan wrote:
> On Thu, Sep 17, 2026 at 6:38 PM Hao Ge <hao.ge@linux.dev> 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@linux.dev> 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@kernel.org>
>>>> Fixes: 4835f747d3ed ("alloc_tag: support for page allocation tag compression")
>>>> Cc: stable@vger.kernel.org
>>>> Signed-off-by: Hao Ge <hao.ge@linux.dev>
>>>> ---
>>>>  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.
>
Thank you for the valuable suggestion, I will take this approach.
I've found there could be a race condition with alloc_tag_top_users.
I'll analyze it.

Thanks
Best Regards
Hao

>> 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@linux.dev/
>> 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
>>>>