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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] mm/vmalloc: infinite loop in vunmap_pte_range() on unaligned error rollback
- [High] mm/vmalloc: __vmap_pages_range_noflush() calls cond_resched() in atomic context
- [Medium] mm/vmalloc: vmap_page_range() calls flush_cache_vmap() before error rollback
--

commit 043026437ba63b051df5d14470fe69ce8fa8825c
Author: Hao Ge <hao.ge@linux.dev>

mm/vmalloc: undo partial mappings inside the mapping functions

This commit adds rollback logic using __vunmap_range_noflush() and
kmsan_vunmap_range_noflush() into vmap_page_range(),
__vmap_pages_range_noflush(), and vmap_pages_range_noflush() when page
table installation or KMSAN metadata setup fails. It also adds range and
page_shift validation in __vmap_pages_range_noflush().

> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> index 44e0eb32708f1..75aa5acf55660 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;
>  }

[Severity: High]
What happens here when vmap_range_noflush() returns -EINVAL for a range
where end - addr is not PAGE_SIZE-aligned?

In vmap_pte_range(), commit a739e6b557af09 added a check to bail out on
unaligned ranges specifically to avoid an infinite loop:

mm/vmalloc.c:vmap_pte_range() {
	...
	if (WARN_ON_ONCE(!PAGE_ALIGNED(end - addr)))
		return -EINVAL;
}

This can be reached on 64KB page kernels via pci_remap_iospace() ->
vmap_page_range() with 16KB/32KB/48KB PCI I/O ranges (for example via
loongson_pci_probe() or acpi_pci_root_remap_iospace()).

When vmap_pte_range() returns -EINVAL, vmap_page_range() now calls
__vunmap_range_noflush(addr, end) with the unaligned range. Whenever a PMD
in [addr, end) is already populated with a PTE table (such as the
PCI_IOBASE PMD populated during early boot by reserve_pio_range(), or an
earlier PMD mapped in the same call), __vunmap_range_noflush() descends
into vunmap_pte_range(), which has no alignment check:

mm/vmalloc.c:vunmap_pte_range() {
	...
	do {
		...
			ptent = ptep_get_and_clear(&init_mm, addr, pte);
		WARN_ON(!pte_none(ptent) && !pte_present(ptent));
	} while (pte += (size >> PAGE_SHIFT), addr += size, addr != end);
}

Because end - addr is not a multiple of PAGE_SIZE, won't addr step past end
and loop infinitely, clearing unrelated kernel PTEs and corrupting memory
past the PTE table?

[Severity: Medium]
Also in vmap_page_range(), flush_cache_vmap(addr, end) is called
unconditionally before checking err and before rolling back with
__vunmap_range_noflush(addr, end):

mm/vmalloc.c:vmap_page_range() {
	err = vmap_range_noflush(addr, end, phys_addr, pgprot_nx(prot),
				 ioremap_max_page_shift);
	flush_cache_vmap(addr, 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;
}

On UML, flush_cache_vmap is defined as flush_tlb_kernel_range:

arch/um/include/asm/cacheflush.h:
#define flush_cache_vmap flush_tlb_kernel_range

When vmap_range_noflush() fails partway through or
kmsan_ioremap_page_range() fails, flush_cache_vmap() runs while the
partially installed PTEs are still pte_present(), causing
update_pte_range() in arch/um/kernel/tlb.c to map them into the host OS via
ops->mmap() and clear _PAGE_NEEDSYNC:

arch/um/kernel/tlb.c:update_pte_range() {
	...
		if (pte_present(*pte)) {
			...
			ret = ops->mmap(ops->mm_idp, addr, PAGE_SIZE,
					prot, fd, offset);
		} else
			ret = ops->unmap(ops->mm_idp, addr, PAGE_SIZE);

		*pte = pte_mkuptodate(*pte);
}

When __vunmap_range_noflush(addr, end) then clears the software PTEs
without a subsequent TLB flush or unmap notification, won't those host OS
mmap mappings be left behind? Should flush_cache_vmap(addr, end) only be
called after both vmap_range_noflush() and kmsan_ioremap_page_range()
succeed?

> @@ -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;
>  
>  	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;
>  }

[Severity: High]
Similarly here, __vmap_pages_range_noflush() checks addr >= end, but does
not check PAGE_ALIGNED(addr) or PAGE_ALIGNED(end).

If a caller passes a sub-page or unaligned range (for example, 0 < end -
addr < PAGE_SIZE), nr = (end - addr) >> PAGE_SHIFT evaluates to 0, so the
huge-page path still maps nothing and returns 0, while the small-page path
loops infinitely in vmap_pages_pte_range(). And if the huge-page path fails
on an unaligned range, __vunmap_range_noflush(start, end) will hit the same
infinite loop in vunmap_pte_range().

Should both vmap_page_range() and __vmap_pages_range_noflush() validate
PAGE_ALIGNED(addr) && PAGE_ALIGNED(end) upfront before attempting to map or
unmap?

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

[Severity: High]
Can calling __vunmap_range_noflush() and kmsan_vunmap_range_noflush() here
trigger a sleeping-in-atomic-context bug when __vmalloc() or kvmalloc() is
called with GFP_ATOMIC or GFP_NOWAIT?

__vmalloc_area_node() supports non-blocking allocations by wrapping
__vmap_pages_range() in memalloc_apply_gfp_scope(gfp_mask):

mm/vmalloc.c:__vmalloc_area_node() {
	...
	flags = memalloc_apply_gfp_scope(gfp_mask);
	do {
		ret = __vmap_pages_range(addr, addr + size, prot, area->pages,
				page_shift, nested_gfp);
	} while ((gfp_mask & __GFP_NOFAIL) && (ret < 0));
	memalloc_restore_gfp_scope(flags);
}

When a GFP_ATOMIC or GFP_NOWAIT allocation runs in atomic context (such as
under a spinlock, preempt_disable(), or rcu_read_lock()) and page table
allocation fails in vmap_small_pages_range_noflush() after at least one PMD
in [start, end) is populated (or after kmsan_vmap_pages_range_noflush()
populated the KMSAN shadow/origin ranges), __vmap_pages_range_noflush() and
kmsan_vunmap_range_noflush() synchronously call __vunmap_range_noflush().

__vunmap_range_noflush() walks the populated PMDs via vunmap_pmd_range(),
which unconditionally calls cond_resched():

mm/vmalloc.c:vunmap_pmd_range() {
	...
	do {
		next = pmd_addr_end(addr, end);
		if (pmd_none_or_clear_bad(pmd))
			continue;
		vunmap_pte_range(pmd, addr, next, mask);

		cond_resched();
	} while (pmd++, addr = next, addr != end);
}

Commit 9c47753167a6 ("mm/vmalloc: defer freeing partly initialized
vm_struct") deferred error-path cleanup in __vmalloc_area_node() to a
workqueue specifically because unmapping on failure can sleep. Doesn't
calling __vunmap_range_noflush() synchronously inside
__vmap_pages_range_noflush() and vmap_pages_range_noflush() reintroduce
cond_resched() in atomic context?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019054531.87652-1-hao.ge@linux.dev?part=3