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@xxxxxxxxx> 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@xxxxxxxxx> 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@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.
> > > > > >
> > > > >
> > > > > 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