Re: [PATCH v2 3/5] mm/vmalloc: undo partial mappings inside the mapping functions

[Hao Ge <hao.ge@linux.dev>]

Hi Dev


On 10/20/26 13:46, Dev Jain wrote:
>
> On 19/10/26 11:15 am, 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. pcpu_map_pages() and kmsan_ioremap_page_range()
>> roll back what they mapped before the failure, while
>> vm_module_tags_populate() and the __GFP_NOFAIL retry loop in
>> __vmalloc_area_node() rely on the mapping functions cleaning up and
>> do not call anything like vunmap_range() themselves. When the same
>> range is mapped again, the attempt hits the leftovers and fails, with
>> a BUG() in vmap_pte_range() for huge mappings and a warning on the
>> small-page path.
>>
>> After discussing with Suren and Ulad, we decided to put the rollback
>> into the entry points of the vmap API rather than into every low-level
>> helper. __vmap_pages_range_noflush() undoes the whole range it was
>> asked to map when it fails, and vmap_page_range() undoes its range
>> for the ioremap-style mappings.
>>
>> The rollback is __vunmap_range_noflush(), it only clears the PTEs,
>> no TLB flush, nothing has touched these mappings.
>> vmap_pages_range_noflush() drops the KMSAN metadata when the data
>> mapping fails, and an empty range bails out with -EINVAL now, which
>> used to BUG_ON() on the small-page path and map nothing at all on
>> the huge-page one.
>>
>> Reported-by: Sashiko <sashiko-bot@kernel.org>
>> Suggested-by: Uladyslau Rezki <urezki@gmail.com>
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Hao Ge <hao.ge@linux.dev>
>> ---
>>   mm/vmalloc.c | 44 ++++++++++++++++++++++++++++++++------------
>>   1 file changed, 32 insertions(+), 12 deletions(-)
>>
>> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
>> index 44e0eb32708f..75aa5acf5566 100644
>> --- a/mm/vmalloc.c
>> +++ b/mm/vmalloc.c
>> @@ -363,6 +363,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;
>>   }
>>   
>> @@ -683,27 +686,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 = 0;
>> +
>> +	if (WARN_ON_ONCE(addr >= end))
>> +		return -EINVAL;
> IIUC this is a general change independent of this patch? Caller could pass
> addr >= end and if we take the huge mapping path, we lose the sanity check.
>
> So this could become a separate patch.


OK,  and Sashiko has flagged another case:

when 0 < end - addr < PAGE_SIZE, nr = (end - addr) >> PAGE_SHIFT 
evaluates to 0,

so the huge-page path still maps nothing and returns 0.

So this separate patch would cover this case as well.


Thanks

Best Regards

Hao


>
>
>>   
>>   	if (WARN_ON_ONCE(page_shift < PAGE_SHIFT))
>>   		return -EINVAL;
>>   
>>   	if (!IS_ENABLED(CONFIG_HAVE_ARCH_HUGE_VMALLOC) ||
>> -			page_shift == PAGE_SHIFT)
>> -		return vmap_small_pages_range_noflush(addr, end, prot, pages);
>> -
>> -	for (i = 0; i < nr; i += 1U << (page_shift - PAGE_SHIFT)) {
>> -		int err;
>> -
>> -		err = vmap_range_noflush(addr, addr + (1UL << page_shift),
>> +			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;
>> +	/* Undo the PTEs installed before the failure. */
>> +	if (err)
>> +		__vunmap_range_noflush(start, end);
>> +
>> +	return err;
>>   }
>>   
>>   int vmap_pages_range_noflush(unsigned long addr, unsigned long end,
>> @@ -715,7 +726,16 @@ 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);
>> +	/*
>> +	 * The page tables undo themselves on failure. Tear down the
>> +	 * metadata that was fully set up before the mapping failed.
>> +	 */
>> +	if (ret)
>> +		kmsan_vunmap_range_noflush(addr, end);
>> +
>> +	return ret;
>>   }
>>   
>>   static int __vmap_pages_range(unsigned long addr, unsigned long end,