Re: [PATCH 02/12] userfaultfd: refuse UFFDIO_MOVE into a non-present huge destination PMD

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [Medium] userfaultfd: spurious -EFAULT for WP markers in
  move_pages_ptes() [1]
--

commit cae4a6b3bdb227f2a82bf72d15644fbdf0a7b4b5
Author: Tal Zussman <tz2294@columbia.edu>

userfaultfd: refuse UFFDIO_MOVE into a non-present huge destination PMD

This commit changes move_pages() to check the destination PMD with
pmd_is_huge() instead of pmd_trans_huge() so that UFFDIO_MOVE returns -EEXIST
for non-present huge PMD leaves such as device-private THPs or THPs under
migration.

> diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
> index 61a712e1f9f53..ad9d243dc64b0 100644
> --- a/mm/userfaultfd.c
> +++ b/mm/userfaultfd.c
> @@ -2021,12 +2021,13 @@ static ssize_t move_pages(struct userfaultfd_ctx *ctx, unsigned long dst_start,
>  
>  		dst_pmdval = pmdp_get_lockless(dst_pmd);
>  		/*
> -		 * If the dst_pmd is mapped as THP don't override it and just
> -		 * be strict. If dst_pmd changes into TPH after this check, the
> -		 * move_pages_huge_pmd() will detect the change and retry
> -		 * while move_pages_pte() will detect the change and fail.
> +		 * If the dst_pmd is a huge leaf, present or not, don't
> +		 * override it and just be strict. If dst_pmd changes into a
> +		 * leaf after this check, move_pages_huge_pmd() will detect the
> +		 * change and retry while move_pages_ptes() will fail to map
> +		 * the PTE table and the next iteration will see the leaf.
>  		 */
> -		if (unlikely(pmd_trans_huge(dst_pmdval))) {
> +		if (unlikely(pmd_is_huge(dst_pmdval))) {
>  			err = -EEXIST;
>  			break;
>  		}

[Severity: Medium]
This problem wasn't introduced by this patch, but does move_pages_ptes()
return a spurious -EFAULT [1] when UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES
encounters an unpopulated PTE carrying a UFFD write-protect marker?

mm/userfaultfd.c:move_pages_ptes() {
	...
	if (pte_none(orig_src_pte)) {
		if (!(mode & UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES))
			ret = -ENOENT;
		else /* nothing to do to move a hole */
			ret = PAGE_SIZE;
		goto out;
	}
	...
	if (pte_present(orig_src_pte)) {
		...
	} else { /* !pte_present() */
		struct folio *folio = NULL;
		const softleaf_t entry = softleaf_from_pte(orig_src_pte);

		if (softleaf_is_migration(entry)) {
			...
		} else if (!softleaf_is_swap(entry)) {
			ret = -EFAULT;
			goto out;
		}
	...
}

Because a UFFD WP marker is a non-zero PTE entry, pte_none(orig_src_pte) is
false and execution falls through to the non-present branch.  PTE markers are
not swap entries, so softleaf_is_swap(entry) is also false and
move_pages_ptes() aborts the move with -EFAULT instead of handling the
write-protected hole.

[1] https://sashiko.dev/#/bug/linux-5f33b673-0c7c-4b59-b85e-71d45e428896

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111-uffd-state-fixes-v1-0-00713d368976@columbia.edu?part=2