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

[Ryan Roberts <ryan.roberts@arm.com>]

On 23/01/2025 17:40, Peter Xu wrote:
> 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, 

Yes, of course. And the loop over the old ptes actually skips doing anything if
the old pte is none without doing any operations on the new pte; so it must be
none by default. OK panic over! I just need to fix the arm64 issue I raised in
the other email. I'm going to send a bunch of fixes/improvements in that area.

Thanks,
Ryan


> 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,
>