Re: [PATCH v3 1/2] mm/huge_memory: recheck a special PMD under the lock in copy_huge_pmd()

["David Hildenbrand (Arm)" <david@kernel.org>]

On 10/14/26 17:46, Konstantin Khorenko wrote:
> copy_huge_pmd() reads a special PMD without the page table lock and
> copies that value to the child. The mmap write lock held by fork does
> not stop unmap_mapping_range(): it zaps under i_mmap_rwsem and the PMD
> lock.

For a special PMD (that is not the huge zero folio), we need to go through
vmf_insert_pfn_pmd()->insert_pmd().

I would have thought that unmap_mapping_pages() is only called in corner cases
when we would have concurrent fork().

Like vfio_pci_zap_bars().

But yeah, likely there is a theoretical race.

> 
> The zap visits the parent VMA before the child's, as dup_mmap() links
> the child right after the parent in i_mmap. If it clears the parent PMD
> after fork has read it, and passes the child before fork copies it, the
> child keeps a mapping the driver has revoked:
> 
> - vfio-pci: the child reaches the BAR after the device memory was
>   disabled or the device was reset, instead of getting SIGBUS;
> - panthor: the child maps the pages of an evicted buffer object after
>   they are freed.
> 
> Read the PMD again under the source PMD lock, which the zap takes to
> clear it. Either the zap got there first, and the PMD is none, or the
> copy is in place before the zap reaches the child. Faults are locked
> out, so only a zap can change the PMD, and pmd_none() is the test.
> 
> Fixes: bc02afbd4d73 ("mm/fork: accept huge pfnmap entries")
> Cc: stable@vger.kernel.org
> Assisted-by: LLM
> Signed-off-by: Konstantin Khorenko <khorenko@virtuozzo.com>
> ---
>  mm/huge_memory.c | 23 ++++++++++++++++-------
>  1 file changed, 16 insertions(+), 7 deletions(-)
> 
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index 1e5d68acf62a5..bf788bb231691 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -1965,13 +1965,22 @@ int copy_huge_pmd(struct mm_struct *dst_mm, struct mm_struct *src_mm,
>  		src_ptl = pmd_lockptr(src_mm, src_pmd);
>  		spin_lock_nested(src_ptl, SINGLE_DEPTH_NESTING);
>  		/*
> -		 * No need to recheck the pmd, it can't change with write
> -		 * mmap lock held here.
> -		 *
> -		 * Meanwhile, making sure it's not a CoW VMA with writable
> -		 * mapping, otherwise it means either the anon page wrongly
> -		 * applied special bit, or we made the PRIVATE mapping be
> -		 * able to wrongly write to the backend MMIO.
> +		 * The mmap and VMA write locks of the source keep faults
> +		 * out, but unmap_mapping_range() does not take the mmap
> +		 * lock and may have cleared the pmd since the lockless
> +		 * read. Let copy_pmd_range() skip it then.
> +		 */

TBH, I think we can drop this comment entirely. That's just simply the right
thing to do? :)

> +		pmd = pmdp_get(src_pmd);
> +		if (unlikely(pmd_none(pmd))) {
> +			spin_unlock(src_ptl);
> +			spin_unlock(dst_ptl);
> +			return -EAGAIN;
> +		}
> +		/*
> +		 * Make sure it's not a CoW VMA with writable mapping,
> +		 * otherwise it means either the anon page wrongly applied
> +		 * special bit, or we made the PRIVATE mapping be able to
> +		 * wrongly write to the backend MMIO.
>  		 */
>  		VM_WARN_ON_ONCE(vma_is_cow_mapping(src_vma) && pmd_write(pmd));
>  		goto set_pmd;
> 

But why do we do this early copy dance *at all*? I think I complained about this
vma_is_anonymous handling in the past. vma_needs_copy() is the one deciding
whether something should get copied or not.

So, can't we just get rid of the vma_is_anonymous() handling and perform the
weird special-pte check (that I hate entirely) when already holding the PTL?


-- 
Cheers,

David