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

[Yan Zhao]

On Tue, Sep 22, 2026 at 06:47:05AM -0700, Sean Christopherson wrote:
> On Tue, Sep 22, 2026, Yan Zhao wrote:
> > On Mon, Sep 21, 2026 at 05:13:32PM -0700, Sean Christopherson wrote:
> > > Drop the superfluous WRITE_ONCE() when setting a memslot's guest_memfd file
> > > during initial binding, as the memslot *must* be inactive and unreachable.
> > > The superfluous WRITE_ONCE() was added by commit 67b43038ce14 ("KVM:
> > > guest_memfd: Remove RCU-protected attribute from slot->gmem.file") to
> > > maintain rough "parity" with the existing rcu_assign_pointer(), not
> > > realizing that the only reason rcu_assign_pointer() was used was to make
> > > sparse and other checkers happy.
> > > 
> > > Cc: Yan Zhao <yan.y.zhao@xxxxxxxxx>
> > > Reviewed-by: David Hildenbrand (Arm) <david@xxxxxxxxxx>
> > > Reviewed-by: Ackerley Tng <ackerleytng@xxxxxxxxxx>
> > > Signed-off-by: Sean Christopherson <seanjc@xxxxxxxxxx>
> > > ---
> > >  virt/kvm/guest_memfd.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
> > > index 80932f4ec4a3..826d26036926 100644
> > > --- a/virt/kvm/guest_memfd.c
> > > +++ b/virt/kvm/guest_memfd.c
> > > @@ -677,7 +677,7 @@ int kvm_gmem_prepare_memory_region(struct kvm *kvm, struct kvm_memory_slot *slot
> > >  	 * kvm_gmem_bind() must occur on a new memslot.  Because the memslot
> > >  	 * is not visible yet, kvm_gmem_get_pfn() is guaranteed to see the file.
> > >  	 */
> > > -	WRITE_ONCE(slot->gmem.file, file);
> > > +	slot->gmem.file = file;
> > >  	slot->gmem.pgoff = offset >> PAGE_SHIFT;
> > >  	if (kvm_gmem_supports_mmap(inode))
> > >  		slot->flags |= KVM_MEMSLOT_GMEM_ONLY;
> > > 
> > Thanks for the fix.
> > Reviewed-by: Yan Zhao <yan.y.zhao@xxxxxxxxx>
> > 
> > BTW: some questions regarding read/write to slot->gmem.file:
> > 
> > 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.

> 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.
It should be superfluous even before commit 0ee2c883b62d, since "the caller is
responsible for ensuring the slot is unreachable before unbinding" ?

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