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 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 --- 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