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

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 slots->gmem.file, until
kvm_gmem_unbind() finishes.

Reported-by: Sashiko <[email protected]>
Closes: https://lore.kernel.org/all/[email protected]
Fixes: ae431059e75d ("KVM: guest_memfd: Remove bindings on memslot deletion when gmem is dying")
Signed-off-by: Shivank Garg <[email protected]>
---
 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 db57c5766ab6..45cbdf4801ec 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.
+	 */
+	filemap_invalidate_lock(gmem_file->f_mapping);
+	__kvm_gmem_unbind(slot, gmem_file->private_data);
+	filemap_invalidate_unlock(gmem_file->f_mapping);
 }
 
 /* Returns a locked folio on success.  */

-- 
2.43.0