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

Yan Zhao [off-list ref] writes:

On Mon, Aug 10, 2026 at 07:17:11PM -0700, Ackerley Tng wrote:

quoted

Yan Zhao [off-list ref] writes:

quoted

On Mon, Aug 10, 2026 at 02:06:06PM -0700, Ackerley Tng wrote:

quoted

Yan Zhao [off-list ref] writes:

quoted

[...snip...]

quoted

quoted

@@ -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!

quoted

quoted

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.

quoted

quoted

quoted

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.

quoted

I think filemap_invalidate_unlock_shared(folio->mapping) is a little
asymmetric...

quoted

quoted

quoted

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

quoted

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

quoted

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.