Yan Zhao <[email protected]> writes: > 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. >
Oh sorry, I missed that. I did see *page = NULL but I didn't connect the dots. Wait but in the above trace, if CPU 1 got the page correctly, wouldnt it take the !r branch where page is not set to NULL? It's only later that CPU 0 truncates the page, but by that time KVM MMU already holds a non-NULL page pointer? Unless on both branches guest_memfd just doesn't provide the page to KVM MMU? >> (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(). > I guess long term this is what guest_memfd would like, that KVM should just be getting PFNs from guest_memfd. >> 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. Which stress selftest is this? I can try running this on my side too. > I have no strong opinion on whether it should be fixed after this series > lands. > But the fix I proposed is quite small :) Small code change, big implications! Might be nice to have this for the future where guest_memfd no longer hands out pages.
