| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <aF7nBzakfWRU_A0Z@x1.local>
Date: Fri, 27 Jun 2025 14:46:31 -0400
From: Peter Xu <peterx@...hat.com>
To: Nikita Kalyazin <kalyazin@...zon.com>
Cc: linux-mm@...ck.org, linux-kernel@...r.kernel.org,
Hugh Dickins <hughd@...gle.com>, Oscar Salvador <osalvador@...e.de>,
Michal Hocko <mhocko@...e.com>,
David Hildenbrand <david@...hat.com>,
Muchun Song <muchun.song@...ux.dev>,
Andrea Arcangeli <aarcange@...hat.com>,
Ujwal Kundur <ujwal.kundur@...il.com>,
Suren Baghdasaryan <surenb@...gle.com>,
Andrew Morton <akpm@...ux-foundation.org>,
Vlastimil Babka <vbabka@...e.cz>,
"Liam R . Howlett" <Liam.Howlett@...cle.com>,
James Houghton <jthoughton@...gle.com>,
Mike Rapoport <rppt@...nel.org>,
Lorenzo Stoakes <lorenzo.stoakes@...cle.com>,
Axel Rasmussen <axelrasmussen@...gle.com>
Subject: Re: [PATCH 0/4] mm/userfaultfd: modulize memory types
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
Powered by blists - more mailing lists