Re: [PATCH v1 1/2] mm: Clear uffd-wp PTE/PMD state on mremap()

[Peter Xu]

On Thu, Jan 23, 2025 at 02:38:46PM +0000, Ryan Roberts wrote:
> > @@ -5470,7 +5471,18 @@ static void move_huge_pte(struct vm_area_struct *vma, unsigned long old_addr,
> >  		spin_lock_nested(src_ptl, SINGLE_DEPTH_NESTING);
> >  
> >  	pte = huge_ptep_get_and_clear(mm, old_addr, src_pte);
> > -	set_huge_pte_at(mm, new_addr, dst_pte, pte, sz);
> > +
> > +	if (need_clear_uffd_wp && pte_marker_uffd_wp(pte))
> > +		huge_pte_clear(mm, new_addr, dst_pte, sz);
> 
> This is checking if the source huge_pte is a uffd-wp marker and clearing the
> destination if so. The destination could have previously held arbitrary valid
> mappings, I guess?

I think it should be all cleared.  I didn't check all mremap paths, but for
MREMAP_FIXED at least there should be:

	if (flags & MREMAP_FIXED) {
		/*
		 * In mremap_to().
		 * VMA is moved to dst address, and munmap dst first.
		 * do_munmap will check if dst is sealed.
		 */
		ret = do_munmap(mm, new_addr, new_len, uf_unmap_early);
		if (ret)
			goto out;
	}

It also doesn't sound right to leave anything in dest range, e.g. if there
can be any leftover dest ptes in move_page_tables(), then it means
HPAGE_P[MU]D won't work, as they install huge entries directly.  For that I
do see a hint in the comment too in that path:

move_normal_pud():
	/*
	 * The destination pud shouldn't be established, free_pgtables()
	 * should have released it.
	 */
	if (WARN_ON_ONCE(!pud_none(*new_pud)))
		return false;

PMD path has similar implications.

Thanks,

-- 
Peter Xu