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

[Hao Ge]

Hi Uladzislau


On 2026/9/30 18:12, Uladzislau Rezki 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")
>> 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?
>
The hunk was for the failure path in kmsan_vmap_pages_range_noflush().
Shadow and origin are mapped there as two separate calls, and when                                                                   
the origin one fails the shadow is already up. __vmap_pages_range_noflush()                                                          
cannot do that undo, it only sees its own range.

kmsan_vmap_pages_range_noflush() has a single caller, so if we were to move this
logic into vmalloc, it would require some modifications as follows:

@@ -712,9 +724,14 @@ int vmap_pages_range_noflush(unsigned long addr, unsigned long end,
        int ret = kmsan_vmap_pages_range_noflush(addr, end, prot, pages,
                                                page_shift, gfp_mask);
 
+       if (!ret)
+               ret = __vmap_pages_range_noflush(addr, end, prot, pages,
+                                                page_shift);
+       /* Undo whatever was mapped before the failure. */
        if (ret)
-               return ret;
-       return __vmap_pages_range_noflush(addr, end, prot, pages, page_shift);
+               kmsan_vunmap_range_noflush(addr, end);
+
+       return ret;
 }

>> 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:
>

I see. right, __vmap_pages_range_noflush() is just above the
actual page‑table mapping, so it's low‑level enough.
Thanks a lot for your prototype.

I'm tidying up the code and will post a REF patch afterwards.

Thanks
Best Regards
Hao

> <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