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

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