On Tue, 2026-08-25 at 15:13 -0700, Sean Christopherson wrote:
> 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 <[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 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);
> }
Thanks Sean.
This version is more simple and clear.
Best regards,
Shivank
