Re: [PATCH v10 11/41] KVM: guest_memfd: Ensure pages are not in use before conversion

[Yan Zhao <yan.y.zhao@intel.com>]

On Mon, Aug 10, 2026 at 07:17:11PM -0700, Ackerley Tng wrote:
> Yan Zhao <yan.y.zhao@intel.com> writes:
> 
> > On Mon, Aug 10, 2026 at 02:06:06PM -0700, Ackerley Tng wrote:
> >> Yan Zhao <yan.y.zhao@intel.com> writes:
> >>
> >> >
> >> > [...snip...]
> >> >
> >> >> > @@ -542,8 +576,21 @@ static int __kvm_gmem_set_attributes(struct inode *inode, pgoff_t start,
> >> >> >
> >> >> >  	mas_init(&mas, mt, start);
> >> >> >  	r = kvm_gmem_mas_preallocate(&mas, attrs, start, nr_pages);
> >> >> > -	if (r)
> >> >> > +	if (r) {
> >> >> > +		*err_index = start;
> >> >> >  		goto out;
> >> >> > +	}
> >> >> > +
> >> >> > +	if (to_private) {
> >> >> > +		unmap_mapping_pages(mapping, start, nr_pages, false);
> >> >> > +
> >> >> > +		if (!kvm_gmem_is_safe_for_conversion(inode, start, nr_pages,
> >> >> > +						     err_index)) {
> >> >> Note: conversion failures could occur if another vCPU is attempting to map a GFN
> >> >> within this range.
> >> >>
> >> >> CPU 0 (setting attributes)          CPU 1 (attempting to map)
> >> >> --------------------------          --------------------
> >> >>                                  A: mmu_invalidate_retry_gfn_unsafe
> >> >>                                     filemap_invalidate_lock_shared
> >> >>                                     __kvm_gmem_get_pfn ==> folio refcount++
> >> >>                                     filemap_invalidate_unlock_shared
> >> >>
> >> >> filemap_invalidate_lock
> >> >> filemap_get_folios
> >> >> check folio_ref_count(folio) ==> Not match !!
> >> >> filemap_invalidate_unlock
> >> >>
> >> >>                                  B: read_lock(&vcpu->kvm->mmu_lock);
> >> >>                                     is_page_fault_stale
> >> >>                                     kvm_mmu_finish_page_fault ==>folio recount--
> >> >> 				    read_unlock(&vcpu->kvm->mmu_lock);
> >> >>
> >> >>
> >>
> >> Thanks for reporting this!
> >>
> >> >> Retrying in kvm_gmem_is_safe_for_conversion() or moving the invocation of
> >> >> kvm_mmu_invalidate_start() + kvm_mmu_invalidate_range_add() to an earlier
> >> >> position does not help as long as CPU 1 stays at stage A.
> >> >>
> >>
> >> IIUC CPU 1 isn't blocked by a conversion so stages A and B should
> >> complete fine, and CPU 0 would already be retrying for other reasons
> >> anyway, like speculative refcounts from elsewhere in the kernel, so the
> >> conversion would take longer but it'd work out.
> >>
> >> Is that understanding right, that this doesn't completely break
> >> conversions?
> > It depends on the timing. The max retry count cannot be expected under unlucky
> > conditions.
> >
> >> >> So, should we avoid this failure?
> >> >> e.g., by moving filemap_invalidate_unlock_shared() from stage A to after
> >> >> stage B?
> >>
> >> Not really sure about this, how will control go back to guest_memfd
> >> after the fault finishes for guest_memfd to unlock the filemap?
> > I don't understand your question. But I find this solution is less ideal than my
> > below proposal.
> >
> 
> When kvm_gmem_get_pfn() is called from kvm_mmu_faultin_pfn_gmem(), KVM
> MMU takes over from there, there isn't another call when KVM MMU
> finishes mapping the page into the stage 2 page tables back into
> guest_memfd.
> 
> After stage B, how is filemap_invalidate_unlock_shared() going to be
> called? Do you mean filemap_invalidate_unlock_shared(folio->mapping)?
Something like this. However, since this would cause the shared filemap
invalidate lock to be held longer, conversions may have to wait for any on-going
faults regardless of the GFN range, which I don't quite like.

> I think filemap_invalidate_unlock_shared(folio->mapping) is a little
> asymmetric...
> 
> >> > Or what about having KVM always treat gmem page as non-refcounted, and have
> >> > kvm_gmem_get_pfn() put folio refcount before releasing the filemap invalidate
> >> > lock?
> >> > Below patch is applied and tested at the end of this series.
> >> >
> >> > From 8c2f29bc15bceb6a8fa103cf2585ec11354fd74e Mon Sep 17 00:00:00 2001
> >> > From: Yan Zhao <yan.y.zhao@intel.com>
> >> > Date: Mon, 10 Aug 2026 06:24:52 +0800
> >> > Subject: [PATCH] KVM: guest_memfd: Return gmem page as non-refcounted
> >> >
> >> > Have kvm_gmem_get_pfn() put gmem page refcount before releasing filemap
> >> > invalidate lock and return the gmem page as non-refcounted. This avoids
> >> > gmem memory attribute conversion failure caused by temporarily holding gmem
> >> > page after faulting and before completing mapping.
> >> >
> >> > guest_memfd always holds gmem page in filemap cache. TDX does not increment
> >> > gmem page refcount when having gmem pages mapped in S-EPT. Additionally,
> >> > as gmem pages are not swappable, setting dirty or accessed bit is not
> >> > necessary. Therefore, there's no need to treat gmem pages as refcounted
> >> > pages.
> >> >
> >> > Signed-off-by: Yan Zhao <yan.y.zhao@intel.com>
> >> > ---
> >> >  virt/kvm/guest_memfd.c | 5 ++---
> >> >  1 file changed, 2 insertions(+), 3 deletions(-)
> >> >
> >> > diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
> >> > index 2115e73e455a..e357b4ffa777 100644
> >> > --- a/virt/kvm/guest_memfd.c
> >> > +++ b/virt/kvm/guest_memfd.c
> >> > @@ -1332,11 +1332,10 @@ int kvm_gmem_get_pfn(struct kvm *kvm, struct kvm_memory_slot *slot,
> >> >  #endif
> >> >
> >> >  	folio_unlock(folio);
> >> > +	folio_put(folio);
> >> >
> >> >  	if (!r)
> >> > -		*page = folio_file_page(folio, index);
> >> > -	else
> >> > -		folio_put(folio);
> >> > +		*page = NULL;
> >> >
> >> >  out:
> >> >  	filemap_invalidate_unlock_shared(file_inode(file)->i_mapping);
> >> > --
> >> > 2.43.2
> >>
> >> Hmm going with the above CPU 0 and 1 illustration, if instead CPU 0
> >> truncates the folio and the folio ends up being freed, then KVM's MMU
> >> has a pointer to a page that is already
> >> freed. kvm_release_faultin_page() is passed the pointer to this page and
> >> will dereference the page.
> > Not really. It's just like KVM mapping non-refcounted pages.
> > kvm_release_faultin_page() does not access the non-refcounted pages.
> > The invalidate protocol also ensures no mapping of stale pfn.
> >
> > As below, if CPU 0 truncates the folio, it needs to hold filemap invalidate lock,
> > add KVM mmu invalidate range, hold mmu_lock, zap KVM mappings before the
> > truncation.
> >
> >
> > CPU 0                                      CPU 1
> > -----                                    --------
> >                                          Save fault->mmu_seq
> >
> >                                      B1. filemap_invalidate_lock_shared
> >                                          __kvm_gmem_get_pfn
> >                                          folio_put
> >                                          filemap_invalidate_unlock_shared
> >
> > 				     B2. read_lock
> >                                          is_page_fault_stale
> >
> >                                      B3. kvm_tdp_mmu_map
> >                                      B4. kvm_mmu_finish_page_fault
> >                                          read_unlock
> > A1. filemap_invalidate_lock
> >     kvm_gmem_invalidate_start
> >
> > A2. write_lock
> >     zap KVM MMU
> >     write_unlock
> >     truncate
> >
> > A3. kvm_gmem_invalidate_end
> >     filemap_invalidate_unlock
> >
> >
> > A1 occurs either before or after B1.
> > 1) If A1 occurs before B1, B1 will find the correct pfn.
> > 2) If A1 occurs after B1 and before B2,
> >    a. if A2 is before B2, B2 must find the fault is stale, so it's fine.
> >    b. if A2 is after B2, A2 must be after B4 as well. So, accessing stale pfn
> >       in CPU 1 is fine.
> > 3) If A1 occurs after B2 and before B3,
> > 4) If A1 occurs after B3 and before B4,
> > 5) If A1 occurs after B4,
> >    A2 must be after B4 (for 3-5 conditions).
> >    So, accessing stale pfn in CPU 1 is fine.
> 
> It's not about the stale PFN, if there's no refcount on the page
> returned from B1, then after A3, the page can be freed.
>
> Contractually, I think KVM is allowed to reference the page?
> kvm_release_page_clean() calls kvm_set_page_accessed() on the page
In my patch, "*page = NULL;" is returned in kvm_gmem_get_pfn(). So,
fault->refcounted_page is NULL. With it, kvm_release_faultin_page() does
not access the faultin PFN or the page.
So, no worry about UAF.

> (UAF?). Not sure which other KVM architectures reference the struct page
> itself. Are we going to teach the rest of KVM to not reference and not
> do kvm_release_page_*?
It's just like how KVM maps non-refcounted pages.
And guest_memfd actually asks consumers (like TDX) not to take page refcount.
__kvm_gmem_populate() also puts the folio refcount before invoking
filemap_invalidate_unlock().

> Would like to see what Sean thinks of this. Either way, is it okay to
> follow up after conversions lands?
Let's see what Sean thinks of this :)
I raised this because the issue was encountered by one TDX's stress selftest.
I have no strong opinion on whether it should be fixed after this series lands.
But the fix I proposed is quite small :)