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

[Sean Christopherson <seanjc@google.com>]

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