Re: [PATCH v10 6/6] alloc_tag: Defer /proc/allocinfo removal to a workqueue
From: Hao Ge
Date: Thu Sep 17 2026 - 21:38:39 EST
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 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
>>