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