[PATCH v1] mm/userfaultfd: UFFDIO_MOVE implementation should use ptep_get()

[PATCH v1] mm/userfaultfd: UFFDIO_MOVE implementation should use ptep_get()

[Ryan Roberts]

Commit c33c794828f2 ("mm: ptep_get() conversion") converted all
(non-arch) call sites to use ptep_get() instead of doing a direct
dereference of the pte. Full rationale can be found in that commit's
log.

Since then, UFFDIO_MOVE has been implemented which does 7 direct pte
dereferences. Let's fix those up to use ptep_get().

Fixes: adef440691ba ("userfaultfd: UFFDIO_MOVE uABI")
Signed-off-by: Ryan Roberts <ryan.roberts@arm.com>
---
Hi All,

This applies on top of v6.8-rc1. I'm hoping this can be merged into the
next rc.

I've asserted in the past that there is no reliable automated mechanism
to catch these; I'm relying on a combination of Coccinelle (which throws
up a lot of false positives) and some compiler magic to force a compiler
error on dereference. But given the frequency with which new issues are
coming up, I'll add it to my todo list to try to find an automated
solution.

Thanks,
Ryan

 mm/userfaultfd.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
index 20e3b0d9cf7e..aaa7b9821342 100644
--- a/mm/userfaultfd.c
+++ b/mm/userfaultfd.c
@@ -891,8 +891,8 @@ static int move_present_pte(struct mm_struct *mm,

 	double_pt_lock(dst_ptl, src_ptl);

-	if (!pte_same(*src_pte, orig_src_pte) ||
-	    !pte_same(*dst_pte, orig_dst_pte)) {
+	if (!pte_same(ptep_get(src_pte), orig_src_pte) ||
+	    !pte_same(ptep_get(dst_pte), orig_dst_pte)) {
 		err = -EAGAIN;
 		goto out;
 	}
@@ -935,8 +935,8 @@ static int move_swap_pte(struct mm_struct *mm,

 	double_pt_lock(dst_ptl, src_ptl);

-	if (!pte_same(*src_pte, orig_src_pte) ||
-	    !pte_same(*dst_pte, orig_dst_pte)) {
+	if (!pte_same(ptep_get(src_pte), orig_src_pte) ||
+	    !pte_same(ptep_get(dst_pte), orig_dst_pte)) {
 		double_pt_unlock(dst_ptl, src_ptl);
 		return -EAGAIN;
 	}
@@ -1005,7 +1005,7 @@ static int move_pages_pte(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd,
 	}

 	spin_lock(dst_ptl);
-	orig_dst_pte = *dst_pte;
+	orig_dst_pte = ptep_get(dst_pte);
 	spin_unlock(dst_ptl);
 	if (!pte_none(orig_dst_pte)) {
 		err = -EEXIST;
@@ -1013,7 +1013,7 @@ static int move_pages_pte(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd,
 	}

 	spin_lock(src_ptl);
-	orig_src_pte = *src_pte;
+	orig_src_pte = ptep_get(src_pte);
 	spin_unlock(src_ptl);
 	if (pte_none(orig_src_pte)) {
 		if (!(mode & UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES))
@@ -1043,7 +1043,7 @@ static int move_pages_pte(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd,
 			 * page isn't freed under us
 			 */
 			spin_lock(src_ptl);
-			if (!pte_same(orig_src_pte, *src_pte)) {
+			if (!pte_same(orig_src_pte, ptep_get(src_pte))) {
 				spin_unlock(src_ptl);
 				err = -EAGAIN;
 				goto out;
--
2.25.1

Re: [PATCH v1] mm/userfaultfd: UFFDIO_MOVE implementation should use ptep_get()

[Suren Baghdasaryan]

On Tue, Jan 23, 2024 at 6:18 AM Ryan Roberts <ryan.roberts@arm.com> wrote:
>
> Commit c33c794828f2 ("mm: ptep_get() conversion") converted all
> (non-arch) call sites to use ptep_get() instead of doing a direct
> dereference of the pte. Full rationale can be found in that commit's
> log.
>
> Since then, UFFDIO_MOVE has been implemented which does 7 direct pte
> dereferences. Let's fix those up to use ptep_get().
>
> Fixes: adef440691ba ("userfaultfd: UFFDIO_MOVE uABI")
> Signed-off-by: Ryan Roberts <ryan.roberts@arm.com>

Looks like it does convert all instances introduced by UFFDIO_MOVE
patch. Thanks!

Reviewed-by: Suren Baghdasaryan <surenb@google.com>

> ---
> Hi All,
>
> This applies on top of v6.8-rc1. I'm hoping this can be merged into the
> next rc.
>
> I've asserted in the past that there is no reliable automated mechanism
> to catch these; I'm relying on a combination of Coccinelle (which throws
> up a lot of false positives) and some compiler magic to force a compiler
> error on dereference. But given the frequency with which new issues are
> coming up, I'll add it to my todo list to try to find an automated
> solution.
>
> Thanks,
> Ryan
>
>  mm/userfaultfd.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
> index 20e3b0d9cf7e..aaa7b9821342 100644
> --- a/mm/userfaultfd.c
> +++ b/mm/userfaultfd.c
> @@ -891,8 +891,8 @@ static int move_present_pte(struct mm_struct *mm,
>
>         double_pt_lock(dst_ptl, src_ptl);
>
> -       if (!pte_same(*src_pte, orig_src_pte) ||
> -           !pte_same(*dst_pte, orig_dst_pte)) {
> +       if (!pte_same(ptep_get(src_pte), orig_src_pte) ||
> +           !pte_same(ptep_get(dst_pte), orig_dst_pte)) {
>                 err = -EAGAIN;
>                 goto out;
>         }
> @@ -935,8 +935,8 @@ static int move_swap_pte(struct mm_struct *mm,
>
>         double_pt_lock(dst_ptl, src_ptl);
>
> -       if (!pte_same(*src_pte, orig_src_pte) ||
> -           !pte_same(*dst_pte, orig_dst_pte)) {
> +       if (!pte_same(ptep_get(src_pte), orig_src_pte) ||
> +           !pte_same(ptep_get(dst_pte), orig_dst_pte)) {
>                 double_pt_unlock(dst_ptl, src_ptl);
>                 return -EAGAIN;
>         }
> @@ -1005,7 +1005,7 @@ static int move_pages_pte(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd,
>         }
>
>         spin_lock(dst_ptl);
> -       orig_dst_pte = *dst_pte;
> +       orig_dst_pte = ptep_get(dst_pte);
>         spin_unlock(dst_ptl);
>         if (!pte_none(orig_dst_pte)) {
>                 err = -EEXIST;
> @@ -1013,7 +1013,7 @@ static int move_pages_pte(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd,
>         }
>
>         spin_lock(src_ptl);
> -       orig_src_pte = *src_pte;
> +       orig_src_pte = ptep_get(src_pte);
>         spin_unlock(src_ptl);
>         if (pte_none(orig_src_pte)) {
>                 if (!(mode & UFFDIO_MOVE_MODE_ALLOW_SRC_HOLES))
> @@ -1043,7 +1043,7 @@ static int move_pages_pte(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd,
>                          * page isn't freed under us
>                          */
>                         spin_lock(src_ptl);
> -                       if (!pte_same(orig_src_pte, *src_pte)) {
> +                       if (!pte_same(orig_src_pte, ptep_get(src_pte))) {
>                                 spin_unlock(src_ptl);
>                                 err = -EAGAIN;
>                                 goto out;
> --
> 2.25.1
>

Re: [PATCH v1] mm/userfaultfd: UFFDIO_MOVE implementation should use ptep_get()

[David Hildenbrand]

On 23.01.24 15:17, Ryan Roberts wrote:
> Commit c33c794828f2 ("mm: ptep_get() conversion") converted all
> (non-arch) call sites to use ptep_get() instead of doing a direct
> dereference of the pte. Full rationale can be found in that commit's
> log.
> 
> Since then, UFFDIO_MOVE has been implemented which does 7 direct pte
> dereferences. Let's fix those up to use ptep_get().
> 
> Fixes: adef440691ba ("userfaultfd: UFFDIO_MOVE uABI")
> Signed-off-by: Ryan Roberts <ryan.roberts@arm.com>
> ---
> Hi All,
> 
> This applies on top of v6.8-rc1. I'm hoping this can be merged into the
> next rc.
> 
> I've asserted in the past that there is no reliable automated mechanism
> to catch these; I'm relying on a combination of Coccinelle (which throws
> up a lot of false positives) and some compiler magic to force a compiler
> error on dereference. But given the frequency with which new issues are
> coming up, I'll add it to my todo list to try to find an automated
> solution.
If we'd use a distinct type for pte pointers that are only passed around 
(and not worked on), the compiler would bail out when doing such 
assignments.

Like

typedef struct { unsigned long pte; } pte_t;
typedef struct { unsigned long pte; } pte2_t;


pte_t pte = { 2 };
pte2_t pte2;

pte2 = pte;

-> "error: incompatible types when assigning to type 'pte2_t' from type 
'pte_t'"


pte_get() would do the conversion.


... but just thinking about it this would require a lot of work.

-- 
Cheers,

David / dhildenb