[PATCH] KVM: guest_memfd: Define a CLASS to get+put guest_memfd file from a memslot

[PATCH] KVM: guest_memfd: Define a CLASS to get+put guest_memfd file from a memslot

[Sean Christopherson]

Add a CLASS to handle getting and putting a guest_memfd file given a
memslot to reduce the amount of related boilerplate, and more importantly
to minimize the chances of forgetting to put the file (thankfully the bug
that prompted this didn't escape initial testing).

Define a CLASS instead of using __free(fput) as _free() comes with subtle
caveats related to FILO ordering (objects are freed in the order in which
they are declared), and the recommended solution/workaround (declare file
pointers exactly when they are initialized) is visually jarring relative
to KVM's (and the kernel's) overall strict adherence to not mixing
declarations and code.  E.g. the use in kvm_gmem_populate() would be:

	slot = gfn_to_memslot(kvm, start_gfn);
	if (!kvm_slot_has_gmem(slot))
		return -EINVAL;

	struct file *file __free(fput) = kvm_gmem_get_file(slot;
	if (!file)
		return -EFAULT;

	filemap_invalidate_lock(file->f_mapping);

Note, using CLASS() still declares variables in the middle of code, but
the syntactic sugar obfuscates the declaration, i.e. hides the anomaly to
a large extent.

No functional change intended.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 virt/kvm/guest_memfd.c | 22 ++++++++--------------
 1 file changed, 8 insertions(+), 14 deletions(-)

diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
index 94bafd6c558c..130244e46326 100644
--- a/virt/kvm/guest_memfd.c
+++ b/virt/kvm/guest_memfd.c
@@ -307,6 +307,9 @@ static inline struct file *kvm_gmem_get_file(struct kvm_memory_slot *slot)
 	return get_file_active(&slot->gmem.file);
 }
 
+DEFINE_CLASS(gmem_get_file, struct file *, if (_T) fput(_T),
+	     kvm_gmem_get_file(slot), struct kvm_memory_slot *slot);
+
 static pgoff_t kvm_gmem_get_index(struct kvm_memory_slot *slot, gfn_t gfn)
 {
 	return gfn - slot->base_gfn + slot->gmem.pgoff;
@@ -605,13 +608,12 @@ void kvm_gmem_unbind(struct kvm_memory_slot *slot)
 	unsigned long start = slot->gmem.pgoff;
 	unsigned long end = start + slot->npages;
 	struct kvm_gmem *gmem;
-	struct file *file;
 
 	/*
 	 * Nothing to do if the underlying file was already closed (or is being
 	 * closed right now), kvm_gmem_release() invalidates all bindings.
 	 */
-	file = kvm_gmem_get_file(slot);
+	CLASS(gmem_get_file, file)(slot);
 	if (!file)
 		return;
 
@@ -626,8 +628,6 @@ void kvm_gmem_unbind(struct kvm_memory_slot *slot)
 	 */
 	WRITE_ONCE(slot->gmem.file, NULL);
 	filemap_invalidate_unlock(file->f_mapping);
-
-	fput(file);
 }
 
 /* Returns a locked folio on success.  */
