Re: [PATCH v11 2/7] mm/vmalloc: undo partial mappings inside the mapping functions

[Suren Baghdasaryan]

On Wed, Sep 30, 2026 at 12:12 PM Uladzislau Rezki <urezki@xxxxxxxxx> wrote:
>
> On Tue, Sep 29, 2026 at 04:20:09PM +0800, Hao Ge wrote:
> > __vmap_pages_range_noflush() and friends can install some PTEs before
> > failing and leave them mapped, and the kernel callers do not agree on
> > who cleans them up. For example, pcpu_map_pages() and
> > kmsan_ioremap_page_range() unmap the leftovers themselves, while
> > vm_module_tags_populate() and the __GFP_NOFAIL retry loop in
> > __vmalloc_area_node() relied on the mapping functions cleaning up and
> > did not call anything like vunmap_range() themselves. When the same
> > range is mapped again, the attempt hits the leftovers and fails, with
> > BUG() in vmap_pte_range() for huge mappings.
> >
> > After discussing with Suren and Ulad, we decided the cleanup belongs
> > to __vmap_pages_range_noflush() and friends, so the callers no longer
> > need to unmap the partial mappings themselves. Each function now
> > undoes the PTEs it installed itself.
> >
> > The rollback calls the low-level __vunmap_range_noflush(), it just
> > clears the PTEs of the range it is given, which is all a rollback
> > needs. It cannot use vunmap_range_noflush() because these mapping
> > functions also map the KMSAN shadow and origin, and for a metadata
> > range its hook would look up the metadata of the metadata, get 0
> > and BUG() on addr >= end. The failed mappings were never accessed,
> > no TLB flush needed.
> >
> > Fixes: 9376130c390a ("mm/vmalloc: add support for __GFP_NOFAIL")
> > Fixes: 0f9b685626da ("alloc_tag: populate memory for module tags as needed")

I don't think this fixes 0f9b685626da. I mean it fixes it but
indirectly. That patch expects vmap_pages_range() to undo the mappings
when it fails, which is a reasonable assumption. By fixing
vmap_pages_xxx we fix all its users, including
vm_module_tags_populate(). So, it indirectly fixes several callers but
when adding a Fixes tag we do not list all patches we indirectly fix.

> > Reported-by: Sashiko <sashiko-bot@xxxxxxxxxx>
> > Cc: stable@xxxxxxxxxxxxxxx
> > Signed-off-by: Hao Ge <hao.ge@xxxxxxxxx>
> > ---
> >  mm/kmsan/shadow.c |  4 ++++
> >  mm/vmalloc.c      | 31 +++++++++++++++++++++++++++++--
> >  2 files changed, 33 insertions(+), 2 deletions(-)
> >
> > diff --git a/mm/kmsan/shadow.c b/mm/kmsan/shadow.c
> > index 0c88d89bf0d6..2166086d3dc3 100644
> > --- a/mm/kmsan/shadow.c
> > +++ b/mm/kmsan/shadow.c
> > @@ -258,6 +258,10 @@ int kmsan_vmap_pages_range_noflush(unsigned long start, unsigned long end,
> >                                           o_pages, page_shift);
> >       kmsan_leave_runtime();
> >       if (mapped) {
> > +             /* Undo the shadow mapping set up above. */
> > +             kmsan_enter_runtime();
> > +             __vunmap_range_noflush(shadow_start, shadow_end);
> > +             kmsan_leave_runtime();
> >               err = mapped;
> >               goto ret;
> >       }
> >
> Can we just do that inside the vmalloc? For example in the
> __vmap_pages_range_noflush() as entry function for all(?) helpers?
> So we do not need do it manually?

Agree, that would be better.

