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: [email protected]
> Assisted-by: LLM
> Signed-off-by: Konstantin Khorenko <[email protected]>
> ---
> 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
