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