Re: [PATCH v2 4/4] KVM: guest_memfd: Stop returning struct page from PFN lookup

On Thu, Aug 20, 2026 at 07:47:51AM -0700, Ackerley Tng wrote:
> Yan Zhao <[email protected]> writes:
> 
> > On Tue, Aug 18, 2026 at 09:15:55AM +0000, Ackerley Tng wrote:
> >> From: Sean Christopherson <[email protected]>
> >>
> >> KVM currently expects guest_memfd PFN lookups to return a refcounted
> >> struct page, which callers hold across fault handling.
> >>
> >> Holding a page reference across fault handling is problematic for
> >> guest_memfd. In-place memory conversions between confidential
> >> computing shared and private states inspect folio refcounts to ensure
> >> exclusive ownership by guest_memfd. A concurrent guest page fault
> >> taking a reference on the folio causes conversions to fail due to an
> >> elevated refcount.
> > Nit:
> > As this series is based on kvm-x86/next, where there's no in-place memory
> > conversion yet, kvm_gmem_get_pfn() does not hold shared filemap invalidate lock.
> >
> > However, the benefit of dropping the folio reference immediately before
> > returning from the guest_memfd PFN lookup -- preventing conversion failures due
> > to an elevated refcount -- should be effective only if the reference is dropped
> > before releasing the shared filemap invalidate lock.
> >
> > Do we need to make this info clear, since I think it's important? :)
> >
> 
> Is this what you meant?
> 
>   In-place conversions uses the filemap_invalidate_lock() for
>   synchronization of shared/private state. In kvm_gmem_get_pfn(), the
>   PFN needs to be prepared according to its shared/private state. Hence,
>   the filemap_invalidate_lock() is held while guest_memfd gets a folio
>   and decides to make private before returning a PFN.
> 
>   In kvm_gmem_get_pfn(), the folio refcount is dropped before releasing
>   filemap_invalidate_lock(). This ensures that a competing conversion
>   grabbing the filemap_invalidate_lock() will never see an elevated
>   refcount due to guest_memfd's folio-getting process.
> 
> It seems a bit weird to fit this into the commit message for this
> patch. I think I could put the above two paragraphs into the patch that
> introduces the filemap_invalidate_lock in kvm_gmem_get_pfn()?
Ok. Makes sense.

> >> guest_memfd already notifies KVM of page invalidations, so callers
> >> within KVM only need to respect the MMU invalidation protocol to safely
> >> rely on guest_memfd for page presence.
> >>
> >> Furthermore, removing struct page from the guest_memfd PFN lookup moves
> >> KVM closer toward supporting memory backends that are not backed by
> >> struct page.
> >
> > Could we also explain why the lack of SetPageDirty() (and mark_page_accessed())
> > for a gmem page, due to NULL being passed to kvm_release_faultin_page(), is
> > harmless?
> >
> 
> Sounds good. What do you think of this, continuing from the paragraph
> beginning "Furthermore":
> 
>   Drop the folio reference immediately before returning from the
>   guest_memfd PFN lookup, and stop returning the struct page pointer.
> 
>   ARM's gmem_abort() is guest_memfd specific. Since guest_memfd no longer
>   returns a page pointer, there's also no need to do any
>   freeing. kvm_release_faultin_page() originally also serves to set the page
>   dirty and accessed under some conditions. The dirty and accessed flags
>   don't matter for guest_memfd anyway, so it is safe to just drop the call to
>   kvm_release_faultin_page().
> 
>   For ARM's kvm_translate_vncr(), initialize the local page pointer to NULL
>   so that the shared cleanup path that releases fault-in pages safely no-ops
>   for guest_memfd.
> 
>   For x86, no additional changes are required in the MMU fault path
>   because the page fault tracking structure is zero-initialized at the
>   start of page fault handling, ensuring the refcounted page pointer is
>   already NULL.
LGTM. Thank you!