[PATCH v5 0/6] KVM: guest_memfd: Fix binding bugs

[PATCH v5 0/6] KVM: guest_memfd: Fix binding bugs

[Sean Christopherson]

Sorry for the spam, I am hoping to get this applied before the in-place
conversion series, and I really want to get that series applied this week.
I finally gave up and split bind() into prepare()+commit(), and I actually
like the end result.  The only really ugly part is an extra #ifdef, but
otherwise gifting the gmem file reference back to kvm_set_memory_region()
avoids the TOCTOU issues and weird juggling that led me to initially reject
a prepare()+commit() solution.

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

v5:
 - Split bind() into prepare()+commit() so that the memslot has all the gmem
   goodies before calling kvm_arch_prepare_memory_region(). [Sashiko]

v4:
 - https://lore.kernel.org/all/20260921210616.1024168-1-seanjc@google.com
 - Drop intermediate xar variable. [David, Ackerley]
 - Collect reviews. [David, Ackerley]
 - Restrict kvm_gmem_bind() to CREATE as calling kvm_arch_free_memslot() on
   FLAGS_ONLY changes is unsafe, and doing the right thing for dirty bitmaps
   is tricky for similar reasons.  As a bonus, this eliminates the ugly
   almost-duplicate code that David pointed out.
 - Explain why KVM must deal with the unwind during bind(). [Ackerley]

v3:
 - https://lore.kernel.org/all/20260904004342.3162959-1-seanjc@google.com
 - 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 (6):
  KVM: guest_memfd: Gracefully handle xarray errors when binding a
    memslot
  KVM: Use goto to handle errors during memslot preparation
  KVM: Only bind memslot to guest_memfd instance for CREATE operations
  KVM: guest_memfd: Split bind() into prepare()+commit() phases
  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 | 68 ++++++++++++++++++++++++++----------------
 virt/kvm/guest_memfd.h | 19 ++++++++----
 virt/kvm/kvm_main.c    | 65 ++++++++++++++++++++++++++--------------
 3 files changed, 99 insertions(+), 53 deletions(-)


base-commit: 70c944caf570fda2d79baa71435589a8db39f048
-- 
2.55.0.1082.g2b9226bbc0-goog

[PATCH v5 1/6] 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.

Regarding the unwind, the slot must be fully prepared before inserting it
into the bindings, at which point the slot becomes reachable.  I.e. waiting
to update the slot in order to avoid the ugly unwind isn't an option.  And
as part of the unwind, explicitly nullify the relevant bindings, as xarray
can store a subset of entries when populating a range.

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
Reviewed-by: David Hildenbrand (Arm) <david@kernel.org>
Reviewed-by: Ackerley Tng <ackerleytng@google.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 virt/kvm/guest_memfd.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
index 63943aa253d4..c094611f7c7a 100644
--- a/virt/kvm/guest_memfd.c
+++ b/virt/kvm/guest_memfd.c
@@ -654,6 +654,9 @@ int kvm_gmem_bind(struct kvm *kvm, struct kvm_memory_slot *slot,
 	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;
@@ -692,7 +695,13 @@ 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);
+	r = xa_err(xa_store_range(&f->bindings, start, end - 1, slot, GFP_KERNEL));
+	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);
 
 	/*
@@ -700,7 +709,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.1082.g2b9226bbc0-goog

[PATCH v5 2/6] 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.

Cc: stable@vger.kernel.org
Reviewed-by: David Hildenbrand (Arm) <david@kernel.org>
Reviewed-by: Ackerley Tng <ackerleytng@google.com>
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 d9da8b51614a..24cf96840827 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -1941,21 +1941,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
@@ -1987,6 +1974,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.1082.g2b9226bbc0-goog

[PATCH v5 3/6] KVM: Only bind memslot to guest_memfd instance for CREATE operations

[Sean Christopherson]

For additional defense-in-depth, and to avoid having to handle impossible
unwind scenarios when binding to a memslot fails, bind a memslot to a gmem
instance only when for CREATE operations, i.e. don't attempt to establish a
binding for MOVE and FLAGS_ONLY operations.  And when FLAGS_ONLY operations
are eventually supported (this is currently all dead code), creating a new
binding would be incorrect; KVM instead needs to do a 1:1 replacement of
the existing binding, i.e. FLAGS_ONLY will need its own dedicated handling.

Update the relevant TODO to make a better guess as to what needs to be done
to support toggling dirty logging for guest_memfd memslots.

Because it's dead code, no functional change intended.

Cc: stable@vger.kernel.org
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 virt/kvm/kvm_main.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 24cf96840827..ccfd5f5102a5 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -1752,10 +1752,10 @@ static void kvm_commit_memory_region(struct kvm *kvm,
 			kvm_destroy_dirty_bitmap(old);
 
 		/*
-		 * Unbind the guest_memfd instance as needed; the @new slot has
-		 * already created its own binding.  TODO: Drop the WARN when
-		 * dirty logging guest_memfd memslots is supported.  Until then,
-		 * flags-only changes on guest_memfd slots should be impossible.
+		 * TODO: Drop the WARN and do the unbind() call only for MOVE
+		 * when dirty logging guest_memfd memslots is supported.  Until
+		 * then, flags-only changes on guest_memfd slots should also be
+		 * impossible; unbind the old memslot for defense-in-depth.
 		 */
 		if (WARN_ON_ONCE(old->flags & KVM_MEM_GUEST_MEMFD))
 			kvm_gmem_unbind(old);
