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

Hi Suren、David、Lorenzo

On 2026/9/17 09:12, Hao Ge wrote:

Hi Suren

On 2026/9/17 03:07, Suren Baghdasaryan wrote:

quoted

On Wed, Sep 16, 2026 at 9:31 AM Suren Baghdasaryan [off-list ref] wrote:

quoted

On Tue, Sep 15, 2026 at 11:00 PM Hao Ge [off-list ref] wrote:

quoted

Hi Suren

Thanks for you review.

On 2026/9/16 05:09, Suren Baghdasaryan wrote:

quoted

On Mon, Sep 14, 2026 at 11:59 PM Hao Ge [off-list ref] wrote:

quoted

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.

quoted

quoted

quoted

quoted

quoted

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.

quoted

quoted

guys think? Shouldn't vmap_pages_range() undo its possible partial
mapping when it fails?

quoted

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

quoted

quoted

quoted

Thanks
Best Regards
Hao

quoted

quoted

+                        */
+                       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.

quoted

                        return ERR_PTR(grow_res);
                }
        }
--
2.25.1