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

[Yan Zhao <yan.y.zhao@intel.com>]

On Wed, Sep 23, 2026 at 07:26:04AM -0700, Sean Christopherson wrote:
> 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.
Yes, I mentioned that with the hope of proving that the invalidate lock does not
make unbind() safer about dropping WRITE_ONCE(). :)


> > 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.
Makes sense!

> 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));
>
LGTM.

> 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.
I see. Thanks for the explanation!