On Mon, Aug 10, 2026 at 07:17:11PM -0700, Ackerley Tng wrote: > 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)? 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 <[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 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 :)
