On Tue, Jan 23, 2024 at 6:18 AM Ryan Roberts <ryan.roberts@xxxxxxx> 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@xxxxxxx>
Looks like it does convert all instances introduced by UFFDIO_MOVE
patch. Thanks!
Reviewed-by: Suren Baghdasaryan <surenb@xxxxxxxxxx>
> ---
> 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
>
