Received-SPF: pass (google.com: domain of linux-kernel+bounces-132078-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240404171726.2302435-1-lokeshgidra@google.com> <Zg7hrt5HudXLBUn_@casper.infradead.org>
In-Reply-To: <Zg7hrt5HudXLBUn_@casper.infradead.org>
From: Suren Baghdasaryan <surenb@google.com>
Date: Thu, 4 Apr 2024 13:07:25 -0700
Message-ID: <CAJuCfpHvb5Jt6J1P9aHVdRWYUHmBL1edfp7QHn=jF88gJRD7RA@mail.gmail.com>
Subject: Re: [PATCH] userfaultfd: change src_folio after ensuring it's
 unpinned in UFFDIO_MOVE
To: Matthew Wilcox <willy@infradead.org>
Cc: Lokesh Gidra <lokeshgidra@google.com>, akpm@linux-foundation.org, linux-mm@kvack.org, 
	linux-kernel@vger.kernel.org, kernel-team@android.com, aarcange@redhat.com, 
	peterx@redhat.com, david@redhat.com, zhengqi.arch@bytedance.com, 
	kaleshsingh@google.com, ngeoffray@google.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Thu, Apr 4, 2024 at 10:21=E2=80=AFAM Matthew Wilcox <willy@infradead.org=
> wrote:
>
> On Thu, Apr 04, 2024 at 10:17:26AM -0700, Lokesh Gidra wrote:
> > -             folio_move_anon_rmap(src_folio, dst_vma);
> > -             WRITE_ONCE(src_folio->index, linear_page_index(dst_vma, d=
st_addr));
> > -
> >               src_pmdval =3D pmdp_huge_clear_flush(src_vma, src_addr, s=
rc_pmd);
> >               /* Folio got pinned from under us. Put it back and fail t=
he move. */
> >               if (folio_maybe_dma_pinned(src_folio)) {
> > @@ -2270,6 +2267,9 @@ int move_pages_huge_pmd(struct mm_struct *mm, pmd=
_t *dst_pmd, pmd_t *src_pmd, pm
> >                       goto unlock_ptls;
> >               }
> >
> > +             folio_move_anon_rmap(src_folio, dst_vma);
> > +             WRITE_ONCE(src_folio->index, linear_page_index(dst_vma, d=
st_addr));
> > +
>
> This use of WRITE_ONCE scares me.  We hold the folio locked.  Why do
> we need to use WRITE_ONCE?  Who's looking at folio->index without
> holding the folio lock?

Indeed that seems to be unnecessary here. Both here and in
move_present_pte() we are holding folio lock while moving the page. I
must have just blindly copied that from Andrea's original patch [1].

https://gitlab.com/aarcange/aa/-/commit/2aec7aea56b10438a3881a20a411aa4b1fc=
19e92