lkml.org 
[lkml]   [2021]   [Feb]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH v5 05/10] userfaultfd: add minor fault registration mode
    On Fri, Feb 12, 2021 at 11:18 AM Mike Kravetz <mike.kravetz@oracle.com> wrote:
    >
    > On 2/10/21 1:21 PM, Axel Rasmussen wrote:
    > > This feature allows userspace to intercept "minor" faults. By "minor"
    > > faults, I mean the following situation:
    > >
    > > Let there exist two mappings (i.e., VMAs) to the same page(s). One of
    > > the mappings is registered with userfaultfd (in minor mode), and the
    > > other is not. Via the non-UFFD mapping, the underlying pages have
    > > already been allocated & filled with some contents. The UFFD mapping
    > > has not yet been faulted in; when it is touched for the first time,
    > > this results in what I'm calling a "minor" fault. As a concrete
    > > example, when working with hugetlbfs, we have huge_pte_none(), but
    > > find_lock_page() finds an existing page.
    >
    > Do we want to intercept the fault if it is for a private mapping that
    > will COW the page in the page cache? I think 'yes' but just want to
    > confirm. The code added to hugetlb_no_page will intercept these COW
    > accesses.

    I can at least say this is intentional, although I admit I don't have
    a precise use case in mind for the UFFD mapping being private. I
    suppose it's something like, the UFFD poll thread is supposed to
    (maybe) update the page contents, *before* I CoW it, and then once
    it's been CoW-ed I don't want that poll thread to be able to see
    whatever changes I've made?

    Unless there's some different use case for this, I believe this is the
    behavior we want.

    >
    > <snip>
    >
    > > diff --git a/mm/hugetlb.c b/mm/hugetlb.c
    > > index e41b77cf6cc2..f150b10981a8 100644
    > > --- a/mm/hugetlb.c
    > > +++ b/mm/hugetlb.c
    > > @@ -4366,6 +4366,38 @@ static vm_fault_t hugetlb_no_page(struct mm_struct *mm,
    > > VM_FAULT_SET_HINDEX(hstate_index(h));
    > > goto backout_unlocked;
    > > }
    > > +
    > > + /* Check for page in userfault range. */
    > > + if (userfaultfd_minor(vma)) {
    > > + u32 hash;
    > > + struct vm_fault vmf = {
    > > + .vma = vma,
    > > + .address = haddr,
    > > + .flags = flags,
    > > + /*
    > > + * Hard to debug if it ends up being used by a
    > > + * callee that assumes something about the
    > > + * other uninitialized fields... same as in
    > > + * memory.c
    > > + */
    > > + };
    > > +
    > > + unlock_page(page);
    > > +
    > > + /*
    > > + * hugetlb_fault_mutex and i_mmap_rwsem must be dropped
    > > + * before handling userfault. Reacquire after handling
    > > + * fault to make calling code simpler.
    > > + */
    > > +
    > > + hash = hugetlb_fault_mutex_hash(mapping, idx);
    > > + mutex_unlock(&hugetlb_fault_mutex_table[hash]);
    > > + i_mmap_unlock_read(mapping);
    >
    > After dropping all the locks, we only hold a reference to the page in the
    > page cache. I 'think' someone else could hole punch the page and remove it
    > from the cache. IIUC, state changing while processing uffd faults is something
    > that users need to deal with? Just need to make sure there are no assumptions
    > in the kernel code.

    Yeah, this seems possible. What I'd expect to happen in that case is
    something like:

    1. hugetlb_no_page() calls into handle_userfault().
    2. Someone hole punches the page, removing it from the page cache.
    3. The UFFD poll thread gets the fault event, and issues a
    UFFDIO_CONTINUE. (Say we instead were going to write an update, and
    *then* UFFDIO_CONTINUE: I think the hole punch by another thread could
    also happen between those two events.)
    4. This calls down into hugetlb_mcopy_atomic_pte, where we try to
    find_lock_page(). This returns NULL, so we bail with -EFAULT.
    5. Userspace detects and deals with this error - maybe by writing to
    the non-UFFD mapping, thereby putting a page back in the page cache,
    or by issuing a UFFDIO_COPY or such?

    Which, as far as I can see is fine? But, I am by no means an expert
    yet so please correct me if this seems problematic. :)

    >
    > > + ret = handle_userfault(&vmf, VM_UFFD_MINOR);
    > > + i_mmap_lock_read(mapping);
    > > + mutex_lock(&hugetlb_fault_mutex_table[hash]);
    > > + goto out;
    > > + }
    > > }
    > >
    > > /*
    > >
    >
    > --
    > Mike Kravetz

    \
     
     \ /
      Last update: 2021-02-12 20:36    [W:4.228 / U:0.424 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site