[PATCH v10 1/6] alloc_tag: move release_module_tags() above reserve_module_tags()

[PATCH v10 1/6] alloc_tag: move release_module_tags() above reserve_module_tags()

[Hao Ge]

release_module_tags() is a cleanup helper. reserve_module_tags() can
also fail after storing the reservation in the maple tree, in which
case it should call release_module_tags() to undo it. Move the helper
above reserve_module_tags() so no forward declaration is needed.

No functional change.

Fixes: 4835f747d3ed ("alloc_tag: support for page allocation tag compression")
Fixes: 0f9b685626da ("alloc_tag: populate memory for module tags as needed")
Reported-by: Sashiko <sashiko-bot@kernel.org>
Cc: stable@vger.kernel.org
Acked-by: Suren Baghdasaryan <surenb@google.com>
Signed-off-by: Hao Ge <hao.ge@linux.dev>
---
 mm/alloc_tag.c | 92 +++++++++++++++++++++++++-------------------------
 1 file changed, 46 insertions(+), 46 deletions(-)

diff --git a/mm/alloc_tag.c b/mm/alloc_tag.c
index b33410310477..2070e682fe10 100644
--- a/mm/alloc_tag.c
+++ b/mm/alloc_tag.c
@@ -835,6 +835,52 @@ static int vm_module_tags_populate(void)
 	return 0;
 }
 
