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@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. 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
