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@xxxxxxxxxx> wrote:
>>
>> On Tue, Sep 15, 2026 at 11:00 PM Hao Ge <hao.ge@xxxxxxxxx> 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@xxxxxxxxx> 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@xxxxxxxxxx>
>>>>> Cc: stable@xxxxxxxxxxxxxxx
>>>>> Signed-off-by: Hao Ge <hao.ge@xxxxxxxxx>
>>>>> ---
>>>>>  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
>>>>>