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 <[email protected]>
> >> wrote:
> >>>
> >>> On Tue, Sep 15, 2026 at 11:00 PM Hao Ge <[email protected]> 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 <[email protected]> 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 <[email protected]>
> >>>>>> Cc: [email protected]
> >>>>>> Signed-off-by: Hao Ge <[email protected]>
> >>>>>> ---
> >>>>>> 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>
--
Uladzislau Rezki
