Yan Zhao <[email protected]> writes: > On Mon, Aug 10, 2026 at 02:06:06PM -0700, Ackerley Tng wrote: >> Yan Zhao <[email protected]> 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)? 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 <[email protected]> >> > 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 <[email protected]> >> > --- >> > 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 (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_*? Would like to see what Sean thinks of this. Either way, is it okay to follow up after conversions lands?