>
> > diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> > index 859e6d2d57a3..9bbf75706627 100644
> > --- a/mm/vmalloc.c
> > +++ b/mm/vmalloc.c
> > @@ -349,6 +349,10 @@ static int vmap_range_noflush(unsigned long addr, unsigned long end,
> >       if (mask & ARCH_PAGE_TABLE_SYNC_MASK)
> >               arch_sync_kernel_mappings(start, end);
> >
> > +     /* Undo the PTEs installed before the failure. */
> > +     if (err)
> > +             __vunmap_range_noflush(start, end);
> > +
> >       return err;
> >  }
> >
> > @@ -363,6 +367,9 @@ int vmap_page_range(unsigned long addr, unsigned long end,
> >       if (!err)
> >               err = kmsan_ioremap_page_range(addr, end, phys_addr, prot,
> >                                              ioremap_max_page_shift);
> > +     if (err)
> > +             __vunmap_range_noflush(addr, end);
> > +
> >       return err;
> >  }
> >
> > @@ -667,6 +674,10 @@ static int vmap_small_pages_range_noflush(unsigned long addr, unsigned long end,
> >       if (mask & ARCH_PAGE_TABLE_SYNC_MASK)
> >               arch_sync_kernel_mappings(start, end);
> >
> > +     /* Undo the PTEs installed before the failure. */
> > +     if (err)
> > +             __vunmap_range_noflush(start, end);
> > +
> >       return err;
> >  }
> >
> The only one user of that function is __vmap_pages_range_noflush()
> so we can cover both cases there small and huge path.
>
> Is that enough just to do unroll in the below:
>
> __vmap_pages_range_noflush()
> vmap_page_range()
>
> functions? It will also fix NOFAIL case:
>
> <snip>
> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> index 89c327a6ce7d..fa1f357ecf3f 100644
> --- a/mm/vmalloc.c
> +++ b/mm/vmalloc.c
> @@ -359,10 +359,19 @@ int vmap_page_range(unsigned long addr, unsigned long end,
>
>         err = vmap_range_noflush(addr, end, phys_addr, pgprot_nx(prot),
>                                  ioremap_max_page_shift);
> +       if (err)
> +               goto error_cleanup_range;
> +
>         flush_cache_vmap(addr, end);
> -       if (!err)
> -               err = kmsan_ioremap_page_range(addr, end, phys_addr, prot,
> +       err = kmsan_ioremap_page_range(addr, end, phys_addr, prot,
>                                                ioremap_max_page_shift);
> +       if (err)
> +               goto error_cleanup_range;
> +
> +       return 0;
> +
> +error_cleanup_range:
> +       __vunmap_range_noflush(addr, end);
>         return err;
>  }
>
> @@ -683,26 +692,35 @@ int __vmap_pages_range_noflush(unsigned long addr, unsigned long end,
>                 pgprot_t prot, struct page **pages, unsigned int page_shift)
>  {
>         unsigned int i, nr = (end - addr) >> PAGE_SHIFT;
> +       unsigned long start = addr;
> +       int err;
>
> -       WARN_ON(page_shift < PAGE_SHIFT);
> -
> -       if (!IS_ENABLED(CONFIG_HAVE_ARCH_HUGE_VMALLOC) ||
> -                       page_shift == PAGE_SHIFT)
> -               return vmap_small_pages_range_noflush(addr, end, prot, pages);
> +       if (WARN_ON(addr >= end))
> +               return -EINVAL;
>
> -       for (i = 0; i < nr; i += 1U << (page_shift - PAGE_SHIFT)) {
> -               int err;
> +       if (WARN_ON(page_shift < PAGE_SHIFT))
> +               return -EINVAL;
>
> -               err = vmap_range_noflush(addr, addr + (1UL << page_shift),
> +       if (!IS_ENABLED(CONFIG_HAVE_ARCH_HUGE_VMALLOC) ||
> +               page_shift == PAGE_SHIFT) {
> +               err = vmap_small_pages_range_noflush(addr, end, prot, pages);
> +       } else {
> +               for (i = 0; i < nr; i += 1U << (page_shift - PAGE_SHIFT)) {
> +                       err = vmap_range_noflush(addr, addr + (1UL << page_shift),
>                                         page_to_phys(pages[i]), prot,
>                                         page_shift);
> -               if (err)
> -                       return err;
> +                       if (err)
> +                               break;
>
> -               addr += 1UL << page_shift;
> +                       addr += 1UL << page_shift;
> +               }
>         }
>
> -       return 0;
> +       if (err)
> +               __vunmap_range_noflush(start, end);
> +
> +       /* 0 on success. */
> +       return err;
>  }
>
>  int vmap_pages_range_noflush(unsigned long addr, unsigned long end,
> @@ -714,7 +732,13 @@ int vmap_pages_range_noflush(unsigned long addr, unsigned long end,
>
>         if (ret)
>                 return ret;
> -       return __vmap_pages_range_noflush(addr, end, prot, pages, page_shift);
> +
> +       ret = __vmap_pages_range_noflush(addr, end, prot, pages, page_shift);
> +       if (ret)
> +               /* Cleanup KMSAN metadata. */
> +               kmsan_vunmap_range_noflush(addr, end);
> +
> +       return ret;
>  }
>
>  static int __vmap_pages_range(unsigned long addr, unsigned long end,
> <snip>
>
> --
> Uladzislau Rezki