+static void release_module_tags(struct module *mod, bool used)
+{
+	MA_STATE(mas, &mod_area_mt, module_tags.size, module_tags.size);
+	struct alloc_tag *start_tag;
+	struct alloc_tag *end_tag;
+	struct module *val;
+
+	mas_lock(&mas);
+	mas_for_each_rev(&mas, val, 0)
+		if (val == mod)
+			break;
+
+	if (!val) /* module not found */
+		goto out;
+
+	if (!used)
+		goto release_area;
+
+	start_tag = (struct alloc_tag *)(module_tags.start_addr + mas.index);
+	end_tag = (struct alloc_tag *)(module_tags.start_addr + mas.last);
+	if (!clean_unused_counters(start_tag, end_tag)) {
+		struct alloc_tag *tag;
+
+		for (tag = start_tag; tag <= end_tag; tag++) {
+			struct alloc_tag_counters counter;
+
+			if (!tag->counters)
+				continue;
+
+			counter = alloc_tag_read(tag);
+			pr_info("%s:%u module %s func:%s has %llu allocated at module unload\n",
+				tag->ct.filename, tag->ct.lineno, tag->ct.modname,
+				tag->ct.function, counter.bytes);
+		}
+	} else {
+		used = false;
+	}
+release_area:
+	mas_store(&mas, used ? &unloaded_mod : NULL);
+	val = mas_prev_range(&mas, 0);
+	if (val == &prepend_mod)
+		mas_store(&mas, NULL);
+out:
+	mas_unlock(&mas);
+}
+
 static void *reserve_module_tags(struct module *mod, unsigned long size,
 				 unsigned int prepend, unsigned long align)
 {
@@ -922,52 +968,6 @@ static void *reserve_module_tags(struct module *mod, unsigned long size,
 	return (struct alloc_tag *)(module_tags.start_addr + offset);
 }
 
-static void release_module_tags(struct module *mod, bool used)
-{
-	MA_STATE(mas, &mod_area_mt, module_tags.size, module_tags.size);
-	struct alloc_tag *start_tag;
-	struct alloc_tag *end_tag;
-	struct module *val;
-
-	mas_lock(&mas);
-	mas_for_each_rev(&mas, val, 0)
-		if (val == mod)
-			break;
-
-	if (!val) /* module not found */
-		goto out;
-
-	if (!used)
-		goto release_area;
-
-	start_tag = (struct alloc_tag *)(module_tags.start_addr + mas.index);
-	end_tag = (struct alloc_tag *)(module_tags.start_addr + mas.last);
-	if (!clean_unused_counters(start_tag, end_tag)) {
-		struct alloc_tag *tag;
-
-		for (tag = start_tag; tag <= end_tag; tag++) {
-			struct alloc_tag_counters counter;
-
-			if (!tag->counters)
-				continue;
-
-			counter = alloc_tag_read(tag);
-			pr_info("%s:%u module %s func:%s has %llu allocated at module unload\n",
-				tag->ct.filename, tag->ct.lineno, tag->ct.modname,
-				tag->ct.function, counter.bytes);
-		}
-	} else {
-		used = false;
-	}
-release_area:
-	mas_store(&mas, used ? &unloaded_mod : NULL);
-	val = mas_prev_range(&mas, 0);
-	if (val == &prepend_mod)
-		mas_store(&mas, NULL);
-out:
-	mas_unlock(&mas);
-}
-
 static int load_module(struct module *mod, struct codetag *start, struct codetag *stop)
 {
 	/* Allocate module alloc_tag percpu counters */
-- 
2.25.1

[PATCH v10 2/6] alloc_tag: clean up the populate failure path

[Hao Ge]

The reservation is already stored in the maple tree when
vm_module_tags_populate() fails. A failed load never unloads the
module, so nothing releases the entry. Release it and roll
module_tags.size back. Without the rollback a concurrent load that
already passed needs_section_mem() can reuse the freed gap, skip
vm_module_tags_populate() and write to unmapped memory.

vmap_pages_range() may have installed some PTEs before failing. A
retry to populate the same range would BUG on them, so undo them,
but only if vmap actually ran.

Fixes: 4835f747d3ed ("alloc_tag: support for page allocation tag compression")
Fixes: 0f9b685626da ("alloc_tag: populate memory for module tags as needed")
Reported-by: Sashiko <sashiko-bot@kernel.org>
Cc: stable@vger.kernel.org
Signed-off-by: Hao Ge <hao.ge@linux.dev>
---
 mm/alloc_tag.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/mm/alloc_tag.c b/mm/alloc_tag.c
index 2070e682fe10..95ddf5b743d0 100644
--- a/mm/alloc_tag.c
+++ b/mm/alloc_tag.c
@@ -804,6 +804,13 @@ static int vm_module_tags_populate(void)
 				     next_page, PAGE_SHIFT) < 0) {
 			release_pages_arg arg = { .pages = next_page };
 
+			/*
+			 * vmap_pages_range() only runs once all pages were
+			 * allocated, and it may have installed some mappings
+			 * before failing. Undo them.
+			 */
+			if (nr == more_pages)
+				vunmap_range(phys_end, phys_end + (nr << PAGE_SHIFT));
 			/* Clean up and error out */
 			release_pages(arg, nr);
 			return -ENOMEM;
@@ -947,6 +954,7 @@ static void *reserve_module_tags(struct module *mod, unsigned long size,
 		return ret;
 
 	if (module_tags.size < offset + size) {
+		unsigned long prev_size = module_tags.size;
 		int grow_res;
 
 		module_tags.size = offset + size;
@@ -961,6 +969,8 @@ static void *reserve_module_tags(struct module *mod, unsigned long size,
 			shutdown_mem_profiling(true);
 			pr_err("Failed to allocate memory for allocation tags in the module %s. Memory allocation profiling is disabled!\n",
 			       mod->name);
+			release_module_tags(mod, false);
+			module_tags.size = prev_size;
 			return ERR_PTR(grow_res);
 		}
 	}
-- 
2.25.1

Re: [PATCH v10 2/6] alloc_tag: clean up the populate failure path

[Suren Baghdasaryan]

On Mon, Sep 14, 2026 at 11:59 PM Hao Ge <hao.ge@linux.dev> wrote:
>
> The reservation is already stored in the maple tree when
> vm_module_tags_populate() fails. A failed load never unloads the
> module, so nothing releases the entry. Release it and roll
> module_tags.size back. Without the rollback a concurrent load that
> already passed needs_section_mem() can reuse the freed gap, skip
> vm_module_tags_populate() and write to unmapped memory.
>
> vmap_pages_range() may have installed some PTEs before failing. A
> retry to populate the same range would BUG on them, so undo them,
> but only if vmap actually ran.
>
> Fixes: 4835f747d3ed ("alloc_tag: support for page allocation tag compression")
> Fixes: 0f9b685626da ("alloc_tag: populate memory for module tags as needed")

If a patch fixes two other patches, it likely needs to be split so
that each part fixes one patch only. But read on please. I'm not sure
if one of the fixes is correct.

> Reported-by: Sashiko <sashiko-bot@kernel.org>
> Cc: stable@vger.kernel.org
> Signed-off-by: Hao Ge <hao.ge@linux.dev>
> ---
>  mm/alloc_tag.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/mm/alloc_tag.c b/mm/alloc_tag.c
> index 2070e682fe10..95ddf5b743d0 100644
> --- a/mm/alloc_tag.c
> +++ b/mm/alloc_tag.c
> @@ -804,6 +804,13 @@ static int vm_module_tags_populate(void)
>                                      next_page, PAGE_SHIFT) < 0) {
>                         release_pages_arg arg = { .pages = next_page };
>
> +                       /*
> +                        * vmap_pages_range() only runs once all pages were
> +                        * allocated, and it may have installed some mappings
> +                        * before failing. Undo them.

If vmap_pages_range() failed to map the range, why should we need to
undo it? If it indeed leaves some partial mapping when failing then I
would argue that vmap_pages_range() should be undoing these partial
mappings itself before returning the error.

> +                        */
> +                       if (nr == more_pages)
> +                               vunmap_range(phys_end, phys_end + (nr << PAGE_SHIFT));
>                         /* Clean up and error out */
>                         release_pages(arg, nr);
>                         return -ENOMEM;
> @@ -947,6 +954,7 @@ static void *reserve_module_tags(struct module *mod, unsigned long size,
>                 return ret;
>
>         if (module_tags.size < offset + size) {
> +               unsigned long prev_size = module_tags.size;
>                 int grow_res;
>
>                 module_tags.size = offset + size;
> @@ -961,6 +969,8 @@ static void *reserve_module_tags(struct module *mod, unsigned long size,
>                         shutdown_mem_profiling(true);
>                         pr_err("Failed to allocate memory for allocation tags in the module %s. Memory allocation profiling is disabled!\n",
>                                mod->name);
> +                       release_module_tags(mod, false);
> +                       module_tags.size = prev_size;

Yes, this one is a valid fix. It fixes 0f9b685626da commit.

>                         return ERR_PTR(grow_res);
>                 }
>         }
> --
> 2.25.1
>

Re: [PATCH v10 2/6] alloc_tag: clean up the populate failure path

[Hao Ge]

Hi Suren

Thanks for you review.

On 2026/9/16 05:09, Suren Baghdasaryan wrote:
> On Mon, Sep 14, 2026 at 11:59 PM Hao Ge <hao.ge@linux.dev> wrote:
>>
>> The reservation is already stored in the maple tree when
>> vm_module_tags_populate() fails. A failed load never unloads the
>> module, so nothing releases the entry. Release it and roll
>> module_tags.size back. Without the rollback a concurrent load that
>> already passed needs_section_mem() can reuse the freed gap, skip
>> vm_module_tags_populate() and write to unmapped memory.
>>
>> vmap_pages_range() may have installed some PTEs before failing. A
>> retry to populate the same range would BUG on them, so undo them,
>> but only if vmap actually ran.
>>
>> Fixes: 4835f747d3ed ("alloc_tag: support for page allocation tag compression")
>> Fixes: 0f9b685626da ("alloc_tag: populate memory for module tags as needed")
> 
> If a patch fixes two other patches, it likely needs to be split so
> that each part fixes one patch only. But read on please. I'm not sure
> if one of the fixes is correct.
>

Kept them in one patch because they only make sense together.

After a failed populate the reservation is released and the
size rolled back, so the next load takes the same gap and calls
vm_module_tags_populate() again, and that retry would trip over
the PTEs the failed vmap left behind.

Without the rollback a load that already passed needs_section_mem()
just reuses the gap, skips populate and writes to unmapped memory.
So splitting them leaves each half broken.                                   

>> Reported-by: Sashiko <sashiko-bot@kernel.org>
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Hao Ge <hao.ge@linux.dev>
>> ---
>>  mm/alloc_tag.c | 10 ++++++++++
>>  1 file changed, 10 insertions(+)
>>
>> diff --git a/mm/alloc_tag.c b/mm/alloc_tag.c
>> index 2070e682fe10..95ddf5b743d0 100644
>> --- a/mm/alloc_tag.c
>> +++ b/mm/alloc_tag.c
>> @@ -804,6 +804,13 @@ static int vm_module_tags_populate(void)
>>                                      next_page, PAGE_SHIFT) < 0) {
>>                         release_pages_arg arg = { .pages = next_page };
>>
>> +                       /*
>> +                        * vmap_pages_range() only runs once all pages were
>> +                        * allocated, and it may have installed some mappings
>> +                        * before failing. Undo them.
> 
> If vmap_pages_range() failed to map the range, why should we need to
> undo it? If it indeed leaves some partial mapping when failing then I
> would argue that vmap_pages_range() should be undoing these partial
> mappings itself before returning the error.
>
Yeah it does - none of the paths under vmap_pages_range() undo themselves on error.
pcpu_map_pages does perform cleanup on its own as well. There may be other similar sites.
https://elixir.bootlin.com/linux/v7.3-rc3/source/mm/percpu-vm.c#L255
so I did the same here.

If we really want vmap_pages_range() to handle this cleanup internally, I believe that would
be a relatively large change, and we would need to consider many more cases. So I think we keep
the current behaviour for now.

Thanks
Best Regards
Hao

>> +                        */
>> +                       if (nr == more_pages)
>> +                               vunmap_range(phys_end, phys_end + (nr << PAGE_SHIFT));
>>                         /* Clean up and error out */
>>                         release_pages(arg, nr);
>>                         return -ENOMEM;
>> @@ -947,6 +954,7 @@ static void *reserve_module_tags(struct module *mod, unsigned long size,
>>                 return ret;
>>
>>         if (module_tags.size < offset + size) {
>> +               unsigned long prev_size = module_tags.size;
>>                 int grow_res;
>>
>>                 module_tags.size = offset + size;
>> @@ -961,6 +969,8 @@ static void *reserve_module_tags(struct module *mod, unsigned long size,
>>                         shutdown_mem_profiling(true);
>>                         pr_err("Failed to allocate memory for allocation tags in the module %s. Memory allocation profiling is disabled!\n",
>>                                mod->name);
>> +                       release_module_tags(mod, false);
>> +                       module_tags.size = prev_size;
> 
> Yes, this one is a valid fix. It fixes 0f9b685626da commit.
> 
>>                         return ERR_PTR(grow_res);
>>                 }
>>         }
>> --
>> 2.25.1
>>

Re: [PATCH v10 2/6] alloc_tag: clean up the populate failure path

[Suren Baghdasaryan]

On Tue, Sep 15, 2026 at 11:00 PM Hao Ge <hao.ge@linux.dev> wrote:
>
> Hi Suren
>
> Thanks for you review.
>
> On 2026/9/16 05:09, Suren Baghdasaryan wrote:
> > On Mon, Sep 14, 2026 at 11:59 PM Hao Ge <hao.ge@linux.dev> wrote:
> >>
> >> The reservation is already stored in the maple tree when
> >> vm_module_tags_populate() fails. A failed load never unloads the
> >> module, so nothing releases the entry. Release it and roll
> >> module_tags.size back. Without the rollback a concurrent load that
> >> already passed needs_section_mem() can reuse the freed gap, skip
> >> vm_module_tags_populate() and write to unmapped memory.
> >>
> >> vmap_pages_range() may have installed some PTEs before failing. A
> >> retry to populate the same range would BUG on them, so undo them,
> >> but only if vmap actually ran.
> >>
> >> Fixes: 4835f747d3ed ("alloc_tag: support for page allocation tag compression")
> >> Fixes: 0f9b685626da ("alloc_tag: populate memory for module tags as needed")
> >
> > If a patch fixes two other patches, it likely needs to be split so
> > that each part fixes one patch only. But read on please. I'm not sure
> > if one of the fixes is correct.
> >
>
> Kept them in one patch because they only make sense together.
>
> After a failed populate the reservation is released and the
> size rolled back, so the next load takes the same gap and calls
> vm_module_tags_populate() again, and that retry would trip over
> the PTEs the failed vmap left behind.
>
> Without the rollback a load that already passed needs_section_mem()
> just reuses the gap, skips populate and writes to unmapped memory.
> So splitting them leaves each half broken.

Yeah, that's why I think vmap_pages_range() cleanup should be a
completely separate patch with vmap_pages_range() doing cleanup
itself.

>
> >> Reported-by: Sashiko <sashiko-bot@kernel.org>
> >> Cc: stable@vger.kernel.org
> >> Signed-off-by: Hao Ge <hao.ge@linux.dev>
> >> ---
> >>  mm/alloc_tag.c | 10 ++++++++++
> >>  1 file changed, 10 insertions(+)
> >>
> >> diff --git a/mm/alloc_tag.c b/mm/alloc_tag.c
> >> index 2070e682fe10..95ddf5b743d0 100644
> >> --- a/mm/alloc_tag.c
> >> +++ b/mm/alloc_tag.c
> >> @@ -804,6 +804,13 @@ static int vm_module_tags_populate(void)
> >>                                      next_page, PAGE_SHIFT) < 0) {
> >>                         release_pages_arg arg = { .pages = next_page };
> >>
> >> +                       /*
> >> +                        * vmap_pages_range() only runs once all pages were
> >> +                        * allocated, and it may have installed some mappings
> >> +                        * before failing. Undo them.
> >
> > If vmap_pages_range() failed to map the range, why should we need to
> > undo it? If it indeed leaves some partial mapping when failing then I
> > would argue that vmap_pages_range() should be undoing these partial
> > mappings itself before returning the error.
> >
> Yeah it does - none of the paths under vmap_pages_range() undo themselves on error.

Hmm. I wonder if this behavior is intentional or requires a fix. The
only possible reason I can think of is performance but I can't imagine
a failure to map is a performance-critical case.

+David Hildenbrand (Red Hat), +Lorenzo Stoakes (Oracle) what do you
guys think? Shouldn't vmap_pages_range() undo its possible partial
mapping when it fails?


> pcpu_map_pages does perform cleanup on its own as well. There may be other similar sites.
> https://elixir.bootlin.com/linux/v7.3-rc3/source/mm/percpu-vm.c#L255
> so I did the same here.
>
> If we really want vmap_pages_range() to handle this cleanup internally, I believe that would
> be a relatively large change, and we would need to consider many more cases. So I think we keep
> the current behaviour for now.

Yeah, I understand it would require a deeper cut but if that's the
right way to handle this we might as well do that. Let's see what MM
folks think.

>
> Thanks
> Best Regards
> Hao
>
> >> +                        */
> >> +                       if (nr == more_pages)
> >> +                               vunmap_range(phys_end, phys_end + (nr << PAGE_SHIFT));
> >>                         /* Clean up and error out */
> >>                         release_pages(arg, nr);
> >>                         return -ENOMEM;
> >> @@ -947,6 +954,7 @@ static void *reserve_module_tags(struct module *mod, unsigned long size,
> >>                 return ret;
> >>
> >>         if (module_tags.size < offset + size) {
> >> +               unsigned long prev_size = module_tags.size;
> >>                 int grow_res;
> >>
> >>                 module_tags.size = offset + size;
> >> @@ -961,6 +969,8 @@ static void *reserve_module_tags(struct module *mod, unsigned long size,
> >>                         shutdown_mem_profiling(true);
> >>                         pr_err("Failed to allocate memory for allocation tags in the module %s. Memory allocation profiling is disabled!\n",
> >>                                mod->name);
> >> +                       release_module_tags(mod, false);
> >> +                       module_tags.size = prev_size;
> >
> > Yes, this one is a valid fix. It fixes 0f9b685626da commit.
> >
> >>                         return ERR_PTR(grow_res);
> >>                 }
> >>         }
> >> --
> >> 2.25.1
> >>

Re: [PATCH v10 2/6] alloc_tag: clean up the populate failure path

[Suren Baghdasaryan]

On Wed, Sep 16, 2026 at 9:31 AM Suren Baghdasaryan <surenb@google.com> wrote:
>
> On Tue, Sep 15, 2026 at 11:00 PM Hao Ge <hao.ge@linux.dev> wrote:
> >
> > Hi Suren
> >
> > Thanks for you review.
> >
> > On 2026/9/16 05:09, Suren Baghdasaryan wrote:
> > > On Mon, Sep 14, 2026 at 11:59 PM Hao Ge <hao.ge@linux.dev> wrote:
> > >>
> > >> The reservation is already stored in the maple tree when
> > >> vm_module_tags_populate() fails. A failed load never unloads the
> > >> module, so nothing releases the entry. Release it and roll
> > >> module_tags.size back. Without the rollback a concurrent load that
> > >> already passed needs_section_mem() can reuse the freed gap, skip
> > >> vm_module_tags_populate() and write to unmapped memory.
> > >>
> > >> vmap_pages_range() may have installed some PTEs before failing. A
> > >> retry to populate the same range would BUG on them, so undo them,
> > >> but only if vmap actually ran.
> > >>
> > >> Fixes: 4835f747d3ed ("alloc_tag: support for page allocation tag compression")
> > >> Fixes: 0f9b685626da ("alloc_tag: populate memory for module tags as needed")
> > >
> > > If a patch fixes two other patches, it likely needs to be split so
> > > that each part fixes one patch only. But read on please. I'm not sure
> > > if one of the fixes is correct.
> > >
> >
> > Kept them in one patch because they only make sense together.
> >
> > After a failed populate the reservation is released and the
> > size rolled back, so the next load takes the same gap and calls
> > vm_module_tags_populate() again, and that retry would trip over
> > the PTEs the failed vmap left behind.
> >
> > Without the rollback a load that already passed needs_section_mem()
> > just reuses the gap, skips populate and writes to unmapped memory.
> > So splitting them leaves each half broken.
>
> Yeah, that's why I think vmap_pages_range() cleanup should be a
> completely separate patch with vmap_pages_range() doing cleanup
> itself.
>
> >
> > >> Reported-by: Sashiko <sashiko-bot@kernel.org>
> > >> Cc: stable@vger.kernel.org
> > >> Signed-off-by: Hao Ge <hao.ge@linux.dev>
> > >> ---
> > >>  mm/alloc_tag.c | 10 ++++++++++
> > >>  1 file changed, 10 insertions(+)
> > >>
> > >> diff --git a/mm/alloc_tag.c b/mm/alloc_tag.c
> > >> index 2070e682fe10..95ddf5b743d0 100644
> > >> --- a/mm/alloc_tag.c
> > >> +++ b/mm/alloc_tag.c
> > >> @@ -804,6 +804,13 @@ static int vm_module_tags_populate(void)
> > >>                                      next_page, PAGE_SHIFT) < 0) {
> > >>                         release_pages_arg arg = { .pages = next_page };
> > >>
> > >> +                       /*
> > >> +                        * vmap_pages_range() only runs once all pages were
> > >> +                        * allocated, and it may have installed some mappings
> > >> +                        * before failing. Undo them.
> > >
> > > If vmap_pages_range() failed to map the range, why should we need to
> > > undo it? If it indeed leaves some partial mapping when failing then I
> > > would argue that vmap_pages_range() should be undoing these partial
> > > mappings itself before returning the error.
> > >
> > Yeah it does - none of the paths under vmap_pages_range() undo themselves on error.
>
> Hmm. I wonder if this behavior is intentional or requires a fix. The
> only possible reason I can think of is performance but I can't imagine
> a failure to map is a performance-critical case.
>
> +David Hildenbrand (Red Hat), +Lorenzo Stoakes (Oracle) what do you

Huh, my gmail thinks you are still at your previous companies :)

> guys think? Shouldn't vmap_pages_range() undo its possible partial
> mapping when it fails?
>
>
> > pcpu_map_pages does perform cleanup on its own as well. There may be other similar sites.
> > https://elixir.bootlin.com/linux/v7.3-rc3/source/mm/percpu-vm.c#L255
> > so I did the same here.
> >
> > If we really want vmap_pages_range() to handle this cleanup internally, I believe that would
> > be a relatively large change, and we would need to consider many more cases. So I think we keep
> > the current behaviour for now.
>
> Yeah, I understand it would require a deeper cut but if that's the
> right way to handle this we might as well do that. Let's see what MM
> folks think.
>
> >
> > Thanks
> > Best Regards
> > Hao
> >
> > >> +                        */
> > >> +                       if (nr == more_pages)
> > >> +                               vunmap_range(phys_end, phys_end + (nr << PAGE_SHIFT));
> > >>                         /* Clean up and error out */
> > >>                         release_pages(arg, nr);
> > >>                         return -ENOMEM;
> > >> @@ -947,6 +954,7 @@ static void *reserve_module_tags(struct module *mod, unsigned long size,
> > >>                 return ret;
> > >>
> > >>         if (module_tags.size < offset + size) {
> > >> +               unsigned long prev_size = module_tags.size;
> > >>                 int grow_res;
> > >>
> > >>                 module_tags.size = offset + size;
> > >> @@ -961,6 +969,8 @@ static void *reserve_module_tags(struct module *mod, unsigned long size,
> > >>                         shutdown_mem_profiling(true);
> > >>                         pr_err("Failed to allocate memory for allocation tags in the module %s. Memory allocation profiling is disabled!\n",
> > >>                                mod->name);
> > >> +                       release_module_tags(mod, false);
> > >> +                       module_tags.size = prev_size;
> > >
> > > Yes, this one is a valid fix. It fixes 0f9b685626da commit.
> > >
> > >>                         return ERR_PTR(grow_res);
> > >>                 }
> > >>         }
> > >> --
> > >> 2.25.1
> > >>

Re: [PATCH v10 2/6] alloc_tag: clean up the populate failure path

[Hao Ge]

Hi Suren

On 2026/9/17 03:07, Suren Baghdasaryan wrote:
> On Wed, Sep 16, 2026 at 9:31 AM Suren Baghdasaryan <surenb@google.com> wrote:
>>
>> On Tue, Sep 15, 2026 at 11:00 PM Hao Ge <hao.ge@linux.dev> wrote:
>>>
>>> Hi Suren
>>>
>>> Thanks for you review.
>>>
>>> On 2026/9/16 05:09, Suren Baghdasaryan wrote:
>>>> On Mon, Sep 14, 2026 at 11:59 PM Hao Ge <hao.ge@linux.dev> wrote:
>>>>>
>>>>> The reservation is already stored in the maple tree when
>>>>> vm_module_tags_populate() fails. A failed load never unloads the
>>>>> module, so nothing releases the entry. Release it and roll
>>>>> module_tags.size back. Without the rollback a concurrent load that
>>>>> already passed needs_section_mem() can reuse the freed gap, skip
>>>>> vm_module_tags_populate() and write to unmapped memory.
>>>>>
>>>>> vmap_pages_range() may have installed some PTEs before failing. A
>>>>> retry to populate the same range would BUG on them, so undo them,
>>>>> but only if vmap actually ran.
>>>>>
>>>>> Fixes: 4835f747d3ed ("alloc_tag: support for page allocation tag compression")
>>>>> Fixes: 0f9b685626da ("alloc_tag: populate memory for module tags as needed")
>>>>
>>>> If a patch fixes two other patches, it likely needs to be split so
>>>> that each part fixes one patch only. But read on please. I'm not sure
>>>> if one of the fixes is correct.
>>>>
>>>
>>> Kept them in one patch because they only make sense together.
>>>
>>> After a failed populate the reservation is released and the
>>> size rolled back, so the next load takes the same gap and calls
>>> vm_module_tags_populate() again, and that retry would trip over
>>> the PTEs the failed vmap left behind.
>>>
>>> Without the rollback a load that already passed needs_section_mem()
>>> just reuses the gap, skips populate and writes to unmapped memory.
>>> So splitting them leaves each half broken.
>>
>> Yeah, that's why I think vmap_pages_range() cleanup should be a
>> completely separate patch with vmap_pages_range() doing cleanup
>> itself.
>>

I think I get where you're coming from now.
Thanks for taking the time to explain.

>>>
>>>>> Reported-by: Sashiko <sashiko-bot@kernel.org>
>>>>> Cc: stable@vger.kernel.org
>>>>> Signed-off-by: Hao Ge <hao.ge@linux.dev>
>>>>> ---
>>>>>  mm/alloc_tag.c | 10 ++++++++++
>>>>>  1 file changed, 10 insertions(+)
>>>>>
>>>>> diff --git a/mm/alloc_tag.c b/mm/alloc_tag.c
>>>>> index 2070e682fe10..95ddf5b743d0 100644
>>>>> --- a/mm/alloc_tag.c
>>>>> +++ b/mm/alloc_tag.c
>>>>> @@ -804,6 +804,13 @@ static int vm_module_tags_populate(void)
>>>>>                                      next_page, PAGE_SHIFT) < 0) {
>>>>>                         release_pages_arg arg = { .pages = next_page };
>>>>>
>>>>> +                       /*
>>>>> +                        * vmap_pages_range() only runs once all pages were
>>>>> +                        * allocated, and it may have installed some mappings
>>>>> +                        * before failing. Undo them.
>>>>
>>>> If vmap_pages_range() failed to map the range, why should we need to
>>>> undo it? If it indeed leaves some partial mapping when failing then I
>>>> would argue that vmap_pages_range() should be undoing these partial
>>>> mappings itself before returning the error.
>>>>
>>> Yeah it does - none of the paths under vmap_pages_range() undo themselves on error.
>>
>> Hmm. I wonder if this behavior is intentional or requires a fix. The
>> only possible reason I can think of is performance but I can't imagine
>> a failure to map is a performance-critical case.
>>
>> +David Hildenbrand (Red Hat), +Lorenzo Stoakes (Oracle) what do you
> 
> Huh, my gmail thinks you are still at your previous companies :)
> 

I manually fixed it on the mailing list for this thread.

>> guys think? Shouldn't vmap_pages_range() undo its possible partial
>> mapping when it fails?
>>
>>
>>> pcpu_map_pages does perform cleanup on its own as well. There may be other similar sites.
>>> https://elixir.bootlin.com/linux/v7.3-rc3/source/mm/percpu-vm.c#L255
>>> so I did the same here.
>>>
>>> If we really want vmap_pages_range() to handle this cleanup internally, I believe that would
>>> be a relatively large change, and we would need to consider many more cases. So I think we keep
>>> the current behaviour for now.
>>
>> Yeah, I understand it would require a deeper cut but if that's the
>> right way to handle this we might as well do that. Let's see what MM
>> folks think.
>>

Sounds good to me. Let's wait and hear what the MM folks say.

Thanks
Best Regards
Hao

>>>
>>> Thanks
>>> Best Regards
>>> Hao
>>>
>>>>> +                        */
>>>>> +                       if (nr == more_pages)
>>>>> +                               vunmap_range(phys_end, phys_end + (nr << PAGE_SHIFT));
>>>>>                         /* Clean up and error out */
>>>>>                         release_pages(arg, nr);
>>>>>                         return -ENOMEM;
>>>>> @@ -947,6 +954,7 @@ static void *reserve_module_tags(struct module *mod, unsigned long size,
>>>>>                 return ret;
>>>>>
>>>>>         if (module_tags.size < offset + size) {
>>>>> +               unsigned long prev_size = module_tags.size;
>>>>>                 int grow_res;
>>>>>
>>>>>                 module_tags.size = offset + size;
>>>>> @@ -961,6 +969,8 @@ static void *reserve_module_tags(struct module *mod, unsigned long size,
>>>>>                         shutdown_mem_profiling(true);
>>>>>                         pr_err("Failed to allocate memory for allocation tags in the module %s. Memory allocation profiling is disabled!\n",
>>>>>                                mod->name);
>>>>> +                       release_module_tags(mod, false);
>>>>> +                       module_tags.size = prev_size;
>>>>
>>>> Yes, this one is a valid fix. It fixes 0f9b685626da commit.
>>>>
>>>>>                         return ERR_PTR(grow_res);
>>>>>                 }
>>>>>         }
>>>>> --
>>>>> 2.25.1
>>>>>

Re: [PATCH v10 2/6] alloc_tag: clean up the populate failure path

[Hao Ge]

Hi Suren、David、Lorenzo

On 2026/9/17 09:12, Hao Ge wrote:
> Hi Suren
> 
> On 2026/9/17 03:07, Suren Baghdasaryan wrote:
>> On Wed, Sep 16, 2026 at 9:31 AM Suren Baghdasaryan <surenb@google.com> wrote:
>>>
>>> On Tue, Sep 15, 2026 at 11:00 PM Hao Ge <hao.ge@linux.dev> wrote:
>>>>
>>>> Hi Suren
>>>>
>>>> Thanks for you review.
>>>>
>>>> On 2026/9/16 05:09, Suren Baghdasaryan wrote:
>>>>> On Mon, Sep 14, 2026 at 11:59 PM Hao Ge <hao.ge@linux.dev> wrote:
>>>>>>
>>>>>> The reservation is already stored in the maple tree when
>>>>>> vm_module_tags_populate() fails. A failed load never unloads the
>>>>>> module, so nothing releases the entry. Release it and roll
>>>>>> module_tags.size back. Without the rollback a concurrent load that
>>>>>> already passed needs_section_mem() can reuse the freed gap, skip
>>>>>> vm_module_tags_populate() and write to unmapped memory.
>>>>>>
>>>>>> vmap_pages_range() may have installed some PTEs before failing. A
>>>>>> retry to populate the same range would BUG on them, so undo them,
>>>>>> but only if vmap actually ran.
>>>>>>
>>>>>> Fixes: 4835f747d3ed ("alloc_tag: support for page allocation tag compression")
>>>>>> Fixes: 0f9b685626da ("alloc_tag: populate memory for module tags as needed")
>>>>>
>>>>> If a patch fixes two other patches, it likely needs to be split so
>>>>> that each part fixes one patch only. But read on please. I'm not sure
>>>>> if one of the fixes is correct.
>>>>>
>>>>
>>>> Kept them in one patch because they only make sense together.
>>>>
>>>> After a failed populate the reservation is released and the
>>>> size rolled back, so the next load takes the same gap and calls
>>>> vm_module_tags_populate() again, and that retry would trip over
>>>> the PTEs the failed vmap left behind.
>>>>
>>>> Without the rollback a load that already passed needs_section_mem()
>>>> just reuses the gap, skips populate and writes to unmapped memory.
>>>> So splitting them leaves each half broken.
>>>
>>> Yeah, that's why I think vmap_pages_range() cleanup should be a
>>> completely separate patch with vmap_pages_range() doing cleanup
>>> itself.
>>>
> 
> I think I get where you're coming from now.
> Thanks for taking the time to explain.
> 
>>>>
>>>>>> Reported-by: Sashiko <sashiko-bot@kernel.org>
>>>>>> Cc: stable@vger.kernel.org
>>>>>> Signed-off-by: Hao Ge <hao.ge@linux.dev>
>>>>>> ---
>>>>>>  mm/alloc_tag.c | 10 ++++++++++
>>>>>>  1 file changed, 10 insertions(+)
>>>>>>
>>>>>> diff --git a/mm/alloc_tag.c b/mm/alloc_tag.c
>>>>>> index 2070e682fe10..95ddf5b743d0 100644
>>>>>> --- a/mm/alloc_tag.c
>>>>>> +++ b/mm/alloc_tag.c
>>>>>> @@ -804,6 +804,13 @@ static int vm_module_tags_populate(void)
>>>>>>                                      next_page, PAGE_SHIFT) < 0) {
>>>>>>                         release_pages_arg arg = { .pages = next_page };
>>>>>>
>>>>>> +                       /*
>>>>>> +                        * vmap_pages_range() only runs once all pages were
>>>>>> +                        * allocated, and it may have installed some mappings
>>>>>> +                        * before failing. Undo them.
>>>>>
>>>>> If vmap_pages_range() failed to map the range, why should we need to
>>>>> undo it? If it indeed leaves some partial mapping when failing then I
>>>>> would argue that vmap_pages_range() should be undoing these partial
>>>>> mappings itself before returning the error.
>>>>>
>>>> Yeah it does - none of the paths under vmap_pages_range() undo themselves on error.
>>>
>>> Hmm. I wonder if this behavior is intentional or requires a fix. The
>>> only possible reason I can think of is performance but I can't imagine
>>> a failure to map is a performance-critical case.
>>>
>>> +David Hildenbrand (Red Hat), +Lorenzo Stoakes (Oracle) what do you
>>
>> Huh, my gmail thinks you are still at your previous companies :)
>>
> 
> I manually fixed it on the mailing list for this thread.
> 
>>> guys think? Shouldn't vmap_pages_range() undo its possible partial
>>> mapping when it fails?
>>>
>>>
>>>> pcpu_map_pages does perform cleanup on its own as well. There may be other similar sites.
>>>> https://elixir.bootlin.com/linux/v7.3-rc3/source/mm/percpu-vm.c#L255
>>>> so I did the same here.
>>>>
>>>> If we really want vmap_pages_range() to handle this cleanup internally, I believe that would
>>>> be a relatively large change, and we would need to consider many more cases. So I think we keep
>>>> the current behaviour for now.
>>>
>>> Yeah, I understand it would require a deeper cut but if that's the
>>> right way to handle this we might as well do that. Let's see what MM
>>> folks think.
>>>
> 
> Sounds good to me. Let's wait and hear what the MM folks say.
> 

I just looked into the call sites for __vmap_pages_range.
And I found there's a bug in one vmalloc caller [1].
When gfp_mask has __GFP_NOFAIL, the code keeps retrying
__vmap_pages_range. If there are leftover PTE mappings,
this can trigger a BUG(); [2]
https://elixir.bootlin.com/linux/v7.3-rc3/source/mm/vmalloc.c#L3946 [1]
https://elixir.bootlin.com/linux/v7.3-rc3/source/mm/vmalloc.c#L121  [2]

> Thanks
> Best Regards
> Hao
> 
>>>>
>>>> Thanks
>>>> Best Regards
>>>> Hao
>>>>
>>>>>> +                        */
>>>>>> +                       if (nr == more_pages)
>>>>>> +                               vunmap_range(phys_end, phys_end + (nr << PAGE_SHIFT));
>>>>>>                         /* Clean up and error out */
>>>>>>                         release_pages(arg, nr);
>>>>>>                         return -ENOMEM;
>>>>>> @@ -947,6 +954,7 @@ static void *reserve_module_tags(struct module *mod, unsigned long size,
>>>>>>                 return ret;
>>>>>>
>>>>>>         if (module_tags.size < offset + size) {
>>>>>> +               unsigned long prev_size = module_tags.size;
>>>>>>                 int grow_res;
>>>>>>
>>>>>>                 module_tags.size = offset + size;
>>>>>> @@ -961,6 +969,8 @@ static void *reserve_module_tags(struct module *mod, unsigned long size,
>>>>>>                         shutdown_mem_profiling(true);
>>>>>>                         pr_err("Failed to allocate memory for allocation tags in the module %s. Memory allocation profiling is disabled!\n",
>>>>>>                                mod->name);
>>>>>> +                       release_module_tags(mod, false);
>>>>>> +                       module_tags.size = prev_size;
>>>>>
>>>>> Yes, this one is a valid fix. It fixes 0f9b685626da commit.
>>>>>
>>>>>>                         return ERR_PTR(grow_res);
>>>>>>                 }
>>>>>>         }
>>>>>> --
>>>>>> 2.25.1
>>>>>>

Re: [PATCH v10 2/6] alloc_tag: clean up the populate failure path

[Uladzislau Rezki]

On Thu, Sep 17, 2026 at 10:32:48AM +0800, Hao Ge wrote:
> Hi Suren、David、Lorenzo
> 
> On 2026/9/17 09:12, Hao Ge wrote:
> > Hi Suren
> > 
> > On 2026/9/17 03:07, Suren Baghdasaryan wrote:
> >> On Wed, Sep 16, 2026 at 9:31 AM Suren Baghdasaryan <surenb@google.com> wrote:
> >>>
> >>> On Tue, Sep 15, 2026 at 11:00 PM Hao Ge <hao.ge@linux.dev> wrote:
> >>>>
> >>>> Hi Suren
> >>>>
> >>>> Thanks for you review.
> >>>>
> >>>> On 2026/9/16 05:09, Suren Baghdasaryan wrote:
> >>>>> On Mon, Sep 14, 2026 at 11:59 PM Hao Ge <hao.ge@linux.dev> wrote:
> >>>>>>
> >>>>>> The reservation is already stored in the maple tree when
> >>>>>> vm_module_tags_populate() fails. A failed load never unloads the
> >>>>>> module, so nothing releases the entry. Release it and roll
> >>>>>> module_tags.size back. Without the rollback a concurrent load that
> >>>>>> already passed needs_section_mem() can reuse the freed gap, skip
> >>>>>> vm_module_tags_populate() and write to unmapped memory.
> >>>>>>
> >>>>>> vmap_pages_range() may have installed some PTEs before failing. A
> >>>>>> retry to populate the same range would BUG on them, so undo them,
> >>>>>> but only if vmap actually ran.
> >>>>>>
> >>>>>> Fixes: 4835f747d3ed ("alloc_tag: support for page allocation tag compression")
> >>>>>> Fixes: 0f9b685626da ("alloc_tag: populate memory for module tags as needed")
> >>>>>
> >>>>> If a patch fixes two other patches, it likely needs to be split so
> >>>>> that each part fixes one patch only. But read on please. I'm not sure
> >>>>> if one of the fixes is correct.
> >>>>>
> >>>>
> >>>> Kept them in one patch because they only make sense together.
> >>>>
> >>>> After a failed populate the reservation is released and the
> >>>> size rolled back, so the next load takes the same gap and calls
> >>>> vm_module_tags_populate() again, and that retry would trip over
> >>>> the PTEs the failed vmap left behind.
> >>>>
> >>>> Without the rollback a load that already passed needs_section_mem()
> >>>> just reuses the gap, skips populate and writes to unmapped memory.
> >>>> So splitting them leaves each half broken.
> >>>
> >>> Yeah, that's why I think vmap_pages_range() cleanup should be a
> >>> completely separate patch with vmap_pages_range() doing cleanup
> >>> itself.
> >>>
> > 
> > I think I get where you're coming from now.
> > Thanks for taking the time to explain.
> > 
> >>>>
> >>>>>> Reported-by: Sashiko <sashiko-bot@kernel.org>
> >>>>>> Cc: stable@vger.kernel.org
> >>>>>> Signed-off-by: Hao Ge <hao.ge@linux.dev>
> >>>>>> ---
> >>>>>>  mm/alloc_tag.c | 10 ++++++++++
> >>>>>>  1 file changed, 10 insertions(+)
> >>>>>>
> >>>>>> diff --git a/mm/alloc_tag.c b/mm/alloc_tag.c
> >>>>>> index 2070e682fe10..95ddf5b743d0 100644
> >>>>>> --- a/mm/alloc_tag.c
> >>>>>> +++ b/mm/alloc_tag.c
> >>>>>> @@ -804,6 +804,13 @@ static int vm_module_tags_populate(void)
> >>>>>>                                      next_page, PAGE_SHIFT) < 0) {
> >>>>>>                         release_pages_arg arg = { .pages = next_page };
> >>>>>>
> >>>>>> +                       /*
> >>>>>> +                        * vmap_pages_range() only runs once all pages were
> >>>>>> +                        * allocated, and it may have installed some mappings
> >>>>>> +                        * before failing. Undo them.
> >>>>>
> >>>>> If vmap_pages_range() failed to map the range, why should we need to
> >>>>> undo it? If it indeed leaves some partial mapping when failing then I
> >>>>> would argue that vmap_pages_range() should be undoing these partial
> >>>>> mappings itself before returning the error.
> >>>>>
> >>>> Yeah it does - none of the paths under vmap_pages_range() undo themselves on error.
> >>>
> >>> Hmm. I wonder if this behavior is intentional or requires a fix. The
> >>> only possible reason I can think of is performance but I can't imagine
> >>> a failure to map is a performance-critical case.
> >>>
> >>> +David Hildenbrand (Red Hat), +Lorenzo Stoakes (Oracle) what do you
> >>
> >> Huh, my gmail thinks you are still at your previous companies :)
> >>
> > 
> > I manually fixed it on the mailing list for this thread.
> > 
> >>> guys think? Shouldn't vmap_pages_range() undo its possible partial
> >>> mapping when it fails?
> >>>
> >>>
> >>>> pcpu_map_pages does perform cleanup on its own as well. There may be other similar sites.
> >>>> https://elixir.bootlin.com/linux/v7.3-rc3/source/mm/percpu-vm.c#L255
> >>>> so I did the same here.
> >>>>
> >>>> If we really want vmap_pages_range() to handle this cleanup internally, I believe that would
> >>>> be a relatively large change, and we would need to consider many more cases. So I think we keep
> >>>> the current behaviour for now.
> >>>
> >>> Yeah, I understand it would require a deeper cut but if that's the
> >>> right way to handle this we might as well do that. Let's see what MM
> >>> folks think.
> >>>
> > 
> > Sounds good to me. Let's wait and hear what the MM folks say.
> > 
> 
> I just looked into the call sites for __vmap_pages_range.
> And I found there's a bug in one vmalloc caller [1].
> When gfp_mask has __GFP_NOFAIL, the code keeps retrying
> __vmap_pages_range. If there are leftover PTE mappings,
> this can trigger a BUG(); [2]
> https://elixir.bootlin.com/linux/v7.3-rc3/source/mm/vmalloc.c#L3946 [1]
> https://elixir.bootlin.com/linux/v7.3-rc3/source/mm/vmalloc.c#L121  [2]
> 
Indeed the below code is incomplete:

<snip>
	/*
	 * page tables allocations ignore external gfp mask, enforce it
	 * by the scope API
	 */
	flags = memalloc_apply_gfp_scope(gfp_mask);
	do {
		ret = __vmap_pages_range(addr, addr + size, prot, area->pages,
				page_shift, nested_gfp);
		if (nofail && (ret < 0))
			schedule_timeout_uninterruptible(1);
	} while (nofail && (ret < 0));
	memalloc_restore_scope(flags);
<snip>

and it should be fixed. As the __vmap_pages_range() can actually fail
and leave some installed mappings, we have to perform some cleanups
on failure or on repeat(if nofail is true).

Below code:

+			/*
+			 * vmap_pages_range() only runs once all pages were
+			 * allocated, and it may have installed some mappings
+			 * before failing. Undo them.
+			 */
+			if (nr == more_pages)
+				vunmap_range(phys_end, phys_end + (nr << PAGE_SHIFT));

we can use but we need to check if partially mapped space is not an
issue for the vunmap_range(). And it looks like it is not.

<snip>
@@ -3953,8 +3990,15 @@ static void *__vmalloc_area_node(struct vm_struct *area, gfp_t gfp_mask,
        do {
                ret = __vmap_pages_range(addr, addr + size, prot, area->pages,
                                page_shift, nested_gfp);
-               if (nofail && (ret < 0))
-                       schedule_timeout_uninterruptible(1);
+               if (ret < 0) {
+                       /*
+                        * __vmap_pages_range() may have installed some mappings
+                        * before failing. Undo them before returning or retrying.
+                        */
+                       vunmap_range(addr, addr + size);
+                       if (nofail)
+                               schedule_timeout_uninterruptible(1);
+               }
        } while (nofail && (ret < 0));
        memalloc_restore_scope(flags);
<snip>

--
Uladzislau Rezki

Re: [PATCH v10 2/6] alloc_tag: clean up the populate failure path

[Uladzislau Rezki]

On Mon, Sep 28, 2026 at 01:09:13PM +0200, Uladzislau Rezki wrote:
> On Thu, Sep 17, 2026 at 10:32:48AM +0800, Hao Ge wrote:
> > Hi Suren、David、Lorenzo
> > 
> > On 2026/9/17 09:12, Hao Ge wrote:
> > > Hi Suren
> > > 
> > > On 2026/9/17 03:07, Suren Baghdasaryan wrote:
> > >> On Wed, Sep 16, 2026 at 9:31 AM Suren Baghdasaryan <surenb@google.com> wrote:
> > >>>
> > >>> On Tue, Sep 15, 2026 at 11:00 PM Hao Ge <hao.ge@linux.dev> wrote:
> > >>>>
> > >>>> Hi Suren
> > >>>>
> > >>>> Thanks for you review.
> > >>>>
> > >>>> On 2026/9/16 05:09, Suren Baghdasaryan wrote:
> > >>>>> On Mon, Sep 14, 2026 at 11:59 PM Hao Ge <hao.ge@linux.dev> wrote:
> > >>>>>>
> > >>>>>> The reservation is already stored in the maple tree when
> > >>>>>> vm_module_tags_populate() fails. A failed load never unloads the
> > >>>>>> module, so nothing releases the entry. Release it and roll
> > >>>>>> module_tags.size back. Without the rollback a concurrent load that
> > >>>>>> already passed needs_section_mem() can reuse the freed gap, skip
> > >>>>>> vm_module_tags_populate() and write to unmapped memory.
> > >>>>>>
> > >>>>>> vmap_pages_range() may have installed some PTEs before failing. A
> > >>>>>> retry to populate the same range would BUG on them, so undo them,
> > >>>>>> but only if vmap actually ran.
> > >>>>>>
> > >>>>>> Fixes: 4835f747d3ed ("alloc_tag: support for page allocation tag compression")
> > >>>>>> Fixes: 0f9b685626da ("alloc_tag: populate memory for module tags as needed")
> > >>>>>
> > >>>>> If a patch fixes two other patches, it likely needs to be split so
> > >>>>> that each part fixes one patch only. But read on please. I'm not sure
> > >>>>> if one of the fixes is correct.
> > >>>>>
> > >>>>
> > >>>> Kept them in one patch because they only make sense together.
> > >>>>
> > >>>> After a failed populate the reservation is released and the
> > >>>> size rolled back, so the next load takes the same gap and calls
> > >>>> vm_module_tags_populate() again, and that retry would trip over
> > >>>> the PTEs the failed vmap left behind.
> > >>>>
> > >>>> Without the rollback a load that already passed needs_section_mem()
> > >>>> just reuses the gap, skips populate and writes to unmapped memory.
> > >>>> So splitting them leaves each half broken.
> > >>>
> > >>> Yeah, that's why I think vmap_pages_range() cleanup should be a
> > >>> completely separate patch with vmap_pages_range() doing cleanup
> > >>> itself.
> > >>>
> > > 
> > > I think I get where you're coming from now.
> > > Thanks for taking the time to explain.
> > > 
> > >>>>
> > >>>>>> Reported-by: Sashiko <sashiko-bot@kernel.org>
> > >>>>>> Cc: stable@vger.kernel.org
> > >>>>>> Signed-off-by: Hao Ge <hao.ge@linux.dev>
> > >>>>>> ---
> > >>>>>>  mm/alloc_tag.c | 10 ++++++++++
> > >>>>>>  1 file changed, 10 insertions(+)
> > >>>>>>
> > >>>>>> diff --git a/mm/alloc_tag.c b/mm/alloc_tag.c
> > >>>>>> index 2070e682fe10..95ddf5b743d0 100644
> > >>>>>> --- a/mm/alloc_tag.c
> > >>>>>> +++ b/mm/alloc_tag.c
> > >>>>>> @@ -804,6 +804,13 @@ static int vm_module_tags_populate(void)
> > >>>>>>                                      next_page, PAGE_SHIFT) < 0) {
> > >>>>>>                         release_pages_arg arg = { .pages = next_page };
> > >>>>>>
> > >>>>>> +                       /*
> > >>>>>> +                        * vmap_pages_range() only runs once all pages were
> > >>>>>> +                        * allocated, and it may have installed some mappings
> > >>>>>> +                        * before failing. Undo them.
> > >>>>>
> > >>>>> If vmap_pages_range() failed to map the range, why should we need to
> > >>>>> undo it? If it indeed leaves some partial mapping when failing then I
> > >>>>> would argue that vmap_pages_range() should be undoing these partial
> > >>>>> mappings itself before returning the error.
> > >>>>>
> > >>>> Yeah it does - none of the paths under vmap_pages_range() undo themselves on error.
> > >>>
> > >>> Hmm. I wonder if this behavior is intentional or requires a fix. The
> > >>> only possible reason I can think of is performance but I can't imagine
> > >>> a failure to map is a performance-critical case.
> > >>>
> > >>> +David Hildenbrand (Red Hat), +Lorenzo Stoakes (Oracle) what do you
> > >>
> > >> Huh, my gmail thinks you are still at your previous companies :)
> > >>
> > > 
> > > I manually fixed it on the mailing list for this thread.
> > > 
> > >>> guys think? Shouldn't vmap_pages_range() undo its possible partial
> > >>> mapping when it fails?
> > >>>
> > >>>
> > >>>> pcpu_map_pages does perform cleanup on its own as well. There may be other similar sites.
> > >>>> https://elixir.bootlin.com/linux/v7.3-rc3/source/mm/percpu-vm.c#L255
> > >>>> so I did the same here.
> > >>>>
> > >>>> If we really want vmap_pages_range() to handle this cleanup internally, I believe that would
> > >>>> be a relatively large change, and we would need to consider many more cases. So I think we keep
> > >>>> the current behaviour for now.
> > >>>
> > >>> Yeah, I understand it would require a deeper cut but if that's the
> > >>> right way to handle this we might as well do that. Let's see what MM
> > >>> folks think.
> > >>>
> > > 
> > > Sounds good to me. Let's wait and hear what the MM folks say.
> > > 
> > 
> > I just looked into the call sites for __vmap_pages_range.
> > And I found there's a bug in one vmalloc caller [1].
> > When gfp_mask has __GFP_NOFAIL, the code keeps retrying
> > __vmap_pages_range. If there are leftover PTE mappings,
> > this can trigger a BUG(); [2]
> > https://elixir.bootlin.com/linux/v7.3-rc3/source/mm/vmalloc.c#L3946 [1]
> > https://elixir.bootlin.com/linux/v7.3-rc3/source/mm/vmalloc.c#L121  [2]
> > 
> Indeed the below code is incomplete:
> 
> <snip>
> 	/*
> 	 * page tables allocations ignore external gfp mask, enforce it
> 	 * by the scope API
> 	 */
> 	flags = memalloc_apply_gfp_scope(gfp_mask);
> 	do {
> 		ret = __vmap_pages_range(addr, addr + size, prot, area->pages,
> 				page_shift, nested_gfp);
> 		if (nofail && (ret < 0))
> 			schedule_timeout_uninterruptible(1);
> 	} while (nofail && (ret < 0));
> 	memalloc_restore_scope(flags);
> <snip>
> 
> and it should be fixed. As the __vmap_pages_range() can actually fail
> and leave some installed mappings, we have to perform some cleanups
> on failure or on repeat(if nofail is true).
> 
> Below code:
> 
> +			/*
> +			 * vmap_pages_range() only runs once all pages were
> +			 * allocated, and it may have installed some mappings
> +			 * before failing. Undo them.
> +			 */
> +			if (nr == more_pages)
> +				vunmap_range(phys_end, phys_end + (nr << PAGE_SHIFT));
> 
> we can use but we need to check if partially mapped space is not an
> issue for the vunmap_range(). And it looks like it is not.
> 
> <snip>
> @@ -3953,8 +3990,15 @@ static void *__vmalloc_area_node(struct vm_struct *area, gfp_t gfp_mask,
>         do {
>                 ret = __vmap_pages_range(addr, addr + size, prot, area->pages,
>                                 page_shift, nested_gfp);
> -               if (nofail && (ret < 0))
> -                       schedule_timeout_uninterruptible(1);
> +               if (ret < 0) {
> +                       /*
> +                        * __vmap_pages_range() may have installed some mappings
> +                        * before failing. Undo them before returning or retrying.
> +                        */
> +                       vunmap_range(addr, addr + size);
> +                       if (nofail)
> +                               schedule_timeout_uninterruptible(1);
> +               }
>         } while (nofail && (ret < 0));
>         memalloc_restore_scope(flags);
> <snip>
> 
And looking further. If the __vmap_pages_range()/__vmap_pages_range_noflush()
and friends fail, a proper cleanups should be done inside, IMO.

But i am checking callers and see that some do some not:

<snip hooks.c>
	__vunmap_range_noflush(
		vmalloc_shadow(start),
		vmalloc_shadow(start + clean * PAGE_SIZE));
	__vunmap_range_noflush(
		vmalloc_origin(start),
		vmalloc_origin(start + clean * PAGE_SIZE));
<snip hooks.c>

kmsan_ioremap_page_range() it does clean ups;
kmsan_vmap_pages_range_noflush() looks like does not care in shadow.c;
__vmalloc_area_node() as noted, it is broken as there is a BUG() on next try if nofail;
pcpu_map_pages() it does cleanups.
...

and so on.

--
Uladzislau Rezki

Re: [PATCH v10 2/6] alloc_tag: clean up the populate failure path

[Suren Baghdasaryan]

On Mon, Sep 28, 2026 at 8:09 AM Uladzislau Rezki <urezki@gmail.com> wrote:
>
> On Mon, Sep 28, 2026 at 01:09:13PM +0200, Uladzislau Rezki wrote:
> > On Thu, Sep 17, 2026 at 10:32:48AM +0800, Hao Ge wrote:
> > > Hi Suren、David、Lorenzo
> > >
> > > On 2026/9/17 09:12, Hao Ge wrote:
> > > > Hi Suren
> > > >
> > > > On 2026/9/17 03:07, Suren Baghdasaryan wrote:
> > > >> On Wed, Sep 16, 2026 at 9:31 AM Suren Baghdasaryan <surenb@google.com> wrote:
> > > >>>
> > > >>> On Tue, Sep 15, 2026 at 11:00 PM Hao Ge <hao.ge@linux.dev> wrote:
> > > >>>>
> > > >>>> Hi Suren
> > > >>>>
> > > >>>> Thanks for you review.
> > > >>>>
> > > >>>> On 2026/9/16 05:09, Suren Baghdasaryan wrote:
> > > >>>>> On Mon, Sep 14, 2026 at 11:59 PM Hao Ge <hao.ge@linux.dev> wrote:
> > > >>>>>>
> > > >>>>>> The reservation is already stored in the maple tree when
> > > >>>>>> vm_module_tags_populate() fails. A failed load never unloads the
> > > >>>>>> module, so nothing releases the entry. Release it and roll
> > > >>>>>> module_tags.size back. Without the rollback a concurrent load that
> > > >>>>>> already passed needs_section_mem() can reuse the freed gap, skip
> > > >>>>>> vm_module_tags_populate() and write to unmapped memory.
> > > >>>>>>
> > > >>>>>> vmap_pages_range() may have installed some PTEs before failing. A
> > > >>>>>> retry to populate the same range would BUG on them, so undo them,
> > > >>>>>> but only if vmap actually ran.
> > > >>>>>>
> > > >>>>>> Fixes: 4835f747d3ed ("alloc_tag: support for page allocation tag compression")
> > > >>>>>> Fixes: 0f9b685626da ("alloc_tag: populate memory for module tags as needed")
> > > >>>>>
> > > >>>>> If a patch fixes two other patches, it likely needs to be split so
> > > >>>>> that each part fixes one patch only. But read on please. I'm not sure
> > > >>>>> if one of the fixes is correct.
> > > >>>>>
> > > >>>>
> > > >>>> Kept them in one patch because they only make sense together.
> > > >>>>
> > > >>>> After a failed populate the reservation is released and the
> > > >>>> size rolled back, so the next load takes the same gap and calls
> > > >>>> vm_module_tags_populate() again, and that retry would trip over
> > > >>>> the PTEs the failed vmap left behind.
> > > >>>>
> > > >>>> Without the rollback a load that already passed needs_section_mem()
> > > >>>> just reuses the gap, skips populate and writes to unmapped memory.
> > > >>>> So splitting them leaves each half broken.
> > > >>>
> > > >>> Yeah, that's why I think vmap_pages_range() cleanup should be a
> > > >>> completely separate patch with vmap_pages_range() doing cleanup
> > > >>> itself.
> > > >>>
> > > >
> > > > I think I get where you're coming from now.
> > > > Thanks for taking the time to explain.
> > > >
> > > >>>>
> > > >>>>>> Reported-by: Sashiko <sashiko-bot@kernel.org>
> > > >>>>>> Cc: stable@vger.kernel.org
> > > >>>>>> Signed-off-by: Hao Ge <hao.ge@linux.dev>
> > > >>>>>> ---
> > > >>>>>>  mm/alloc_tag.c | 10 ++++++++++
> > > >>>>>>  1 file changed, 10 insertions(+)
> > > >>>>>>
> > > >>>>>> diff --git a/mm/alloc_tag.c b/mm/alloc_tag.c
> > > >>>>>> index 2070e682fe10..95ddf5b743d0 100644
> > > >>>>>> --- a/mm/alloc_tag.c
> > > >>>>>> +++ b/mm/alloc_tag.c
> > > >>>>>> @@ -804,6 +804,13 @@ static int vm_module_tags_populate(void)
> > > >>>>>>                                      next_page, PAGE_SHIFT) < 0) {
> > > >>>>>>                         release_pages_arg arg = { .pages = next_page };
> > > >>>>>>
> > > >>>>>> +                       /*
> > > >>>>>> +                        * vmap_pages_range() only runs once all pages were
> > > >>>>>> +                        * allocated, and it may have installed some mappings
> > > >>>>>> +                        * before failing. Undo them.
> > > >>>>>
> > > >>>>> If vmap_pages_range() failed to map the range, why should we need to
> > > >>>>> undo it? If it indeed leaves some partial mapping when failing then I
> > > >>>>> would argue that vmap_pages_range() should be undoing these partial
> > > >>>>> mappings itself before returning the error.
> > > >>>>>
> > > >>>> Yeah it does - none of the paths under vmap_pages_range() undo themselves on error.
> > > >>>
> > > >>> Hmm. I wonder if this behavior is intentional or requires a fix. The
> > > >>> only possible reason I can think of is performance but I can't imagine
> > > >>> a failure to map is a performance-critical case.
> > > >>>
> > > >>> +David Hildenbrand (Red Hat), +Lorenzo Stoakes (Oracle) what do you
> > > >>
> > > >> Huh, my gmail thinks you are still at your previous companies :)
> > > >>
> > > >
> > > > I manually fixed it on the mailing list for this thread.
> > > >
> > > >>> guys think? Shouldn't vmap_pages_range() undo its possible partial
> > > >>> mapping when it fails?
> > > >>>
> > > >>>
> > > >>>> pcpu_map_pages does perform cleanup on its own as well. There may be other similar sites.
> > > >>>> https://elixir.bootlin.com/linux/v7.3-rc3/source/mm/percpu-vm.c#L255
> > > >>>> so I did the same here.
> > > >>>>
> > > >>>> If we really want vmap_pages_range() to handle this cleanup internally, I believe that would
> > > >>>> be a relatively large change, and we would need to consider many more cases. So I think we keep
> > > >>>> the current behaviour for now.
> > > >>>
> > > >>> Yeah, I understand it would require a deeper cut but if that's the
> > > >>> right way to handle this we might as well do that. Let's see what MM
> > > >>> folks think.
> > > >>>
> > > >
> > > > Sounds good to me. Let's wait and hear what the MM folks say.
> > > >
> > >
> > > I just looked into the call sites for __vmap_pages_range.
> > > And I found there's a bug in one vmalloc caller [1].
> > > When gfp_mask has __GFP_NOFAIL, the code keeps retrying
> > > __vmap_pages_range. If there are leftover PTE mappings,
> > > this can trigger a BUG(); [2]
> > > https://elixir.bootlin.com/linux/v7.3-rc3/source/mm/vmalloc.c#L3946 [1]
> > > https://elixir.bootlin.com/linux/v7.3-rc3/source/mm/vmalloc.c#L121  [2]
> > >
> > Indeed the below code is incomplete:
> >
> > <snip>
> >       /*
> >        * page tables allocations ignore external gfp mask, enforce it
> >        * by the scope API
> >        */
> >       flags = memalloc_apply_gfp_scope(gfp_mask);
> >       do {
> >               ret = __vmap_pages_range(addr, addr + size, prot, area->pages,
> >                               page_shift, nested_gfp);
> >               if (nofail && (ret < 0))
> >                       schedule_timeout_uninterruptible(1);
> >       } while (nofail && (ret < 0));
> >       memalloc_restore_scope(flags);
> > <snip>
> >
> > and it should be fixed. As the __vmap_pages_range() can actually fail
> > and leave some installed mappings, we have to perform some cleanups
> > on failure or on repeat(if nofail is true).
> >
> > Below code:
> >
> > +                     /*
> > +                      * vmap_pages_range() only runs once all pages were
> > +                      * allocated, and it may have installed some mappings
> > +                      * before failing. Undo them.
> > +                      */
> > +                     if (nr == more_pages)
> > +                             vunmap_range(phys_end, phys_end + (nr << PAGE_SHIFT));
> >
> > we can use but we need to check if partially mapped space is not an
> > issue for the vunmap_range(). And it looks like it is not.
> >
> > <snip>
> > @@ -3953,8 +3990,15 @@ static void *__vmalloc_area_node(struct vm_struct *area, gfp_t gfp_mask,
> >         do {
> >                 ret = __vmap_pages_range(addr, addr + size, prot, area->pages,
> >                                 page_shift, nested_gfp);
> > -               if (nofail && (ret < 0))
> > -                       schedule_timeout_uninterruptible(1);
> > +               if (ret < 0) {
> > +                       /*
> > +                        * __vmap_pages_range() may have installed some mappings
> > +                        * before failing. Undo them before returning or retrying.
> > +                        */
> > +                       vunmap_range(addr, addr + size);
> > +                       if (nofail)
> > +                               schedule_timeout_uninterruptible(1);
> > +               }
> >         } while (nofail && (ret < 0));
> >         memalloc_restore_scope(flags);
> > <snip>
> >
> And looking further. If the __vmap_pages_range()/__vmap_pages_range_noflush()
> and friends fail, a proper cleanups should be done inside, IMO.

Sounds like we are in agreement.

>
> But i am checking callers and see that some do some not:
>
> <snip hooks.c>
>         __vunmap_range_noflush(
>                 vmalloc_shadow(start),
>                 vmalloc_shadow(start + clean * PAGE_SIZE));
>         __vunmap_range_noflush(
>                 vmalloc_origin(start),
>                 vmalloc_origin(start + clean * PAGE_SIZE));
> <snip hooks.c>
>
> kmsan_ioremap_page_range() it does clean ups;
> kmsan_vmap_pages_range_noflush() looks like does not care in shadow.c;
> __vmalloc_area_node() as noted, it is broken as there is a BUG() on next try if nofail;
> pcpu_map_pages() it does cleanups.
> ...
>
> and so on.

Yeah, the callers that do the cleanup should stop doing that once the
vmap functions handle it themselves. I expect an overall reduction in
code size.

>
> --
> Uladzislau Rezki

Re: [PATCH v10 2/6] alloc_tag: clean up the populate failure path

[Uladzislau Rezki]

On Mon, Sep 28, 2026 at 08:16:18AM -0700, Suren Baghdasaryan wrote:
> On Mon, Sep 28, 2026 at 8:09 AM Uladzislau Rezki <urezki@gmail.com> wrote:
> >
> > On Mon, Sep 28, 2026 at 01:09:13PM +0200, Uladzislau Rezki wrote:
> > > On Thu, Sep 17, 2026 at 10:32:48AM +0800, Hao Ge wrote:
> > > > Hi Suren、David、Lorenzo
> > > >
> > > > On 2026/9/17 09:12, Hao Ge wrote:
> > > > > Hi Suren
> > > > >
> > > > > On 2026/9/17 03:07, Suren Baghdasaryan wrote:
> > > > >> On Wed, Sep 16, 2026 at 9:31 AM Suren Baghdasaryan <surenb@google.com> wrote:
> > > > >>>
> > > > >>> On Tue, Sep 15, 2026 at 11:00 PM Hao Ge <hao.ge@linux.dev> wrote:
> > > > >>>>
> > > > >>>> Hi Suren
> > > > >>>>
> > > > >>>> Thanks for you review.
> > > > >>>>
> > > > >>>> On 2026/9/16 05:09, Suren Baghdasaryan wrote:
> > > > >>>>> On Mon, Sep 14, 2026 at 11:59 PM Hao Ge <hao.ge@linux.dev> wrote:
> > > > >>>>>>
> > > > >>>>>> The reservation is already stored in the maple tree when
> > > > >>>>>> vm_module_tags_populate() fails. A failed load never unloads the
> > > > >>>>>> module, so nothing releases the entry. Release it and roll
> > > > >>>>>> module_tags.size back. Without the rollback a concurrent load that
> > > > >>>>>> already passed needs_section_mem() can reuse the freed gap, skip
> > > > >>>>>> vm_module_tags_populate() and write to unmapped memory.
> > > > >>>>>>
> > > > >>>>>> vmap_pages_range() may have installed some PTEs before failing. A
> > > > >>>>>> retry to populate the same range would BUG on them, so undo them,
> > > > >>>>>> but only if vmap actually ran.
> > > > >>>>>>
> > > > >>>>>> Fixes: 4835f747d3ed ("alloc_tag: support for page allocation tag compression")
> > > > >>>>>> Fixes: 0f9b685626da ("alloc_tag: populate memory for module tags as needed")
> > > > >>>>>
> > > > >>>>> If a patch fixes two other patches, it likely needs to be split so
> > > > >>>>> that each part fixes one patch only. But read on please. I'm not sure
> > > > >>>>> if one of the fixes is correct.
> > > > >>>>>
> > > > >>>>
> > > > >>>> Kept them in one patch because they only make sense together.
> > > > >>>>
> > > > >>>> After a failed populate the reservation is released and the
> > > > >>>> size rolled back, so the next load takes the same gap and calls
> > > > >>>> vm_module_tags_populate() again, and that retry would trip over
> > > > >>>> the PTEs the failed vmap left behind.
> > > > >>>>
> > > > >>>> Without the rollback a load that already passed needs_section_mem()
> > > > >>>> just reuses the gap, skips populate and writes to unmapped memory.
> > > > >>>> So splitting them leaves each half broken.
> > > > >>>
> > > > >>> Yeah, that's why I think vmap_pages_range() cleanup should be a
> > > > >>> completely separate patch with vmap_pages_range() doing cleanup
> > > > >>> itself.
> > > > >>>
> > > > >
> > > > > I think I get where you're coming from now.
> > > > > Thanks for taking the time to explain.
> > > > >
> > > > >>>>
> > > > >>>>>> Reported-by: Sashiko <sashiko-bot@kernel.org>
> > > > >>>>>> Cc: stable@vger.kernel.org
> > > > >>>>>> Signed-off-by: Hao Ge <hao.ge@linux.dev>
> > > > >>>>>> ---
> > > > >>>>>>  mm/alloc_tag.c | 10 ++++++++++
> > > > >>>>>>  1 file changed, 10 insertions(+)
> > > > >>>>>>
> > > > >>>>>> diff --git a/mm/alloc_tag.c b/mm/alloc_tag.c
> > > > >>>>>> index 2070e682fe10..95ddf5b743d0 100644
> > > > >>>>>> --- a/mm/alloc_tag.c
> > > > >>>>>> +++ b/mm/alloc_tag.c
> > > > >>>>>> @@ -804,6 +804,13 @@ static int vm_module_tags_populate(void)
> > > > >>>>>>                                      next_page, PAGE_SHIFT) < 0) {
> > > > >>>>>>                         release_pages_arg arg = { .pages = next_page };
> > > > >>>>>>
> > > > >>>>>> +                       /*
> > > > >>>>>> +                        * vmap_pages_range() only runs once all pages were
> > > > >>>>>> +                        * allocated, and it may have installed some mappings
> > > > >>>>>> +                        * before failing. Undo them.
> > > > >>>>>
> > > > >>>>> If vmap_pages_range() failed to map the range, why should we need to
> > > > >>>>> undo it? If it indeed leaves some partial mapping when failing then I
> > > > >>>>> would argue that vmap_pages_range() should be undoing these partial
> > > > >>>>> mappings itself before returning the error.
> > > > >>>>>
> > > > >>>> Yeah it does - none of the paths under vmap_pages_range() undo themselves on error.
> > > > >>>
> > > > >>> Hmm. I wonder if this behavior is intentional or requires a fix. The
> > > > >>> only possible reason I can think of is performance but I can't imagine
> > > > >>> a failure to map is a performance-critical case.
> > > > >>>
> > > > >>> +David Hildenbrand (Red Hat), +Lorenzo Stoakes (Oracle) what do you
> > > > >>
> > > > >> Huh, my gmail thinks you are still at your previous companies :)
> > > > >>
> > > > >
> > > > > I manually fixed it on the mailing list for this thread.
> > > > >
> > > > >>> guys think? Shouldn't vmap_pages_range() undo its possible partial
> > > > >>> mapping when it fails?
> > > > >>>
> > > > >>>
> > > > >>>> pcpu_map_pages does perform cleanup on its own as well. There may be other similar sites.
> > > > >>>> https://elixir.bootlin.com/linux/v7.3-rc3/source/mm/percpu-vm.c#L255
> > > > >>>> so I did the same here.
> > > > >>>>
> > > > >>>> If we really want vmap_pages_range() to handle this cleanup internally, I believe that would
> > > > >>>> be a relatively large change, and we would need to consider many more cases. So I think we keep
> > > > >>>> the current behaviour for now.
> > > > >>>
> > > > >>> Yeah, I understand it would require a deeper cut but if that's the
> > > > >>> right way to handle this we might as well do that. Let's see what MM
> > > > >>> folks think.
> > > > >>>
> > > > >
> > > > > Sounds good to me. Let's wait and hear what the MM folks say.
> > > > >
> > > >
> > > > I just looked into the call sites for __vmap_pages_range.
> > > > And I found there's a bug in one vmalloc caller [1].
> > > > When gfp_mask has __GFP_NOFAIL, the code keeps retrying
> > > > __vmap_pages_range. If there are leftover PTE mappings,
> > > > this can trigger a BUG(); [2]
> > > > https://elixir.bootlin.com/linux/v7.3-rc3/source/mm/vmalloc.c#L3946 [1]
> > > > https://elixir.bootlin.com/linux/v7.3-rc3/source/mm/vmalloc.c#L121  [2]
> > > >
> > > Indeed the below code is incomplete:
> > >
> > > <snip>
> > >       /*
> > >        * page tables allocations ignore external gfp mask, enforce it
> > >        * by the scope API
> > >        */
> > >       flags = memalloc_apply_gfp_scope(gfp_mask);
> > >       do {
> > >               ret = __vmap_pages_range(addr, addr + size, prot, area->pages,
> > >                               page_shift, nested_gfp);
> > >               if (nofail && (ret < 0))
> > >                       schedule_timeout_uninterruptible(1);
> > >       } while (nofail && (ret < 0));
> > >       memalloc_restore_scope(flags);
> > > <snip>
> > >
> > > and it should be fixed. As the __vmap_pages_range() can actually fail
> > > and leave some installed mappings, we have to perform some cleanups
> > > on failure or on repeat(if nofail is true).
> > >
> > > Below code:
> > >
> > > +                     /*
> > > +                      * vmap_pages_range() only runs once all pages were
> > > +                      * allocated, and it may have installed some mappings
> > > +                      * before failing. Undo them.
> > > +                      */
> > > +                     if (nr == more_pages)
> > > +                             vunmap_range(phys_end, phys_end + (nr << PAGE_SHIFT));
> > >
> > > we can use but we need to check if partially mapped space is not an
> > > issue for the vunmap_range(). And it looks like it is not.
> > >
> > > <snip>
> > > @@ -3953,8 +3990,15 @@ static void *__vmalloc_area_node(struct vm_struct *area, gfp_t gfp_mask,
> > >         do {
> > >                 ret = __vmap_pages_range(addr, addr + size, prot, area->pages,
> > >                                 page_shift, nested_gfp);
> > > -               if (nofail && (ret < 0))
> > > -                       schedule_timeout_uninterruptible(1);
> > > +               if (ret < 0) {
> > > +                       /*
> > > +                        * __vmap_pages_range() may have installed some mappings
> > > +                        * before failing. Undo them before returning or retrying.
> > > +                        */
> > > +                       vunmap_range(addr, addr + size);
> > > +                       if (nofail)
> > > +                               schedule_timeout_uninterruptible(1);
> > > +               }
> > >         } while (nofail && (ret < 0));
> > >         memalloc_restore_scope(flags);
> > > <snip>
> > >
> > And looking further. If the __vmap_pages_range()/__vmap_pages_range_noflush()
> > and friends fail, a proper cleanups should be done inside, IMO.
> 
> Sounds like we are in agreement.
> 
> >
> > But i am checking callers and see that some do some not:
> >
> > <snip hooks.c>
> >         __vunmap_range_noflush(
> >                 vmalloc_shadow(start),
> >                 vmalloc_shadow(start + clean * PAGE_SIZE));
> >         __vunmap_range_noflush(
> >                 vmalloc_origin(start),
> >                 vmalloc_origin(start + clean * PAGE_SIZE));
> > <snip hooks.c>
> >
> > kmsan_ioremap_page_range() it does clean ups;
> > kmsan_vmap_pages_range_noflush() looks like does not care in shadow.c;
> > __vmalloc_area_node() as noted, it is broken as there is a BUG() on next try if nofail;
> > pcpu_map_pages() it does cleanups.
> > ...
> >
> > and so on.
> 
> Yeah, the callers that do the cleanup should stop doing that once the
> vmap functions handle it themselves. I expect an overall reduction in
> code size.
> 
Agree. If mapping API is called and it fails the caller should expect
the state as it was before calling that API function, IMO.

--
Uladzislau Rezki

Re: [PATCH v10 2/6] alloc_tag: clean up the populate failure path

[Suren Baghdasaryan]

On Mon, Sep 28, 2026 at 10:47 AM Uladzislau Rezki <urezki@gmail.com> wrote:
>
> On Mon, Sep 28, 2026 at 08:16:18AM -0700, Suren Baghdasaryan wrote:
> > On Mon, Sep 28, 2026 at 8:09 AM Uladzislau Rezki <urezki@gmail.com> wrote:
> > >
> > > On Mon, Sep 28, 2026 at 01:09:13PM +0200, Uladzislau Rezki wrote:
> > > > On Thu, Sep 17, 2026 at 10:32:48AM +0800, Hao Ge wrote:
> > > > > Hi Suren、David、Lorenzo
> > > > >
> > > > > On 2026/9/17 09:12, Hao Ge wrote:
> > > > > > Hi Suren
> > > > > >
> > > > > > On 2026/9/17 03:07, Suren Baghdasaryan wrote:
> > > > > >> On Wed, Sep 16, 2026 at 9:31 AM Suren Baghdasaryan <surenb@google.com> wrote:
> > > > > >>>
> > > > > >>> On Tue, Sep 15, 2026 at 11:00 PM Hao Ge <hao.ge@linux.dev> wrote:
> > > > > >>>>
> > > > > >>>> Hi Suren
> > > > > >>>>
> > > > > >>>> Thanks for you review.
> > > > > >>>>
> > > > > >>>> On 2026/9/16 05:09, Suren Baghdasaryan wrote:
> > > > > >>>>> On Mon, Sep 14, 2026 at 11:59 PM Hao Ge <hao.ge@linux.dev> wrote:
> > > > > >>>>>>
> > > > > >>>>>> The reservation is already stored in the maple tree when
> > > > > >>>>>> vm_module_tags_populate() fails. A failed load never unloads the
> > > > > >>>>>> module, so nothing releases the entry. Release it and roll
> > > > > >>>>>> module_tags.size back. Without the rollback a concurrent load that
> > > > > >>>>>> already passed needs_section_mem() can reuse the freed gap, skip
> > > > > >>>>>> vm_module_tags_populate() and write to unmapped memory.
> > > > > >>>>>>
> > > > > >>>>>> vmap_pages_range() may have installed some PTEs before failing. A
> > > > > >>>>>> retry to populate the same range would BUG on them, so undo them,
> > > > > >>>>>> but only if vmap actually ran.
> > > > > >>>>>>
> > > > > >>>>>> Fixes: 4835f747d3ed ("alloc_tag: support for page allocation tag compression")
> > > > > >>>>>> Fixes: 0f9b685626da ("alloc_tag: populate memory for module tags as needed")
> > > > > >>>>>
> > > > > >>>>> If a patch fixes two other patches, it likely needs to be split so
> > > > > >>>>> that each part fixes one patch only. But read on please. I'm not sure
> > > > > >>>>> if one of the fixes is correct.
> > > > > >>>>>
> > > > > >>>>
> > > > > >>>> Kept them in one patch because they only make sense together.
> > > > > >>>>
> > > > > >>>> After a failed populate the reservation is released and the
> > > > > >>>> size rolled back, so the next load takes the same gap and calls
> > > > > >>>> vm_module_tags_populate() again, and that retry would trip over
> > > > > >>>> the PTEs the failed vmap left behind.
> > > > > >>>>
> > > > > >>>> Without the rollback a load that already passed needs_section_mem()
> > > > > >>>> just reuses the gap, skips populate and writes to unmapped memory.
> > > > > >>>> So splitting them leaves each half broken.
> > > > > >>>
> > > > > >>> Yeah, that's why I think vmap_pages_range() cleanup should be a
> > > > > >>> completely separate patch with vmap_pages_range() doing cleanup
> > > > > >>> itself.
> > > > > >>>
> > > > > >
> > > > > > I think I get where you're coming from now.
> > > > > > Thanks for taking the time to explain.
> > > > > >
> > > > > >>>>
> > > > > >>>>>> Reported-by: Sashiko <sashiko-bot@kernel.org>
> > > > > >>>>>> Cc: stable@vger.kernel.org
> > > > > >>>>>> Signed-off-by: Hao Ge <hao.ge@linux.dev>
> > > > > >>>>>> ---
> > > > > >>>>>>  mm/alloc_tag.c | 10 ++++++++++
> > > > > >>>>>>  1 file changed, 10 insertions(+)
> > > > > >>>>>>
> > > > > >>>>>> diff --git a/mm/alloc_tag.c b/mm/alloc_tag.c
> > > > > >>>>>> index 2070e682fe10..95ddf5b743d0 100644
> > > > > >>>>>> --- a/mm/alloc_tag.c
> > > > > >>>>>> +++ b/mm/alloc_tag.c
> > > > > >>>>>> @@ -804,6 +804,13 @@ static int vm_module_tags_populate(void)
> > > > > >>>>>>                                      next_page, PAGE_SHIFT) < 0) {
> > > > > >>>>>>                         release_pages_arg arg = { .pages = next_page };
> > > > > >>>>>>
> > > > > >>>>>> +                       /*
> > > > > >>>>>> +                        * vmap_pages_range() only runs once all pages were
> > > > > >>>>>> +                        * allocated, and it may have installed some mappings
> > > > > >>>>>> +                        * before failing. Undo them.
> > > > > >>>>>
> > > > > >>>>> If vmap_pages_range() failed to map the range, why should we need to
> > > > > >>>>> undo it? If it indeed leaves some partial mapping when failing then I
> > > > > >>>>> would argue that vmap_pages_range() should be undoing these partial
> > > > > >>>>> mappings itself before returning the error.
> > > > > >>>>>
> > > > > >>>> Yeah it does - none of the paths under vmap_pages_range() undo themselves on error.
> > > > > >>>
> > > > > >>> Hmm. I wonder if this behavior is intentional or requires a fix. The
> > > > > >>> only possible reason I can think of is performance but I can't imagine
> > > > > >>> a failure to map is a performance-critical case.
> > > > > >>>
> > > > > >>> +David Hildenbrand (Red Hat), +Lorenzo Stoakes (Oracle) what do you
> > > > > >>
> > > > > >> Huh, my gmail thinks you are still at your previous companies :)
> > > > > >>
> > > > > >
> > > > > > I manually fixed it on the mailing list for this thread.
> > > > > >
> > > > > >>> guys think? Shouldn't vmap_pages_range() undo its possible partial
> > > > > >>> mapping when it fails?
> > > > > >>>
> > > > > >>>
> > > > > >>>> pcpu_map_pages does perform cleanup on its own as well. There may be other similar sites.
> > > > > >>>> https://elixir.bootlin.com/linux/v7.3-rc3/source/mm/percpu-vm.c#L255
> > > > > >>>> so I did the same here.
> > > > > >>>>
> > > > > >>>> If we really want vmap_pages_range() to handle this cleanup internally, I believe that would
> > > > > >>>> be a relatively large change, and we would need to consider many more cases. So I think we keep
> > > > > >>>> the current behaviour for now.
> > > > > >>>
> > > > > >>> Yeah, I understand it would require a deeper cut but if that's the
> > > > > >>> right way to handle this we might as well do that. Let's see what MM
> > > > > >>> folks think.
> > > > > >>>
> > > > > >
> > > > > > Sounds good to me. Let's wait and hear what the MM folks say.
> > > > > >
> > > > >
> > > > > I just looked into the call sites for __vmap_pages_range.
> > > > > And I found there's a bug in one vmalloc caller [1].
> > > > > When gfp_mask has __GFP_NOFAIL, the code keeps retrying
> > > > > __vmap_pages_range. If there are leftover PTE mappings,
> > > > > this can trigger a BUG(); [2]
> > > > > https://elixir.bootlin.com/linux/v7.3-rc3/source/mm/vmalloc.c#L3946 [1]
> > > > > https://elixir.bootlin.com/linux/v7.3-rc3/source/mm/vmalloc.c#L121  [2]
> > > > >
> > > > Indeed the below code is incomplete:
> > > >
> > > > <snip>
> > > >       /*
> > > >        * page tables allocations ignore external gfp mask, enforce it
> > > >        * by the scope API
> > > >        */
> > > >       flags = memalloc_apply_gfp_scope(gfp_mask);
> > > >       do {
> > > >               ret = __vmap_pages_range(addr, addr + size, prot, area->pages,
> > > >                               page_shift, nested_gfp);
> > > >               if (nofail && (ret < 0))
> > > >                       schedule_timeout_uninterruptible(1);
> > > >       } while (nofail && (ret < 0));
> > > >       memalloc_restore_scope(flags);
> > > > <snip>
> > > >
> > > > and it should be fixed. As the __vmap_pages_range() can actually fail
> > > > and leave some installed mappings, we have to perform some cleanups
> > > > on failure or on repeat(if nofail is true).
> > > >
> > > > Below code:
> > > >
> > > > +                     /*
> > > > +                      * vmap_pages_range() only runs once all pages were
> > > > +                      * allocated, and it may have installed some mappings
> > > > +                      * before failing. Undo them.
> > > > +                      */
> > > > +                     if (nr == more_pages)
> > > > +                             vunmap_range(phys_end, phys_end + (nr << PAGE_SHIFT));
> > > >
> > > > we can use but we need to check if partially mapped space is not an
> > > > issue for the vunmap_range(). And it looks like it is not.
> > > >
> > > > <snip>
> > > > @@ -3953,8 +3990,15 @@ static void *__vmalloc_area_node(struct vm_struct *area, gfp_t gfp_mask,
> > > >         do {
> > > >                 ret = __vmap_pages_range(addr, addr + size, prot, area->pages,
> > > >                                 page_shift, nested_gfp);
> > > > -               if (nofail && (ret < 0))
> > > > -                       schedule_timeout_uninterruptible(1);
> > > > +               if (ret < 0) {
> > > > +                       /*
> > > > +                        * __vmap_pages_range() may have installed some mappings
> > > > +                        * before failing. Undo them before returning or retrying.
> > > > +                        */
> > > > +                       vunmap_range(addr, addr + size);
> > > > +                       if (nofail)
> > > > +                               schedule_timeout_uninterruptible(1);
> > > > +               }
> > > >         } while (nofail && (ret < 0));
> > > >         memalloc_restore_scope(flags);
> > > > <snip>
> > > >
> > > And looking further. If the __vmap_pages_range()/__vmap_pages_range_noflush()
> > > and friends fail, a proper cleanups should be done inside, IMO.
> >
> > Sounds like we are in agreement.
> >
> > >
> > > But i am checking callers and see that some do some not:
> > >
> > > <snip hooks.c>
> > >         __vunmap_range_noflush(
> > >                 vmalloc_shadow(start),
> > >                 vmalloc_shadow(start + clean * PAGE_SIZE));
> > >         __vunmap_range_noflush(
> > >                 vmalloc_origin(start),
> > >                 vmalloc_origin(start + clean * PAGE_SIZE));
> > > <snip hooks.c>
> > >
> > > kmsan_ioremap_page_range() it does clean ups;
> > > kmsan_vmap_pages_range_noflush() looks like does not care in shadow.c;
> > > __vmalloc_area_node() as noted, it is broken as there is a BUG() on next try if nofail;
> > > pcpu_map_pages() it does cleanups.
> > > ...
> > >
> > > and so on.
> >
> > Yeah, the callers that do the cleanup should stop doing that once the
> > vmap functions handle it themselves. I expect an overall reduction in
> > code size.
> >
> Agree. If mapping API is called and it fails the caller should expect
> the state as it was before calling that API function, IMO.

Thanks for confirmation!

Hao, do you need help with making the change in vmap functions to
cleanup upon a failure?

>
> --
> Uladzislau Rezki

Re: [PATCH v10 2/6] alloc_tag: clean up the populate failure path

[Hao Ge]

Hi Suren and Uladzislau

On 2026/9/29 01:54, Suren Baghdasaryan wrote:
> On Mon, Sep 28, 2026 at 10:47 AM Uladzislau Rezki <urezki@gmail.com> wrote:
>>
>> On Mon, Sep 28, 2026 at 08:16:18AM -0700, Suren Baghdasaryan wrote:
>>> On Mon, Sep 28, 2026 at 8:09 AM Uladzislau Rezki <urezki@gmail.com> wrote:
>>>>
>>>> On Mon, Sep 28, 2026 at 01:09:13PM +0200, Uladzislau Rezki wrote:
>>>>> On Thu, Sep 17, 2026 at 10:32:48AM +0800, Hao Ge wrote:
>>>>>> Hi Suren、David、Lorenzo
>>>>>>
>>>>>> On 2026/9/17 09:12, Hao Ge wrote:
>>>>>>> Hi Suren
>>>>>>>
>>>>>>> On 2026/9/17 03:07, Suren Baghdasaryan wrote:
>>>>>>>> On Wed, Sep 16, 2026 at 9:31 AM Suren Baghdasaryan <surenb@google.com> wrote:
>>>>>>>>>
>>>>>>>>> On Tue, Sep 15, 2026 at 11:00 PM Hao Ge <hao.ge@linux.dev> wrote:
>>>>>>>>>>
>>>>>>>>>> Hi Suren
>>>>>>>>>>
>>>>>>>>>> Thanks for you review.
>>>>>>>>>>
>>>>>>>>>> On 2026/9/16 05:09, Suren Baghdasaryan wrote:
>>>>>>>>>>> On Mon, Sep 14, 2026 at 11:59 PM Hao Ge <hao.ge@linux.dev> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>> The reservation is already stored in the maple tree when
>>>>>>>>>>>> vm_module_tags_populate() fails. A failed load never unloads the
>>>>>>>>>>>> module, so nothing releases the entry. Release it and roll
>>>>>>>>>>>> module_tags.size back. Without the rollback a concurrent load that
>>>>>>>>>>>> already passed needs_section_mem() can reuse the freed gap, skip
>>>>>>>>>>>> vm_module_tags_populate() and write to unmapped memory.
>>>>>>>>>>>>
>>>>>>>>>>>> vmap_pages_range() may have installed some PTEs before failing. A
>>>>>>>>>>>> retry to populate the same range would BUG on them, so undo them,
>>>>>>>>>>>> but only if vmap actually ran.
>>>>>>>>>>>>
>>>>>>>>>>>> Fixes: 4835f747d3ed ("alloc_tag: support for page allocation tag compression")
>>>>>>>>>>>> Fixes: 0f9b685626da ("alloc_tag: populate memory for module tags as needed")
>>>>>>>>>>>
>>>>>>>>>>> If a patch fixes two other patches, it likely needs to be split so
>>>>>>>>>>> that each part fixes one patch only. But read on please. I'm not sure
>>>>>>>>>>> if one of the fixes is correct.
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Kept them in one patch because they only make sense together.
>>>>>>>>>>
>>>>>>>>>> After a failed populate the reservation is released and the
>>>>>>>>>> size rolled back, so the next load takes the same gap and calls
>>>>>>>>>> vm_module_tags_populate() again, and that retry would trip over
>>>>>>>>>> the PTEs the failed vmap left behind.
>>>>>>>>>>
>>>>>>>>>> Without the rollback a load that already passed needs_section_mem()
>>>>>>>>>> just reuses the gap, skips populate and writes to unmapped memory.
>>>>>>>>>> So splitting them leaves each half broken.
>>>>>>>>>
>>>>>>>>> Yeah, that's why I think vmap_pages_range() cleanup should be a
>>>>>>>>> completely separate patch with vmap_pages_range() doing cleanup
>>>>>>>>> itself.
>>>>>>>>>
>>>>>>>
>>>>>>> I think I get where you're coming from now.
>>>>>>> Thanks for taking the time to explain.
>>>>>>>
>>>>>>>>>>
>>>>>>>>>>>> Reported-by: Sashiko <sashiko-bot@kernel.org>
>>>>>>>>>>>> Cc: stable@vger.kernel.org
>>>>>>>>>>>> Signed-off-by: Hao Ge <hao.ge@linux.dev>
>>>>>>>>>>>> ---
>>>>>>>>>>>>  mm/alloc_tag.c | 10 ++++++++++
>>>>>>>>>>>>  1 file changed, 10 insertions(+)
>>>>>>>>>>>>
>>>>>>>>>>>> diff --git a/mm/alloc_tag.c b/mm/alloc_tag.c
>>>>>>>>>>>> index 2070e682fe10..95ddf5b743d0 100644
>>>>>>>>>>>> --- a/mm/alloc_tag.c
>>>>>>>>>>>> +++ b/mm/alloc_tag.c
>>>>>>>>>>>> @@ -804,6 +804,13 @@ static int vm_module_tags_populate(void)
>>>>>>>>>>>>                                      next_page, PAGE_SHIFT) < 0) {
>>>>>>>>>>>>                         release_pages_arg arg = { .pages = next_page };
>>>>>>>>>>>>
>>>>>>>>>>>> +                       /*
>>>>>>>>>>>> +                        * vmap_pages_range() only runs once all pages were
>>>>>>>>>>>> +                        * allocated, and it may have installed some mappings
>>>>>>>>>>>> +                        * before failing. Undo them.
>>>>>>>>>>>
>>>>>>>>>>> If vmap_pages_range() failed to map the range, why should we need to
>>>>>>>>>>> undo it? If it indeed leaves some partial mapping when failing then I
>>>>>>>>>>> would argue that vmap_pages_range() should be undoing these partial
>>>>>>>>>>> mappings itself before returning the error.
>>>>>>>>>>>
>>>>>>>>>> Yeah it does - none of the paths under vmap_pages_range() undo themselves on error.
>>>>>>>>>
>>>>>>>>> Hmm. I wonder if this behavior is intentional or requires a fix. The
>>>>>>>>> only possible reason I can think of is performance but I can't imagine
>>>>>>>>> a failure to map is a performance-critical case.
>>>>>>>>>
>>>>>>>>> +David Hildenbrand (Red Hat), +Lorenzo Stoakes (Oracle) what do you
>>>>>>>>
>>>>>>>> Huh, my gmail thinks you are still at your previous companies :)
>>>>>>>>
>>>>>>>
>>>>>>> I manually fixed it on the mailing list for this thread.
>>>>>>>
>>>>>>>>> guys think? Shouldn't vmap_pages_range() undo its possible partial
>>>>>>>>> mapping when it fails?
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>> pcpu_map_pages does perform cleanup on its own as well. There may be other similar sites.
>>>>>>>>>> https://elixir.bootlin.com/linux/v7.3-rc3/source/mm/percpu-vm.c#L255
>>>>>>>>>> so I did the same here.
>>>>>>>>>>
>>>>>>>>>> If we really want vmap_pages_range() to handle this cleanup internally, I believe that would
>>>>>>>>>> be a relatively large change, and we would need to consider many more cases. So I think we keep
>>>>>>>>>> the current behaviour for now.
>>>>>>>>>
>>>>>>>>> Yeah, I understand it would require a deeper cut but if that's the
>>>>>>>>> right way to handle this we might as well do that. Let's see what MM
>>>>>>>>> folks think.
>>>>>>>>>
>>>>>>>
>>>>>>> Sounds good to me. Let's wait and hear what the MM folks say.
>>>>>>>
>>>>>>
>>>>>> I just looked into the call sites for __vmap_pages_range.
>>>>>> And I found there's a bug in one vmalloc caller [1].
>>>>>> When gfp_mask has __GFP_NOFAIL, the code keeps retrying
>>>>>> __vmap_pages_range. If there are leftover PTE mappings,
>>>>>> this can trigger a BUG(); [2]
>>>>>> https://elixir.bootlin.com/linux/v7.3-rc3/source/mm/vmalloc.c#L3946 [1]
>>>>>> https://elixir.bootlin.com/linux/v7.3-rc3/source/mm/vmalloc.c#L121  [2]
>>>>>>
>>>>> Indeed the below code is incomplete:
>>>>>
>>>>> <snip>
>>>>>       /*
>>>>>        * page tables allocations ignore external gfp mask, enforce it
>>>>>        * by the scope API
>>>>>        */
>>>>>       flags = memalloc_apply_gfp_scope(gfp_mask);
>>>>>       do {
>>>>>               ret = __vmap_pages_range(addr, addr + size, prot, area->pages,
>>>>>                               page_shift, nested_gfp);
>>>>>               if (nofail && (ret < 0))
>>>>>                       schedule_timeout_uninterruptible(1);
>>>>>       } while (nofail && (ret < 0));
>>>>>       memalloc_restore_scope(flags);
>>>>> <snip>
>>>>>
>>>>> and it should be fixed. As the __vmap_pages_range() can actually fail
>>>>> and leave some installed mappings, we have to perform some cleanups
>>>>> on failure or on repeat(if nofail is true).
>>>>>
>>>>> Below code:
>>>>>
>>>>> +                     /*
>>>>> +                      * vmap_pages_range() only runs once all pages were
>>>>> +                      * allocated, and it may have installed some mappings
>>>>> +                      * before failing. Undo them.
>>>>> +                      */
>>>>> +                     if (nr == more_pages)
>>>>> +                             vunmap_range(phys_end, phys_end + (nr << PAGE_SHIFT));
>>>>>
>>>>> we can use but we need to check if partially mapped space is not an
>>>>> issue for the vunmap_range(). And it looks like it is not.
>>>>>
>>>>> <snip>
>>>>> @@ -3953,8 +3990,15 @@ static void *__vmalloc_area_node(struct vm_struct *area, gfp_t gfp_mask,
>>>>>         do {
>>>>>                 ret = __vmap_pages_range(addr, addr + size, prot, area->pages,
>>>>>                                 page_shift, nested_gfp);
>>>>> -               if (nofail && (ret < 0))
>>>>> -                       schedule_timeout_uninterruptible(1);
>>>>> +               if (ret < 0) {
>>>>> +                       /*
>>>>> +                        * __vmap_pages_range() may have installed some mappings
>>>>> +                        * before failing. Undo them before returning or retrying.
>>>>> +                        */
>>>>> +                       vunmap_range(addr, addr + size);
>>>>> +                       if (nofail)
>>>>> +                               schedule_timeout_uninterruptible(1);
>>>>> +               }
>>>>>         } while (nofail && (ret < 0));
>>>>>         memalloc_restore_scope(flags);
>>>>> <snip>
>>>>>
>>>> And looking further. If the __vmap_pages_range()/__vmap_pages_range_noflush()
>>>> and friends fail, a proper cleanups should be done inside, IMO.
>>>
>>> Sounds like we are in agreement.
>>>
>>>>
>>>> But i am checking callers and see that some do some not:
>>>>
>>>> <snip hooks.c>
>>>>         __vunmap_range_noflush(
>>>>                 vmalloc_shadow(start),
>>>>                 vmalloc_shadow(start + clean * PAGE_SIZE));
>>>>         __vunmap_range_noflush(
>>>>                 vmalloc_origin(start),
>>>>                 vmalloc_origin(start + clean * PAGE_SIZE));
>>>> <snip hooks.c>
>>>>
>>>> kmsan_ioremap_page_range() it does clean ups;
>>>> kmsan_vmap_pages_range_noflush() looks like does not care in shadow.c;
>>>> __vmalloc_area_node() as noted, it is broken as there is a BUG() on next try if nofail;
>>>> pcpu_map_pages() it does cleanups.
>>>> ...
>>>>
>>>> and so on.
>>>
>>> Yeah, the callers that do the cleanup should stop doing that once the
>>> vmap functions handle it themselves. I expect an overall reduction in
>>> code size.
>>>
>> Agree. If mapping API is called and it fails the caller should expect
>> the state as it was before calling that API function, IMO.
> 
> Thanks for confirmation!
> 
> Hao, do you need help with making the change in vmap functions to
> cleanup upon a failure?
>

Thanks everyone for your help. I think we have reached a consensus.
I spent a day sorting out the logic and generated the patches below:
https://lore.kernel.org/all/20260929082014.160587-3-hao.ge@linux.dev/
Some edge cases might still be missing, please kindly review.

Thanks
Best Regards
Hao

>>
>> --
>> Uladzislau Rezki

[PATCH v10 3/6] module: introduce SH_ENTSIZE_STANDALONE for separately allocated sections

[Hao Ge]

SHF_ALLOC means, per the ELF spec, that a section occupies memory
during process execution. Some module sections occupy memory
outside the regular module layout, for example the percpu section
with its per-CPU allocations. The loader currently excludes such
a section from the layout by clearing its SHF_ALLOC, which
overloads the flag with a loader-internal meaning.
apply_relocations() needs a special case for the section, and
find_sec(".data..percpu") returns different results before and
after layout_and_allocate().

Introduce SH_ENTSIZE_STANDALONE to mark sections with a separate
allocation. The percpu section is its first user. layout_sections()
and move_module() skip marked sections, and apply_relocations() goes
back to testing only SHF_ALLOC. Based on a patch by Petr Pavlu [1].

.data..percpu keeps SHF_ALLOC, so it would now show up under
/sys/module/*/sections/. The section has one instance per CPU and no
single address to report, and the entry never existed before, so
skip it in add_sect_attrs(). add_notes_attrs() indexes its attrs[]
array and skips it too. No functional change otherwise.

Fixes: 4835f747d3ed ("alloc_tag: support for page allocation tag compression")
Reported-by: Sashiko <sashiko-bot@kernel.org>
Link: https://lore.kernel.org/all/499bb60c-c6e3-43a3-bd92-95a0567ece5e@suse.com/ [1]
Suggested-by: Petr Pavlu <petr.pavlu@suse.com>
Reviewed-by: Petr Pavlu <petr.pavlu@suse.com>
Cc: stable@vger.kernel.org
Signed-off-by: Hao Ge <hao.ge@linux.dev>
---
 include/linux/module.h   |  2 ++
 kernel/module/internal.h |  8 ++++++++
 kernel/module/kallsyms.c | 13 +++----------
 kernel/module/main.c     | 32 +++++++++++++++++---------------
 kernel/module/sysfs.c    | 17 +++++++++++++----
 5 files changed, 43 insertions(+), 29 deletions(-)

diff --git a/include/linux/module.h b/include/linux/module.h
index 96cc98568eea..0c6f32ddcbf2 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -325,6 +325,8 @@ enum mod_mem_type {
 	MOD_INIT_RODATA,
 
 	MOD_MEM_NUM_TYPES,
+
+	MOD_STANDALONE = -2,
 	MOD_INVALID = -1,
 };
 
diff --git a/kernel/module/internal.h b/kernel/module/internal.h
index 061161cc79d9..4c738074a27b 100644
--- a/kernel/module/internal.h
+++ b/kernel/module/internal.h
@@ -29,6 +29,14 @@
 #define SH_ENTSIZE_TYPE_MASK	((1UL << SH_ENTSIZE_TYPE_BITS) - 1)
 #define SH_ENTSIZE_OFFSET_MASK	((1UL << (BITS_PER_LONG - SH_ENTSIZE_TYPE_BITS)) - 1)
 
+/*
+ * Marker for sections with a separate allocation, which are not placed
+ * into mod->mem[].
+ */
+#define SH_ENTSIZE_STANDALONE					\
+	(((unsigned long)MOD_STANDALONE & SH_ENTSIZE_TYPE_MASK)	\
+	 << SH_ENTSIZE_TYPE_SHIFT)
+
 /* Maximum number of characters written by module_flags() */
 #define MODULE_FLAGS_BUF_SIZE (TAINT_FLAGS_COUNT + 4)
 
diff --git a/kernel/module/kallsyms.c b/kernel/module/kallsyms.c
index f23126d804b2..0dca6d40160e 100644
--- a/kernel/module/kallsyms.c
+++ b/kernel/module/kallsyms.c
@@ -76,7 +76,7 @@ static char elf_type(const Elf_Sym *sym, const struct load_info *info)
 }
 
 static bool is_core_symbol(const Elf_Sym *src, const Elf_Shdr *sechdrs,
-			   unsigned int shnum, unsigned int pcpundx)
+			   unsigned int shnum)
 {
 	const Elf_Shdr *sec;
 	enum mod_mem_type type;
@@ -86,11 +86,6 @@ static bool is_core_symbol(const Elf_Sym *src, const Elf_Shdr *sechdrs,
 	    !src->st_name)
 		return false;
 
-#ifdef CONFIG_KALLSYMS_ALL
-	if (src->st_shndx == pcpundx)
-		return true;
-#endif
-
 	sec = sechdrs + src->st_shndx;
 	type = sec->sh_entsize >> SH_ENTSIZE_TYPE_SHIFT;
 	if (!(sec->sh_flags & SHF_ALLOC)
@@ -131,8 +126,7 @@ void layout_symtab(struct module *mod, struct load_info *info)
 	/* Compute total space required for the core symbols' strtab. */
 	for (ndst = i = 0; i < nsrc; i++) {
 		if (i == 0 || is_livepatch_module(mod) ||
-		    is_core_symbol(src + i, info->sechdrs, info->hdr->e_shnum,
-				   info->index.pcpu)) {
+		    is_core_symbol(src + i, info->sechdrs, info->hdr->e_shnum)) {
 			strtab_size += strlen(&info->strtab[src[i].st_name]) + 1;
 			ndst++;
 		}
@@ -199,8 +193,7 @@ void add_kallsyms(struct module *mod, const struct load_info *info)
 	for (ndst = i = 0; i < kallsyms->num_symtab; i++) {
 		kallsyms->typetab[i] = elf_type(src + i, info);
 		if (i == 0 || is_livepatch_module(mod) ||
-		    is_core_symbol(src + i, info->sechdrs, info->hdr->e_shnum,
-				   info->index.pcpu)) {
+		    is_core_symbol(src + i, info->sechdrs, info->hdr->e_shnum)) {
 			ssize_t ret;
 
 			mod->core_kallsyms.typetab[ndst] =
diff --git a/kernel/module/main.c b/kernel/module/main.c
index c1b34dc1e89a..ae2678ac7840 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -1619,12 +1619,13 @@ static int apply_relocations(struct module *mod, const struct load_info *info)
 
 		/*
 		 * Don't bother with non-allocated sections.
-		 * An exception is the percpu section, which has separate allocations
-		 * for individual CPUs. We relocate the percpu section in the initial
-		 * ELF template and subsequently copy it to the per-CPU destinations.
+		 *
+		 * Note that .data..percpu has separate allocations for
+		 * individual CPUs. We relocate the section in the
+		 * initial ELF template and subsequently copy it to the
+		 * per-CPU destinations.
 		 */
-		if (!(info->sechdrs[infosec].sh_flags & SHF_ALLOC) &&
-		    (!infosec || infosec != info->index.pcpu))
+		if (!(info->sechdrs[infosec].sh_flags & SHF_ALLOC))
 			continue;
 
 		if (info->sechdrs[i].sh_flags & SHF_RELA_LIVEPATCH)
@@ -1715,7 +1716,7 @@ static void __layout_sections(struct module *mod, struct load_info *info, bool i
 
 			if ((s->sh_flags & masks[m][0]) != masks[m][0]
 			    || (s->sh_flags & masks[m][1])
-			    || s->sh_entsize != ~0UL
+			    || s->sh_entsize != ~0UL /* offset or standalone */
 			    || is_init != module_init_layout_section(sname))
 				continue;
 
@@ -1745,16 +1746,10 @@ static void __layout_sections(struct module *mod, struct load_info *info, bool i
 /*
  * Lay out the SHF_ALLOC sections in a way not dissimilar to how ld
  * might -- code, read-only data, read-write data, small data.  Tally
- * sizes, and place the offsets into sh_entsize fields: high bit means it
- * belongs in init.
+ * sizes, and place the offsets into sh_entsize fields.
  */
 static void layout_sections(struct module *mod, struct load_info *info)
 {
-	unsigned int i;
-
-	for (i = 0; i < info->hdr->e_shnum; i++)
-		info->sechdrs[i].sh_entsize = ~0UL;
-
 	pr_debug("Core section allocation order for %s:\n", mod->name);
 	__layout_sections(mod, info, false);
 
@@ -2822,7 +2817,8 @@ static int move_module(struct module *mod, struct load_info *info)
 		Elf_Shdr *shdr = &info->sechdrs[i];
 		const char *sname;
 
-		if (!(shdr->sh_flags & SHF_ALLOC))
+		if (!(shdr->sh_flags & SHF_ALLOC)
+		    || shdr->sh_entsize == SH_ENTSIZE_STANDALONE)
 			continue;
 
 		sname = info->secstrings + shdr->sh_name;
@@ -2954,6 +2950,7 @@ core_param(module_blacklist, module_blacklist, charp, 0400);
 static struct module *layout_and_allocate(struct load_info *info, int flags)
 {
 	struct module *mod;
+	unsigned int i;
 	int err;
 
 	/* Allow arches to frob section contents and sizes.  */
@@ -2967,8 +2964,13 @@ static struct module *layout_and_allocate(struct load_info *info, int flags)
 	if (err < 0)
 		return ERR_PTR(err);
 
+	/* Repurpose sh_entsize to track where each section is allocated. */
+	for (i = 0; i < info->hdr->e_shnum; i++)
+		info->sechdrs[i].sh_entsize = ~0UL;
+
 	/* We will do a special allocation for per-cpu sections later. */
-	info->sechdrs[info->index.pcpu].sh_flags &= ~(unsigned long)SHF_ALLOC;
+	if (info->index.pcpu)
+		info->sechdrs[info->index.pcpu].sh_entsize = SH_ENTSIZE_STANDALONE;
 
 	/*
 	 * Mark relevant sections as SHF_RO_AFTER_INIT so layout_sections() can
diff --git a/kernel/module/sysfs.c b/kernel/module/sysfs.c
index 01c65d608873..15c6baeb39c4 100644
--- a/kernel/module/sysfs.c
+++ b/kernel/module/sysfs.c
@@ -62,6 +62,15 @@ static void free_sect_attrs(struct module_sect_attrs *sect_attrs)
 	kfree(sect_attrs);
 }
 
+/*
+ * .data..percpu has a separate allocation per CPU and no single
+ * address to report.
+ */
+static bool sect_visible(const struct load_info *info, unsigned int i)
+{
+	return !sect_empty(&info->sechdrs[i]) && i != info->index.pcpu;
+}
+
 static int add_sect_attrs(struct module *mod, const struct load_info *info)
 {
 	struct module_sect_attrs *sect_attrs;
@@ -72,7 +81,7 @@ static int add_sect_attrs(struct module *mod, const struct load_info *info)
 
 	/* Count loaded sections and allocate structures */
 	for (i = 0; i < info->hdr->e_shnum; i++)
-		if (!sect_empty(&info->sechdrs[i]))
+		if (sect_visible(info, i))
 			nloaded++;
 	sect_attrs = kzalloc_flex(*sect_attrs, attrs, nloaded);
 	if (!sect_attrs)
@@ -92,7 +101,7 @@ static int add_sect_attrs(struct module *mod, const struct load_info *info)
 	for (i = 0; i < info->hdr->e_shnum; i++) {
 		Elf_Shdr *sec = &info->sechdrs[i];
 
-		if (sect_empty(sec))
+		if (!sect_visible(info, i))
 			continue;
 		sysfs_bin_attr_init(sattr);
 		sattr->attr.name =
@@ -159,7 +168,7 @@ static int add_notes_attrs(struct module *mod, const struct load_info *info)
 	/* Count notes sections and allocate structures.  */
 	notes = 0;
 	for (i = 0; i < info->hdr->e_shnum; i++)
-		if (!sect_empty(&info->sechdrs[i]) &&
+		if (sect_visible(info, i) &&
 		    info->sechdrs[i].sh_type == SHT_NOTE)
 			++notes;
 
@@ -181,7 +190,7 @@ static int add_notes_attrs(struct module *mod, const struct load_info *info)
 
 	nattr = &notes_attrs->attrs[0];
 	for (loaded = i = 0; i < info->hdr->e_shnum; ++i) {
-		if (sect_empty(&info->sechdrs[i]))
+		if (!sect_visible(info, i))
 			continue;
 		if (info->sechdrs[i].sh_type == SHT_NOTE) {
 			sysfs_bin_attr_init(nattr);
-- 
2.25.1

Re: [PATCH v10 3/6] module: introduce SH_ENTSIZE_STANDALONE for separately allocated sections

[Suren Baghdasaryan]

On Mon, Sep 14, 2026 at 11:59 PM Hao Ge <hao.ge@linux.dev> wrote:
>
> SHF_ALLOC means, per the ELF spec, that a section occupies memory
> during process execution. Some module sections occupy memory
> outside the regular module layout, for example the percpu section
> with its per-CPU allocations. The loader currently excludes such
> a section from the layout by clearing its SHF_ALLOC, which
> overloads the flag with a loader-internal meaning.
> apply_relocations() needs a special case for the section, and
> find_sec(".data..percpu") returns different results before and
> after layout_and_allocate().
>
> Introduce SH_ENTSIZE_STANDALONE to mark sections with a separate
> allocation. The percpu section is its first user. layout_sections()
> and move_module() skip marked sections, and apply_relocations() goes
> back to testing only SHF_ALLOC. Based on a patch by Petr Pavlu [1].
>
> .data..percpu keeps SHF_ALLOC, so it would now show up under
> /sys/module/*/sections/. The section has one instance per CPU and no
> single address to report, and the entry never existed before, so
> skip it in add_sect_attrs(). add_notes_attrs() indexes its attrs[]
> array and skips it too. No functional change otherwise.

And IIUC no userspace-visible changes too, right?

>
> Fixes: 4835f747d3ed ("alloc_tag: support for page allocation tag compression")
> Reported-by: Sashiko <sashiko-bot@kernel.org>
> Link: https://lore.kernel.org/all/499bb60c-c6e3-43a3-bd92-95a0567ece5e@suse.com/ [1]
> Suggested-by: Petr Pavlu <petr.pavlu@suse.com>
> Reviewed-by: Petr Pavlu <petr.pavlu@suse.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Hao Ge <hao.ge@linux.dev>

Reviewed-by: Suren Baghdasaryan <surenb@google.com>

> ---
>  include/linux/module.h   |  2 ++
>  kernel/module/internal.h |  8 ++++++++
>  kernel/module/kallsyms.c | 13 +++----------
>  kernel/module/main.c     | 32 +++++++++++++++++---------------
>  kernel/module/sysfs.c    | 17 +++++++++++++----
>  5 files changed, 43 insertions(+), 29 deletions(-)
>
> diff --git a/include/linux/module.h b/include/linux/module.h
> index 96cc98568eea..0c6f32ddcbf2 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -325,6 +325,8 @@ enum mod_mem_type {
>         MOD_INIT_RODATA,
>
>         MOD_MEM_NUM_TYPES,
> +
> +       MOD_STANDALONE = -2,
>         MOD_INVALID = -1,
>  };
>
> diff --git a/kernel/module/internal.h b/kernel/module/internal.h
> index 061161cc79d9..4c738074a27b 100644
> --- a/kernel/module/internal.h
> +++ b/kernel/module/internal.h
> @@ -29,6 +29,14 @@
>  #define SH_ENTSIZE_TYPE_MASK   ((1UL << SH_ENTSIZE_TYPE_BITS) - 1)
>  #define SH_ENTSIZE_OFFSET_MASK ((1UL << (BITS_PER_LONG - SH_ENTSIZE_TYPE_BITS)) - 1)
>
> +/*
> + * Marker for sections with a separate allocation, which are not placed
> + * into mod->mem[].
> + */
> +#define SH_ENTSIZE_STANDALONE                                  \
> +       (((unsigned long)MOD_STANDALONE & SH_ENTSIZE_TYPE_MASK) \
> +        << SH_ENTSIZE_TYPE_SHIFT)
> +
>  /* Maximum number of characters written by module_flags() */
>  #define MODULE_FLAGS_BUF_SIZE (TAINT_FLAGS_COUNT + 4)
>
> diff --git a/kernel/module/kallsyms.c b/kernel/module/kallsyms.c
> index f23126d804b2..0dca6d40160e 100644
> --- a/kernel/module/kallsyms.c
> +++ b/kernel/module/kallsyms.c
> @@ -76,7 +76,7 @@ static char elf_type(const Elf_Sym *sym, const struct load_info *info)
>  }
>
>  static bool is_core_symbol(const Elf_Sym *src, const Elf_Shdr *sechdrs,
> -                          unsigned int shnum, unsigned int pcpundx)
> +                          unsigned int shnum)
>  {
>         const Elf_Shdr *sec;
>         enum mod_mem_type type;
> @@ -86,11 +86,6 @@ static bool is_core_symbol(const Elf_Sym *src, const Elf_Shdr *sechdrs,
>             !src->st_name)
>                 return false;
>
> -#ifdef CONFIG_KALLSYMS_ALL
> -       if (src->st_shndx == pcpundx)
> -               return true;
> -#endif
> -
>         sec = sechdrs + src->st_shndx;
>         type = sec->sh_entsize >> SH_ENTSIZE_TYPE_SHIFT;
>         if (!(sec->sh_flags & SHF_ALLOC)
> @@ -131,8 +126,7 @@ void layout_symtab(struct module *mod, struct load_info *info)
>         /* Compute total space required for the core symbols' strtab. */
>         for (ndst = i = 0; i < nsrc; i++) {
>                 if (i == 0 || is_livepatch_module(mod) ||
> -                   is_core_symbol(src + i, info->sechdrs, info->hdr->e_shnum,
> -                                  info->index.pcpu)) {
> +                   is_core_symbol(src + i, info->sechdrs, info->hdr->e_shnum)) {
>                         strtab_size += strlen(&info->strtab[src[i].st_name]) + 1;
>                         ndst++;
>                 }
> @@ -199,8 +193,7 @@ void add_kallsyms(struct module *mod, const struct load_info *info)
>         for (ndst = i = 0; i < kallsyms->num_symtab; i++) {
>                 kallsyms->typetab[i] = elf_type(src + i, info);
>                 if (i == 0 || is_livepatch_module(mod) ||
> -                   is_core_symbol(src + i, info->sechdrs, info->hdr->e_shnum,
> -                                  info->index.pcpu)) {
> +                   is_core_symbol(src + i, info->sechdrs, info->hdr->e_shnum)) {
>                         ssize_t ret;
>
>                         mod->core_kallsyms.typetab[ndst] =
> diff --git a/kernel/module/main.c b/kernel/module/main.c
> index c1b34dc1e89a..ae2678ac7840 100644
> --- a/kernel/module/main.c
> +++ b/kernel/module/main.c
> @@ -1619,12 +1619,13 @@ static int apply_relocations(struct module *mod, const struct load_info *info)
>
>                 /*
>                  * Don't bother with non-allocated sections.
> -                * An exception is the percpu section, which has separate allocations
> -                * for individual CPUs. We relocate the percpu section in the initial
> -                * ELF template and subsequently copy it to the per-CPU destinations.
> +                *
> +                * Note that .data..percpu has separate allocations for
> +                * individual CPUs. We relocate the section in the
> +                * initial ELF template and subsequently copy it to the
> +                * per-CPU destinations.
>                  */
> -               if (!(info->sechdrs[infosec].sh_flags & SHF_ALLOC) &&
> -                   (!infosec || infosec != info->index.pcpu))
> +               if (!(info->sechdrs[infosec].sh_flags & SHF_ALLOC))
>                         continue;
>
>                 if (info->sechdrs[i].sh_flags & SHF_RELA_LIVEPATCH)
> @@ -1715,7 +1716,7 @@ static void __layout_sections(struct module *mod, struct load_info *info, bool i
>
>                         if ((s->sh_flags & masks[m][0]) != masks[m][0]
>                             || (s->sh_flags & masks[m][1])
> -                           || s->sh_entsize != ~0UL
> +                           || s->sh_entsize != ~0UL /* offset or standalone */
>                             || is_init != module_init_layout_section(sname))
>                                 continue;
>
> @@ -1745,16 +1746,10 @@ static void __layout_sections(struct module *mod, struct load_info *info, bool i
>  /*
>   * Lay out the SHF_ALLOC sections in a way not dissimilar to how ld
>   * might -- code, read-only data, read-write data, small data.  Tally
> - * sizes, and place the offsets into sh_entsize fields: high bit means it
> - * belongs in init.
> + * sizes, and place the offsets into sh_entsize fields.
>   */
>  static void layout_sections(struct module *mod, struct load_info *info)
>  {
> -       unsigned int i;
> -
> -       for (i = 0; i < info->hdr->e_shnum; i++)
> -               info->sechdrs[i].sh_entsize = ~0UL;
> -
>         pr_debug("Core section allocation order for %s:\n", mod->name);
>         __layout_sections(mod, info, false);
>
> @@ -2822,7 +2817,8 @@ static int move_module(struct module *mod, struct load_info *info)
>                 Elf_Shdr *shdr = &info->sechdrs[i];
>                 const char *sname;
>
> -               if (!(shdr->sh_flags & SHF_ALLOC))
> +               if (!(shdr->sh_flags & SHF_ALLOC)
> +                   || shdr->sh_entsize == SH_ENTSIZE_STANDALONE)
>                         continue;
>
>                 sname = info->secstrings + shdr->sh_name;
> @@ -2954,6 +2950,7 @@ core_param(module_blacklist, module_blacklist, charp, 0400);
>  static struct module *layout_and_allocate(struct load_info *info, int flags)
>  {
>         struct module *mod;
> +       unsigned int i;
>         int err;
>
>         /* Allow arches to frob section contents and sizes.  */
> @@ -2967,8 +2964,13 @@ static struct module *layout_and_allocate(struct load_info *info, int flags)
>         if (err < 0)
>                 return ERR_PTR(err);
>
> +       /* Repurpose sh_entsize to track where each section is allocated. */
> +       for (i = 0; i < info->hdr->e_shnum; i++)
> +               info->sechdrs[i].sh_entsize = ~0UL;
> +
>         /* We will do a special allocation for per-cpu sections later. */
> -       info->sechdrs[info->index.pcpu].sh_flags &= ~(unsigned long)SHF_ALLOC;
> +       if (info->index.pcpu)
> +               info->sechdrs[info->index.pcpu].sh_entsize = SH_ENTSIZE_STANDALONE;
>
>         /*
>          * Mark relevant sections as SHF_RO_AFTER_INIT so layout_sections() can
> diff --git a/kernel/module/sysfs.c b/kernel/module/sysfs.c
> index 01c65d608873..15c6baeb39c4 100644
> --- a/kernel/module/sysfs.c
> +++ b/kernel/module/sysfs.c
> @@ -62,6 +62,15 @@ static void free_sect_attrs(struct module_sect_attrs *sect_attrs)
>         kfree(sect_attrs);
>  }
>
> +/*
> + * .data..percpu has a separate allocation per CPU and no single
> + * address to report.
> + */
> +static bool sect_visible(const struct load_info *info, unsigned int i)
> +{
> +       return !sect_empty(&info->sechdrs[i]) && i != info->index.pcpu;
> +}
> +
>  static int add_sect_attrs(struct module *mod, const struct load_info *info)
>  {
>         struct module_sect_attrs *sect_attrs;
> @@ -72,7 +81,7 @@ static int add_sect_attrs(struct module *mod, const struct load_info *info)
>
>         /* Count loaded sections and allocate structures */
>         for (i = 0; i < info->hdr->e_shnum; i++)
> -               if (!sect_empty(&info->sechdrs[i]))
> +               if (sect_visible(info, i))
>                         nloaded++;
>         sect_attrs = kzalloc_flex(*sect_attrs, attrs, nloaded);
>         if (!sect_attrs)
> @@ -92,7 +101,7 @@ static int add_sect_attrs(struct module *mod, const struct load_info *info)
>         for (i = 0; i < info->hdr->e_shnum; i++) {
>                 Elf_Shdr *sec = &info->sechdrs[i];
>
> -               if (sect_empty(sec))
> +               if (!sect_visible(info, i))
>                         continue;
>                 sysfs_bin_attr_init(sattr);
>                 sattr->attr.name =
> @@ -159,7 +168,7 @@ static int add_notes_attrs(struct module *mod, const struct load_info *info)
>         /* Count notes sections and allocate structures.  */
>         notes = 0;
>         for (i = 0; i < info->hdr->e_shnum; i++)
> -               if (!sect_empty(&info->sechdrs[i]) &&
> +               if (sect_visible(info, i) &&
>                     info->sechdrs[i].sh_type == SHT_NOTE)
>                         ++notes;
>
> @@ -181,7 +190,7 @@ static int add_notes_attrs(struct module *mod, const struct load_info *info)
>
>         nattr = &notes_attrs->attrs[0];
>         for (loaded = i = 0; i < info->hdr->e_shnum; ++i) {
> -               if (sect_empty(&info->sechdrs[i]))
> +               if (!sect_visible(info, i))
>                         continue;
>                 if (info->sechdrs[i].sh_type == SHT_NOTE) {
>                         sysfs_bin_attr_init(nattr);
> --
> 2.25.1
>

Re: [PATCH v10 3/6] module: introduce SH_ENTSIZE_STANDALONE for separately allocated sections

[Hao Ge]

Hi Suren

On 2026/9/18 01:25, Suren Baghdasaryan wrote:
> On Mon, Sep 14, 2026 at 11:59 PM Hao Ge <hao.ge@linux.dev> wrote:
>>
>> SHF_ALLOC means, per the ELF spec, that a section occupies memory
>> during process execution. Some module sections occupy memory
>> outside the regular module layout, for example the percpu section
>> with its per-CPU allocations. The loader currently excludes such
>> a section from the layout by clearing its SHF_ALLOC, which
>> overloads the flag with a loader-internal meaning.
>> apply_relocations() needs a special case for the section, and
>> find_sec(".data..percpu") returns different results before and
>> after layout_and_allocate().
>>
>> Introduce SH_ENTSIZE_STANDALONE to mark sections with a separate
>> allocation. The percpu section is its first user. layout_sections()
>> and move_module() skip marked sections, and apply_relocations() goes
>> back to testing only SHF_ALLOC. Based on a patch by Petr Pavlu [1].
>>
>> .data..percpu keeps SHF_ALLOC, so it would now show up under
>> /sys/module/*/sections/. The section has one instance per CPU and no
>> single address to report, and the entry never existed before, so
>> skip it in add_sect_attrs(). add_notes_attrs() indexes its attrs[]
>> array and skips it too. No functional change otherwise.
> 
> And IIUC no userspace-visible changes too, right?
>

Right, I'll fold this into the commit message for the next version:
"No functional or userspace-visible changes otherwise."

>>
>> Fixes: 4835f747d3ed ("alloc_tag: support for page allocation tag compression")
>> Reported-by: Sashiko <sashiko-bot@kernel.org>
>> Link: https://lore.kernel.org/all/499bb60c-c6e3-43a3-bd92-95a0567ece5e@suse.com/ [1]
>> Suggested-by: Petr Pavlu <petr.pavlu@suse.com>
>> Reviewed-by: Petr Pavlu <petr.pavlu@suse.com>
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Hao Ge <hao.ge@linux.dev>
> 
> Reviewed-by: Suren Baghdasaryan <surenb@google.com>
>

Thanks.

Best Regards
Hao

>> ---
>>  include/linux/module.h   |  2 ++
>>  kernel/module/internal.h |  8 ++++++++
>>  kernel/module/kallsyms.c | 13 +++----------
>>  kernel/module/main.c     | 32 +++++++++++++++++---------------
>>  kernel/module/sysfs.c    | 17 +++++++++++++----
>>  5 files changed, 43 insertions(+), 29 deletions(-)
>>
>> diff --git a/include/linux/module.h b/include/linux/module.h
>> index 96cc98568eea..0c6f32ddcbf2 100644
>> --- a/include/linux/module.h
>> +++ b/include/linux/module.h
>> @@ -325,6 +325,8 @@ enum mod_mem_type {
>>         MOD_INIT_RODATA,
>>
>>         MOD_MEM_NUM_TYPES,
>> +
>> +       MOD_STANDALONE = -2,
>>         MOD_INVALID = -1,
>>  };
>>
>> diff --git a/kernel/module/internal.h b/kernel/module/internal.h
>> index 061161cc79d9..4c738074a27b 100644
>> --- a/kernel/module/internal.h
>> +++ b/kernel/module/internal.h
>> @@ -29,6 +29,14 @@
>>  #define SH_ENTSIZE_TYPE_MASK   ((1UL << SH_ENTSIZE_TYPE_BITS) - 1)
>>  #define SH_ENTSIZE_OFFSET_MASK ((1UL << (BITS_PER_LONG - SH_ENTSIZE_TYPE_BITS)) - 1)
>>
>> +/*
>> + * Marker for sections with a separate allocation, which are not placed
>> + * into mod->mem[].
>> + */
>> +#define SH_ENTSIZE_STANDALONE                                  \
>> +       (((unsigned long)MOD_STANDALONE & SH_ENTSIZE_TYPE_MASK) \
>> +        << SH_ENTSIZE_TYPE_SHIFT)
>> +
>>  /* Maximum number of characters written by module_flags() */
>>  #define MODULE_FLAGS_BUF_SIZE (TAINT_FLAGS_COUNT + 4)
>>
>> diff --git a/kernel/module/kallsyms.c b/kernel/module/kallsyms.c
>> index f23126d804b2..0dca6d40160e 100644
>> --- a/kernel/module/kallsyms.c
>> +++ b/kernel/module/kallsyms.c
>> @@ -76,7 +76,7 @@ static char elf_type(const Elf_Sym *sym, const struct load_info *info)
>>  }
>>
>>  static bool is_core_symbol(const Elf_Sym *src, const Elf_Shdr *sechdrs,
>> -                          unsigned int shnum, unsigned int pcpundx)
>> +                          unsigned int shnum)
>>  {
>>         const Elf_Shdr *sec;
>>         enum mod_mem_type type;
>> @@ -86,11 +86,6 @@ static bool is_core_symbol(const Elf_Sym *src, const Elf_Shdr *sechdrs,
>>             !src->st_name)
>>                 return false;
>>
>> -#ifdef CONFIG_KALLSYMS_ALL
>> -       if (src->st_shndx == pcpundx)
>> -               return true;
>> -#endif
>> -
>>         sec = sechdrs + src->st_shndx;
>>         type = sec->sh_entsize >> SH_ENTSIZE_TYPE_SHIFT;
>>         if (!(sec->sh_flags & SHF_ALLOC)
>> @@ -131,8 +126,7 @@ void layout_symtab(struct module *mod, struct load_info *info)
>>         /* Compute total space required for the core symbols' strtab. */
>>         for (ndst = i = 0; i < nsrc; i++) {
>>                 if (i == 0 || is_livepatch_module(mod) ||
>> -                   is_core_symbol(src + i, info->sechdrs, info->hdr->e_shnum,
>> -                                  info->index.pcpu)) {
>> +                   is_core_symbol(src + i, info->sechdrs, info->hdr->e_shnum)) {
>>                         strtab_size += strlen(&info->strtab[src[i].st_name]) + 1;
>>                         ndst++;
>>                 }
>> @@ -199,8 +193,7 @@ void add_kallsyms(struct module *mod, const struct load_info *info)
>>         for (ndst = i = 0; i < kallsyms->num_symtab; i++) {
>>                 kallsyms->typetab[i] = elf_type(src + i, info);
>>                 if (i == 0 || is_livepatch_module(mod) ||
>> -                   is_core_symbol(src + i, info->sechdrs, info->hdr->e_shnum,
>> -                                  info->index.pcpu)) {
>> +                   is_core_symbol(src + i, info->sechdrs, info->hdr->e_shnum)) {
>>                         ssize_t ret;
>>
>>                         mod->core_kallsyms.typetab[ndst] =
>> diff --git a/kernel/module/main.c b/kernel/module/main.c
>> index c1b34dc1e89a..ae2678ac7840 100644
>> --- a/kernel/module/main.c
>> +++ b/kernel/module/main.c
>> @@ -1619,12 +1619,13 @@ static int apply_relocations(struct module *mod, const struct load_info *info)
>>
>>                 /*
>>                  * Don't bother with non-allocated sections.
>> -                * An exception is the percpu section, which has separate allocations
>> -                * for individual CPUs. We relocate the percpu section in the initial
>> -                * ELF template and subsequently copy it to the per-CPU destinations.
>> +                *
>> +                * Note that .data..percpu has separate allocations for
>> +                * individual CPUs. We relocate the section in the
>> +                * initial ELF template and subsequently copy it to the
>> +                * per-CPU destinations.
>>                  */
>> -               if (!(info->sechdrs[infosec].sh_flags & SHF_ALLOC) &&
>> -                   (!infosec || infosec != info->index.pcpu))
>> +               if (!(info->sechdrs[infosec].sh_flags & SHF_ALLOC))
>>                         continue;
>>
>>                 if (info->sechdrs[i].sh_flags & SHF_RELA_LIVEPATCH)
>> @@ -1715,7 +1716,7 @@ static void __layout_sections(struct module *mod, struct load_info *info, bool i
>>
>>                         if ((s->sh_flags & masks[m][0]) != masks[m][0]
>>                             || (s->sh_flags & masks[m][1])
>> -                           || s->sh_entsize != ~0UL
>> +                           || s->sh_entsize != ~0UL /* offset or standalone */
>>                             || is_init != module_init_layout_section(sname))
>>                                 continue;
>>
>> @@ -1745,16 +1746,10 @@ static void __layout_sections(struct module *mod, struct load_info *info, bool i
>>  /*
>>   * Lay out the SHF_ALLOC sections in a way not dissimilar to how ld
>>   * might -- code, read-only data, read-write data, small data.  Tally
>> - * sizes, and place the offsets into sh_entsize fields: high bit means it
>> - * belongs in init.
>> + * sizes, and place the offsets into sh_entsize fields.
>>   */
>>  static void layout_sections(struct module *mod, struct load_info *info)
>>  {
>> -       unsigned int i;
>> -
>> -       for (i = 0; i < info->hdr->e_shnum; i++)
>> -               info->sechdrs[i].sh_entsize = ~0UL;
>> -
>>         pr_debug("Core section allocation order for %s:\n", mod->name);
>>         __layout_sections(mod, info, false);
>>
>> @@ -2822,7 +2817,8 @@ static int move_module(struct module *mod, struct load_info *info)
>>                 Elf_Shdr *shdr = &info->sechdrs[i];
>>                 const char *sname;
>>
>> -               if (!(shdr->sh_flags & SHF_ALLOC))
>> +               if (!(shdr->sh_flags & SHF_ALLOC)
>> +                   || shdr->sh_entsize == SH_ENTSIZE_STANDALONE)
>>                         continue;
>>
>>                 sname = info->secstrings + shdr->sh_name;
>> @@ -2954,6 +2950,7 @@ core_param(module_blacklist, module_blacklist, charp, 0400);
>>  static struct module *layout_and_allocate(struct load_info *info, int flags)
>>  {
>>         struct module *mod;
>> +       unsigned int i;
>>         int err;
>>
>>         /* Allow arches to frob section contents and sizes.  */
>> @@ -2967,8 +2964,13 @@ static struct module *layout_and_allocate(struct load_info *info, int flags)
>>         if (err < 0)
>>                 return ERR_PTR(err);
>>
>> +       /* Repurpose sh_entsize to track where each section is allocated. */
>> +       for (i = 0; i < info->hdr->e_shnum; i++)
>> +               info->sechdrs[i].sh_entsize = ~0UL;
>> +
>>         /* We will do a special allocation for per-cpu sections later. */
>> -       info->sechdrs[info->index.pcpu].sh_flags &= ~(unsigned long)SHF_ALLOC;
>> +       if (info->index.pcpu)
>> +               info->sechdrs[info->index.pcpu].sh_entsize = SH_ENTSIZE_STANDALONE;
>>
>>         /*
>>          * Mark relevant sections as SHF_RO_AFTER_INIT so layout_sections() can
>> diff --git a/kernel/module/sysfs.c b/kernel/module/sysfs.c
>> index 01c65d608873..15c6baeb39c4 100644
>> --- a/kernel/module/sysfs.c
>> +++ b/kernel/module/sysfs.c
>> @@ -62,6 +62,15 @@ static void free_sect_attrs(struct module_sect_attrs *sect_attrs)
>>         kfree(sect_attrs);
>>  }
>>
>> +/*
>> + * .data..percpu has a separate allocation per CPU and no single
>> + * address to report.
>> + */
>> +static bool sect_visible(const struct load_info *info, unsigned int i)
>> +{
>> +       return !sect_empty(&info->sechdrs[i]) && i != info->index.pcpu;
>> +}
>> +
>>  static int add_sect_attrs(struct module *mod, const struct load_info *info)
>>  {
>>         struct module_sect_attrs *sect_attrs;
>> @@ -72,7 +81,7 @@ static int add_sect_attrs(struct module *mod, const struct load_info *info)
>>
>>         /* Count loaded sections and allocate structures */
>>         for (i = 0; i < info->hdr->e_shnum; i++)
>> -               if (!sect_empty(&info->sechdrs[i]))
>> +               if (sect_visible(info, i))
>>                         nloaded++;
>>         sect_attrs = kzalloc_flex(*sect_attrs, attrs, nloaded);
>>         if (!sect_attrs)
>> @@ -92,7 +101,7 @@ static int add_sect_attrs(struct module *mod, const struct load_info *info)
>>         for (i = 0; i < info->hdr->e_shnum; i++) {
>>                 Elf_Shdr *sec = &info->sechdrs[i];
>>
>> -               if (sect_empty(sec))
>> +               if (!sect_visible(info, i))
>>                         continue;
>>                 sysfs_bin_attr_init(sattr);
>>                 sattr->attr.name =
>> @@ -159,7 +168,7 @@ static int add_notes_attrs(struct module *mod, const struct load_info *info)
>>         /* Count notes sections and allocate structures.  */
>>         notes = 0;
>>         for (i = 0; i < info->hdr->e_shnum; i++)
>> -               if (!sect_empty(&info->sechdrs[i]) &&
>> +               if (sect_visible(info, i) &&
>>                     info->sechdrs[i].sh_type == SHT_NOTE)
>>                         ++notes;
>>
>> @@ -181,7 +190,7 @@ static int add_notes_attrs(struct module *mod, const struct load_info *info)
>>
>>         nattr = &notes_attrs->attrs[0];
>>         for (loaded = i = 0; i < info->hdr->e_shnum; ++i) {
>> -               if (sect_empty(&info->sechdrs[i]))
>> +               if (!sect_visible(info, i))
>>                         continue;
>>                 if (info->sechdrs[i].sh_type == SHT_NOTE) {
>>                         sysfs_bin_attr_init(nattr);
>> --
>> 2.25.1
>>

[PATCH v10 4/6] module: allocate codetag sections before the regular module layout

[Hao Ge]

Whether a codetag section goes to the codetag region is decided by
layout_sections() and asked again in move_module(). A concurrent
load can shut profiling down in between, and move_module() then
copies the section to offset 0 of its regular destination,
overwriting whatever is there.

Decide and allocate in one pass, before the layout. Allocation
errors fail the load. On a tag area overflow profiling is already
disabled, so -EAGAIN makes the section fall back to regular module
data and the module still loads. The reservation is released and
module_tags.size rolled back, so a concurrent load which already
passed needs_section_mem() does not skip vm_module_tags_populate().

An SHT_NOBITS codetag section is zeroed explicitly, the tag area
pages are not zeroed on allocation.

When profiling was toggled off the overflow check did not run, a
module could load with more tags than the page flags can address,
and re-enabling profiling then silently corrupted /proc/allocinfo.
The check no longer depends on mem_alloc_profiling_enabled().

Based on a patch by Petr Pavlu [1].

Fixes: 4835f747d3ed ("alloc_tag: support for page allocation tag compression")
Reported-by: Sashiko <sashiko-bot@kernel.org>
Link: https://lore.kernel.org/all/499bb60c-c6e3-43a3-bd92-95a0567ece5e@suse.com/ [1]
Reviewed-by: Petr Pavlu <petr.pavlu@suse.com>
Cc: stable@vger.kernel.org
Signed-off-by: Hao Ge <hao.ge@linux.dev>
---
 kernel/module/main.c | 101 +++++++++++++++++++++++--------------------
 mm/alloc_tag.c       |   9 ++--
 2 files changed, 60 insertions(+), 50 deletions(-)

diff --git a/kernel/module/main.c b/kernel/module/main.c
index ae2678ac7840..b7ebcc40bdda 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -1723,20 +1723,6 @@ static void __layout_sections(struct module *mod, struct load_info *info, bool i
 			if (WARN_ON_ONCE(type == MOD_INVALID))
 				continue;
 
-			/*
-			 * Do not allocate codetag memory as we load it into
-			 * preallocated contiguous memory.
-			 */
-			if (codetag_needs_module_section(mod, sname, s->sh_size)) {
-				/*
-				 * s->sh_entsize won't be used but populate the
-				 * type field to avoid confusion.
-				 */
-				s->sh_entsize = ((unsigned long)(type) & SH_ENTSIZE_TYPE_MASK)
-						<< SH_ENTSIZE_TYPE_SHIFT;
-				continue;
-			}
-
 			s->sh_entsize = module_get_offset_and_type(mod, type, s, i);
 			pr_debug("\t%s\n", sname);
 		}
@@ -2795,7 +2781,6 @@ static int move_module(struct module *mod, struct load_info *info)
 {
 	int i, ret;
 	enum mod_mem_type t = MOD_MEM_NUM_TYPES;
-	bool codetag_section_found = false;
 
 	for_each_mod_mem_type(type) {
 		if (!mod->mem[type].size) {
@@ -2815,35 +2800,13 @@ static int move_module(struct module *mod, struct load_info *info)
 	for (i = 0; i < info->hdr->e_shnum; i++) {
 		void *dest;
 		Elf_Shdr *shdr = &info->sechdrs[i];
-		const char *sname;
 
 		if (!(shdr->sh_flags & SHF_ALLOC)
 		    || shdr->sh_entsize == SH_ENTSIZE_STANDALONE)
 			continue;
 
-		sname = info->secstrings + shdr->sh_name;
-		/*
-		 * Load codetag sections separately as they might still be used
-		 * after module unload.
-		 */
-		if (codetag_needs_module_section(mod, sname, shdr->sh_size)) {
-			dest = codetag_alloc_module_section(mod, sname, shdr->sh_size,
-					arch_mod_section_prepend(mod, i), shdr->sh_addralign);
-			if (WARN_ON(!dest)) {
-				ret = -EINVAL;
-				goto out_err;
-			}
-			if (IS_ERR(dest)) {
-				ret = PTR_ERR(dest);
-				goto out_err;
-			}
-			codetag_section_found = true;
-		} else {
-			enum mod_mem_type type = shdr->sh_entsize >> SH_ENTSIZE_TYPE_SHIFT;
-			unsigned long offset = shdr->sh_entsize & SH_ENTSIZE_OFFSET_MASK;
-
-			dest = mod->mem[type].base + offset;
-		}
+		dest = mod->mem[shdr->sh_entsize >> SH_ENTSIZE_TYPE_SHIFT].base +
+		       (shdr->sh_entsize & SH_ENTSIZE_OFFSET_MASK);
 
 		if (shdr->sh_type != SHT_NOBITS) {
 			/*
@@ -2875,8 +2838,6 @@ static int move_module(struct module *mod, struct load_info *info)
 	module_memory_restore_rox(mod);
 	while (t--)
 		module_memory_free(mod, t);
-	if (codetag_section_found)
-		codetag_free_module_sections(mod);
 
 	return ret;
 }
@@ -2947,6 +2908,49 @@ static bool blacklisted(const char *module_name)
 }
 core_param(module_blacklist, module_blacklist, charp, 0400);
 
+/*
+ * Allocate codetag sections separately. They are loaded into preallocated
+ * contiguous memory because they may still be used after the module is
+ * unloaded.
+ *
+ * If the separate allocation overflows, allocate the section normally
+ * so that the module can still be loaded.
+ */
+static int allocate_codetag_sections(struct load_info *info)
+{
+	for (unsigned int i = 1; i < info->hdr->e_shnum; i++) {
+		Elf_Shdr *shdr = &info->sechdrs[i];
+		const char *sname = info->secstrings + shdr->sh_name;
+		void *dest;
+
+		if (!codetag_needs_module_section(info->mod, sname, shdr->sh_size))
+			continue;
+
+		dest = codetag_alloc_module_section(info->mod, sname, shdr->sh_size,
+				arch_mod_section_prepend(info->mod, i), shdr->sh_addralign);
+		if (WARN_ON(!dest)) {
+			codetag_free_module_sections(info->mod);
+			return -EINVAL;
+		}
+		if (dest == ERR_PTR(-EAGAIN))
+			/* Allocate the section as a regular section. */
+			continue;
+		if (IS_ERR(dest)) {
+			codetag_free_module_sections(info->mod);
+			return PTR_ERR(dest);
+		}
+
+		if (shdr->sh_type != SHT_NOBITS)
+			memcpy(dest, (void *)shdr->sh_addr, shdr->sh_size);
+		else
+			memset(dest, 0, shdr->sh_size);
+		shdr->sh_addr = (unsigned long)dest;
+		shdr->sh_entsize = SH_ENTSIZE_STANDALONE;
+	}
+
+	return 0;
+}
+
 static struct module *layout_and_allocate(struct load_info *info, int flags)
 {
 	struct module *mod;
@@ -2979,18 +2983,21 @@ static struct module *layout_and_allocate(struct load_info *info, int flags)
 	 */
 	module_mark_ro_after_init(info->hdr, info->sechdrs, info->secstrings);
 
-	/*
-	 * Determine total sizes, and put offsets in sh_entsize.  For now
-	 * this is done generically; there doesn't appear to be any
-	 * special cases for the architectures.
-	 */
+	/* Allow codetag sections to be allocated separately first. */
+	err = allocate_codetag_sections(info);
+	if (err)
+		return ERR_PTR(err);
+
+	/* Determine total sizes and put offsets in sh_entsize. */
 	layout_sections(info->mod, info);
 	layout_symtab(info->mod, info);
 
 	/* Allocate and move to the final place */
 	err = move_module(info->mod, info);
-	if (err)
+	if (err) {
+		codetag_free_module_sections(info->mod);
 		return ERR_PTR(err);
+	}
 
 	/* Module has been copied to its final place now: return it. */
 	mod = (void *)info->sechdrs[info->index.mod].sh_addr;
diff --git a/mm/alloc_tag.c b/mm/alloc_tag.c
index 95ddf5b743d0..5836803898ad 100644
--- a/mm/alloc_tag.c
+++ b/mm/alloc_tag.c
@@ -958,10 +958,13 @@ static void *reserve_module_tags(struct module *mod, unsigned long size,
 		int grow_res;
 
 		module_tags.size = offset + size;
-		if (mem_alloc_profiling_enabled() && !tags_addressable()) {
+		if (!tags_addressable()) {
 			shutdown_mem_profiling(true);
-			pr_warn("With module %s there are too many tags to fit in %d page flag bits. Memory allocation profiling is disabled!\n",
-				mod->name, NR_UNUSED_PAGEFLAG_BITS);
+			pr_warn_once("With module %s there are too many tags to fit in %d page flag bits. Memory allocation profiling is disabled!\n",
+				     mod->name, NR_UNUSED_PAGEFLAG_BITS);
+			release_module_tags(mod, false);
+			module_tags.size = prev_size;
+			return ERR_PTR(-EAGAIN);
 		}
 
 		grow_res = vm_module_tags_populate();
-- 
2.25.1

Re: [PATCH v10 4/6] module: allocate codetag sections before the regular module layout

[Suren Baghdasaryan]

On Mon, Sep 14, 2026 at 11:59 PM Hao Ge <hao.ge@linux.dev> wrote:
>
> Whether a codetag section goes to the codetag region is decided by
> layout_sections() and asked again in move_module(). A concurrent
> load can shut profiling down in between, and move_module() then
> copies the section to offset 0 of its regular destination,
> overwriting whatever is there.
>
> Decide and allocate in one pass, before the layout. Allocation
> errors fail the load. On a tag area overflow profiling is already
> disabled, so -EAGAIN makes the section fall back to regular module
> data and the module still loads. The reservation is released and
> module_tags.size rolled back, so a concurrent load which already
> passed needs_section_mem() does not skip vm_module_tags_populate().
>
> An SHT_NOBITS codetag section is zeroed explicitly, the tag area
> pages are not zeroed on allocation.
>
> When profiling was toggled off the overflow check did not run, a
> module could load with more tags than the page flags can address,
> and re-enabling profiling then silently corrupted /proc/allocinfo.
> The check no longer depends on mem_alloc_profiling_enabled().
>
> Based on a patch by Petr Pavlu [1].
>
> Fixes: 4835f747d3ed ("alloc_tag: support for page allocation tag compression")
> Reported-by: Sashiko <sashiko-bot@kernel.org>
> Link: https://lore.kernel.org/all/499bb60c-c6e3-43a3-bd92-95a0567ece5e@suse.com/ [1]
> Reviewed-by: Petr Pavlu <petr.pavlu@suse.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Hao Ge <hao.ge@linux.dev>

Makes the logic much simpler. Thanks!

Reviewed-by: Suren Baghdasaryan <surenb@google.com>

> ---
>  kernel/module/main.c | 101 +++++++++++++++++++++++--------------------
>  mm/alloc_tag.c       |   9 ++--
>  2 files changed, 60 insertions(+), 50 deletions(-)
>
> diff --git a/kernel/module/main.c b/kernel/module/main.c
> index ae2678ac7840..b7ebcc40bdda 100644
> --- a/kernel/module/main.c
> +++ b/kernel/module/main.c
> @@ -1723,20 +1723,6 @@ static void __layout_sections(struct module *mod, struct load_info *info, bool i
>                         if (WARN_ON_ONCE(type == MOD_INVALID))
>                                 continue;
>
> -                       /*
> -                        * Do not allocate codetag memory as we load it into
> -                        * preallocated contiguous memory.
> -                        */
> -                       if (codetag_needs_module_section(mod, sname, s->sh_size)) {
> -                               /*
> -                                * s->sh_entsize won't be used but populate the
> -                                * type field to avoid confusion.
> -                                */
> -                               s->sh_entsize = ((unsigned long)(type) & SH_ENTSIZE_TYPE_MASK)
> -                                               << SH_ENTSIZE_TYPE_SHIFT;
> -                               continue;
> -                       }
> -
>                         s->sh_entsize = module_get_offset_and_type(mod, type, s, i);
>                         pr_debug("\t%s\n", sname);
>                 }
> @@ -2795,7 +2781,6 @@ static int move_module(struct module *mod, struct load_info *info)
>  {
>         int i, ret;
>         enum mod_mem_type t = MOD_MEM_NUM_TYPES;
> -       bool codetag_section_found = false;
>
>         for_each_mod_mem_type(type) {
>                 if (!mod->mem[type].size) {
> @@ -2815,35 +2800,13 @@ static int move_module(struct module *mod, struct load_info *info)
>         for (i = 0; i < info->hdr->e_shnum; i++) {
>                 void *dest;
>                 Elf_Shdr *shdr = &info->sechdrs[i];
> -               const char *sname;
>
>                 if (!(shdr->sh_flags & SHF_ALLOC)
>                     || shdr->sh_entsize == SH_ENTSIZE_STANDALONE)
>                         continue;
>
> -               sname = info->secstrings + shdr->sh_name;
> -               /*
> -                * Load codetag sections separately as they might still be used
> -                * after module unload.
> -                */
> -               if (codetag_needs_module_section(mod, sname, shdr->sh_size)) {
> -                       dest = codetag_alloc_module_section(mod, sname, shdr->sh_size,
> -                                       arch_mod_section_prepend(mod, i), shdr->sh_addralign);
> -                       if (WARN_ON(!dest)) {
> -                               ret = -EINVAL;
> -                               goto out_err;
> -                       }
> -                       if (IS_ERR(dest)) {
> -                               ret = PTR_ERR(dest);
> -                               goto out_err;
> -                       }
> -                       codetag_section_found = true;
> -               } else {
> -                       enum mod_mem_type type = shdr->sh_entsize >> SH_ENTSIZE_TYPE_SHIFT;
> -                       unsigned long offset = shdr->sh_entsize & SH_ENTSIZE_OFFSET_MASK;
> -
> -                       dest = mod->mem[type].base + offset;
> -               }
> +               dest = mod->mem[shdr->sh_entsize >> SH_ENTSIZE_TYPE_SHIFT].base +
> +                      (shdr->sh_entsize & SH_ENTSIZE_OFFSET_MASK);
>
>                 if (shdr->sh_type != SHT_NOBITS) {
>                         /*
> @@ -2875,8 +2838,6 @@ static int move_module(struct module *mod, struct load_info *info)
>         module_memory_restore_rox(mod);
>         while (t--)
>                 module_memory_free(mod, t);
> -       if (codetag_section_found)
> -               codetag_free_module_sections(mod);
>
>         return ret;
>  }
> @@ -2947,6 +2908,49 @@ static bool blacklisted(const char *module_name)
>  }
>  core_param(module_blacklist, module_blacklist, charp, 0400);
>
> +/*
> + * Allocate codetag sections separately. They are loaded into preallocated
> + * contiguous memory because they may still be used after the module is
> + * unloaded.
> + *
> + * If the separate allocation overflows, allocate the section normally
> + * so that the module can still be loaded.
> + */
> +static int allocate_codetag_sections(struct load_info *info)
> +{
> +       for (unsigned int i = 1; i < info->hdr->e_shnum; i++) {
> +               Elf_Shdr *shdr = &info->sechdrs[i];
> +               const char *sname = info->secstrings + shdr->sh_name;
> +               void *dest;
> +
> +               if (!codetag_needs_module_section(info->mod, sname, shdr->sh_size))
> +                       continue;
> +
> +               dest = codetag_alloc_module_section(info->mod, sname, shdr->sh_size,
> +                               arch_mod_section_prepend(info->mod, i), shdr->sh_addralign);
> +               if (WARN_ON(!dest)) {
> +                       codetag_free_module_sections(info->mod);
> +                       return -EINVAL;
> +               }
> +               if (dest == ERR_PTR(-EAGAIN))
> +                       /* Allocate the section as a regular section. */
> +                       continue;
> +               if (IS_ERR(dest)) {
> +                       codetag_free_module_sections(info->mod);
> +                       return PTR_ERR(dest);
> +               }
> +
> +               if (shdr->sh_type != SHT_NOBITS)
> +                       memcpy(dest, (void *)shdr->sh_addr, shdr->sh_size);
> +               else
> +                       memset(dest, 0, shdr->sh_size);
> +               shdr->sh_addr = (unsigned long)dest;
> +               shdr->sh_entsize = SH_ENTSIZE_STANDALONE;
> +       }
> +
> +       return 0;
> +}
> +
>  static struct module *layout_and_allocate(struct load_info *info, int flags)
>  {
>         struct module *mod;
> @@ -2979,18 +2983,21 @@ static struct module *layout_and_allocate(struct load_info *info, int flags)
>          */
>         module_mark_ro_after_init(info->hdr, info->sechdrs, info->secstrings);
>
> -       /*
> -        * Determine total sizes, and put offsets in sh_entsize.  For now
> -        * this is done generically; there doesn't appear to be any
> -        * special cases for the architectures.
> -        */
> +       /* Allow codetag sections to be allocated separately first. */
> +       err = allocate_codetag_sections(info);
> +       if (err)
> +               return ERR_PTR(err);
> +
> +       /* Determine total sizes and put offsets in sh_entsize. */
>         layout_sections(info->mod, info);
>         layout_symtab(info->mod, info);
>
>         /* Allocate and move to the final place */
>         err = move_module(info->mod, info);
> -       if (err)
> +       if (err) {
> +               codetag_free_module_sections(info->mod);
>                 return ERR_PTR(err);
> +       }
>
>         /* Module has been copied to its final place now: return it. */
>         mod = (void *)info->sechdrs[info->index.mod].sh_addr;
> diff --git a/mm/alloc_tag.c b/mm/alloc_tag.c
> index 95ddf5b743d0..5836803898ad 100644
> --- a/mm/alloc_tag.c
> +++ b/mm/alloc_tag.c
> @@ -958,10 +958,13 @@ static void *reserve_module_tags(struct module *mod, unsigned long size,
>                 int grow_res;
>
>                 module_tags.size = offset + size;
> -               if (mem_alloc_profiling_enabled() && !tags_addressable()) {
> +               if (!tags_addressable()) {
>                         shutdown_mem_profiling(true);
> -                       pr_warn("With module %s there are too many tags to fit in %d page flag bits. Memory allocation profiling is disabled!\n",
> -                               mod->name, NR_UNUSED_PAGEFLAG_BITS);
> +                       pr_warn_once("With module %s there are too many tags to fit in %d page flag bits. Memory allocation profiling is disabled!\n",
> +                                    mod->name, NR_UNUSED_PAGEFLAG_BITS);
> +                       release_module_tags(mod, false);
> +                       module_tags.size = prev_size;
> +                       return ERR_PTR(-EAGAIN);
>                 }
>
>                 grow_res = vm_module_tags_populate();
> --
> 2.25.1
>

[PATCH v10 5/6] alloc_tag: skip percpu counter allocation when profiling is disabled

[Hao Ge]

After shutdown_mem_profiling() clears mem_profiling_support,
needs_section_mem() returns false, so later modules have their codetag
section placed as regular data and never enter the alloc_tag maple tree.
codetag_load_module() still called load_module(), which allocated a percpu
counter for every tag; release_module_tags() could not find these modules
on unload, so the counters leaked.

Return -EOPNOTSUPP from load_module() when profiling is off:
codetag_module_init() drops the module's cmod, no counters are allocated
and the module loads without its tags. codetag_unload_module() now always
calls free_section_mem(), since a module whose module_load() returned
-EOPNOTSUPP is not in the idr but may still hold a reserved section.

Reported-by: Sashiko <sashiko-bot@kernel.org>
Fixes: 4835f747d3ed ("alloc_tag: support for page allocation tag compression")
Cc: stable@vger.kernel.org
Suggested-by: Suren Baghdasaryan <surenb@google.com>
Acked-by: Suren Baghdasaryan <surenb@google.com>
Signed-off-by: Hao Ge <hao.ge@linux.dev>
---
 lib/codetag.c  | 10 ++++++++--
 mm/alloc_tag.c |  4 ++++
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/lib/codetag.c b/lib/codetag.c
index a9cda4c962a3..a0b600720afc 100644
--- a/lib/codetag.c
+++ b/lib/codetag.c
@@ -240,7 +240,9 @@ static int codetag_module_init(struct codetag_type *cttype, struct module *mod)
 
 	if (err < 0) {
 		kfree(cmod);
-		return err;
+		/* -EOPNOTSUPP means we can load the module without its tag. */
+		if (err != -EOPNOTSUPP)
+			return err;
 	}
 
 	return 0;
@@ -388,7 +390,11 @@ void codetag_unload_module(struct module *mod)
 			++cttype->content_id;
 		}
 		up_write(&cttype->mod_lock);
-		if (found && cttype->desc.free_section_mem)
+		/*
+		 * A module whose module_load() returned -EOPNOTSUPP is not
+		 * in the idr but may still hold reserved section memory.
+		 */
+		if (cttype->desc.free_section_mem)
 			cttype->desc.free_section_mem(mod, true);
 	}
 	mutex_unlock(&codetag_lock);
diff --git a/mm/alloc_tag.c b/mm/alloc_tag.c
index 5836803898ad..1ca0409b492b 100644
--- a/mm/alloc_tag.c
+++ b/mm/alloc_tag.c
@@ -988,6 +988,10 @@ static int load_module(struct module *mod, struct codetag *start, struct codetag
 	struct alloc_tag *stop_tag;
 	struct alloc_tag *tag;
 
+	/* Profiling disabled: load the module without its tags. */
+	if (!mem_profiling_support)
+		return -EOPNOTSUPP;
+
 	/* percpu counters for core allocations are already statically allocated */
 	if (!mod)
 		return 0;
-- 
2.25.1

Re: [PATCH v10 5/6] alloc_tag: skip percpu counter allocation when profiling is disabled

[Suren Baghdasaryan]

On Mon, Sep 14, 2026 at 11:59 PM Hao Ge <hao.ge@linux.dev> wrote:
>
> After shutdown_mem_profiling() clears mem_profiling_support,
> needs_section_mem() returns false, so later modules have their codetag
> section placed as regular data and never enter the alloc_tag maple tree.
> codetag_load_module() still called load_module(), which allocated a percpu
> counter for every tag; release_module_tags() could not find these modules
> on unload, so the counters leaked.
>
> Return -EOPNOTSUPP from load_module() when profiling is off:
> codetag_module_init() drops the module's cmod, no counters are allocated
> and the module loads without its tags. codetag_unload_module() now always
> calls free_section_mem(), since a module whose module_load() returned
> -EOPNOTSUPP is not in the idr but may still hold a reserved section.
>
> Reported-by: Sashiko <sashiko-bot@kernel.org>
> Fixes: 4835f747d3ed ("alloc_tag: support for page allocation tag compression")
> Cc: stable@vger.kernel.org
> Suggested-by: Suren Baghdasaryan <surenb@google.com>
> Acked-by: Suren Baghdasaryan <surenb@google.com>

Yeah, I guess I didn't think about alternatives...

When mem_profiling_support=false, why can't load_module() skip
allocating percpu counters, zero out the whole area between start_tag
and stop_tag and return 0 as success (mem_profiling_support is not
enabled, so we acted accordingly)?
Module will be added into maple tree and later codetag_unload_module()
will find the module, call release_module_tags() (which is a NOOP
because tag->counter==NULL) and finally call free_section_mem(). This
seems more natural than special-casing with EOPNOTSUPP. WDYT?

> Signed-off-by: Hao Ge <hao.ge@linux.dev>
> ---
>  lib/codetag.c  | 10 ++++++++--
>  mm/alloc_tag.c |  4 ++++
>  2 files changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/lib/codetag.c b/lib/codetag.c
> index a9cda4c962a3..a0b600720afc 100644
> --- a/lib/codetag.c
> +++ b/lib/codetag.c
> @@ -240,7 +240,9 @@ static int codetag_module_init(struct codetag_type *cttype, struct module *mod)
>
>         if (err < 0) {
>                 kfree(cmod);
> -               return err;
> +               /* -EOPNOTSUPP means we can load the module without its tag. */
> +               if (err != -EOPNOTSUPP)
> +                       return err;
>         }
>
>         return 0;
> @@ -388,7 +390,11 @@ void codetag_unload_module(struct module *mod)
>                         ++cttype->content_id;
>                 }
>                 up_write(&cttype->mod_lock);
> -               if (found && cttype->desc.free_section_mem)
> +               /*
> +                * A module whose module_load() returned -EOPNOTSUPP is not
> +                * in the idr but may still hold reserved section memory.
> +                */
> +               if (cttype->desc.free_section_mem)
>                         cttype->desc.free_section_mem(mod, true);
>         }
>         mutex_unlock(&codetag_lock);
> diff --git a/mm/alloc_tag.c b/mm/alloc_tag.c
> index 5836803898ad..1ca0409b492b 100644
> --- a/mm/alloc_tag.c
> +++ b/mm/alloc_tag.c
> @@ -988,6 +988,10 @@ static int load_module(struct module *mod, struct codetag *start, struct codetag
>         struct alloc_tag *stop_tag;
>         struct alloc_tag *tag;
>
> +       /* Profiling disabled: load the module without its tags. */
> +       if (!mem_profiling_support)
> +               return -EOPNOTSUPP;
> +
>         /* percpu counters for core allocations are already statically allocated */
>         if (!mod)
>                 return 0;
> --
> 2.25.1
>

Re: [PATCH v10 5/6] alloc_tag: skip percpu counter allocation when profiling is disabled

[Hao Ge]

Hi Suren

On 2026/9/18 08:53, Suren Baghdasaryan wrote:
> On Mon, Sep 14, 2026 at 11:59 PM Hao Ge <hao.ge@linux.dev> wrote:
>>
>> After shutdown_mem_profiling() clears mem_profiling_support,
>> needs_section_mem() returns false, so later modules have their codetag
>> section placed as regular data and never enter the alloc_tag maple tree.
>> codetag_load_module() still called load_module(), which allocated a percpu
>> counter for every tag; release_module_tags() could not find these modules
>> on unload, so the counters leaked.
>>
>> Return -EOPNOTSUPP from load_module() when profiling is off:
>> codetag_module_init() drops the module's cmod, no counters are allocated
>> and the module loads without its tags. codetag_unload_module() now always
>> calls free_section_mem(), since a module whose module_load() returned
>> -EOPNOTSUPP is not in the idr but may still hold a reserved section.
>>
>> Reported-by: Sashiko <sashiko-bot@kernel.org>
>> Fixes: 4835f747d3ed ("alloc_tag: support for page allocation tag compression")
>> Cc: stable@vger.kernel.org
>> Suggested-by: Suren Baghdasaryan <surenb@google.com>
>> Acked-by: Suren Baghdasaryan <surenb@google.com>
> 
> Yeah, I guess I didn't think about alternatives...
> 
> When mem_profiling_support=false, why can't load_module() skip
> allocating percpu counters, zero out the whole area between start_tag
> and stop_tag and return 0 as success (mem_profiling_support is not
> enabled, so we acted accordingly)?
> Module will be added into maple tree and later codetag_unload_module()
> will find the module, call release_module_tags() (which is a NOOP
> because tag->counter==NULL) and finally call free_section_mem(). This
> seems more natural than special-casing with EOPNOTSUPP. WDYT?
> 

I see, but the module would then sit in the idr with NULL-counter
tags that become visible to readers which today never expect that state.

alloc_tag_top_users calls codetag_next_ct() to find the next codetag
and alloc_tag_read() would do per_cpu_ptr(NULL) for each cpu - plain
pointer arithmetic onto a wild address, an oops straight out of the
OOM report. 

https://elixir.bootlin.com/linux/v7.3-rc3/source/mm/alloc_tag.c#L503

So if the codetag section within this codetag_type ends up meaningless,
is there a reason we still return success? We will still maintain cttype->count
and allocate a slot for this module inside mod_idr.

I'd rather keep the -EOPNOTSUPP version (your original suggestion),
also because the comments in load_module() and codetag_module_init()
already document this scenario.

WDYT?

Thanks
Best Regards
Hao

>> Signed-off-by: Hao Ge <hao.ge@linux.dev>
>> ---
>>  lib/codetag.c  | 10 ++++++++--
>>  mm/alloc_tag.c |  4 ++++
>>  2 files changed, 12 insertions(+), 2 deletions(-)
>>
>> diff --git a/lib/codetag.c b/lib/codetag.c
>> index a9cda4c962a3..a0b600720afc 100644
>> --- a/lib/codetag.c
>> +++ b/lib/codetag.c
>> @@ -240,7 +240,9 @@ static int codetag_module_init(struct codetag_type *cttype, struct module *mod)
>>
>>         if (err < 0) {
>>                 kfree(cmod);
>> -               return err;
>> +               /* -EOPNOTSUPP means we can load the module without its tag. */
>> +               if (err != -EOPNOTSUPP)
>> +                       return err;
>>         }
>>
>>         return 0;
>> @@ -388,7 +390,11 @@ void codetag_unload_module(struct module *mod)
>>                         ++cttype->content_id;
>>                 }
>>                 up_write(&cttype->mod_lock);
>> -               if (found && cttype->desc.free_section_mem)
>> +               /*
>> +                * A module whose module_load() returned -EOPNOTSUPP is not
>> +                * in the idr but may still hold reserved section memory.
>> +                */
>> +               if (cttype->desc.free_section_mem)
>>                         cttype->desc.free_section_mem(mod, true);
>>         }
>>         mutex_unlock(&codetag_lock);
>> diff --git a/mm/alloc_tag.c b/mm/alloc_tag.c
>> index 5836803898ad..1ca0409b492b 100644
>> --- a/mm/alloc_tag.c
>> +++ b/mm/alloc_tag.c
>> @@ -988,6 +988,10 @@ static int load_module(struct module *mod, struct codetag *start, struct codetag
>>         struct alloc_tag *stop_tag;
>>         struct alloc_tag *tag;
>>
>> +       /* Profiling disabled: load the module without its tags. */
>> +       if (!mem_profiling_support)
>> +               return -EOPNOTSUPP;
>> +
>>         /* percpu counters for core allocations are already statically allocated */
>>         if (!mod)
>>                 return 0;
>> --
>> 2.25.1
>>

Re: [PATCH v10 5/6] alloc_tag: skip percpu counter allocation when profiling is disabled

[Suren Baghdasaryan]

On Thu, Sep 17, 2026 at 7:36 PM Hao Ge <hao.ge@linux.dev> wrote:
>
> Hi Suren
>
> On 2026/9/18 08:53, Suren Baghdasaryan wrote:
> > On Mon, Sep 14, 2026 at 11:59 PM Hao Ge <hao.ge@linux.dev> wrote:
> >>
> >> After shutdown_mem_profiling() clears mem_profiling_support,
> >> needs_section_mem() returns false, so later modules have their codetag
> >> section placed as regular data and never enter the alloc_tag maple tree.
> >> codetag_load_module() still called load_module(), which allocated a percpu
> >> counter for every tag; release_module_tags() could not find these modules
> >> on unload, so the counters leaked.
> >>
> >> Return -EOPNOTSUPP from load_module() when profiling is off:
> >> codetag_module_init() drops the module's cmod, no counters are allocated
> >> and the module loads without its tags. codetag_unload_module() now always
> >> calls free_section_mem(), since a module whose module_load() returned
> >> -EOPNOTSUPP is not in the idr but may still hold a reserved section.
> >>
> >> Reported-by: Sashiko <sashiko-bot@kernel.org>
> >> Fixes: 4835f747d3ed ("alloc_tag: support for page allocation tag compression")
> >> Cc: stable@vger.kernel.org
> >> Suggested-by: Suren Baghdasaryan <surenb@google.com>
> >> Acked-by: Suren Baghdasaryan <surenb@google.com>
> >
> > Yeah, I guess I didn't think about alternatives...
> >
> > When mem_profiling_support=false, why can't load_module() skip
> > allocating percpu counters, zero out the whole area between start_tag
> > and stop_tag and return 0 as success (mem_profiling_support is not
> > enabled, so we acted accordingly)?
> > Module will be added into maple tree and later codetag_unload_module()
> > will find the module, call release_module_tags() (which is a NOOP
> > because tag->counter==NULL) and finally call free_section_mem(). This
> > seems more natural than special-casing with EOPNOTSUPP. WDYT?
> >
>
> I see, but the module would then sit in the idr with NULL-counter
> tags that become visible to readers which today never expect that state.
>
> alloc_tag_top_users calls codetag_next_ct() to find the next codetag
> and alloc_tag_read() would do per_cpu_ptr(NULL) for each cpu - plain
> pointer arithmetic onto a wild address, an oops straight out of the
> OOM report.
>
> https://elixir.bootlin.com/linux/v7.3-rc3/source/mm/alloc_tag.c#L503
>
> So if the codetag section within this codetag_type ends up meaningless,
> is there a reason we still return success? We will still maintain cttype->count
> and allocate a slot for this module inside mod_idr.
>
> I'd rather keep the -EOPNOTSUPP version (your original suggestion),
> also because the comments in load_module() and codetag_module_init()
> already document this scenario.
>
> WDYT?

Ok, fair enough. I think your current fix is the simplest way to
resolve this issue, so let's keep it simple.

Acked-by: Suren Baghdasaryan <surenb@google.com>

>
> Thanks
> Best Regards
> Hao
>
> >> Signed-off-by: Hao Ge <hao.ge@linux.dev>
> >> ---
> >>  lib/codetag.c  | 10 ++++++++--
> >>  mm/alloc_tag.c |  4 ++++
> >>  2 files changed, 12 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/lib/codetag.c b/lib/codetag.c
> >> index a9cda4c962a3..a0b600720afc 100644
> >> --- a/lib/codetag.c
> >> +++ b/lib/codetag.c
> >> @@ -240,7 +240,9 @@ static int codetag_module_init(struct codetag_type *cttype, struct module *mod)
> >>
> >>         if (err < 0) {
> >>                 kfree(cmod);
> >> -               return err;
> >> +               /* -EOPNOTSUPP means we can load the module without its tag. */
> >> +               if (err != -EOPNOTSUPP)
> >> +                       return err;
> >>         }
> >>
> >>         return 0;
> >> @@ -388,7 +390,11 @@ void codetag_unload_module(struct module *mod)
> >>                         ++cttype->content_id;
> >>                 }
> >>                 up_write(&cttype->mod_lock);
> >> -               if (found && cttype->desc.free_section_mem)
> >> +               /*
> >> +                * A module whose module_load() returned -EOPNOTSUPP is not
> >> +                * in the idr but may still hold reserved section memory.
> >> +                */
> >> +               if (cttype->desc.free_section_mem)
> >>                         cttype->desc.free_section_mem(mod, true);
> >>         }
> >>         mutex_unlock(&codetag_lock);
> >> diff --git a/mm/alloc_tag.c b/mm/alloc_tag.c
> >> index 5836803898ad..1ca0409b492b 100644
> >> --- a/mm/alloc_tag.c
> >> +++ b/mm/alloc_tag.c
> >> @@ -988,6 +988,10 @@ static int load_module(struct module *mod, struct codetag *start, struct codetag
> >>         struct alloc_tag *stop_tag;
> >>         struct alloc_tag *tag;
> >>
> >> +       /* Profiling disabled: load the module without its tags. */
> >> +       if (!mem_profiling_support)
> >> +               return -EOPNOTSUPP;
> >> +
> >>         /* percpu counters for core allocations are already statically allocated */
> >>         if (!mod)
> >>                 return 0;
> >> --
> >> 2.25.1
> >>

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

[Hao Ge]

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);
+		return -ENOMEM;
+	}
+
 	return 0;
 }
 module_init(alloc_tag_init);
-- 
2.25.1

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

[Suren Baghdasaryan]

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.

> +               return -ENOMEM;
> +       }
> +
>         return 0;
>  }
>  module_init(alloc_tag_init);
> --
> 2.25.1
>

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

[Hao Ge]

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

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

[Suren Baghdasaryan]

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.

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

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

[Hao Ge]

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