From: Hao Ge <hao.ge@linux.dev>
To: Suren Baghdasaryan <surenb@google.com>
Cc: Luis Chamberlain <mcgrof@kernel.org>,
Petr Pavlu <petr.pavlu@suse.com>,
Daniel Gomez <da.gomez@kernel.org>,
Sami Tolvanen <samitolvanen@google.com>,
Aaron Tomlin <atomlin@atomlin.com>,
Kent Overstreet <kent.overstreet@linux.dev>,
Andrew Morton <akpm@linux-foundation.org>,
linux-modules@vger.kernel.org, linux-mm@kvack.org,
linux-kernel@vger.kernel.org, Sashiko <sashiko-bot@kernel.org>,
stable@vger.kernel.org
Subject: Re: [PATCH v10 6/6] alloc_tag: Defer /proc/allocinfo removal to a workqueue
Date: Fri, 18 Sep 2026 09:39:12 +0800 [thread overview]
Message-ID: <d7327cdd-43df-46f8-92d4-c25dda7ae684@linux.dev> (raw)
In-Reply-To: <CAJuCfpGMQqjHsAvW=XUA1Q3AyaVJKM2bKYVja_fgn0wa1tk7DA@mail.gmail.com>
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 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
>>
next prev parent reply other threads:[~2026-09-18 1:38 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-09-15 6:59 [PATCH v10 0/6] alloc_tag and module codetag section fixes Hao Ge
2026-09-15 6:59 ` [PATCH v10 1/6] alloc_tag: move release_module_tags() above reserve_module_tags() Hao Ge
2026-09-15 6:59 ` [PATCH v10 2/6] alloc_tag: clean up the populate failure path Hao Ge
2026-09-15 21:09 ` Suren Baghdasaryan
2026-09-16 6:01 ` Hao Ge
2026-09-16 16:31 ` Suren Baghdasaryan
2026-09-16 19:07 ` Suren Baghdasaryan
2026-09-17 1:12 ` Hao Ge
2026-09-17 2:32 ` Hao Ge
2026-09-18 16:35 ` Lorenzo Stoakes (ARM)
2026-09-15 6:59 ` [PATCH v10 3/6] module: introduce SH_ENTSIZE_STANDALONE for separately allocated sections Hao Ge
2026-09-17 17:25 ` Suren Baghdasaryan
2026-09-18 2:51 ` Hao Ge
2026-09-15 6:59 ` [PATCH v10 4/6] module: allocate codetag sections before the regular module layout Hao Ge
2026-09-18 0:05 ` Suren Baghdasaryan
2026-09-15 7:00 ` [PATCH v10 5/6] alloc_tag: skip percpu counter allocation when profiling is disabled Hao Ge
2026-09-18 0:53 ` Suren Baghdasaryan
2026-09-18 2:37 ` Hao Ge
2026-09-15 7:00 ` [PATCH v10 6/6] alloc_tag: Defer /proc/allocinfo removal to a workqueue Hao Ge
2026-09-18 1:09 ` Suren Baghdasaryan
2026-09-18 1:39 ` Hao Ge [this message]
2026-09-15 18:23 ` [PATCH v10 0/6] alloc_tag and module codetag section fixes Suren Baghdasaryan
2026-09-16 5:03 ` Hao Ge
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=d7327cdd-43df-46f8-92d4-c25dda7ae684@linux.dev \
--to=hao.ge@linux.dev \
--cc=akpm@linux-foundation.org \
--cc=atomlin@atomlin.com \
--cc=da.gomez@kernel.org \
--cc=kent.overstreet@linux.dev \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=linux-modules@vger.kernel.org \
--cc=mcgrof@kernel.org \
--cc=petr.pavlu@suse.com \
--cc=samitolvanen@google.com \
--cc=sashiko-bot@kernel.org \
--cc=stable@vger.kernel.org \
--cc=surenb@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®