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

[David Stevens <stevensd@chromium.org>]

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.

-David