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
