Hi Suren On 2026/9/23 10:41, Suren Baghdasaryan wrote: > On Thu, Sep 17, 2026 at 6:38 PM Hao Ge <[email protected]> wrote: >> >> Hi Suren >> >> >> On 2026/9/18 09:09, Suren Baghdasaryan wrote: >>> On Mon, Sep 14, 2026 at 11:59 PM Hao Ge <[email protected]> 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 <[email protected]> >>>> Fixes: 4835f747d3ed ("alloc_tag: support for page allocation tag >>>> compression") >>>> Cc: [email protected] >>>> Signed-off-by: Hao Ge <[email protected]> >>>> --- >>>> 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/[email protected]/ >> 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 >>>>
