Re: [PATCH v10 11/41] KVM: guest_memfd: Ensure pages are not in use before conversion

[Ackerley Tng]

Yan Zhao <yan.y.zhao@xxxxxxxxx> writes:

> On Mon, Aug 10, 2026 at 07:17:11PM -0700, Ackerley Tng wrote:
>> Yan Zhao <yan.y.zhao@xxxxxxxxx> writes:
>>
>> > On Mon, Aug 10, 2026 at 02:06:06PM -0700, Ackerley Tng wrote:
>> >> Yan Zhao <yan.y.zhao@xxxxxxxxx> 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 <yan.y.zhao@xxxxxxxxx>
>> >> > 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 <yan.y.zhao@xxxxxxxxx>
>> >> > ---
>> >> >  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.