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

["Garg, Shivank" <shivankg@amd.com>]

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 <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);
>  }


Thanks Sean.
This version is more simple and clear.

Best regards,
Shivank