On Wed, Feb 15, 2023 at 7:35 AM Peter Xu <peterx@xxxxxxxxxx> wrote: > > Hi, David, > > On Tue, Feb 14, 2023 at 04:57:10PM +0900, David Stevens wrote: > > From: David Stevens <stevensd@xxxxxxxxxxxx> > > > > 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@xxxxxxxxxxxx> > > --- > > 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
