Re: [PATCH v5 07/38] mm: add mmap action for discontiguous kernel page mapping

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] mm: missing error return after VM_WARN_ON_ONCE for VMA_PFNMAP
--

commit dfcef6542267c02a389d52c15954f3b2b60a0695
Author: Lorenzo Stoakes (ARM) <ljs@kernel.org>
Subject: mm: add mmap action for discontiguous kernel page mapping

This commit introduces discontig_kernel_page_ops to allow drivers to map
discontiguous arrays of page pointers into VMAs. It also updates
vm_insert_page() and vm_insert_pages() to assert that the mmap write lock
is held, converting BUG_ON checks to VM_WARN_ON_ONCE while unifying the
code paths.

> diff --git a/mm/memory.c b/mm/memory.c
> index 448342883e9da..e5c1d1c35bc70 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -2599,27 +2599,34 @@ int vm_insert_pages(struct vm_area_struct *vma, unsigned long addr,
>  
>  	if (!range_in_vma(vma, addr, end))
>  		return -EFAULT;
> -	if (!(vma->vm_flags & VM_MIXEDMAP)) {
> -		VM_WARN_ON_ONCE(mmap_read_trylock(vma->vm_mm));
> -		VM_WARN_ON_ONCE(vma->vm_flags & VM_PFNMAP);
> -		vm_flags_set(vma, VM_MIXEDMAP);
> +	if (!vma_test(vma, VMA_MIXEDMAP_BIT)) {
> +		mmap_assert_write_locked(vma->vm_mm);
> +		VM_WARN_ON_ONCE(vma_test(vma, VMA_PFNMAP_BIT));

[Severity: High]
If this warning triggers, execution will fall through because there is no
error return. On production kernels where VM_WARN_ON_ONCE is a no-op, this
will silently set VMA_MIXEDMAP_BIT and insert page entries into a
PFN-mapped VMA. 

During VMA teardown, zap_pte_range() will not decrement the reference
counts for these pages since vm_normal_page() returns NULL for VMAs with
VM_PFNMAP set. 

Could this result in a permanent memory leak? Should this return an error
such as -EINVAL if the bit is set?

> +		vma_start_write(vma);
> +		vma_set_flags(vma, VMA_MIXEDMAP_BIT);
>  	}
>  	/* Defer page refcount checking till we're about to map that page. */
>  	return insert_pages(vma, addr, pages, num, vma->vm_page_prot);
>  }

[ ... ]

> +static void __map_kernel_pages_prepare(struct vm_area_desc *desc)
> +{
> +	if (vma_desc_test(desc, VMA_MIXEDMAP_BIT))
> +		return;
> +
> +	mmap_assert_write_locked(desc->mm);
> +	VM_WARN_ON_ONCE(vma_desc_test(desc, VMA_PFNMAP_BIT));

[Severity: High]
Similar to the check in vm_insert_pages(), this warns but does not abort
the operation if the VMA has the PFNMAP bit set. Should this function be
changed to return an int so it can propagate an error and prevent inserting
refcounted pages into a PFN-mapped VMA?

> +	vma_desc_set_flags(desc, VMA_MIXEDMAP_BIT);
> +}

[ ... ]

> @@ -2675,10 +2774,11 @@ int vm_insert_page(struct vm_area_struct *vma, unsigned long addr,
>  {
>  	if (addr < vma->vm_start || addr >= vma->vm_end)
>  		return -EFAULT;
> -	if (!(vma->vm_flags & VM_MIXEDMAP)) {
> -		BUG_ON(mmap_read_trylock(vma->vm_mm));
> -		BUG_ON(vma->vm_flags & VM_PFNMAP);
> -		vm_flags_set(vma, VM_MIXEDMAP);
> +	if (!vma_test(vma, VMA_MIXEDMAP_BIT)) {
> +		mmap_assert_write_locked(vma->vm_mm);
> +		VM_WARN_ON_ONCE(vma_test(vma, VMA_PFNMAP_BIT));

[Severity: High]
This replaces a strict BUG_ON() with a warning but allows execution to
continue. Does this need an early return here as well to prevent a
potential page reference leak?

> +		vma_start_write(vma);
> +		vma_set_flags(vma, VMA_MIXEDMAP_BIT);
>  	}
>  	return insert_page(vma, addr, page, vma->vm_page_prot, false);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009-b4-mmap-prepare-vma-flag-sanify-v5-0-923b70125b1e@kernel.org?part=7