Re: [PATCH v11 09/10] mm/vmalloc: map contiguous pages in batches for vmap() if possible

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] mm: physical memory overrun in vmap_pages_range_batched() across PMDs
- [High] mm/vmalloc: missing VM_UNINITIALIZED in vmap() during huge PMD promotion
--

commit 1baf52bf8f22bab279e2ffbca6ca7296ae8e5c51
Author: Barry Song (Xiaomi) <baohua@kernel.org>

mm/vmalloc: map contiguous pages in batches for vmap() if possible

This commit optimizes vmap() by scanning the input page array with
num_pages_contiguous() to detect physically contiguous page ranges and
coalescing ranges with the same mapping order into batched calls to
vmap_pages_range_noflush_walk().

> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> index ee2c1ec4ff1f3..42b3addd20f98 100644
> --- a/mm/vmalloc.c
> +++ b/mm/vmalloc.c
> @@ -3613,6 +3613,85 @@ static inline unsigned int vm_shift(pgprot_t prot, unsigned long size)
>  	return arch_vmap_pte_supported_shift(size);
>  }
>  
> +static inline int get_vmap_mapping_order(struct page **pages,
> +		pgprot_t prot, unsigned int nr_pages)
> +{
> +	unsigned long pfn;
> +	unsigned int nr_contig;
> +	int order;
> +
> +	if (!IS_ENABLED(CONFIG_HAVE_ARCH_HUGE_VMAP))
> +		return 0;
> +
> +	/* Limit nr_pages by pfn alignment */
> +	pfn = page_to_pfn(*pages);
> +	if (pfn > 0)
> +		nr_pages = min_t(size_t, nr_pages, 1UL << __ffs(pfn));
> +
> +	nr_contig = num_pages_contiguous(pages, nr_pages);
> +	if (nr_contig < 2)
> +		return 0;
> +
> +	order = ilog2(nr_contig);
> +
> +	if (vm_shift(prot, PAGE_SIZE << order) == PAGE_SHIFT)
> +		return 0;
> +
> +	return order;
> +}
> +
> +static int vmap_pages_range_batched(unsigned long addr, unsigned long end,
> +		pgprot_t prot, struct page **pages)
> +{
> +	const unsigned int nr_pages = (end - addr) >> PAGE_SHIFT;
> +	unsigned int prev_shift = 0, batch_idx = 0;
> +	unsigned long batch_start = addr, batch_end = addr;
> +	int err;
> +
> +	err = kmsan_vmap_pages_range_noflush(addr, end, prot, pages,
> +					     PAGE_SHIFT, GFP_KERNEL);
> +
> +	if (err)
> +		goto out;
> +
> +	for (unsigned int i = 0; i < nr_pages; ) {
> +		unsigned int shift = PAGE_SHIFT +
> +			get_vmap_mapping_order(pages + i, prot, nr_pages - i);
> +
> +		if (!i)
> +			prev_shift = shift;
> +
> +		if (shift != prev_shift) {
> +			err = vmap_pages_range_noflush_walk(batch_start, batch_end,
> +					prot, pages + batch_idx, prev_shift);
> +			if (err)
> +				goto out;
> +			prev_shift = shift;
> +			batch_start = batch_end;
> +			batch_idx = i;
> +		}
> +
> +		/*
> +		 * Once we fail to batch pages, we expect to fail batching
> +		 * for all remaining pages, so just give up.
> +		 */
> +		if (shift == PAGE_SHIFT)
> +			break;
> +
> +		batch_end += 1UL << shift;
> +		i += 1U << (shift - PAGE_SHIFT);
> +	}
> +
> +	/* Remaining */
> +	if (batch_start < end)
> +		err = vmap_pages_range_noflush_walk(batch_start, end, prot,
> +				pages + batch_idx, prev_shift);

[Severity: Critical]
Can vmap_pages_range_batched() overrun a physical chunk and map unowned
physical memory when a coalesced batch of same-order chunks straddles a PMD
boundary?

In get_vmap_mapping_order(), nr_pages is bounded by physical PFN alignment
and physical contiguity via num_pages_contiguous(), returning the full
contiguity order (for example, order 8, shift 20 for 1MB chunks) without
bounding the order by the virtual address alignment of the current mapping
address.

vmap_pages_range_batched() then coalesces consecutive physically
discontiguous chunks that share the same shift into a single
[batch_start, batch_end) range passed to vmap_pages_range_noflush_walk().

In this commit, get_vm_area_caller() in vmap() only aligns area->addr to
PAGE_SIZE. Even with follow-up commit bba985c9d433 ("mm/vmalloc: align
vm_area so vmap() can batch mappings"), vmap_get_aligned_vm_area() only
aligns area->addr to 1UL << arch_vmap_pte_supported_shift(size)
(CONT_PTE_SIZE, 64KB on 4K-page arm64) whenever size < PMD_SIZE (or when a
PMD_SIZE-aligned KVA allocation falls back), and any preceding smaller-order
batch (such as an order-4 chunk before order-8 chunks) can also leave
batch_start aligned to CONT_PTE_SIZE rather than 1UL << prev_shift.

Looking at the call chain on arm64:

  vmap()
    vmap_pages_range_batched()
      vmap_pages_range_noflush_walk()
        vmap_pages_pmd_range()
          vmap_pages_pte_range()
            vmap_set_ptes()
              arch_vmap_pte_range_map_size()
              vmap_pte_set_huge()

arch/arm64/include/asm/vmalloc.h:arch_vmap_pte_range_map_size() checks only
CONT_PTE_SIZE alignment and then maps up to 1UL << max_page_shift:

    if (!IS_ALIGNED(addr, CONT_PTE_SIZE))
        return PAGE_SIZE;

    if (!IS_ALIGNED(PFN_PHYS(pfn), CONT_PTE_SIZE))
        return PAGE_SIZE;

    size = min(end - addr, 1UL << max_page_shift);
    size = round_down(size, CONT_PTE_SIZE);
    return size;

Consider two physically discontiguous order-8 (1MB, 256-page, prev_shift 20)
chunks at PFNs P0..P0+255 and P1..P1+255 coalesced into a 2MB batch starting
at a CONT_PTE_SIZE-aligned address that is 512KB (128 pages) below a PMD
boundary (for example, batch_start = 0xffff800080180000,
batch_end = 0xffff800080380000 across PMD boundary 0xffff800080200000):

1. In the first PMD [0xffff800080180000, 0xffff800080200000),
   vmap_pages_pmd_range() clips next to the PMD boundary (512KB).
   arch_vmap_pte_range_map_size() returns min(512KB, 1MB) = 512KB, mapping
   pages[0..127] (PFNs P0..P0+127) and advancing *nr to 128.

2. At the start of the second PMD [0xffff800080200000, 0xffff800080380000),
   *nr is 128 (pages[128], PFN P0+128), so only 128 contiguous pages (512KB,
   PFNs P0+128..P0+255) remain in the first chunk before the physical
   discontinuity at pages[256] (PFN P1).

3. Because both addr (0xffff800080200000) and PFN_PHYS(P0+128) are aligned to
   CONT_PTE_SIZE (64KB) and end - addr is 1.5MB >= 1MB,
   arch_vmap_pte_range_map_size() returns size = min(1.5MB, 1UL << 20) = 1MB
   (256 pages).

4. vmap_pte_set_huge() then maps 256 contiguous physical pages starting at
   PFN P0+128 (PFNs P0+128..P0+383), mapping 128 pages (512KB,
   PFNs P0+256..P0+383) of unowned physical memory past the end of the first
   chunk into the vmap() range, while advancing *nr by 256 to 384 and
   skipping pages[256..383] of the second chunk.

The same overrun also occurs for two coalesced order-9 (2MB, prev_shift 21)
chunks when batch_start is CONT_PTE_SIZE-aligned but not PMD_SIZE-aligned,
because vmap_try_huge_pmd() fails in both PMDs and falls back to
vmap_pages_pte_range() with shift = 21.

Should get_vmap_mapping_order() also bound the mapping order by the virtual
address alignment of the current page or by the supported architectural map
size?

[ ... ]

> @@ -3656,8 +3735,7 @@ void *vmap(struct page **pages, unsigned int count,
>  		return NULL;
>  
>  	addr = (unsigned long)area->addr;
> -	if (vmap_pages_range(addr, addr + size, pgprot_nx(prot),
> -				pages, PAGE_SHIFT) < 0) {
> +	if (vmap_pages_range_batched(addr, addr + size, pgprot_nx(prot), pages) < 0) {
>  		vunmap(area->addr);
>  		return NULL;
>  	}

[Severity: High]
Could switching vmap() to vmap_pages_range_batched() without setting
VM_UNINITIALIZED on area expose a race with vread_iter() when a PMD is
promoted to a huge page?

In vmap(), get_vm_area_caller() publishes area in vn->busy without
VM_UNINITIALIZED before vmap_pages_range_batched() populates the page tables.
Previously, vmap() always passed PAGE_SHIFT to vmap_pages_range(), which only
allocated missing page tables and wrote leaf PTEs without freeing existing
page table pages.

With vmap_pages_range_batched(), when get_vmap_mapping_order() returns an
order >= PMD_SHIFT - PAGE_SHIFT over a virtual address range that previously
held 4KB mappings (leaving an empty present PTE table at *pmd after
vunmap_range_noflush()), vmap_pages_pmd_range() calls vmap_try_huge_pmd():

mm/vmalloc.c:vmap_try_huge_pmd() {
    ...
    scoped_cond_guard(mmap_read_lock_try, return 0, &init_mm) {
        if (!pmd_free_pte_page(pmd, addr))
            return 0;
        return pmd_set_huge(pmd, phys_addr, prot);
    }
    ...
}

Concurrently, a reader of /proc/kcore executing vread_iter() checks:

mm/vmalloc.c:vread_iter() {
    ...
    if (vm && (vm->flags & VM_UNINITIALIZED))
        goto next_va;
    ...
}

Because VM_UNINITIALIZED is not set by vmap() (unlike
__vmalloc_node_range_noprof() and kho_restore_vmalloc()) and vread_iter()
does not take init_mm.mmap_lock, vread_iter() proceeds into
aligned_vread_iter() -> vmalloc_to_page() -> __vmalloc_to_page():

mm/vmalloc.c:__vmalloc_to_page() {
    ...
    pmd = pmd_offset(pud, addr);
    if (pmd_none(*pmd))
        return NULL;
    if (pmd_leaf(*pmd))
        return pmd_page(*pmd) + ((addr & ~PMD_MASK) >> PAGE_SHIFT);
    if (WARN_ON_ONCE(pmd_bad(*pmd)))
        return NULL;

    ptep = pte_offset_kernel(pmd, addr);
    pte = ptep_get(ptep);
    ...
}

If pmd_free_pte_page() and pmd_set_huge() run concurrently with
__vmalloc_to_page(), __vmalloc_to_page() can dereference a freed PTE page
table page, hit WARN_ON_ONCE(pmd_bad(*pmd)), or pass a newly installed huge
leaf PMD entry to pte_offset_kernel(pmd, addr).

Should vmap() set VM_UNINITIALIZED when allocating the vm_struct and clear it
via clear_vm_uninitialized_flag() after vmap_pages_range_batched() finishes?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103101658.766905-1-jiangwenxiaomi@gmail.com?part=9