On 2026/10/8 14:53, Hao Ge wrote:
> 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 <[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?
>>
> 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;
> }
>
I'd rather put this logic inside kmsan_vmap_pages_range_noflush.
This function should clean things up on its own, not depend on
the caller to do it, just like kmsan_ioremap_page_range.
https://elixir.bootlin.com/linux/v7.3-rc5/source/mm/kmsan/hooks.c#L185
Uladzislau, WDYT?
Thanks
Best Regards
Hao
>>> 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
