Re: [PATCH v3 1/4] KVM: guest_memfd: Gracefully handle xarray errors when binding a memslot

[Sean Christopherson]

On Mon, Sep 07, 2026, David Hildenbrand (Arm) wrote:
> On 9/4/26 02:43, Sean Christopherson wrote:
> > If inserting a memslot into a guest_memfd's bindings xarray fails,
> > propagate the error back to the caller, i.e. fail memslot creation as well.
> > Signalling success and continuing on with memslot creation results in
> > use-after-free, as the guest_memfd instance will remain reachable via the
> > memslot after the file is freed (kvm_gmem_release() won't nullify the file
> > pointer due to lack of a valid binding).
> > 
> > Opportunistically WARN and reject binding if KVM_MEMSLOT_GMEM_ONLY is
> > already set, partly to guard against goofs elsewhere, but mostly so that
> > KVM doesn't need to worry about clobbering flags when unwinding on failure.
> > 
> > Fixes: a7800aa80ea4 ("KVM: Add KVM_CREATE_GUEST_MEMFD ioctl() for guest-specific backing memory")
> > Cc: stable@xxxxxxxxxxxxxxx
> > Reported-by: Stefan Teodorescu <fane@xxxxxxxxxx>
> > Reported-by: Dennis Tighe <dtighe@xxxxxxxxxx>
> > Reported-by: Sashiko Bot <sashiko-bot@xxxxxxxxxx>
> > Closes: https://lore.kernel.org/all/20260823135031.4F6DC1F000E9%40smtp.kernel.org
> > Signed-off-by: Sean Christopherson <seanjc@xxxxxxxxxx>
> > ---
> >  virt/kvm/guest_memfd.c | 15 +++++++++++++--
> >  1 file changed, 13 insertions(+), 2 deletions(-)
> > 
> > diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
> > index b596486d184c..0b48e9a775aa 100644
> > --- a/virt/kvm/guest_memfd.c
> > +++ b/virt/kvm/guest_memfd.c
> > @@ -612,10 +612,14 @@ int kvm_gmem_bind(struct kvm *kvm, struct kvm_memory_slot *slot,
> >  	struct inode *inode;
> >  	struct file *file;
> >  	int r = -EINVAL;
> > +	void *xar;
> >  
> >  	BUILD_BUG_ON(sizeof(gpa_t) != sizeof(offset));
> >  	BUILD_BUG_ON(sizeof(gfn_t) != sizeof(slot->gmem.pgoff));
> >  
> > +	if (WARN_ON_ONCE(slot->flags & KVM_MEMSLOT_GMEM_ONLY))
> > +		return -EINVAL;
> > +
> >  	file = fget(fd);
> >  	if (!file)
> >  		return -EBADF;
> > @@ -654,7 +658,15 @@ int kvm_gmem_bind(struct kvm *kvm, struct kvm_memory_slot *slot,
> >  	if (kvm_gmem_supports_mmap(inode))
> >  		slot->flags |= KVM_MEMSLOT_GMEM_ONLY;
> >  
> > -	xa_store_range(&f->bindings, start, end - 1, slot, GFP_KERNEL);
> > +	xar = xa_store_range(&f->bindings, start, end - 1, slot, GFP_KERNEL);
> > +
> > +	r = xa_is_err(xar) ? xa_err(xar) : 0;
> 
> 
> r = xa_err(xar);
> 
> Should be sufficient, right?

Yes.  I didn't like relying on what I thought were internal xarray details, but
I missed that xa_err() itself checks xa_is_err().
 
> mm/memremap.c:pagemap_range() uses that and just avoids the intermediate xar
> value completely.
> 
> r = xa_err(xa_store_range(...);

Ya, it's ugly, but I do think it's less ugly than the intermediate xar.

	r = xa_err(xa_store_range(&f->bindings, start, end - 1, slot, GFP_KERNEL));