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

[Hao Ge <hao.ge@linux.dev>]

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