@@ -674,19 +674,17 @@ int kvm_gmem_get_pfn(struct kvm *kvm, struct kvm_memory_slot *slot,
 		     int *max_order)
 {
 	pgoff_t index = kvm_gmem_get_index(slot, gfn);
-	struct file *file = kvm_gmem_get_file(slot);
 	struct folio *folio;
 	bool is_prepared = false;
 	int r = 0;
 
+	CLASS(gmem_get_file, file)(slot);
 	if (!file)
 		return -EFAULT;
 
 	folio = __kvm_gmem_get_pfn(file, slot, index, pfn, &is_prepared, max_order);
-	if (IS_ERR(folio)) {
-		r = PTR_ERR(folio);
-		goto out;
-	}
+	if (IS_ERR(folio))
+		return PTR_ERR(folio);
 
 	if (!is_prepared)
 		r = kvm_gmem_prepare_folio(kvm, slot, gfn, folio);
@@ -698,8 +696,6 @@ int kvm_gmem_get_pfn(struct kvm *kvm, struct kvm_memory_slot *slot,
 	else
 		folio_put(folio);
 
-out:
-	fput(file);
 	return r;
 }
 EXPORT_SYMBOL_FOR_KVM_INTERNAL(kvm_gmem_get_pfn);
@@ -708,7 +704,6 @@ EXPORT_SYMBOL_FOR_KVM_INTERNAL(kvm_gmem_get_pfn);
 long kvm_gmem_populate(struct kvm *kvm, gfn_t start_gfn, void __user *src, long npages,
 		       kvm_gmem_populate_cb post_populate, void *opaque)
 {
-	struct file *file;
 	struct kvm_memory_slot *slot;
 	void __user *p;
 
@@ -724,7 +719,7 @@ long kvm_gmem_populate(struct kvm *kvm, gfn_t start_gfn, void __user *src, long
 	if (!kvm_slot_has_gmem(slot))
 		return -EINVAL;
 
-	file = kvm_gmem_get_file(slot);
+	CLASS(gmem_get_file, file)(slot);
 	if (!file)
 		return -EFAULT;
 
@@ -782,7 +777,6 @@ long kvm_gmem_populate(struct kvm *kvm, gfn_t start_gfn, void __user *src, long
 
 	filemap_invalidate_unlock(file->f_mapping);
 
-	fput(file);
 	return ret && !i ? ret : i;
 }
 EXPORT_SYMBOL_FOR_KVM_INTERNAL(kvm_gmem_populate);

base-commit: 6b36119b94d0b2bb8cea9d512017efafd461d6ac
-- 
2.51.0.710.ga91ca5db03-goog

Re: [PATCH] KVM: guest_memfd: Define a CLASS to get+put guest_memfd file from a memslot

[Ackerley Tng]

Sean Christopherson <seanjc@google.com> writes:

> Add a CLASS to handle getting and putting a guest_memfd file given a
> memslot to reduce the amount of related boilerplate, and more importantly
> to minimize the chances of forgetting to put the file (thankfully the bug
> that prompted this didn't escape initial testing).
>
> Define a CLASS instead of using __free(fput) as _free() comes with subtle
> caveats related to FILO ordering (objects are freed in the order in which
> they are declared), and the recommended solution/workaround (declare file
> pointers exactly when they are initialized) is visually jarring relative
> to KVM's (and the kernel's) overall strict adherence to not mixing
> declarations and code.

This is kind of dangerous, glad you highlighted this!

> E.g. the use in kvm_gmem_populate() would be:
>
> 	slot = gfn_to_memslot(kvm, start_gfn);
> 	if (!kvm_slot_has_gmem(slot))
> 		return -EINVAL;
>
> 	struct file *file __free(fput) = kvm_gmem_get_file(slot;
> 	if (!file)
> 		return -EFAULT;
>
> 	filemap_invalidate_lock(file->f_mapping);
>
> Note, using CLASS() still declares variables in the middle of code, but
> the syntactic sugar obfuscates the declaration, i.e. hides the anomaly to
> a large extent.
>
> No functional change intended.
>
> Signed-off-by: Sean Christopherson <seanjc@google.com>



> ---
>  virt/kvm/guest_memfd.c | 22 ++++++++--------------
>  1 file changed, 8 insertions(+), 14 deletions(-)
>
> diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
> index 94bafd6c558c..130244e46326 100644
> --- a/virt/kvm/guest_memfd.c
> +++ b/virt/kvm/guest_memfd.c
> @@ -307,6 +307,9 @@ static inline struct file *kvm_gmem_get_file(struct kvm_memory_slot *slot)
>  	return get_file_active(&slot->gmem.file);
>  }
>  
> +DEFINE_CLASS(gmem_get_file, struct file *, if (_T) fput(_T),
> +	     kvm_gmem_get_file(slot), struct kvm_memory_slot *slot);
> +
>  static pgoff_t kvm_gmem_get_index(struct kvm_memory_slot *slot, gfn_t gfn)
>  {
>  	return gfn - slot->base_gfn + slot->gmem.pgoff;
> @@ -605,13 +608,12 @@ void kvm_gmem_unbind(struct kvm_memory_slot *slot)
>  	unsigned long start = slot->gmem.pgoff;
>  	unsigned long end = start + slot->npages;
>  	struct kvm_gmem *gmem;
> -	struct file *file;
>  
>  	/*
>  	 * Nothing to do if the underlying file was already closed (or is being
>  	 * closed right now), kvm_gmem_release() invalidates all bindings.
>  	 */
> -	file = kvm_gmem_get_file(slot);
> +	CLASS(gmem_get_file, file)(slot);
>  	if (!file)
>  		return;
>  
> @@ -626,8 +628,6 @@ void kvm_gmem_unbind(struct kvm_memory_slot *slot)
>  	 */
>  	WRITE_ONCE(slot->gmem.file, NULL);
>  	filemap_invalidate_unlock(file->f_mapping);
> -
> -	fput(file);
>  }
>  
>  /* Returns a locked folio on success.  */
> @@ -674,19 +674,17 @@ int kvm_gmem_get_pfn(struct kvm *kvm, struct kvm_memory_slot *slot,
>  		     int *max_order)
>  {
>  	pgoff_t index = kvm_gmem_get_index(slot, gfn);
> -	struct file *file = kvm_gmem_get_file(slot);
>  	struct folio *folio;
>  	bool is_prepared = false;
>  	int r = 0;
>  
> +	CLASS(gmem_get_file, file)(slot);
>  	if (!file)
>  		return -EFAULT;
>  
>  	folio = __kvm_gmem_get_pfn(file, slot, index, pfn, &is_prepared, max_order);
> -	if (IS_ERR(folio)) {
> -		r = PTR_ERR(folio);
> -		goto out;
> -	}
> +	if (IS_ERR(folio))
> +		return PTR_ERR(folio);
>  
>  	if (!is_prepared)
>  		r = kvm_gmem_prepare_folio(kvm, slot, gfn, folio);
> @@ -698,8 +696,6 @@ int kvm_gmem_get_pfn(struct kvm *kvm, struct kvm_memory_slot *slot,
>  	else
>  		folio_put(folio);
>  
> -out:
> -	fput(file);
>  	return r;
>  }
>  EXPORT_SYMBOL_FOR_KVM_INTERNAL(kvm_gmem_get_pfn);
> @@ -708,7 +704,6 @@ EXPORT_SYMBOL_FOR_KVM_INTERNAL(kvm_gmem_get_pfn);
>  long kvm_gmem_populate(struct kvm *kvm, gfn_t start_gfn, void __user *src, long npages,
>  		       kvm_gmem_populate_cb post_populate, void *opaque)
>  {
> -	struct file *file;
>  	struct kvm_memory_slot *slot;
>  	void __user *p;
>  
> @@ -724,7 +719,7 @@ long kvm_gmem_populate(struct kvm *kvm, gfn_t start_gfn, void __user *src, long
>  	if (!kvm_slot_has_gmem(slot))
>  		return -EINVAL;
>  
> -	file = kvm_gmem_get_file(slot);
> +	CLASS(gmem_get_file, file)(slot);
>  	if (!file)
>  		return -EFAULT;
>  
> @@ -782,7 +777,6 @@ long kvm_gmem_populate(struct kvm *kvm, gfn_t start_gfn, void __user *src, long
>  
>  	filemap_invalidate_unlock(file->f_mapping);
>  
> -	fput(file);
>  	return ret && !i ? ret : i;
>  }
>  EXPORT_SYMBOL_FOR_KVM_INTERNAL(kvm_gmem_populate);
>
> base-commit: 6b36119b94d0b2bb8cea9d512017efafd461d6ac

Re: [PATCH] KVM: guest_memfd: Define a CLASS to get+put guest_memfd file from a memslot

[Ackerley Tng]

Sean Christopherson <seanjc@google.com> writes:

> Add a CLASS to handle getting and putting a guest_memfd file given a
> memslot to reduce the amount of related boilerplate, and more importantly
> to minimize the chances of forgetting to put the file (thankfully the bug
> that prompted this didn't escape initial testing).
>
> Define a CLASS instead of using __free(fput) as _free() comes with subtle
> caveats related to FILO ordering (objects are freed in the order in which
> they are declared), and the recommended solution/workaround (declare file
> pointers exactly when they are initialized) is visually jarring relative
> to KVM's (and the kernel's) overall strict adherence to not mixing
> declarations and code.

This is kind of dangerous, glad you highlighted this!

> E.g. the use in kvm_gmem_populate() would be:
>
> 	slot = gfn_to_memslot(kvm, start_gfn);
> 	if (!kvm_slot_has_gmem(slot))
> 		return -EINVAL;
>
> 	struct file *file __free(fput) = kvm_gmem_get_file(slot;
> 	if (!file)
> 		return -EFAULT;
>
> 	filemap_invalidate_lock(file->f_mapping);
>
> Note, using CLASS() still declares variables in the middle of code, but
> the syntactic sugar obfuscates the declaration, i.e. hides the anomaly to
> a large extent.
>
> No functional change intended.
>
> Signed-off-by: Sean Christopherson <seanjc@google.com>

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

> ---
>  virt/kvm/guest_memfd.c | 22 ++++++++--------------
>  1 file changed, 8 insertions(+), 14 deletions(-)
>
> diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
> index 94bafd6c558c..130244e46326 100644
> --- a/virt/kvm/guest_memfd.c
> +++ b/virt/kvm/guest_memfd.c
> @@ -307,6 +307,9 @@ static inline struct file *kvm_gmem_get_file(struct kvm_memory_slot *slot)
>  	return get_file_active(&slot->gmem.file);
>  }
>  
> +DEFINE_CLASS(gmem_get_file, struct file *, if (_T) fput(_T),
> +	     kvm_gmem_get_file(slot), struct kvm_memory_slot *slot);
> +
>  static pgoff_t kvm_gmem_get_index(struct kvm_memory_slot *slot, gfn_t gfn)
>  {
>  	return gfn - slot->base_gfn + slot->gmem.pgoff;
> @@ -605,13 +608,12 @@ void kvm_gmem_unbind(struct kvm_memory_slot *slot)
>  	unsigned long start = slot->gmem.pgoff;
>  	unsigned long end = start + slot->npages;
>  	struct kvm_gmem *gmem;
> -	struct file *file;
>  
>  	/*
>  	 * Nothing to do if the underlying file was already closed (or is being
>  	 * closed right now), kvm_gmem_release() invalidates all bindings.
>  	 */
> -	file = kvm_gmem_get_file(slot);
> +	CLASS(gmem_get_file, file)(slot);
>  	if (!file)
>  		return;
>  
> @@ -626,8 +628,6 @@ void kvm_gmem_unbind(struct kvm_memory_slot *slot)
>  	 */
>  	WRITE_ONCE(slot->gmem.file, NULL);
>  	filemap_invalidate_unlock(file->f_mapping);
> -
> -	fput(file);
>  }
>  
>  /* Returns a locked folio on success.  */
> @@ -674,19 +674,17 @@ int kvm_gmem_get_pfn(struct kvm *kvm, struct kvm_memory_slot *slot,
>  		     int *max_order)
>  {
>  	pgoff_t index = kvm_gmem_get_index(slot, gfn);
> -	struct file *file = kvm_gmem_get_file(slot);
>  	struct folio *folio;
>  	bool is_prepared = false;
>  	int r = 0;
>  
> +	CLASS(gmem_get_file, file)(slot);
>  	if (!file)
>  		return -EFAULT;
>  
>  	folio = __kvm_gmem_get_pfn(file, slot, index, pfn, &is_prepared, max_order);
> -	if (IS_ERR(folio)) {
> -		r = PTR_ERR(folio);
> -		goto out;
> -	}
> +	if (IS_ERR(folio))
> +		return PTR_ERR(folio);
>  
>  	if (!is_prepared)
>  		r = kvm_gmem_prepare_folio(kvm, slot, gfn, folio);
> @@ -698,8 +696,6 @@ int kvm_gmem_get_pfn(struct kvm *kvm, struct kvm_memory_slot *slot,
>  	else
>  		folio_put(folio);
>  
> -out:
> -	fput(file);
>  	return r;
>  }
>  EXPORT_SYMBOL_FOR_KVM_INTERNAL(kvm_gmem_get_pfn);
> @@ -708,7 +704,6 @@ EXPORT_SYMBOL_FOR_KVM_INTERNAL(kvm_gmem_get_pfn);
>  long kvm_gmem_populate(struct kvm *kvm, gfn_t start_gfn, void __user *src, long npages,
>  		       kvm_gmem_populate_cb post_populate, void *opaque)
>  {
> -	struct file *file;
>  	struct kvm_memory_slot *slot;
>  	void __user *p;
>  
> @@ -724,7 +719,7 @@ long kvm_gmem_populate(struct kvm *kvm, gfn_t start_gfn, void __user *src, long
>  	if (!kvm_slot_has_gmem(slot))
>  		return -EINVAL;
>  
> -	file = kvm_gmem_get_file(slot);
> +	CLASS(gmem_get_file, file)(slot);
>  	if (!file)
>  		return -EFAULT;
>  
> @@ -782,7 +777,6 @@ long kvm_gmem_populate(struct kvm *kvm, gfn_t start_gfn, void __user *src, long
>  
>  	filemap_invalidate_unlock(file->f_mapping);
>  
> -	fput(file);
>  	return ret && !i ? ret : i;
>  }
>  EXPORT_SYMBOL_FOR_KVM_INTERNAL(kvm_gmem_populate);
>
> base-commit: 6b36119b94d0b2bb8cea9d512017efafd461d6ac

Re: [PATCH] KVM: guest_memfd: Define a CLASS to get+put guest_memfd file from a memslot

[Sean Christopherson]

On Tue, 07 Oct 2025 15:23:56 -0700, Sean Christopherson wrote:
> Add a CLASS to handle getting and putting a guest_memfd file given a
> memslot to reduce the amount of related boilerplate, and more importantly
> to minimize the chances of forgetting to put the file (thankfully the bug
> that prompted this didn't escape initial testing).
> 
> Define a CLASS instead of using __free(fput) as _free() comes with subtle
> caveats related to FILO ordering (objects are freed in the order in which
> they are declared), and the recommended solution/workaround (declare file
> pointers exactly when they are initialized) is visually jarring relative
> to KVM's (and the kernel's) overall strict adherence to not mixing
> declarations and code.  E.g. the use in kvm_gmem_populate() would be:
> 
> [...]

Applied to kvm-x86 gmem, thanks!

[1/1] KVM: guest_memfd: Define a CLASS to get+put guest_memfd file from a memslot
      https://github.com/kvm-x86/linux/commit/0bb4d9c39b76

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