On Wed, Sep 30, 2026 at 12:12 PM Uladzislau Rezki <[email protected]> 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 <[email protected]> > > Cc: [email protected] > > Signed-off-by: Hao Ge <[email protected]> > > --- > > 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
