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

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

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