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. > 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 > >>
