Re: [PATCH v2 4/4] KVM: guest_memfd: Stop returning struct page from PFN lookup
Michael Roth <[email protected]>
| Newsgroups | gmane.linux.kernel,gmane.comp.emulators.kvm.devel,gmane.linux.ports.arm.kernel |
|---|---|
| Message-ID | <eey6juiwdmunedyslruw2gr5c2h67gtdmfmvdjycih7ib3s7ff@sfx24kvff4df> |
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. > > 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. > > Drop the folio reference immediately before returning from the > guest_memfd PFN lookup, and stop returning the struct page pointer. > > For ARM, 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. > > Reported-by: Yan Zhao <[email protected]> > Closes: https://lore.kernel.org/all/[email protected]/ > Signed-off-by: Sean Christopherson <[email protected]> > Co-developed-by: Yan Zhao <[email protected]> > Signed-off-by: Yan Zhao <[email protected]> > Co-developed-by: Ackerley Tng <[email protected]> > Signed-off-by: Ackerley Tng <[email protected]> Other than what Suzuki pointed out: Reviewed-by: Michael Roth <[email protected]> Tested-by: Michael Roth <[email protected]> > --- > arch/arm64/kvm/mmu.c | 4 ++-- > arch/arm64/kvm/nested.c | 4 ++-- > arch/x86/kvm/mmu/mmu.c | 2 +- > arch/x86/kvm/svm/sev.c | 8 ++------ > include/linux/kvm_host.h | 6 ++---- > virt/kvm/guest_memfd.c | 9 ++------- > 6 files changed, 11 insertions(+), 22 deletions(-) > > diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c > index 6c941aaa10c63..e5d637a5ec558 100644 > --- a/arch/arm64/kvm/mmu.c > +++ b/arch/arm64/kvm/mmu.c > @@ -1613,7 +1613,7 @@ static int gmem_abort(const struct kvm_s2_fault_desc *s2fd) > enum kvm_pgtable_prot prot = KVM_PGTABLE_PROT_R; > struct kvm_pgtable *pgt = s2fd->vcpu->arch.hw_mmu->pgt; > unsigned long mmu_seq; > - struct page *page; > + struct page *page = NULL; > struct kvm *kvm = s2fd->vcpu->kvm; > void *memcache = NULL; > kvm_pfn_t pfn; > @@ -1641,7 +1641,7 @@ static int gmem_abort(const struct kvm_s2_fault_desc *s2fd) > /* Pairs with the smp_wmb() in kvm_mmu_invalidate_end(). */ > smp_rmb(); > > - ret = kvm_gmem_get_pfn(kvm, s2fd->memslot, gfn, &pfn, &page, NULL); > + ret = kvm_gmem_get_pfn(kvm, s2fd->memslot, gfn, &pfn, NULL); > if (ret) { > kvm_prepare_memory_fault_exit(s2fd->vcpu, s2fd->fault_ipa, PAGE_SIZE, > write_fault, exec_fault, false); > diff --git a/arch/arm64/kvm/nested.c b/arch/arm64/kvm/nested.c > index fb54f6dad995c..43523bb17621a 100644 > --- a/arch/arm64/kvm/nested.c > +++ b/arch/arm64/kvm/nested.c > @@ -1360,7 +1360,7 @@ static int kvm_translate_vncr(struct kvm_vcpu *vcpu, bool *is_gmem) > bool write_fault, writable; > unsigned long mmu_seq; > struct vncr_tlb *vt; > - struct page *page; > + struct page *page = NULL; > u64 va, pfn, gfn; > int ret; > > @@ -1411,7 +1411,7 @@ static int kvm_translate_vncr(struct kvm_vcpu *vcpu, bool *is_gmem) > if (is_error_noslot_pfn(pfn) || (write_fault && !writable)) > return -EFAULT; > } else { > - ret = kvm_gmem_get_pfn(vcpu->kvm, memslot, gfn, &pfn, &page, NULL); > + ret = kvm_gmem_get_pfn(vcpu->kvm, memslot, gfn, &pfn, NULL); > if (ret) { > kvm_prepare_memory_fault_exit(vcpu, vt->wr.pa, PAGE_SIZE, > write_fault, false, false); > diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c > index c519e8e8d646f..129d403308051 100644 > --- a/arch/x86/kvm/mmu/mmu.c > +++ b/arch/x86/kvm/mmu/mmu.c > @@ -4604,7 +4604,7 @@ static int kvm_mmu_faultin_pfn_gmem(struct kvm_vcpu *vcpu, > } > > r = kvm_gmem_get_pfn(vcpu->kvm, fault->slot, fault->gfn, &fault->pfn, > - &fault->refcounted_page, &max_order); > + &max_order); > if (r) { > kvm_mmu_prepare_memory_fault_exit(vcpu, fault); > return r; > diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c > index d3d620bc04dce..32db979daaaf0 100644 > --- a/arch/x86/kvm/svm/sev.c > +++ b/arch/x86/kvm/svm/sev.c > @@ -4016,7 +4016,6 @@ static void __sev_snp_reload_vmsa(struct kvm_vcpu *vcpu, gpa_t gpa) > struct kvm *kvm = vcpu->kvm; > gfn_t gfn = gpa_to_gfn(gpa); > unsigned long mmu_seq; > - struct page *page; > kvm_pfn_t pfn; > > lockdep_assert_held(&svm->sev_es.snp_vmsa_mutex); > @@ -4060,9 +4059,8 @@ static void __sev_snp_reload_vmsa(struct kvm_vcpu *vcpu, gpa_t gpa) > * The new VMSA will be private memory guest memory, so retrieve the > * PFN from the gmem backend. > */ > - if (kvm_gmem_get_pfn(vcpu->kvm, slot, gfn, &pfn, &page, NULL)) > + if (kvm_gmem_get_pfn(vcpu->kvm, slot, gfn, &pfn, NULL)) > return; > - kvm_release_page_clean(page); > > read_lock(&kvm->mmu_lock); > /* > @@ -5003,7 +5001,6 @@ void sev_handle_rmp_fault(struct kvm_vcpu *vcpu, gpa_t gpa, u64 error_code) > struct kvm *kvm = vcpu->kvm; > int order, rmp_level, ret; > unsigned long mmu_seq; > - struct page *page; > bool assigned; > kvm_pfn_t pfn; > gfn_t gfn; > @@ -5033,13 +5030,12 @@ void sev_handle_rmp_fault(struct kvm_vcpu *vcpu, gpa_t gpa, u64 error_code) > mmu_seq = kvm->mmu_invalidate_seq; > smp_rmb(); > > - ret = kvm_gmem_get_pfn(kvm, slot, gfn, &pfn, &page, &order); > + ret = kvm_gmem_get_pfn(kvm, slot, gfn, &pfn, &order); > if (ret) { > pr_warn_ratelimited("SEV: Unexpected RMP fault, no backing page for private GPA 0x%llx\n", > gpa); > return; > } > - kvm_release_page_unused(page); > > ret = snp_lookup_rmpentry(pfn, &assigned, &rmp_level); > if (ret || !assigned) { > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h > index 03bfc92864b6e..502465119ca0c 100644 > --- a/include/linux/kvm_host.h > +++ b/include/linux/kvm_host.h > @@ -2586,13 +2586,11 @@ static inline bool kvm_mem_is_private(struct kvm *kvm, gfn_t gfn) > > #ifdef CONFIG_KVM_GUEST_MEMFD > int kvm_gmem_get_pfn(struct kvm *kvm, struct kvm_memory_slot *slot, > - gfn_t gfn, kvm_pfn_t *pfn, struct page **page, > - int *max_order); > + gfn_t gfn, kvm_pfn_t *pfn, int *max_order); > #else > static inline int kvm_gmem_get_pfn(struct kvm *kvm, > struct kvm_memory_slot *slot, gfn_t gfn, > - kvm_pfn_t *pfn, struct page **page, > - int *max_order) > + kvm_pfn_t *pfn, int *max_order) > { > KVM_BUG_ON(1, kvm); > return -EIO; > diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c > index b596486d184ca..589762140c3ef 100644 > --- a/virt/kvm/guest_memfd.c > +++ b/virt/kvm/guest_memfd.c > @@ -751,8 +751,7 @@ static struct folio *__kvm_gmem_get_pfn(struct file *file, > } > > int kvm_gmem_get_pfn(struct kvm *kvm, struct kvm_memory_slot *slot, > - gfn_t gfn, kvm_pfn_t *pfn, struct page **page, > - int *max_order) > + gfn_t gfn, kvm_pfn_t *pfn, int *max_order) > { > pgoff_t index = kvm_gmem_get_index(slot, gfn); > struct folio *folio; > @@ -780,11 +779,7 @@ int kvm_gmem_get_pfn(struct kvm *kvm, struct kvm_memory_slot *slot, > #endif > > folio_unlock(folio); > - > - if (!r) > - *page = folio_file_page(folio, index); > - else > - folio_put(folio); > + folio_put(folio); > > return r; > } > > -- > 2.55.0.699.gb54405d56f-goog >