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

[Suren Baghdasaryan <surenb@google.com>]

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
>