Re: [PATCH 1/5] KVM: guest_memfd: take the invalidate lock when unbinding a dying file

[Sean Christopherson <seanjc@google.com>]

On Sun, Aug 23, 2026, Shivank Garg wrote:
> kvm_gmem_unbind() skips mapping->invalidate_lock when the guest_memfd
> file is already dying. All other paths that modify f->bindings hold
> that lock.
> 
> kvm_gmem_invalidate_{start,end}() checks f->bindings independently to
> decide whether to begin or end KVM MMU invalidations. So, the bindings
> must remain stable between the two calls. If a binding is removed in that
> window, start increments mmu_invalidate_in_progress but end does not
> decrement it. Example, unbind race with memory failure:
> 
>   CPU 0: memory failure               CPU 1: memslot delete
>   ----------------------------------  ---------------------------
>                                       (guest_memfd file is dying)
>   kvm_gmem_error_folio()
>     kvm_gmem_invalidate_start()
>       finds binding
>       mmu_invalidate_in_progress++
>                                       kvm_gmem_unbind()
>                                         get_file_active() fails
>                                         store NULL in bindings
>     kvm_gmem_invalidate_end()
>       no binding found
>       counter stays elevated
> 
> mmu_invalidate_retry() then returns 1 forever, so guest page faults
> retry without ever installing a mapping and the guest hangs.
> 
> Take the invalidate lock in the dying-file path too. This prevents unbind
> from removing a binding and leaking mmu_invalidate_in_progress. This is
> safe because any caller that reaches this path holds slots_lock, so
> kvm_gmem_release() cannot nullify the slot->gmem.file, until
> kvm_gmem_unbind() finishes.
> 
> Reported-by: Sashiko <sashiko-bot@kernel.org>
> Closes: https://lore.kernel.org/all/20260728092027.225CF1F000E9@smtp.kernel.org
> Fixes: ae431059e75d ("KVM: guest_memfd: Remove bindings on memslot deletion when gmem is dying")
> Signed-off-by: Shivank Garg <shivankg@amd.com>
> ---
>  virt/kvm/guest_memfd.c | 23 ++++++++++++++---------
>  1 file changed, 14 insertions(+), 9 deletions(-)
> 
> diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
> index f0e5da490866..f848120af84b 100644
> --- a/virt/kvm/guest_memfd.c
> +++ b/virt/kvm/guest_memfd.c
> @@ -721,6 +721,8 @@ static void __kvm_gmem_unbind(struct kvm_memory_slot *slot, struct gmem_file *f)
>  
>  void kvm_gmem_unbind(struct kvm_memory_slot *slot)
>  {
> +	struct file *gmem_file;
> +
>  	/*
>  	 * Nothing to do if the underlying file was _already_ closed, as
>  	 * kvm_gmem_release() invalidates and nullifies all bindings.
> @@ -733,21 +735,24 @@ void kvm_gmem_unbind(struct kvm_memory_slot *slot)
>  	/*
>  	 * However, if the file is _being_ closed, then the bindings need to be
>  	 * removed as kvm_gmem_release() might not run until after the memslot
> -	 * is freed.  Note, modifying the bindings is safe even though the file
> -	 * is dying as kvm_gmem_release() nullifies slot->gmem.file under
> +	 * is freed.  Note, dereferencing the dying file is safe as
> +	 * kvm_gmem_release() nullifies slot->gmem.file under
>  	 * slots_lock, and only puts its reference to KVM after destroying all
>  	 * bindings.  I.e. reaching this point means kvm_gmem_release() hasn't
>  	 * yet destroyed the bindings or freed the gmem_file, and can't do so
>  	 * until the caller drops slots_lock.
>  	 */
> -	if (!file) {
> -		__kvm_gmem_unbind(slot, slot->gmem.file->private_data);
> -		return;
> -	}
> +	gmem_file = file ?: slot->gmem.file;
>  
> -	filemap_invalidate_lock(file->f_mapping);
> -	__kvm_gmem_unbind(slot, file->private_data);
> -	filemap_invalidate_unlock(file->f_mapping);
> +	/*
> +	 * Take the invalidate lock even for a dying file.  Otherwise,
> +	 * kvm_gmem_invalidate_start() can find the binding and increment
> +	 * mmu_invalidate_in_progress while kvm_gmem_invalidate_end() misses
> +	 * the removed binding and skips decrement.

Hmm, so as called out in commit ae431059e75d ("KVM: guest_memfd: Remove bindings
on memslot deletion when gmem is dying"), this assumes that file->f_mapping and
everything "underneath" remains valid for dying files, which makes me a bit
uncomfortable.

    Deliberately don't acquire filemap invalid lock when the file is dying as
    the lifecycle of f_mapping is outside the purview of KVM.  Dereferencing
    the mapping is *probably* fine, but there's no need to invalidate anything
    as memslot deletion is responsible for zapping SPTEs, and the only code
    that can access the dying file is kvm_gmem_release(), whose core code is
    mutually exclusive with unbinding.

Oh, but kvm_gmem_release() takes the same filemap_invalidate_unlock() and
holding slots_lock guarantees that this code would run before release() if it
sees a non-null slot->gmem.file, i.e. past me's concern is completely unfounded.

Rather than make this seem like something special, IMO we should treat this as
a more normal thing.  kvm->slots_lock is already load bearing, might as well
double down on that.  I.e. the exceptional part is doing all the work even though
the file is dying, but the flows themselves should be identical.

E.g.

diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
index b596486d184c..5cc043466c89 100644
--- a/virt/kvm/guest_memfd.c
+++ b/virt/kvm/guest_memfd.c
@@ -668,48 +668,40 @@ int kvm_gmem_bind(struct kvm *kvm, struct kvm_memory_slot *slot,
 	return r;
 }
 
-static void __kvm_gmem_unbind(struct kvm_memory_slot *slot, struct gmem_file *f)
+void kvm_gmem_unbind(struct kvm_memory_slot *slot)
 {
+	struct file *file = slot->gmem.file;
 	unsigned long start = slot->gmem.pgoff;
 	unsigned long end = start + slot->npages;
+	struct gmem_file *f;
 
-	xa_store_range(&f->bindings, start, end - 1, NULL, GFP_KERNEL);
-
-	/*
-	 * synchronize_srcu(&kvm->srcu) ensured that kvm_gmem_get_pfn()
-	 * cannot see this memslot.
-	 */
-	WRITE_ONCE(slot->gmem.file, NULL);
-}
-
-void kvm_gmem_unbind(struct kvm_memory_slot *slot)
-{
 	/*
 	 * Nothing to do if the underlying file was _already_ closed, as
 	 * kvm_gmem_release() invalidates and nullifies all bindings.
 	 */
-	if (!slot->gmem.file)
+	if (!file)
 		return;
 
-	CLASS(gmem_get_file, file)(slot);
-
 	/*
 	 * However, if the file is _being_ closed, then the bindings need to be
 	 * removed as kvm_gmem_release() might not run until after the memslot
-	 * is freed.  Note, modifying the bindings is safe even though the file
-	 * is dying as kvm_gmem_release() nullifies slot->gmem.file under
-	 * slots_lock, and only puts its reference to KVM after destroying all
-	 * bindings.  I.e. reaching this point means kvm_gmem_release() hasn't
-	 * yet destroyed the bindings or freed the gmem_file, and can't do so
-	 * until the caller drops slots_lock.
+	 * is freed.  Modifying the bindings is safe even if the file is dying
+	 * as kvm_gmem_release() nullifies slot->gmem.file under slots_lock,
+	 * and only puts its reference to KVM after destroying all bindings.
+	 * I.e. reaching this point means kvm_gmem_release() hasn't destroyed
+	 * the bindings or freed the gmem_file and can't do so until the caller
+	 * drops slots_lock, so there's no need to verify the file is live.
 	 */
-	if (!file) {
-		__kvm_gmem_unbind(slot, slot->gmem.file->private_data);
-		return;
-	}
+	f = file->private_data;
 
 	filemap_invalidate_lock(file->f_mapping);
-	__kvm_gmem_unbind(slot, file->private_data);
+	xa_store_range(&f->bindings, start, end - 1, NULL, GFP_KERNEL);
+
+	/*
+	 * synchronize_srcu(&kvm->srcu) ensured that kvm_gmem_get_pfn()
+	 * cannot see this memslot.
+	 */
+	WRITE_ONCE(slot->gmem.file, NULL);
 	filemap_invalidate_unlock(file->f_mapping);
 }