Re: [PATCH 2/2] mm/khugepaged: skip shmem with userfaultfd

[Peter Xu <peterx@redhat.com>]

On Wed, Feb 15, 2023 at 10:57:11AM +0900, David Stevens wrote:
> On Wed, Feb 15, 2023 at 7:35 AM Peter Xu <peterx@redhat.com> wrote:
> >
> > Hi, David,
> >
> > On Tue, Feb 14, 2023 at 04:57:10PM +0900, David Stevens wrote:
> > > From: David Stevens <stevensd@chromium.org>
> > >
> > > Make sure that collapse_file respects any userfaultfds registered with
> > > MODE_MISSING. If userspace has any such userfaultfds registered, then
> > > for any page which it knows to be missing, it may expect a
> > > UFFD_EVENT_PAGEFAULT. This means collapse_file needs to take care when
> > > collapsing a shmem range would result in replacing an empty page with a
> > > THP, so that it doesn't break userfaultfd.
> > >
> > > Synchronization when checking for userfaultfds in collapse_file is
> > > tricky because the mmap locks can't be used to prevent races with the
> > > registration of new userfaultfds. Instead, we provide synchronization by
> > > ensuring that userspace cannot observe the fact that pages are missing
> > > before we check for userfaultfds. Although this allows registration of a
> > > userfaultfd to race with collapse_file, it ensures that userspace cannot
> > > observe any pages transition from missing to present after such a race.
> > > This makes such a race indistinguishable to the collapse occurring
> > > immediately before the userfaultfd registration.
> > >
> > > The first step to provide this synchronization is to stop filling gaps
> > > during the loop iterating over the target range, since the page cache
> > > lock can be dropped during that loop. The second step is to fill the
> > > gaps with XA_RETRY_ENTRY after the page cache lock is acquired the final
> > > time, to avoid races with accesses to the page cache that only take the
> > > RCU read lock.
> > >
> > > This fix is targeted at khugepaged, but the change also applies to
> > > MADV_COLLAPSE. MADV_COLLAPSE on a range with a userfaultfd will now
> > > return EBUSY if there are any missing pages (instead of succeeding on
> > > shmem and returning EINVAL on anonymous memory). There is also now a
> > > window during MADV_COLLAPSE where a fault on a missing page will cause
> > > the syscall to fail with EAGAIN.
> > >
> > > The fact that intermediate page cache state can no longer be observed
> > > before the rollback of a failed collapse is also technically a
> > > userspace-visible change (via at least SEEK_DATA and SEEK_END), but it
> > > is exceedingly unlikely that anything relies on being able to observe
> > > that transient state.
> > >
> > > Signed-off-by: David Stevens <stevensd@chromium.org>
> > > ---
> > >  mm/khugepaged.c | 66 +++++++++++++++++++++++++++++++++++++++++++------
> > >  1 file changed, 58 insertions(+), 8 deletions(-)
> >
> > Could you attach a changelog in your next post (probably with a cover
> > letter when patches more than one)?
> >
> > Your patch 1 reminded me that, I think both lseek and mincore will not
> > report DATA but HOLE on the thp holes during collapse, no matter we fill
> > hpage in (as long as hpage being !uptodate) or not (as what you do with
> > this one).
> >
> > However I don't understand how this new patch can avoid the same race issue
> > I mentioned in the last version at all.
> 
> If find_get_entry sees an XA_RETRY_ENTRY, then it will re-read from
> the xarray. This means find_get_entry will loop while we're finalizing
> the collapse - either until we finalize the collapse with the
> multi-index hpage entry or abort the collapse and clear the retry
> entry. This means that even if userspace registers a userfaultfd and
> calls lseek after khugepage check for userfaultfd, the call to lseek
> will block until the collapse is finished.
> 
> There are a number of other places in filemap.c/shmem.c that do their
> own iteration over the xarray, and they all retry on xas_retry() as
> well.

I've no problem on using RETRY entries (as long as others are fine with it
:).  It seems your logic depends on patch 1 being there already, so right
after the RETRY got replaced with the thp it'll show Uptodate==DATA.

However I doubt whether patch 1 is correct at all..  Maybe that can be
instead fixed by having:

		folio_mark_uptodate(folio);

To be before:

		xas_set_order(&xas, start, HPAGE_PMD_ORDER);
		xas_store(&xas, hpage);

To replace patch 1, but I think there's still some issue in patch 2 even if
it works.  Ouch, I cut the codes..  I'll comment inline in another reply.

-- 
Peter Xu