Re: [PATCH 0/4] mm/userfaultfd: modulize memory types

[Peter Xu <peterx@redhat.com>]

On Fri, Jun 27, 2025 at 05:59:49PM +0100, Nikita Kalyazin wrote:
> 
> 
> On 27/06/2025 14:51, Peter Xu wrote:
> > On Thu, Jun 26, 2025 at 05:09:47PM +0100, Nikita Kalyazin wrote:
> > > 
> > > 
> > > On 25/06/2025 21:17, Peter Xu wrote:
> > > > On Wed, Jun 25, 2025 at 05:56:23PM +0100, Nikita Kalyazin wrote:
> > > > > 
> > > > > 
> > > > > On 20/06/2025 20:03, Peter Xu wrote:
> > > > > > [based on akpm/mm-new]
> > > > > > 
> > > > > > This series is an alternative proposal of what Nikita proposed here on the
> > > > > > initial three patches:
> > > > > > 
> > > > > >      https://lore.kernel.org/r/20250404154352.23078-1-kalyazin@amazon.com
> > > > > > 
> > > > > > This is not yet relevant to any guest-memfd support, but paving way for it.
> > > > > 
> > > > > Hi Peter,
> > > > 
> > > > Hi, Nikita,
> > > > 
> > > > > 
> > > > > Thanks for posting this.  I confirmed that minor fault handling was working
> > > > > for guest_memfd based on this series and looked simple (a draft based on
> > > > > mmap support in guest_memfd v7 [1]):
> > > > 
> > > > Thanks for the quick spin, glad to know it works. Some trivial things to
> > > > mention below..
> > > 
> > > Following up, I drafted UFFDIO_COPY support for guest_memfd to confirm it
> > > works as well:
> > 
> > Appreciated.
> > 
> > Since at it, I'll comment quickly below.
> > 
> > > 
> > > diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
> > > index 8c44e4b9f5f8..b5458a22fff4 100644
> > > --- a/virt/kvm/guest_memfd.c
> > > +++ b/virt/kvm/guest_memfd.c
> > > @@ -349,12 +349,19 @@ static bool kvm_gmem_offset_is_shared(struct file
> > > *file, pgoff_t index)
> > > 
> > >   static vm_fault_t kvm_gmem_fault(struct vm_fault *vmf)
> > >   {
> > > +     struct vm_area_struct *vma = vmf ? vmf->vma : NULL;
> > >        struct inode *inode = file_inode(vmf->vma->vm_file);
> > >        struct folio *folio;
> > >        vm_fault_t ret = VM_FAULT_LOCKED;
> > > 
> > >        filemap_invalidate_lock_shared(inode->i_mapping);
> > > 
> > > +     folio = filemap_get_entry(inode->i_mapping, vmf->pgoff);
> > > +     if (!folio && vma && userfaultfd_missing(vma)) {
> > > +             filemap_invalidate_unlock_shared(inode->i_mapping);
> > > +             return handle_userfault(vmf, VM_UFFD_MISSING);
> > > +     }
> > 
> > Likely a possible refcount leak when folio != NULL here.
> 
> Thank you.  I was only aiming to cover the happy case for know.  I will keep
> it in mind for the future.

Yep that's good enough, thanks.  It's really something I'd comment
passingly, it's definitely reassuring to know the happy case works.

> > > +
> > >        folio = kvm_gmem_get_folio(inode, vmf->pgoff);
> > >        if (IS_ERR(folio)) {
> > >                int err = PTR_ERR(folio);
> > > @@ -438,10 +445,57 @@ static int kvm_gmem_uffd_get_folio(struct inode
> > > *inode, pgoff_t pgoff,
> > >        return 0;
> > >   }
> > > 
> > > +static int kvm_gmem_mfill_atomic_pte(pmd_t *dst_pmd,
> > > +                        struct vm_area_struct *dst_vma,
> > > +                        unsigned long dst_addr,
> > > +                        unsigned long src_addr,
> > > +                        uffd_flags_t flags,
> > > +                        struct folio **foliop)
> > > +{
> > > +     struct inode *inode = file_inode(dst_vma->vm_file);
> > > +     pgoff_t pgoff = linear_page_index(dst_vma, dst_addr);
> > > +     struct folio *folio;
> > > +     int ret;
> > > +
> > > +     folio = kvm_gmem_get_folio(inode, pgoff);
> > > +     if (IS_ERR(folio)) {
> > > +             ret = PTR_ERR(folio);
> > > +             goto out;
> > > +     }
> > > +
> > > +     folio_unlock(folio);
> > > +
> > > +     if (uffd_flags_mode_is(flags, MFILL_ATOMIC_COPY)) {
> > > +             void *vaddr = kmap_local_folio(folio, 0);
> > > +             ret = copy_from_user(vaddr, (const void __user *)src_addr, PAGE_SIZE);
> > > +             kunmap_local(vaddr);
> > > +             if (unlikely(ret)) {
> > > +                     *foliop = folio;
> > > +                     ret = -ENOENT;
> > > +                     goto out;
> > > +             }
> > > +     } else {                /* ZEROPAGE */
> > > +             clear_user_highpage(&folio->page, dst_addr);
> > > +     }
> > > +
> > > +     kvm_gmem_mark_prepared(folio);
> > 
> > Since Faud's series hasn't yet landed, so I'm almost looking at the current
> > code base with an imagination of what might happen.
> > 
> > In general, missing trapping for guest-memfd could start to be slightly
> > trickier.  So far IIUC guest-memfd cache pool needs to be populated only by
> > a prior fallocate() syscall, not during fault.  So I suppose we will need
> > to use uptodate bit to mark folio ready, like what's done here.
> 
> I don't think I'm familiar with the fallocate() requirement in guest_memfd.
> Fuad's v12 [1] (although I think it has been like that from the beginning)
> calls kvm_gmem_get_folio() that populates pagecache in the fault handler
> (kvm_gmem_fault_shared()).  SEV [2] and TDX [3] seem to use
> kvm_gmem_populate() for both allocation and preparation.

I actually didn't notice fault() uses kvm_gmem_get_folio(), which has
FGP_CREAT indeed.

I checked Ackerley's latest 1G patchset, which also did the same that
kvm_gmem_get_folio() will invoke the custom allocator to allocate 1G pages
even during a fault().

Not sure whether it's intentional though, for example, if the tests in
userspace always does fallocate() then the code should run the same, and
FGP_CREAT will just never be used.

Thanks for pointing this out.  I definitely didn't notice this trivial
detail before. Looks like it's not a major issue, if the folio can be
dynamically allocated, then MISSING mode (if/when it'll be supported) can
capture both "!folio" and "folio && !uptodate" cases here as missing.

> 
> [1] https://lore.kernel.org/kvm/20250611133330.1514028-1-tabba@google.com/T/#m15b53a741e4f328e61f995a01afb9c4682ffe611
> [2] https://elixir.bootlin.com/linux/v6.16-rc3/source/arch/x86/kvm/svm/sev.c#L2331
> [3] https://elixir.bootlin.com/linux/v6.16-rc3/source/arch/x86/kvm/vmx/tdx.c#L3236

-- 
Peter Xu