Re: [PATCH v5 6/6] KVM: guest_memfd: Drop superfluous WRITE_ONCE() when binding a memslot

[Sean Christopherson <seanjc@google.com>]

On Wed, Sep 23, 2026, Yan Zhao wrote:
> On Tue, Sep 22, 2026 at 06:47:05AM -0700, Sean Christopherson wrote:
> > On Tue, Sep 22, 2026, Yan Zhao wrote:
> > > The WRITE_ONCE() in kvm_gmem_unbind() and kvm_gmem_release() are also invoked
> > > when the memslot is inactive and unreachable -- are they also superfluous?
> > 
> > The WRITE_ONCE() in release() is necessary, because the file could be freed/released
> > while it is still attached to a memslot.
> Ah, release() can occur on an active memslot, so WRITE_ONCE() is needed to 
> ensure the READ_ONCE() in get_file_active() works correctly.

Yep.

> > I _think_ the one in unbind() is now superfluous after 0ee2c883b62d ("KVM:
> > guest_memfd: take the invalidate lock when unbinding a dying file"), but that one
> > needs more analysis.
> Hmm, the line "CLASS(gmem_get_file, file)(slot)" in kvm_gmem_get_pfn() is not
> protected by the invalidate lock.

CLASS(gmem_get_file) can never be protected by the invalidate lock.  Or rather,
doing CLASS(gmem_get_file) while holding the invalidate lock is nonsensical,
because taking the lock requires a reference to the inode, and if you have a
(stable) reference to the inode, there's no reason to get a reference to a file.

> It should be superfluous even before commit 0ee2c883b62d, since "the caller is
> responsible for ensuring the slot is unreachable before unbinding" ?

Yes, I just haven't spent enough time thinking about it to be 100% confident :-)

> > > Do we need the READ_ONCE() in __kvm_gmem_get_pfn(), considering that other slot
> > > fields (e.g., slot->gmem.pgoff) are read without READ_ONCE()?
> > 
> > Yes, it's needed, because of the aforementioned release().  The other slot fields
> > are only ever modified when the slot is inactive, i.e. unreachable.  That's why
> > I think the unbind() WRITE_ONCE() is unnecessary; KVM should only unbind when the
> > slot is inactive.
> Maybe the READ_ONCE() in __kvm_gmem_get_pfn() is not necessary?
> When __kvm_gmem_get_pfn() is invoked, a file refcount must have been taken, so a
> concurent release() is not possible.

No?  KVM doesn't hold a reference to the file.  Oooh, you're not talking about a
long-term reference, you're talking about the reference acquired by kvm_gmem_get_file().

Oh, duh.  That READ_ONCE() is purely for a sanity check.

	struct file *slot_file = READ_ONCE(slot->gmem.file);

	...

	if (file != slot_file) {
		WARN_ON_ONCE(slot_file);
		return ERR_PTR(-EFAULT);
	}

So it's not strictly necessary, but since the entire point is to verify the slot
pointer hasn't been clobbered, we do want the READ_ONCE() to guarantee the check
is actually performed as intended.

But given that it should be impossible for release() to run concurrently (see
above), and should be impossible for kvm_gmem_unbind() to run on a live memslot,
then I'm pretty sure we can do this:

diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
index a13445c26d9d..1aeffb5bd2b0 100644
--- a/virt/kvm/guest_memfd.c
+++ b/virt/kvm/guest_memfd.c
@@ -1130,14 +1130,11 @@ static struct folio *__kvm_gmem_get_pfn(struct file *file,
 					pgoff_t index, kvm_pfn_t *pfn,
 					int *max_order)
 {
-	struct file *slot_file = READ_ONCE(slot->gmem.file);
 	struct gmem_file *f = file->private_data;
 	struct folio *folio;
 
-	if (file != slot_file) {
-		WARN_ON_ONCE(slot_file);
+	if (WARN_ON_ONCE(file != READ_ONCE(slot->gmem.file)))
 		return ERR_PTR(-EFAULT);
-	}
 
 	if (xa_load(&f->bindings, index) != slot) {
 		WARN_ON_ONCE(xa_load(&f->bindings, index));

Or just drop the check entirely?   But I think it's worth keeping the check,
especially since __kvm_gmem_get_pfn() will run under the invalidate lock once
in-place conversion lands (see "KVM: guest_memfd: Introduce per-gmem attributes,
use to guard user mappings").  At that point, the check would actually provide
meaningful protection against KVM bugs.