[PATCH v3 0/4] KVM: guest_memfd: Fix binding bugs

[PATCH v3 0/4] KVM: guest_memfd: Fix binding bugs

[Sean Christopherson]

Fix guest_memfd bugs related to binding to a memslot:

 - Handle errors when inserting into guest_memfd's binding xarray, e.g. to
   do the right thing on ENOMEM.

 - Bind a memslot only once the memslot is fully prepared (because it becomes
   reachable/visible once its inserted into gmem's bindings arraxy).

Patch 4 is a related cleanup to remove a superflous WRITE_ONCE() (unwinding
the slot update on insertion failure isn't an option if the slot is observable,
i.e. if the WRITE_ONCE() is actually necessary).

v3:
 - Nullify bindings on error before dropping invalidat lock. [Sashiko x3]
 
v2:
 - https://lore.kernel.org/all/20260902182020.2615443-1-seanjc@google.com
 - Fix the binding-too-early bug. [Sashiko]
 - Explicitly zero the bindings entry on failure to ensure there are no
   partial entries. [Sashiko]

v1: https://lore.kernel.org/all/20260826165154.766699-2-seanjc@google.com

Sean Christopherson (4):
  KVM: guest_memfd: Gracefully handle xarray errors when binding a
    memslot
  KVM: Use goto to handle errors during memslot preparation
  KVM: guest_memfd: Establish memslot<=>guest_memfd bindings *after*
    memslot is ready
  KVM: guest_memfd: Drop superfluous WRITE_ONCE() when binding a memslot

 virt/kvm/guest_memfd.c | 17 ++++++++++--
 virt/kvm/kvm_main.c    | 63 +++++++++++++++++++++++++-----------------
 2 files changed, 51 insertions(+), 29 deletions(-)


base-commit: 76671054f9a1ff6abb976583cd8da37650acdc97
-- 
2.55.0.979.g7e5102b832-goog

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

[Sean Christopherson]

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@vger.kernel.org
Reported-by: Stefan Teodorescu <fane@google.com>
Reported-by: Dennis Tighe <dtighe@google.com>
Reported-by: Sashiko Bot <sashiko-bot@kernel.org>
Closes: https://lore.kernel.org/all/20260823135031.4F6DC1F000E9%40smtp.kernel.org
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 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;
+	if (r) {
+		xa_store_range(&f->bindings, start, end - 1, NULL, GFP_KERNEL);
+		slot->gmem.file = NULL;
+		slot->gmem.pgoff = 0;
+		slot->flags &= ~KVM_MEMSLOT_GMEM_ONLY;
+	}
 	filemap_invalidate_unlock(inode->i_mapping);
 
 	/*
@@ -662,7 +674,6 @@ int kvm_gmem_bind(struct kvm *kvm, struct kvm_memory_slot *slot,
 	 * not the other way 'round.  Active bindings are invalidated if the
 	 * file is closed before memslots are destroyed.
 	 */
-	r = 0;
 err:
 	fput(file);
 	return r;
-- 
2.55.0.979.g7e5102b832-goog

[PATCH v3 2/4] KVM: Use goto to handle errors during memslot preparation

[Sean Christopherson]

Use a goto to unwind early memslot changes if preparing for a memslot
operation fails.  This will allow moving the creation of guest_memfd
bindings into kvm_set_memslot() without needing to copy+paste the unwind
logic.

No functional change intended.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 virt/kvm/kvm_main.c | 31 ++++++++++++++++---------------
 1 file changed, 16 insertions(+), 15 deletions(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 65eb26a0520d..3c0dbe60a5b4 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -1931,21 +1931,8 @@ static int kvm_set_memslot(struct kvm *kvm,
 	}
 
 	r = kvm_prepare_memory_region(kvm, old, new, change);
-	if (r) {
-		/*
-		 * For DELETE/MOVE, revert the above INVALID change.  No
-		 * modifications required since the original slot was preserved
-		 * in the inactive slots.  Changing the active memslots also
-		 * release slots_arch_lock.
-		 */
-		if (change == KVM_MR_DELETE || change == KVM_MR_MOVE) {
-			kvm_activate_memslot(kvm, invalid_slot, old);
-			kfree(invalid_slot);
-		} else {
-			mutex_unlock(&kvm->slots_arch_lock);
-		}
-		return r;
-	}
+	if (r)
+		goto err;
 
 	/*
 	 * For DELETE and MOVE, the working slot is now active as the INVALID
@@ -1977,6 +1964,20 @@ static int kvm_set_memslot(struct kvm *kvm,
 	kvm_commit_memory_region(kvm, old, new, change);
 
 	return 0;
+
+err:
+	/*
+	 * For DELETE/MOVE, revert the above INVALID change.  No modifications
+	 * required since the original slot was preserved in the inactive slots.
+	 * Changing the active memslots also release slots_arch_lock.
+	 */
+	if (change == KVM_MR_DELETE || change == KVM_MR_MOVE) {
+		kvm_activate_memslot(kvm, invalid_slot, old);
+		kfree(invalid_slot);
+	} else {
+		mutex_unlock(&kvm->slots_arch_lock);
+	}
+	return r;
 }
 
 static bool kvm_check_memslot_overlap(struct kvm_memslots *slots, int id,
-- 
2.55.0.979.g7e5102b832-goog

[PATCH v3 3/4] KVM: guest_memfd: Establish memslot<=>guest_memfd bindings *after* memslot is ready

[Sean Christopherson]

Wait to bind a memslot to a guest_memfd instance until *after* the memslot
is fully prepared, as creating the binding in guest_memfd will effectively
expose the memslot to readers.  As pointed out by Sashiko, binding the
memslot before it's ready to be exposed to the rest of the world can break
various memslot assumption and rules.  E.g. x86 could observe a NULL rmap
pointer if a PUNCH_HOLE hit the guest_memfd after the binding was created,
but before KVM made it through kvm_prepare_memory_region().

Begrudgingly resort to passing in the guest_memfd fd+offset pair to
kvm_set_memslot(), as creating the binding really does need to happen in
the middle of setting the new memslot.  Alternatively, to preserve the
aesthetically pleasing function prototype, "struct kvm_memory_slot" could
be expanded to track the fd and the file, but that would create the
possibility for TOCTOU bugs on the fd vs. file, and would add zero value
beyond making kvm_set_memslot() look pretty.

Fixes:a7800aa80ea4 ("KVM: Add KVM_CREATE_GUEST_MEMFD ioctl() for guest-specific backing memory")
Cc: stable@vger.kernel.org
Reported-by: Sashiko Bot <sashiko-bot@kernel.org>
Closes: https://lore.kernel.org/all/20260826170551.BEF801F000E9@smtp.kernel.org
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 virt/kvm/kvm_main.c | 34 ++++++++++++++++++++++------------
 1 file changed, 22 insertions(+), 12 deletions(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 3c0dbe60a5b4..21c10cbbac66 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -1887,7 +1887,8 @@ static void kvm_update_flags_memslot(struct kvm *kvm,
 static int kvm_set_memslot(struct kvm *kvm,
 			   struct kvm_memory_slot *old,
 			   struct kvm_memory_slot *new,
-			   enum kvm_mr_change change)
+			   enum kvm_mr_change change,
+			   unsigned int gmem_fd, uoff_t gmem_offset)
 {
 	struct kvm_memory_slot *invalid_slot;
 	int r;
@@ -1934,6 +1935,15 @@ static int kvm_set_memslot(struct kvm *kvm,
 	if (r)
 		goto err;
 
+	if (new && new->flags & KVM_MEM_GUEST_MEMFD) {
+		if (WARN_ON_ONCE(change != KVM_MR_CREATE))
+			goto err_bind;
+
+		r = kvm_gmem_bind(kvm, new, gmem_fd, gmem_offset);
+		if (r)
+			goto err_bind;
+	}
+
 	/*
 	 * For DELETE and MOVE, the working slot is now active as the INVALID
 	 * version of the old slot.  MOVE is particularly special as it reuses
@@ -1965,6 +1975,13 @@ static int kvm_set_memslot(struct kvm *kvm,
 
 	return 0;
 
+err_bind:
+	if (new) {
+		kvm_arch_free_memslot(kvm, new);
+
+		if (new->dirty_bitmap && (!old || !old->dirty_bitmap))
+			kvm_destroy_dirty_bitmap(new);
+	}
 err:
 	/*
 	 * For DELETE/MOVE, revert the above INVALID change.  No modifications
@@ -2059,7 +2076,7 @@ static int kvm_set_memory_region(struct kvm *kvm,
 		if (WARN_ON_ONCE(kvm->nr_memslot_pages < old->npages))
 			return -EIO;
 
-		return kvm_set_memslot(kvm, old, NULL, KVM_MR_DELETE);
+		return kvm_set_memslot(kvm, old, NULL, KVM_MR_DELETE, -1, 0);
 	}
 
 	base_gfn = (mem->guest_phys_addr >> PAGE_SHIFT);
@@ -2106,21 +2123,14 @@ static int kvm_set_memory_region(struct kvm *kvm,
 	new->npages = npages;
 	new->flags = mem->flags;
 	new->userspace_addr = mem->userspace_addr;
-	if (mem->flags & KVM_MEM_GUEST_MEMFD) {
-		r = kvm_gmem_bind(kvm, new, mem->guest_memfd, mem->guest_memfd_offset);
-		if (r)
-			goto out;
-	}
 
-	r = kvm_set_memslot(kvm, old, new, change);
+	r = kvm_set_memslot(kvm, old, new, change,
+			    mem->guest_memfd, mem->guest_memfd_offset);
 	if (r)
-		goto out_unbind;
+		goto out;
 
 	return 0;
 
-out_unbind:
-	if (mem->flags & KVM_MEM_GUEST_MEMFD)
-		kvm_gmem_unbind(new);
 out:
 	kfree(new);
 	return r;
-- 
2.55.0.979.g7e5102b832-goog

[PATCH v3 4/4] KVM: guest_memfd: Drop superfluous WRITE_ONCE() when binding a memslot

[Sean Christopherson]

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@intel.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 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 0b48e9a775aa..6c8df67382fc 100644
--- a/virt/kvm/guest_memfd.c
+++ b/virt/kvm/guest_memfd.c
@@ -653,7 +653,7 @@ int kvm_gmem_bind(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 = start;
 	if (kvm_gmem_supports_mmap(inode))
 		slot->flags |= KVM_MEMSLOT_GMEM_ONLY;
-- 
2.55.0.979.g7e5102b832-goog

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

[David Hildenbrand (Arm)]

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@vger.kernel.org
> Reported-by: Stefan Teodorescu <fane@google.com>
> Reported-by: Dennis Tighe <dtighe@google.com>
> Reported-by: Sashiko Bot <sashiko-bot@kernel.org>
> Closes: https://lore.kernel.org/all/20260823135031.4F6DC1F000E9%40smtp.kernel.org
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  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?

mm/memremap.c:pagemap_range() uses that and just avoids the intermediate xar
value completely.

r = xa_err(xa_store_range(...);

> +	if (r) {
> +		xa_store_range(&f->bindings, start, end - 1, NULL, GFP_KERNEL);
> +		slot->gmem.file = NULL;
> +		slot->gmem.pgoff = 0;
> +		slot->flags &= ~KVM_MEMSLOT_GMEM_ONLY;
> +	}
>  	filemap_invalidate_unlock(inode->i_mapping);

Apart from that

Reviewed-by: David Hildenbrand (Arm) <david@kernel.org>

-- 
Cheers,

David

Re: [PATCH v3 2/4] KVM: Use goto to handle errors during memslot preparation

[David Hildenbrand (Arm)]

On 9/4/26 02:43, Sean Christopherson wrote:
> Use a goto to unwind early memslot changes if preparing for a memslot
> operation fails.  This will allow moving the creation of guest_memfd
> bindings into kvm_set_memslot() without needing to copy+paste the unwind
> logic.
> 
> No functional change intended.
> 
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---

Reviewed-by: David Hildenbrand (Arm) <david@kernel.org>

-- 
Cheers,

David

Re: [PATCH v3 3/4] KVM: guest_memfd: Establish memslot<=>guest_memfd bindings *after* memslot is ready

[David Hildenbrand (Arm)]

On 9/4/26 02:43, Sean Christopherson wrote:
> Wait to bind a memslot to a guest_memfd instance until *after* the memslot
> is fully prepared, as creating the binding in guest_memfd will effectively
> expose the memslot to readers.  As pointed out by Sashiko, binding the
> memslot before it's ready to be exposed to the rest of the world can break
> various memslot assumption and rules.  E.g. x86 could observe a NULL rmap
> pointer if a PUNCH_HOLE hit the guest_memfd after the binding was created,
> but before KVM made it through kvm_prepare_memory_region().
> 
> Begrudgingly resort to passing in the guest_memfd fd+offset pair to
> kvm_set_memslot(), as creating the binding really does need to happen in
> the middle of setting the new memslot.  Alternatively, to preserve the
> aesthetically pleasing function prototype, "struct kvm_memory_slot" could
> be expanded to track the fd and the file, but that would create the
> possibility for TOCTOU bugs on the fd vs. file, and would add zero value
> beyond making kvm_set_memslot() look pretty.
> 
> Fixes:a7800aa80ea4 ("KVM: Add KVM_CREATE_GUEST_MEMFD ioctl() for guest-specific backing memory")
> Cc: stable@vger.kernel.org
> Reported-by: Sashiko Bot <sashiko-bot@kernel.org>
> Closes: https://lore.kernel.org/all/20260826170551.BEF801F000E9@smtp.kernel.org
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  virt/kvm/kvm_main.c | 34 ++++++++++++++++++++++------------
>  1 file changed, 22 insertions(+), 12 deletions(-)
> 
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 3c0dbe60a5b4..21c10cbbac66 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -1887,7 +1887,8 @@ static void kvm_update_flags_memslot(struct kvm *kvm,
>  static int kvm_set_memslot(struct kvm *kvm,
>  			   struct kvm_memory_slot *old,
>  			   struct kvm_memory_slot *new,
> -			   enum kvm_mr_change change)
> +			   enum kvm_mr_change change,
> +			   unsigned int gmem_fd, uoff_t gmem_offset)
>  {
>  	struct kvm_memory_slot *invalid_slot;
>  	int r;
> @@ -1934,6 +1935,15 @@ static int kvm_set_memslot(struct kvm *kvm,
>  	if (r)
>  		goto err;
>  
> +	if (new && new->flags & KVM_MEM_GUEST_MEMFD) {

For readability I'd throw in an extra pair of (). But KVM seems to use both
styles, so there is no clear preference when staring at the existing code :)

> +		if (WARN_ON_ONCE(change != KVM_MR_CREATE))
> +			goto err_bind;
> +
> +		r = kvm_gmem_bind(kvm, new, gmem_fd, gmem_offset);
> +		if (r)
> +			goto err_bind;
> +	}
> +
>  	/*
>  	 * For DELETE and MOVE, the working slot is now active as the INVALID
>  	 * version of the old slot.  MOVE is particularly special as it reuses
> @@ -1965,6 +1975,13 @@ static int kvm_set_memslot(struct kvm *kvm,
>  
>  	return 0;
>  
> +err_bind:
> +	if (new) {

We'd never end up here with !new, right?

> +		kvm_arch_free_memslot(kvm, new);
> +
> +		if (new->dirty_bitmap && (!old || !old->dirty_bitmap))
> +			kvm_destroy_dirty_bitmap(new);

That's essentially the cleanup path in kvm_prepare_memory_region().

I guess with some more reshuffling we could have a single dirty bitmap cleanup
path in this code.

> +	}
>  err:
>  	/*
>  	 * For DELETE/MOVE, revert the above INVALID change.  No modifications
> @@ -2059,7 +2076,7 @@ static int kvm_set_memory_region(struct kvm *kvm,
>  		if (WARN_ON_ONCE(kvm->nr_memslot_pages < old->npages))
>  			return -EIO;
>  
> -		return kvm_set_memslot(kvm, old, NULL, KVM_MR_DELETE);
> +		return kvm_set_memslot(kvm, old, NULL, KVM_MR_DELETE, -1, 0);
>  	}
>  
>  	base_gfn = (mem->guest_phys_addr >> PAGE_SHIFT);
> @@ -2106,21 +2123,14 @@ static int kvm_set_memory_region(struct kvm *kvm,
>  	new->npages = npages;
>  	new->flags = mem->flags;
>  	new->userspace_addr = mem->userspace_addr;
> -	if (mem->flags & KVM_MEM_GUEST_MEMFD) {
> -		r = kvm_gmem_bind(kvm, new, mem->guest_memfd, mem->guest_memfd_offset);
> -		if (r)
> -			goto out;
> -	}
>  
> -	r = kvm_set_memslot(kvm, old, new, change);
> +	r = kvm_set_memslot(kvm, old, new, change,
> +			    mem->guest_memfd, mem->guest_memfd_offset);
>  	if (r)
> -		goto out_unbind;
> +		goto out;
>  
>  	return 0;
>  
> -out_unbind:
> -	if (mem->flags & KVM_MEM_GUEST_MEMFD)
> -		kvm_gmem_unbind(new);
>  out:
>  	kfree(new);
>  	return r;

In general LGTM.

-- 
Cheers,

David

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

[David Hildenbrand (Arm)]

On 9/4/26 02:43, 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@intel.com>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---

Reviewed-by: David Hildenbrand (Arm) <david@kernel.org>

-- 
Cheers,

David

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@vger.kernel.org
> > Reported-by: Stefan Teodorescu <fane@google.com>
> > Reported-by: Dennis Tighe <dtighe@google.com>
> > Reported-by: Sashiko Bot <sashiko-bot@kernel.org>
> > Closes: https://lore.kernel.org/all/20260823135031.4F6DC1F000E9%40smtp.kernel.org
> > Signed-off-by: Sean Christopherson <seanjc@google.com>
> > ---
> >  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));

Re: [PATCH v3 3/4] KVM: guest_memfd: Establish memslot<=>guest_memfd bindings *after* memslot is ready

[Sean Christopherson]

On Mon, Sep 07, 2026, David Hildenbrand (Arm) wrote:
> On 9/4/26 02:43, Sean Christopherson wrote:
> 
> > +		if (WARN_ON_ONCE(change != KVM_MR_CREATE))
> > +			goto err_bind;

This is buggy, it fails to set 'r', i.e. will signal success but not actually do
anything (or worse, half-do something?).  I'm just going to delete this sanity
check, as there are already existing sanity checks that save KVM from the worst
case scenario.  More below.

> > +
> > +		r = kvm_gmem_bind(kvm, new, gmem_fd, gmem_offset);
> > +		if (r)
> > +			goto err_bind;
> > +	}
> > +
> >  	/*
> >  	 * For DELETE and MOVE, the working slot is now active as the INVALID
> >  	 * version of the old slot.  MOVE is particularly special as it reuses
> > @@ -1965,6 +1975,13 @@ static int kvm_set_memslot(struct kvm *kvm,
> >  
> >  	return 0;
> >  
> > +err_bind:
> > +	if (new) {
> 
> We'd never end up here with !new, right?

Correct.  I added the check on "new" partly because it felt so wrong to not have
such a check, but also to guard against any future usage of the unwinding.

Oof, but calling kvm_arch_free_memslot() is safe only for CREATE operations.  For
FLAGS_ONLY operations, x86 and PPC reuse arch metadata, i.e. trying to unwind
prepartion for FLAGS_ONLY would do more harm than good.

So rather than try to provide a goto sequence, I'll add a prep patch to restrict
the kvm_gmem_bind() call to CREATE (which is a nop because it's dead code for
MOVE and FLAGS_ONLY), and then this patch can do:

	if (change == KVM_MR_CREATE && (new->flags & KVM_MEM_GUEST_MEMFD)) {
		r = kvm_gmem_bind(kvm, new, gmem_fd, gmem_offset);
		if (r) {
			kvm_arch_free_memslot(kvm, new);
			kvm_destroy_dirty_bitmap(new);
			goto err;
		}
	}

That addresses the new-can't-be-NULL concern as well as the duplicate code concern,
and can also address the bad sanity check above by adjusting the TODO comment in
kvm_commit_memory_region() about what needs to happen if/when dirty logging is
supported (KVM needs to rebind() here, not do separate bind()+unbind() calls).

And of course calling kvm_destroy_dirty_bitmap() is dead code until dirty logging
of guest_memfd memslots is supported, but it's harmless and IMO far less risky than
hoping future us remembers to add the call when dirty logging support comes along.

> > +		kvm_arch_free_memslot(kvm, new);
> > +
> > +		if (new->dirty_bitmap && (!old || !old->dirty_bitmap))
> > +			kvm_destroy_dirty_bitmap(new);
> 
> That's essentially the cleanup path in kvm_prepare_memory_region().
> 
> I guess with some more reshuffling we could have a single dirty bitmap cleanup
> path in this code.

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

[Ackerley Tng]

"David Hildenbrand (Arm)" <david@kernel.org> writes:

>
> [...snip...]
>
>> -	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?
>
> mm/memremap.c:pagemap_range() uses that and just avoids the intermediate xar
> value completely.
>
> r = xa_err(xa_store_range(...);
>
>> +	if (r) {
>> +		xa_store_range(&f->bindings, start, end - 1, NULL, GFP_KERNEL);
>> +		slot->gmem.file = NULL;
>> +		slot->gmem.pgoff = 0;
>> +		slot->flags &= ~KVM_MEMSLOT_GMEM_ONLY;

Was wondering if the changelog should explain why not move
xa_store_range() before setting up these 3 fields that need undoing.

IIUC the reason is that other parts of gmem code expect any slots in
bindings to have a non-NULL gmem.file?

>> +	}
>>  	filemap_invalidate_unlock(inode->i_mapping);
>
> Apart from that
>
> Reviewed-by: David Hildenbrand (Arm) <david@kernel.org>
>

+1 on using xa_err() directly.

Reviewed-by: Ackerley Tng <ackerleytng@google.com>

> --
> Cheers,
>
> David

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

[Ackerley Tng]

Sean Christopherson <seanjc@google.com> writes:

> 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@intel.com>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  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 0b48e9a775aa..6c8df67382fc 100644
> --- a/virt/kvm/guest_memfd.c
> +++ b/virt/kvm/guest_memfd.c
> @@ -653,7 +653,7 @@ int kvm_gmem_bind(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 = start;
>  	if (kvm_gmem_supports_mmap(inode))
>  		slot->flags |= KVM_MEMSLOT_GMEM_ONLY;
> --
> 2.55.0.979.g7e5102b832-goog

Reviewed-by: Ackerley Tng <ackerleytng@google.com>

Re: [PATCH v3 2/4] KVM: Use goto to handle errors during memslot preparation

[Ackerley Tng]

Sean Christopherson <seanjc@google.com> writes:

> Use a goto to unwind early memslot changes if preparing for a memslot
> operation fails.  This will allow moving the creation of guest_memfd
> bindings into kvm_set_memslot() without needing to copy+paste the unwind
> logic.
>
> No functional change intended.
>
> Signed-off-by: Sean Christopherson <seanjc@google.com>

Reviewed-by: Ackerley Tng <ackerleytng@google.com>

>
> [...snip...]
>

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

[Sean Christopherson]

On Wed, Sep 09, 2026, Ackerley Tng wrote:
> "David Hildenbrand (Arm)" <david@kernel.org> writes:
> 
> >
> > [...snip...]
> >
> >> -	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?
> >
> > mm/memremap.c:pagemap_range() uses that and just avoids the intermediate xar
> > value completely.
> >
> > r = xa_err(xa_store_range(...);
> >
> >> +	if (r) {
> >> +		xa_store_range(&f->bindings, start, end - 1, NULL, GFP_KERNEL);
> >> +		slot->gmem.file = NULL;
> >> +		slot->gmem.pgoff = 0;
> >> +		slot->flags &= ~KVM_MEMSLOT_GMEM_ONLY;
> 
> Was wondering if the changelog should explain why not move
> xa_store_range() before setting up these 3 fields that need undoing.

Ya, I'll add some context.  The TL;DR is "look at patch 3".

> IIUC the reason is that other parts of gmem code expect any slots in
> bindings to have a non-NULL gmem.file?

Not just gmem code, all of KVM.  The instant the binding is created, the memslot
becomes reachable.  Because KVM manages memslots through SRCU-protected pointers,
for all intents and purposes memslots must be immutable if they are reachable,
otherwise readers could see half-baked state, e.g. a memslot with a gmem file but
the wrong pgoff.