[PATCH] KVM: guest_memfd: Hold file reference across memslot preparation and commit

[Ackerley Tng]

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@xxxxxxxxxx>
---
 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...]
>