@@ -2116,7 +2116,7 @@ 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) {
+	if (change == KVM_MR_CREATE && (mem->flags & KVM_MEM_GUEST_MEMFD)) {
 		r = kvm_gmem_bind(kvm, new, mem->guest_memfd, mem->guest_memfd_offset);
 		if (r)
 			goto out;
@@ -2129,7 +2129,7 @@ static int kvm_set_memory_region(struct kvm *kvm,
 	return 0;
 
 out_unbind:
-	if (mem->flags & KVM_MEM_GUEST_MEMFD)
+	if (change == KVM_MR_CREATE && (mem->flags & KVM_MEM_GUEST_MEMFD))
 		kvm_gmem_unbind(new);
 out:
 	kfree(new);
-- 
2.55.0.1082.g2b9226bbc0-goog

Re: [PATCH v5 3/6] KVM: Only bind memslot to guest_memfd instance for CREATE operations

[David Hildenbrand (Arm)]

On 9/22/26 02:13, Sean Christopherson wrote:
> For additional defense-in-depth, and to avoid having to handle impossible
> unwind scenarios when binding to a memslot fails, bind a memslot to a gmem
> instance only when for CREATE operations, i.e. don't attempt to establish a

s/when for/for/

> binding for MOVE and FLAGS_ONLY operations.  And when FLAGS_ONLY operations
> are eventually supported (this is currently all dead code), creating a new
> binding would be incorrect; KVM instead needs to do a 1:1 replacement of
> the existing binding, i.e. FLAGS_ONLY will need its own dedicated handling.
> 
> Update the relevant TODO to make a better guess as to what needs to be done
> to support toggling dirty logging for guest_memfd memslots.
> 
> Because it's dead code, no functional change intended.
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---

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

-- 
Cheers,

David

[PATCH v5 4/6] KVM: guest_memfd: Split bind() into prepare()+commit() phases

[Sean Christopherson]

Split binding a memslot to a guest_memfd instance into prepare() and
commit() phases so that KVM can separate preparing the memslot from binding
the memslot to the gmem instance, i.e. from committing the memslot.  This
will allow waiting to commit the memslot+gmem binding until the memslot is
fully prepared, which is necessary as the memslot becomes reachable when
the binding is created.

As a bonus, drop the unwind-on-failure from the commit phase (other than
nullifying the bindings), as the only reason bind() did the full unwind is
because it technically didn't own the memslot, i.e. "needed" to leave
memslot in the same state it started in.

No functional change intended (the unwinding down on bind() failure was
effectively dead code since KVM simply deletes the memslot on failure,
i.e. there was nothing that could actually observe the unwind).

Cc: stable@vger.kernel.org
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 virt/kvm/guest_memfd.c | 68 ++++++++++++++++++++++++------------------
 virt/kvm/guest_memfd.h | 19 ++++++++----
 virt/kvm/kvm_main.c    | 18 ++++++++++-
 3 files changed, 70 insertions(+), 35 deletions(-)

diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
index c094611f7c7a..80932f4ec4a3 100644
--- a/virt/kvm/guest_memfd.c
+++ b/virt/kvm/guest_memfd.c
@@ -641,15 +641,14 @@ int kvm_gmem_create(struct kvm *kvm, struct kvm_create_guest_memfd *args)
 	return __kvm_gmem_create(kvm, size, flags);
 }
 
-int kvm_gmem_bind(struct kvm *kvm, struct kvm_memory_slot *slot,
-		  unsigned int fd, uoff_t offset)
+int kvm_gmem_prepare_memory_region(struct kvm *kvm, struct kvm_memory_slot *slot,
+				   unsigned int fd, uoff_t offset)
 {
 	uoff_t size = slot->npages << PAGE_SHIFT;
-	unsigned long start, end;
 	struct gmem_file *f;
 	struct inode *inode;
 	struct file *file;
-	int r = -EINVAL;
+
 
 	BUILD_BUG_ON(sizeof(gpa_t) != sizeof(offset));
 	BUILD_BUG_ON(sizeof(gfn_t) != sizeof(slot->gmem.pgoff));
@@ -673,44 +672,55 @@ int kvm_gmem_bind(struct kvm *kvm, struct kvm_memory_slot *slot,
 	if (!PAGE_ALIGNED(offset) || offset + size > i_size_read(inode))
 		goto err;
 
-	filemap_invalidate_lock(inode->i_mapping);
-
-	start = offset >> PAGE_SHIFT;
-	end = start + slot->npages;
-
-	if (!xa_empty(&f->bindings) &&
-	    xa_find(&f->bindings, &start, end - 1, XA_PRESENT)) {
-		r = -EEXIST;
-		filemap_invalidate_unlock(inode->i_mapping);
-		goto err;
-	}
-
 	/*
 	 * memslots of flag KVM_MEM_GUEST_MEMFD are immutable to change, so
 	 * 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.pgoff = start;
+	slot->gmem.pgoff = offset >> PAGE_SHIFT;
 	if (kvm_gmem_supports_mmap(inode))
 		slot->flags |= KVM_MEMSLOT_GMEM_ONLY;
 
-	r = xa_err(xa_store_range(&f->bindings, start, end - 1, slot, GFP_KERNEL));
-	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);
-
 	/*
-	 * Drop the reference to the file, even on success.  The file pins KVM,
-	 * not the other way 'round.  Active bindings are invalidated if the
-	 * file is closed before memslots are destroyed.
+	 * Gift the caller a reference to the file.  The reference will be
+	 * dropped after bindings are established, or if installing the new
+	 * memslot ultimately fails.
 	 */
+	return 0;
+
 err:
 	fput(file);
+	return -EINVAL;
+}
+
+int kvm_gmem_commit_memory_region(struct kvm *kvm, struct kvm_memory_slot *slot)
+{
+	struct gmem_file *f = slot->gmem.file->private_data;
+	struct inode *inode = file_inode(slot->gmem.file);
+	unsigned long start, end;
+	int r;
+
+	if (WARN_ON_ONCE(slot->gmem.file->f_op != &kvm_gmem_fops))
+		return -EIO;
+
+	filemap_invalidate_lock(inode->i_mapping);
+
+	start = slot->gmem.pgoff;
+	end = start + slot->npages;
+
+	if (!xa_empty(&f->bindings) &&
+	    xa_find(&f->bindings, &start, end - 1, XA_PRESENT)) {
+		filemap_invalidate_unlock(inode->i_mapping);
+		return -EEXIST;
+	}
+
+	r = xa_err(xa_store_range(&f->bindings, start, end - 1, slot, GFP_KERNEL));
+	if (r)
+		xa_store_range(&f->bindings, start, end - 1, NULL, GFP_KERNEL);
+
+	filemap_invalidate_unlock(inode->i_mapping);
+
 	return r;
 }
 
diff --git a/virt/kvm/guest_memfd.h b/virt/kvm/guest_memfd.h
index 0f9c6f840838..01bd359d27e3 100644
--- a/virt/kvm/guest_memfd.h
+++ b/virt/kvm/guest_memfd.h
@@ -8,8 +8,9 @@
 int kvm_gmem_init(struct module *module);
 void kvm_gmem_exit(void);
 int kvm_gmem_create(struct kvm *kvm, struct kvm_create_guest_memfd *args);
-int kvm_gmem_bind(struct kvm *kvm, struct kvm_memory_slot *slot,
-		  unsigned int fd, uoff_t offset);
+int kvm_gmem_prepare_memory_region(struct kvm *kvm, struct kvm_memory_slot *slot,
+				   unsigned int fd, uoff_t offset);
+int kvm_gmem_commit_memory_region(struct kvm *kvm, struct kvm_memory_slot *slot);
 void kvm_gmem_unbind(struct kvm_memory_slot *slot);
 #else
 static inline int kvm_gmem_init(struct module *module)
@@ -17,9 +18,17 @@ static inline int kvm_gmem_init(struct module *module)
 	return 0;
 }
 static inline void kvm_gmem_exit(void) {};
-static inline int kvm_gmem_bind(struct kvm *kvm,
-					 struct kvm_memory_slot *slot,
-					 unsigned int fd, uoff_t offset)
+
+static inline int kvm_gmem_prepare_memory_region(struct kvm *kvm,
+						 struct kvm_memory_slot *slot,
+						 unsigned int fd, uoff_t offset)
+{
+	WARN_ON_ONCE(1);
+	return -EIO;
+}
+
+static inline int kvm_gmem_commit_memory_region(struct kvm *kvm,
+						struct kvm_memory_slot *slot)
 {
 	WARN_ON_ONCE(1);
 	return -EIO;
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index ccfd5f5102a5..45b509f4e54b 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -2117,7 +2117,23 @@ static int kvm_set_memory_region(struct kvm *kvm,
 	new->flags = mem->flags;
 	new->userspace_addr = mem->userspace_addr;
 	if (change == KVM_MR_CREATE && (mem->flags & KVM_MEM_GUEST_MEMFD)) {
-		r = kvm_gmem_bind(kvm, new, mem->guest_memfd, mem->guest_memfd_offset);
+		r = kvm_gmem_prepare_memory_region(kvm, new, mem->guest_memfd,
+						   mem->guest_memfd_offset);
+		if (r)
+			goto out;
+
+		r = kvm_gmem_commit_memory_region(kvm, new);
+
+		/*
+		 * Drop the reference to the file, even on success.  The file
+		 * pins KVM, not the other way 'round.  Active bindings are
+		 * invalidated if the file is closed before memslots are
+		 * destroyed.
+		 */
+#ifdef CONFIG_KVM_GUEST_MEMFD
+		 fput(new->gmem.file);
+#endif
+
 		if (r)
 			goto out;
 	}
-- 
2.55.0.1082.g2b9226bbc0-goog

Re: [PATCH v5 4/6] KVM: guest_memfd: Split bind() into prepare()+commit() phases

[David Hildenbrand (Arm)]

On 9/22/26 02:13, Sean Christopherson wrote:
> Split binding a memslot to a guest_memfd instance into prepare() and
> commit() phases so that KVM can separate preparing the memslot from binding
> the memslot to the gmem instance, i.e. from committing the memslot.  This
> will allow waiting to commit the memslot+gmem binding until the memslot is
> fully prepared, which is necessary as the memslot becomes reachable when
> the binding is created.
> 
> As a bonus, drop the unwind-on-failure from the commit phase (other than
> nullifying the bindings), as the only reason bind() did the full unwind is
> because it technically didn't own the memslot, i.e. "needed" to leave
> memslot in the same state it started in.
> 
> No functional change intended (the unwinding down on bind() failure was
> effectively dead code since KVM simply deletes the memslot on failure,
> i.e. there was nothing that could actually observe the unwind).
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---

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

-- 
Cheers,

David

Re: [PATCH v5 4/6] KVM: guest_memfd: Split bind() into prepare()+commit() phases

[Ackerley Tng]

Sean Christopherson <seanjc@google.com> writes:

> Split binding a memslot to a guest_memfd instance into prepare() and
> commit() phases so that KVM can separate preparing the memslot from binding
> the memslot to the gmem instance, i.e. from committing the memslot.  This
> will allow waiting to commit the memslot+gmem binding until the memslot is
> fully prepared, which is necessary as the memslot becomes reachable when
> the binding is created.
>
> As a bonus, drop the unwind-on-failure from the commit phase (other than
> nullifying the bindings), as the only reason bind() did the full unwind is
> because it technically didn't own the memslot, i.e. "needed" to leave
> memslot in the same state it started in.
>
> No functional change intended (the unwinding down on bind() failure was
> effectively dead code since KVM simply deletes the memslot on failure,
> i.e. there was nothing that could actually observe the unwind).
>
> Cc: stable@vger.kernel.org
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  virt/kvm/guest_memfd.c | 68 ++++++++++++++++++++++++------------------
>  virt/kvm/guest_memfd.h | 19 ++++++++----
>  virt/kvm/kvm_main.c    | 18 ++++++++++-
>  3 files changed, 70 insertions(+), 35 deletions(-)
>
> diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
> index c094611f7c7a..80932f4ec4a3 100644
> --- a/virt/kvm/guest_memfd.c
> +++ b/virt/kvm/guest_memfd.c
> @@ -641,15 +641,14 @@ int kvm_gmem_create(struct kvm *kvm, struct kvm_create_guest_memfd *args)
>  	return __kvm_gmem_create(kvm, size, flags);
>  }
>
> -int kvm_gmem_bind(struct kvm *kvm, struct kvm_memory_slot *slot,
> -		  unsigned int fd, uoff_t offset)
> +int kvm_gmem_prepare_memory_region(struct kvm *kvm, struct kvm_memory_slot *slot,
> +				   unsigned int fd, uoff_t offset)
>  {
>  	uoff_t size = slot->npages << PAGE_SHIFT;
> -	unsigned long start, end;
>  	struct gmem_file *f;
>  	struct inode *inode;
>  	struct file *file;
> -	int r = -EINVAL;
> +

An extra empty line was added here.

>
>  	BUILD_BUG_ON(sizeof(gpa_t) != sizeof(offset));
>  	BUILD_BUG_ON(sizeof(gfn_t) != sizeof(slot->gmem.pgoff));
> @@ -673,44 +672,55 @@ int kvm_gmem_bind(struct kvm *kvm, struct kvm_memory_slot *slot,
>  	if (!PAGE_ALIGNED(offset) || offset + size > i_size_read(inode))
>  		goto err;
>
> -	filemap_invalidate_lock(inode->i_mapping);
> -
> -	start = offset >> PAGE_SHIFT;
> -	end = start + slot->npages;
> -
> -	if (!xa_empty(&f->bindings) &&
> -	    xa_find(&f->bindings, &start, end - 1, XA_PRESENT)) {
> -		r = -EEXIST;
> -		filemap_invalidate_unlock(inode->i_mapping);
> -		goto err;
> -	}
> -
>  	/*
>  	 * memslots of flag KVM_MEM_GUEST_MEMFD are immutable to change, so
>  	 * 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.pgoff = start;
> +	slot->gmem.pgoff = offset >> PAGE_SHIFT;
>  	if (kvm_gmem_supports_mmap(inode))
>  		slot->flags |= KVM_MEMSLOT_GMEM_ONLY;
>
> -	r = xa_err(xa_store_range(&f->bindings, start, end - 1, slot, GFP_KERNEL));
> -	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);
> -
>  	/*
> -	 * Drop the reference to the file, even on success.  The file pins KVM,
> -	 * not the other way 'round.  Active bindings are invalidated if the
> -	 * file is closed before memslots are destroyed.
> +	 * Gift the caller a reference to the file.  The reference will be
> +	 * dropped after bindings are established, or if installing the new
> +	 * memslot ultimately fails.
>  	 */

prepare+commit is nice :)

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


How about this instead:

1. Move the fget() in kvm_gmem_prepare_memory_region() into
   kvm_set_memory_region()
2. Move the comment about not taking a refcount into either prepare or
   commit (I think commit is better). Both prepare and commit are
   supposed to be called with a stable file. I think commit is the stage
   where people usually expect a reference transfer, but in this case
   there isn't a reference transfer. Perhaps like:

	/*
	 * No reference is taken on the file, because when the gmem file was
	 * created, it already pins KVM. Active bindings are invalidated if the
	 * file is closed before memslots are destroyed.
	 */

3. Always fput(file) if fget() happened in kvm_set_memory_region().

The gifting seems a bit asymmetric to me. For gmem bindings, always
getting a ref on the file and always dropping would be more
symmetric.

Now that there is a commit stage, explicitly not taking a ref
on the file in the committing stage is clearer imo.

If you'd like to stack the above refactoring as a patch after [5/6],
here's my suggestion:


From 64bc0e119ba87c3642147f2bbb97f98636ffb23c Mon Sep 17 00:00:00 2001
From: Ackerley Tng <ackerleytng@google.com>
Date: Thu, 24 Sep 2026 09:08:51 -0700
Subject: [PATCH] KVM: guest_memfd: Hold file reference across memslot
 preparation and commit

Refactor guest_memfd's prepare + commit functions so that the caller is
required to hold a refcounted file pointer throughout prepare and commit.

This allows symmetrically acquiring and releasing the file reference in
the caller without having to gift file references across helper
boundaries.

Document (actually move documentation) that guest_memfd bindings should
not pin the backing file to the commit stage, where a new reference is
usually expected.

No functional change intended.

Signed-off-by: Ackerley Tng <ackerleytng@google.com>
---
 virt/kvm/guest_memfd.c | 28 +++++++++-------------------
 virt/kvm/guest_memfd.h |  4 ++--
 virt/kvm/kvm_main.c    | 29 ++++++++++++-----------------
 3 files changed, 23 insertions(+), 38 deletions(-)

diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
index 826d26036926a..4cd1425d6fc38 100644
--- a/virt/kvm/guest_memfd.c
+++ b/virt/kvm/guest_memfd.c
@@ -642,13 +642,11 @@ int kvm_gmem_create(struct kvm *kvm, struct
kvm_create_guest_memfd *args)
 }

 int kvm_gmem_prepare_memory_region(struct kvm *kvm, struct
kvm_memory_slot *slot,
-				   unsigned int fd, uoff_t offset)
+				   struct file *file, uoff_t offset)
 {
 	uoff_t size = slot->npages << PAGE_SHIFT;
 	struct gmem_file *f;
 	struct inode *inode;
-	struct file *file;
-

 	BUILD_BUG_ON(sizeof(gpa_t) != sizeof(offset));
 	BUILD_BUG_ON(sizeof(gfn_t) != sizeof(slot->gmem.pgoff));
@@ -656,21 +654,17 @@ int kvm_gmem_prepare_memory_region(struct kvm
*kvm, struct kvm_memory_slot *slot
 	if (WARN_ON_ONCE(slot->flags & KVM_MEMSLOT_GMEM_ONLY))
 		return -EINVAL;

-	file = fget(fd);
-	if (!file)
-		return -EBADF;
-
 	if (file->f_op != &kvm_gmem_fops)
-		goto err;
+		return -EINVAL;

 	f = file->private_data;
 	if (f->kvm != kvm)
-		goto err;
+		return -EINVAL;

 	inode = file_inode(file);

 	if (!PAGE_ALIGNED(offset) || offset + size > i_size_read(inode))
-		goto err;
+		return -EINVAL;

 	/*
 	 * memslots of flag KVM_MEM_GUEST_MEMFD are immutable to change, so
@@ -682,16 +676,7 @@ int kvm_gmem_prepare_memory_region(struct kvm
*kvm, struct kvm_memory_slot *slot
 	if (kvm_gmem_supports_mmap(inode))
 		slot->flags |= KVM_MEMSLOT_GMEM_ONLY;

-	/*
-	 * Gift the caller a reference to the file.  The reference will be
-	 * dropped after bindings are established, or if installing the new
-	 * memslot ultimately fails.
-	 */
 	return 0;
-
-err:
-	fput(file);
-	return -EINVAL;
 }

 int kvm_gmem_commit_memory_region(struct kvm *kvm, struct
kvm_memory_slot *slot)
@@ -715,6 +700,11 @@ int kvm_gmem_commit_memory_region(struct kvm
*kvm, struct kvm_memory_slot *slot)
 		return -EEXIST;
 	}

+	/*
+	 * No reference is taken on the file, because when the gmem file was
+	 * created, it already pins KVM. Active bindings are invalidated if the
+	 * file is closed before memslots are destroyed.
+	 */
 	r = xa_err(xa_store_range(&f->bindings, start, end - 1, slot, GFP_KERNEL));
 	if (r)
 		xa_store_range(&f->bindings, start, end - 1, NULL, GFP_KERNEL);
diff --git a/virt/kvm/guest_memfd.h b/virt/kvm/guest_memfd.h
index 01bd359d27e30..7e9d12e4531ff 100644
--- a/virt/kvm/guest_memfd.h
+++ b/virt/kvm/guest_memfd.h
@@ -9,7 +9,7 @@ int kvm_gmem_init(struct module *module);
 void kvm_gmem_exit(void);
 int kvm_gmem_create(struct kvm *kvm, struct kvm_create_guest_memfd *args);
 int kvm_gmem_prepare_memory_region(struct kvm *kvm, struct
kvm_memory_slot *slot,
-				   unsigned int fd, uoff_t offset);
+				   struct file *file, uoff_t offset);
 int kvm_gmem_commit_memory_region(struct kvm *kvm, struct
kvm_memory_slot *slot);
 void kvm_gmem_unbind(struct kvm_memory_slot *slot);
 #else
@@ -21,7 +21,7 @@ static inline void kvm_gmem_exit(void) {};

 static inline int kvm_gmem_prepare_memory_region(struct kvm *kvm,
 						 struct kvm_memory_slot *slot,
-						 unsigned int fd, uoff_t offset)
+						 struct file *file, uoff_t offset)
 {
 	WARN_ON_ONCE(1);
 	return -EIO;
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 90461880ff854..e783d0ee4cf9d 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -2019,6 +2019,7 @@ static int kvm_set_memory_region(struct kvm *kvm,
 	struct kvm_memslots *slots;
 	enum kvm_mr_change change;
 	unsigned long npages;
+	struct file *file = NULL;
 	gfn_t base_gfn;
 	int as_id, id;
 	int r;
@@ -2126,7 +2127,13 @@ static int kvm_set_memory_region(struct kvm *kvm,
 	new->flags = mem->flags;
 	new->userspace_addr = mem->userspace_addr;
 	if (change == KVM_MR_CREATE && (mem->flags & KVM_MEM_GUEST_MEMFD)) {
-		r = kvm_gmem_prepare_memory_region(kvm, new, mem->guest_memfd,
+		file = fget(mem->guest_memfd);
+		if (!file) {
+			r = -EBADF;
+			goto out;
+		}
+
+		r = kvm_gmem_prepare_memory_region(kvm, new, file,
 						   mem->guest_memfd_offset);
 		if (r)
 			goto out;
@@ -2134,23 +2141,11 @@ static int kvm_set_memory_region(struct kvm *kvm,

 	r = kvm_set_memslot(kvm, old, new, change);

-	/*
-	 * Drop the reference to the gmem file, even on success.  The file pins
-	 * KVM, not the other way 'round.  Active bindings are invalidated if
-	 * the file is closed before memslots are destroyed.
-	 */
-#ifdef CONFIG_KVM_GUEST_MEMFD
-	if (change == KVM_MR_CREATE && (mem->flags & KVM_MEM_GUEST_MEMFD))
-		fput(new->gmem.file);
-#endif
-
-	if (r)
-		goto out;
-
-	return 0;
-
 out:
-	kfree(new);
+	if (file)
+		fput(file);
+	if (r)
+		kfree(new);
 	return r;
 }

-- 
2.56.0.rc1.310.g51773c2048-goog

> +	return 0;
> +
>  err:
>  	fput(file);
> +	return -EINVAL;
> +}
> +

If the refactor above is not preferred,

I think this part at the end

	if (r)
		goto out;

	return 0;

out:
	kfree(new);
	return r;


Could be something like

out:
	if (r)
		kfree(new);

	return r;


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

Re: [PATCH v5 4/6] KVM: guest_memfd: Split bind() into prepare()+commit() phases

[Sean Christopherson]

On Thu, Sep 24, 2026, Ackerley Tng wrote:
> Sean Christopherson <seanjc@google.com> writes:
> The gifting seems a bit asymmetric to me. For gmem bindings, always getting a
> ref on the file and always dropping would be more symmetric.

It's definitely asymmetric, but lack of symmetry isn't inherently bad.  I do
agree that the code isn't the prettiest, but for me the asymmetry itself doesn't
really bother me.
 
> Now that there is a commit stage, explicitly not taking a ref
> on the file in the committing stage is clearer imo.
> 
> If you'd like to stack the above refactoring as a patch after [5/6],
> here's my suggestion:

...

> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 90461880ff854..e783d0ee4cf9d 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -2019,6 +2019,7 @@ static int kvm_set_memory_region(struct kvm *kvm,
>  	struct kvm_memslots *slots;
>  	enum kvm_mr_change change;
>  	unsigned long npages;
> +	struct file *file = NULL;
>  	gfn_t base_gfn;
>  	int as_id, id;
>  	int r;
> @@ -2126,7 +2127,13 @@ static int kvm_set_memory_region(struct kvm *kvm,
>  	new->flags = mem->flags;
>  	new->userspace_addr = mem->userspace_addr;
>  	if (change == KVM_MR_CREATE && (mem->flags & KVM_MEM_GUEST_MEMFD)) {
> -		r = kvm_gmem_prepare_memory_region(kvm, new, mem->guest_memfd,
> +		file = fget(mem->guest_memfd);
> +		if (!file) {
> +			r = -EBADF;
> +			goto out;
> +		}
> +
> +		r = kvm_gmem_prepare_memory_region(kvm, new, file,
>  						   mem->guest_memfd_offset);

I agree with pretty much all of your points, and I actually like many aspects of
this, but I don't love passing in the file pointer.  I don't hate it either, it
just feels too much like the responsibilities of preparing the gmem aspects of
the memslot are being split between core KVM and guest_memfd.

E.g. if kvm_set_memory_region() grabs the file pointer, then it's weird not having
it also set slot->gmem.file.  But then if we set slot->gmem.file, not setting
slot->gmem.pgoff feels weird.  I also don't like that it becomes difficult to see
that the file pointer is getting stashed in the memslot, e.g. begs the question of
"what are we doing with this file?".

Huh.  Ok.  I *was* going to say that my vote would be to make the "gift" more
explicit, by returning the file, to avoid splitting responsibilities while making
the code more obvious and less ugly, e.g.:

	if (mem->flags & KVM_MEM_GUEST_MEMFD) {
		gmem_file = kvm_gmem_prepare_memory_region(kvm, new, change,
							   mem->guest_memfd,
							   mem->guest_memfd_offset);
		if (IS_ERR(gmem_file)) {
			r = PTR_ERR(gmem_file);
			goto out;
		}
	} else {
		gmem_file = NULL;
	}

	r = kvm_set_memslot(kvm, old, new, change);

	/*
	 * Drop the reference to the gmem file, even on success.  The file pins
	 * KVM, not the other way 'round.  Active bindings are invalidated if
	 * the file is closed before memslots are destroyed.
	 */
	if (gmem_file)
		fput(gmem_file);

But after fiddling with this for an hour or so, I realized that if we fully commit
to configuring new.gmem in kvm_set_memory_region(), then we can move the prepare()
call into kvm_prepare_memory_region() without needing to plumb extra parameters,
and make the gmem stuff look a lot more like the rest of the memslot code.  E.g.

	if (mem->flags & KVM_MEM_GUEST_MEMFD) {
#ifdef CONFIG_KVM_GUEST_MEMFD
		new->gmem.file = fget(mem->guest_memfd);
		if (!new->gmem.file) {
			r = -EBADF;
			goto out;
		}

		new->gmem.pgoff = mem->guest_memfd_offset >> PAGE_SHIFT;
#endif
	}

	r = kvm_set_memslot(kvm, old, new, change);

	/*
	 * Drop the reference to the gmem file, even on success.  The file pins
	 * KVM, not the other way 'round.  Active bindings are invalidated if
	 * the file is closed before memslots are destroyed.
	 */
#ifdef CONFIG_KVM_GUEST_MEMFD
	if (new->gmem.file)
		fput(new->gmem.file);
#endif

Not being able to get rid of the #ifdefs is a shame, but from a control flow
perspective, this feels more right than anything else.  Full diff (not fully
tested, and would need to be split into 3+ patches):

diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
index a13445c26d9d..d1e502d3246b 100644
--- a/virt/kvm/guest_memfd.c
+++ b/virt/kvm/guest_memfd.c
@@ -1003,73 +1003,61 @@ int kvm_gmem_create(struct kvm *kvm, struct kvm_create_guest_memfd *args)
 	return __kvm_gmem_create(kvm, size, flags);
 }
 
-int kvm_gmem_prepare_memory_region(struct kvm *kvm, struct kvm_memory_slot *slot,
-				   unsigned int fd, uoff_t offset)
+int kvm_gmem_prepare_memory_region(struct kvm *kvm,
+				   const struct kvm_memory_slot *old,
+				   struct kvm_memory_slot *new,
+				   enum kvm_mr_change change)
 {
-	uoff_t size = slot->npages << PAGE_SHIFT;
 	struct gmem_file *f;
 	struct inode *inode;
-	struct file *file;
 
+	BUILD_BUG_ON(sizeof(gfn_t) != sizeof(new->gmem.pgoff));
 
-	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))
+	if (WARN_ON_ONCE(change != KVM_MR_CREATE))
 		return -EINVAL;
 
-	file = fget(fd);
-	if (!file)
-		return -EBADF;
+	if (WARN_ON_ONCE(new->flags & KVM_MEMSLOT_GMEM_ONLY))
+		return -EINVAL;
 
-	if (file->f_op != &kvm_gmem_fops)
-		goto err;
+	if (new->gmem.file->f_op != &kvm_gmem_fops)
+		return -EINVAL;
 
-	f = file->private_data;
+	f = new->gmem.file->private_data;
 	if (f->kvm != kvm)
-		goto err;
+		return -EINVAL;
 
-	inode = file_inode(file);
+	inode = file_inode(new->gmem.file);
 
-	if (!PAGE_ALIGNED(offset) || offset + size > i_size_read(inode))
-		goto err;
+	if (new->gmem.pgoff + new->npages  > i_size_read(inode) >> PAGE_SHIFT)
+		return -EINVAL;
 
 	/*
 	 * memslots of flag KVM_MEM_GUEST_MEMFD are immutable to change, so
 	 * 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.
 	 */
-	slot->gmem.file = file;
-	slot->gmem.pgoff = offset >> PAGE_SHIFT;
 	if (gmem_in_place_conversion || kvm_gmem_supports_mmap(inode))
-		slot->flags |= KVM_MEMSLOT_GMEM_ONLY;
+		new->flags |= KVM_MEMSLOT_GMEM_ONLY;
 
-	/*
-	 * Gift the caller a reference to the file.  The reference will be
-	 * dropped after bindings are established, or if installing the new
-	 * memslot ultimately fails.
-	 */
 	return 0;
-
-err:
-	fput(file);
-	return -EINVAL;
 }
 
-int kvm_gmem_commit_memory_region(struct kvm *kvm, struct kvm_memory_slot *slot)
+int kvm_gmem_commit_memory_region(struct kvm *kvm, struct kvm_memory_slot *old,
+				  struct kvm_memory_slot *new,
+				  enum kvm_mr_change change)
 {
-	struct gmem_file *f = slot->gmem.file->private_data;
-	struct inode *inode = file_inode(slot->gmem.file);
+	struct gmem_file *f = new->gmem.file->private_data;
+	struct inode *inode = file_inode(new->gmem.file);
 	unsigned long start, end;
 	int r;
 
-	if (WARN_ON_ONCE(slot->gmem.file->f_op != &kvm_gmem_fops))
+	if (WARN_ON_ONCE(new->gmem.file->f_op != &kvm_gmem_fops))
 		return -EIO;
 
 	filemap_invalidate_lock(inode->i_mapping);
 
-	start = slot->gmem.pgoff;
-	end = start + slot->npages;
+	start = new->gmem.pgoff;
+	end = start + new->npages;
 
 	if (!xa_empty(&f->bindings) &&
 	    xa_find(&f->bindings, &start, end - 1, XA_PRESENT)) {
@@ -1077,7 +1065,7 @@ int kvm_gmem_commit_memory_region(struct kvm *kvm, struct kvm_memory_slot *slot)
 		return -EEXIST;
 	}
 
-	r = xa_err(xa_store_range(&f->bindings, start, end - 1, slot, GFP_KERNEL));
+	r = xa_err(xa_store_range(&f->bindings, start, end - 1, new, GFP_KERNEL));
 	if (r)
 		xa_store_range(&f->bindings, start, end - 1, NULL, GFP_KERNEL);
 
diff --git a/virt/kvm/guest_memfd.h b/virt/kvm/guest_memfd.h
index 01bd359d27e3..63723ffee0a5 100644
--- a/virt/kvm/guest_memfd.h
+++ b/virt/kvm/guest_memfd.h
@@ -8,9 +8,13 @@
 int kvm_gmem_init(struct module *module);
 void kvm_gmem_exit(void);
 int kvm_gmem_create(struct kvm *kvm, struct kvm_create_guest_memfd *args);
-int kvm_gmem_prepare_memory_region(struct kvm *kvm, struct kvm_memory_slot *slot,
-				   unsigned int fd, uoff_t offset);
-int kvm_gmem_commit_memory_region(struct kvm *kvm, struct kvm_memory_slot *slot);
+int kvm_gmem_prepare_memory_region(struct kvm *kvm,
+				   const struct kvm_memory_slot *old,
+				   struct kvm_memory_slot *new,
+				   enum kvm_mr_change change);
+int kvm_gmem_commit_memory_region(struct kvm *kvm, struct kvm_memory_slot *old,
+				  struct kvm_memory_slot *new,
+				  enum kvm_mr_change change);
 void kvm_gmem_unbind(struct kvm_memory_slot *slot);
 #else
 static inline int kvm_gmem_init(struct module *module)
@@ -20,15 +24,18 @@ static inline int kvm_gmem_init(struct module *module)
 static inline void kvm_gmem_exit(void) {};
 
 static inline int kvm_gmem_prepare_memory_region(struct kvm *kvm,
-						 struct kvm_memory_slot *slot,
-						 unsigned int fd, uoff_t offset)
+						 const struct kvm_memory_slot *old,
+						 struct kvm_memory_slot *new,
+						 enum kvm_mr_change change)
 {
 	WARN_ON_ONCE(1);
 	return -EIO;
 }
 
 static inline int kvm_gmem_commit_memory_region(struct kvm *kvm,
-						struct kvm_memory_slot *slot)
+						struct kvm_memory_slot *old,
+						struct kvm_memory_slot *new,
+						enum kvm_mr_change change)
 {
 	WARN_ON_ONCE(1);
 	return -EIO;
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 9fff2f4bf2f1..70d70042a9ce 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -1681,6 +1681,12 @@ static int kvm_prepare_memory_region(struct kvm *kvm,
 {
 	int r;
 
+	if (new && (new->flags & KVM_MEM_GUEST_MEMFD)) {
+		r = kvm_gmem_prepare_memory_region(kvm, old, new, change);
+		if (r)
+			return r;
+	}
+
 	/*
 	 * If dirty logging is disabled, nullify the bitmap; the old bitmap
 	 * will be freed on "commit".  If logging is enabled in both old and
@@ -1952,8 +1958,8 @@ static int kvm_set_memslot(struct kvm *kvm,
 	if (r)
 		goto err;
 
-	if (change == KVM_MR_CREATE && (new->flags & KVM_MEM_GUEST_MEMFD)) {
-		r = kvm_gmem_commit_memory_region(kvm, new);
+	if (new && (new->flags & KVM_MEM_GUEST_MEMFD)) {
+		r = kvm_gmem_commit_memory_region(kvm, old, new, change);
 		if (r) {
 			kvm_arch_free_memslot(kvm, new);
 			kvm_destroy_dirty_bitmap(new);
@@ -2133,11 +2139,16 @@ static int kvm_set_memory_region(struct kvm *kvm,
 	new->npages = npages;
 	new->flags = mem->flags;
 	new->userspace_addr = mem->userspace_addr;
-	if (change == KVM_MR_CREATE && (mem->flags & KVM_MEM_GUEST_MEMFD)) {
-		r = kvm_gmem_prepare_memory_region(kvm, new, mem->guest_memfd,
-						   mem->guest_memfd_offset);
-		if (r)
+	if (mem->flags & KVM_MEM_GUEST_MEMFD) {
+#ifdef CONFIG_KVM_GUEST_MEMFD
+		new->gmem.file = fget(mem->guest_memfd);
+		if (!new->gmem.file) {
+			r = -EBADF;
 			goto out;
+		}
+
+		new->gmem.pgoff = mem->guest_memfd_offset >> PAGE_SHIFT;
+#endif
 	}
 
 	r = kvm_set_memslot(kvm, old, new, change);
@@ -2148,7 +2159,7 @@ static int kvm_set_memory_region(struct kvm *kvm,
 	 * the file is closed before memslots are destroyed.
 	 */
 #ifdef CONFIG_KVM_GUEST_MEMFD
-	if (change == KVM_MR_CREATE && (mem->flags & KVM_MEM_GUEST_MEMFD))
+	if (new->gmem.file)
 		fput(new->gmem.file);
 #endif

Re: [PATCH v5 4/6] KVM: guest_memfd: Split bind() into prepare()+commit() phases

[Sean Christopherson]

On Thu, Sep 24, 2026, Sean Christopherson wrote:
> On Thu, Sep 24, 2026, Ackerley Tng wrote:
> But after fiddling with this for an hour or so, I realized that if we fully commit
> to configuring new.gmem in kvm_set_memory_region(), then we can move the prepare()
> call into kvm_prepare_memory_region() without needing to plumb extra parameters,
> and make the gmem stuff look a lot more like the rest of the memslot code.  E.g.
> 
> 	if (mem->flags & KVM_MEM_GUEST_MEMFD) {
> #ifdef CONFIG_KVM_GUEST_MEMFD
> 		new->gmem.file = fget(mem->guest_memfd);
> 		if (!new->gmem.file) {
> 			r = -EBADF;
> 			goto out;
> 		}
> 
> 		new->gmem.pgoff = mem->guest_memfd_offset >> PAGE_SHIFT;
> #endif
> 	}
> 
> 	r = kvm_set_memslot(kvm, old, new, change);
> 
> 	/*
> 	 * Drop the reference to the gmem file, even on success.  The file pins
> 	 * KVM, not the other way 'round.  Active bindings are invalidated if
> 	 * the file is closed before memslots are destroyed.
> 	 */
> #ifdef CONFIG_KVM_GUEST_MEMFD
> 	if (new->gmem.file)
> 		fput(new->gmem.file);
> #endif
> 
> Not being able to get rid of the #ifdefs is a shame, but from a control flow
> perspective, this feels more right than anything else.  Full diff (not fully
> tested, and would need to be split into 3+ patches):

Actually, plumbing in @old and @change can wait.  As much as I want to make the
calls match the other prepare()+commit() hooks, @old and @change aren't needed
until flags-only updates come along, and adding them at that time provide a better
git history as the additional plumbing will directly precede their usage (or maybe
even be in the same patch).

> -	if (!PAGE_ALIGNED(offset) || offset + size > i_size_read(inode))
> -		goto err;
> +	if (new->gmem.pgoff + new->npages  > i_size_read(inode) >> PAGE_SHIFT)
> +		return -EINVAL;

...

> +	if (mem->flags & KVM_MEM_GUEST_MEMFD) {
> +#ifdef CONFIG_KVM_GUEST_MEMFD
> +		new->gmem.file = fget(mem->guest_memfd);
> +		if (!new->gmem.file) {
> +			r = -EBADF;
>  			goto out;
> +		}

Note, I didn't lose the offset check, the existing one in
kvm_gmem_prepare_memory_region() (nee bind()) is redundant with this one in
kvm_set_memory_region():

	if (mem->flags & KVM_MEM_GUEST_MEMFD &&
	    (mem->guest_memfd_offset & (PAGE_SIZE - 1) ||  <========
	     mem->guest_memfd_offset + mem->memory_size < mem->guest_memfd_offset))
		return -EINVAL;
> +
> +		new->gmem.pgoff = mem->guest_memfd_offset >> PAGE_SHIFT;
> +#endif
>  	}
>  
>  	r = kvm_set_memslot(kvm, old, new, change);
> @@ -2148,7 +2159,7 @@ static int kvm_set_memory_region(struct kvm *kvm,
>  	 * the file is closed before memslots are destroyed.
>  	 */
>  #ifdef CONFIG_KVM_GUEST_MEMFD
> -	if (change == KVM_MR_CREATE && (mem->flags & KVM_MEM_GUEST_MEMFD))
> +	if (new->gmem.file)
>  		fput(new->gmem.file);
>  #endif
> 

Re: [PATCH v5 4/6] KVM: guest_memfd: Split bind() into prepare()+commit() phases

[Sean Christopherson]

On Thu, Sep 24, 2026, Sean Christopherson wrote:
> On Thu, Sep 24, 2026, Sean Christopherson wrote:
> > On Thu, Sep 24, 2026, Ackerley Tng wrote:
> > But after fiddling with this for an hour or so, I realized that if we fully commit
> > to configuring new.gmem in kvm_set_memory_region(), then we can move the prepare()
> > call into kvm_prepare_memory_region() without needing to plumb extra parameters,
> > and make the gmem stuff look a lot more like the rest of the memslot code.  E.g.
> > 
> > 	if (mem->flags & KVM_MEM_GUEST_MEMFD) {
> > #ifdef CONFIG_KVM_GUEST_MEMFD
> > 		new->gmem.file = fget(mem->guest_memfd);
> > 		if (!new->gmem.file) {
> > 			r = -EBADF;
> > 			goto out;
> > 		}
> > 
> > 		new->gmem.pgoff = mem->guest_memfd_offset >> PAGE_SHIFT;
> > #endif
> > 	}
> > 
> > 	r = kvm_set_memslot(kvm, old, new, change);
> > 
> > 	/*
> > 	 * Drop the reference to the gmem file, even on success.  The file pins
> > 	 * KVM, not the other way 'round.  Active bindings are invalidated if
> > 	 * the file is closed before memslots are destroyed.
> > 	 */
> > #ifdef CONFIG_KVM_GUEST_MEMFD
> > 	if (new->gmem.file)
> > 		fput(new->gmem.file);
> > #endif
> > 
> > Not being able to get rid of the #ifdefs is a shame, but from a control flow
> > perspective, this feels more right than anything else.  Full diff (not fully
> > tested, and would need to be split into 3+ patches):
> 
> Actually, plumbing in @old and @change can wait.  As much as I want to make the
> calls match the other prepare()+commit() hooks, @old and @change aren't needed
> until flags-only updates come along, and adding them at that time provide a better
> git history as the additional plumbing will directly precede their usage (or maybe
> even be in the same patch).

Aaaaand talking to myself again.  I take this back.  Plumbing in @change is
desirable, otherwise both kvm_set_memory_region() and kvm_prepare_memory_region()
need to check KVM_MR_CREATE, which is ugly and unnecessarily fragile.

Re: [PATCH v5 4/6] KVM: guest_memfd: Split bind() into prepare()+commit() phases

[Ackerley Tng]

Sean Christopherson <seanjc@google.com> writes:

>
> [...snip...]
>
> diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
> index a13445c26d9d..d1e502d3246b 100644
> --- a/virt/kvm/guest_memfd.c
> +++ b/virt/kvm/guest_memfd.c
> @@ -1003,73 +1003,61 @@ int kvm_gmem_create(struct kvm *kvm, struct kvm_create_guest_memfd *args)
>  	return __kvm_gmem_create(kvm, size, flags);
>  }
>
> -int kvm_gmem_prepare_memory_region(struct kvm *kvm, struct kvm_memory_slot *slot,
> -				   unsigned int fd, uoff_t offset)
> +int kvm_gmem_prepare_memory_region(struct kvm *kvm,
> +				   const struct kvm_memory_slot *old,
> +				   struct kvm_memory_slot *new,
> +				   enum kvm_mr_change change)
>  {
> -	uoff_t size = slot->npages << PAGE_SHIFT;
>  	struct gmem_file *f;
>  	struct inode *inode;
> -	struct file *file;
>
> +	BUILD_BUG_ON(sizeof(gfn_t) != sizeof(new->gmem.pgoff));
>
> -	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))
> +	if (WARN_ON_ONCE(change != KVM_MR_CREATE))
>  		return -EINVAL;
>
> -	file = fget(fd);
> -	if (!file)
> -		return -EBADF;
> +	if (WARN_ON_ONCE(new->flags & KVM_MEMSLOT_GMEM_ONLY))
> +		return -EINVAL;
>
> -	if (file->f_op != &kvm_gmem_fops)
> -		goto err;
> +	if (new->gmem.file->f_op != &kvm_gmem_fops)
> +		return -EINVAL;
>
> -	f = file->private_data;
> +	f = new->gmem.file->private_data;
>  	if (f->kvm != kvm)
> -		goto err;
> +		return -EINVAL;
>
> -	inode = file_inode(file);
> +	inode = file_inode(new->gmem.file);
>
> -	if (!PAGE_ALIGNED(offset) || offset + size > i_size_read(inode))
> -		goto err;
> +	if (new->gmem.pgoff + new->npages  > i_size_read(inode) >> PAGE_SHIFT)
> +		return -EINVAL;
>
>  	/*
>  	 * memslots of flag KVM_MEM_GUEST_MEMFD are immutable to change, so
>  	 * kvm_gmem_bind() must occur on a new memslot.  Because the memslot

Just noticed, this comment needs to be updated for the new function name.

>  	 * is not visible yet, kvm_gmem_get_pfn() is guaranteed to see the file.
>  	 */
> -	slot->gmem.file = file;
> -	slot->gmem.pgoff = offset >> PAGE_SHIFT;
>  	if (gmem_in_place_conversion || kvm_gmem_supports_mmap(inode))
> -		slot->flags |= KVM_MEMSLOT_GMEM_ONLY;
> +		new->flags |= KVM_MEMSLOT_GMEM_ONLY;
>
> -	/*
> -	 * Gift the caller a reference to the file.  The reference will be
> -	 * dropped after bindings are established, or if installing the new
> -	 * memslot ultimately fails.
> -	 */
>  	return 0;
> -
> -err:
> -	fput(file);
> -	return -EINVAL;
>  }
>

Looks like const struct kvm_memory_slot *old is unused, though you
already told yourself that. :)

> -int kvm_gmem_commit_memory_region(struct kvm *kvm, struct kvm_memory_slot *slot)
> +int kvm_gmem_commit_memory_region(struct kvm *kvm, struct kvm_memory_slot *old,
> +				  struct kvm_memory_slot *new,
> +				  enum kvm_mr_change change)
>  {
> -	struct gmem_file *f = slot->gmem.file->private_data;
> -	struct inode *inode = file_inode(slot->gmem.file);
> +	struct gmem_file *f = new->gmem.file->private_data;
> +	struct inode *inode = file_inode(new->gmem.file);
>  	unsigned long start, end;
>  	int r;
>
> -	if (WARN_ON_ONCE(slot->gmem.file->f_op != &kvm_gmem_fops))
> +	if (WARN_ON_ONCE(new->gmem.file->f_op != &kvm_gmem_fops))
>  		return -EIO;
>
>  	filemap_invalidate_lock(inode->i_mapping);
>
> -	start = slot->gmem.pgoff;
> -	end = start + slot->npages;
> +	start = new->gmem.pgoff;
> +	end = start + new->npages;
>
>  	if (!xa_empty(&f->bindings) &&
>  	    xa_find(&f->bindings, &start, end - 1, XA_PRESENT)) {
> @@ -1077,7 +1065,7 @@ int kvm_gmem_commit_memory_region(struct kvm *kvm, struct kvm_memory_slot *slot)
>  		return -EEXIST;
>  	}
>
> -	r = xa_err(xa_store_range(&f->bindings, start, end - 1, slot, GFP_KERNEL));
> +	r = xa_err(xa_store_range(&f->bindings, start, end - 1, new, GFP_KERNEL));
>  	if (r)
>  		xa_store_range(&f->bindings, start, end - 1, NULL, GFP_KERNEL);
>
> diff --git a/virt/kvm/guest_memfd.h b/virt/kvm/guest_memfd.h
> index 01bd359d27e3..63723ffee0a5 100644
> --- a/virt/kvm/guest_memfd.h
> +++ b/virt/kvm/guest_memfd.h
> @@ -8,9 +8,13 @@
>  int kvm_gmem_init(struct module *module);
>  void kvm_gmem_exit(void);
>  int kvm_gmem_create(struct kvm *kvm, struct kvm_create_guest_memfd *args);
> -int kvm_gmem_prepare_memory_region(struct kvm *kvm, struct kvm_memory_slot *slot,
> -				   unsigned int fd, uoff_t offset);
> -int kvm_gmem_commit_memory_region(struct kvm *kvm, struct kvm_memory_slot *slot);
> +int kvm_gmem_prepare_memory_region(struct kvm *kvm,
> +				   const struct kvm_memory_slot *old,
> +				   struct kvm_memory_slot *new,
> +				   enum kvm_mr_change change);
> +int kvm_gmem_commit_memory_region(struct kvm *kvm, struct kvm_memory_slot *old,
> +				  struct kvm_memory_slot *new,
> +				  enum kvm_mr_change change);
>  void kvm_gmem_unbind(struct kvm_memory_slot *slot);
>  #else
>  static inline int kvm_gmem_init(struct module *module)
> @@ -20,15 +24,18 @@ static inline int kvm_gmem_init(struct module *module)
>  static inline void kvm_gmem_exit(void) {};
>
>  static inline int kvm_gmem_prepare_memory_region(struct kvm *kvm,
> -						 struct kvm_memory_slot *slot,
> -						 unsigned int fd, uoff_t offset)
> +						 const struct kvm_memory_slot *old,
> +						 struct kvm_memory_slot *new,
> +						 enum kvm_mr_change change)
>  {
>  	WARN_ON_ONCE(1);
>  	return -EIO;
>  }
>
>  static inline int kvm_gmem_commit_memory_region(struct kvm *kvm,
> -						struct kvm_memory_slot *slot)
> +						struct kvm_memory_slot *old,
> +						struct kvm_memory_slot *new,
> +						enum kvm_mr_change change)
>  {
>  	WARN_ON_ONCE(1);
>  	return -EIO;
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 9fff2f4bf2f1..70d70042a9ce 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -1681,6 +1681,12 @@ static int kvm_prepare_memory_region(struct kvm *kvm,
>  {
>  	int r;
>
> +	if (new && (new->flags & KVM_MEM_GUEST_MEMFD)) {
> +		r = kvm_gmem_prepare_memory_region(kvm, old, new, change);
> +		if (r)
> +			return r;
> +	}
> +
>  	/*
>  	 * If dirty logging is disabled, nullify the bitmap; the old bitmap
>  	 * will be freed on "commit".  If logging is enabled in both old and
> @@ -1952,8 +1958,8 @@ static int kvm_set_memslot(struct kvm *kvm,
>  	if (r)
>  		goto err;
>
> -	if (change == KVM_MR_CREATE && (new->flags & KVM_MEM_GUEST_MEMFD)) {
> -		r = kvm_gmem_commit_memory_region(kvm, new);
> +	if (new && (new->flags & KVM_MEM_GUEST_MEMFD)) {
> +		r = kvm_gmem_commit_memory_region(kvm, old, new, change);
>  		if (r) {
>  			kvm_arch_free_memslot(kvm, new);
>  			kvm_destroy_dirty_bitmap(new);
> @@ -2133,11 +2139,16 @@ static int kvm_set_memory_region(struct kvm *kvm,
>  	new->npages = npages;
>  	new->flags = mem->flags;
>  	new->userspace_addr = mem->userspace_addr;
> -	if (change == KVM_MR_CREATE && (mem->flags & KVM_MEM_GUEST_MEMFD)) {
> -		r = kvm_gmem_prepare_memory_region(kvm, new, mem->guest_memfd,
> -						   mem->guest_memfd_offset);
> -		if (r)
> +	if (mem->flags & KVM_MEM_GUEST_MEMFD) {
> +#ifdef CONFIG_KVM_GUEST_MEMFD
> +		new->gmem.file = fget(mem->guest_memfd);
> +		if (!new->gmem.file) {
> +			r = -EBADF;
>  			goto out;
> +		}
> +
> +		new->gmem.pgoff = mem->guest_memfd_offset >> PAGE_SHIFT;
> +#endif
>  	}
>
>  	r = kvm_set_memslot(kvm, old, new, change);
> @@ -2148,7 +2159,7 @@ static int kvm_set_memory_region(struct kvm *kvm,
>  	 * the file is closed before memslots are destroyed.
>  	 */
>  #ifdef CONFIG_KVM_GUEST_MEMFD
> -	if (change == KVM_MR_CREATE && (mem->flags & KVM_MEM_GUEST_MEMFD))
> +	if (new->gmem.file)
>  		fput(new->gmem.file);
>  #endif

And this LGTM too. Thanks!

[PATCH v5 5/6] 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().

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 | 44 +++++++++++++++++++++++---------------------
 1 file changed, 23 insertions(+), 21 deletions(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 45b509f4e54b..90461880ff85 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -1944,6 +1944,15 @@ static int kvm_set_memslot(struct kvm *kvm,
 	if (r)
 		goto err;
 
+	if (change == KVM_MR_CREATE && (new->flags & KVM_MEM_GUEST_MEMFD)) {
+		r = kvm_gmem_commit_memory_region(kvm, new);
+		if (r) {
+			kvm_arch_free_memslot(kvm, new);
+			kvm_destroy_dirty_bitmap(new);
+			goto err;
+		}
+	}
+
 	/*
 	 * 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
@@ -2121,32 +2130,25 @@ static int kvm_set_memory_region(struct kvm *kvm,
 						   mem->guest_memfd_offset);
 		if (r)
 			goto out;
-
-		r = kvm_gmem_commit_memory_region(kvm, new);
-
-		/*
-		 * Drop the reference to the file, even on success.  The file
-		 * pins KVM, not the other way 'round.  Active bindings are
-		 * invalidated if the file is closed before memslots are
-		 * destroyed.
-		 */
-#ifdef CONFIG_KVM_GUEST_MEMFD
-		 fput(new->gmem.file);
-#endif
-
-		if (r)
-			goto out;
 	}
 
 	r = kvm_set_memslot(kvm, old, new, change);
-	if (r)
-		goto out_unbind;
 
-	return 0;
-
-out_unbind:
+	/*
+	 * Drop the reference to the gmem file, even on success.  The file pins
+	 * KVM, not the other way 'round.  Active bindings are invalidated if
+	 * the file is closed before memslots are destroyed.
+	 */
+#ifdef CONFIG_KVM_GUEST_MEMFD
 	if (change == KVM_MR_CREATE && (mem->flags & KVM_MEM_GUEST_MEMFD))
-		kvm_gmem_unbind(new);
+		fput(new->gmem.file);
+#endif
+
+	if (r)
+		goto out;
+
+	return 0;
+
 out:
 	kfree(new);
 	return r;
-- 
2.55.0.1082.g2b9226bbc0-goog

Re: [PATCH v5 5/6] KVM: guest_memfd: Establish memslot<=>guest_memfd bindings *after* memslot is ready

[Ackerley Tng]

Sean Christopherson <seanjc@google.com> writes:

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

Hope you consider my suggestion on [4/6]. Either way,

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

[PATCH v5 6/6] 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>
Reviewed-by: David Hildenbrand (Arm) <david@kernel.org>
Reviewed-by: Ackerley Tng <ackerleytng@google.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 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;
-- 
2.55.0.1082.g2b9226bbc0-goog

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

[Yan Zhao]

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@intel.com>
> Reviewed-by: David Hildenbrand (Arm) <david@kernel.org>
> Reviewed-by: Ackerley Tng <ackerleytng@google.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 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@intel.com>

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?

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()?

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

[Sean Christopherson]

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@intel.com>
> > Reviewed-by: David Hildenbrand (Arm) <david@kernel.org>
> > Reviewed-by: Ackerley Tng <ackerleytng@google.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 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@intel.com>
> 
> 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.

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.

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

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@intel.com>
> > > Reviewed-by: David Hildenbrand (Arm) <david@kernel.org>
> > > Reviewed-by: Ackerley Tng <ackerleytng@google.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 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@intel.com>
> > 
> > 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.

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

[Sean Christopherson]

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.

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

[Yan Zhao]

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!

Re: [PATCH v5 0/6] KVM: guest_memfd: Fix binding bugs

[Sean Christopherson]

On Mon, 21 Sep 2026 17:13:26 -0700, Sean Christopherson wrote:
> Sorry for the spam, I am hoping to get this applied before the in-place
> conversion series, and I really want to get that series applied this week.
> I finally gave up and split bind() into prepare()+commit(), and I actually
> like the end result.  The only really ugly part is an extra #ifdef, but
> otherwise gifting the gmem file reference back to kvm_set_memory_region()
> avoids the TOCTOU issues and weird juggling that led me to initially reject
> a prepare()+commit() solution.
> 
> [...]

Applied to kvm-x86 coco.  I'll send another series to address Ackerley's
feedback (I would do the more agressive cleanups on top no matter what, to keep
the fixes minimal for LTS kernels).

[1/6] KVM: guest_memfd: Gracefully handle xarray errors when binding a memslot
      https://github.com/kvm-x86/linux/commit/e6ecb88711d9
[2/6] KVM: Use goto to handle errors during memslot preparation
      https://github.com/kvm-x86/linux/commit/23a205797cec
[3/6] KVM: Only bind memslot to guest_memfd instance for CREATE operations
      https://github.com/kvm-x86/linux/commit/513333a55460
[4/6] KVM: guest_memfd: Split bind() into prepare()+commit() phases
      https://github.com/kvm-x86/linux/commit/83411f32b8ad
[5/6] KVM: guest_memfd: Establish memslot<=>guest_memfd bindings *after* memslot is ready
      https://github.com/kvm-x86/linux/commit/3fe5d2944e5a
[6/6] KVM: guest_memfd: Drop superfluous WRITE_ONCE() when binding a memslot
      https://github.com/kvm-x86/linux/commit/48252615eb78

--
https://github.com/kvm-x86/linux/tree/next