Re: [PATCH v7 16/42] KVM: guest_memfd: Determine invalidation filter from memory attributes

[Sean Christopherson <seanjc@google.com>]

On Wed, May 27, 2026, Ackerley Tng wrote:
> > In __kvm_gmem_invalidate_begin(), the code iterates over f->bindings using
> > xa_for_each_range(). The retrieved slot pointers are protected by the kvm
> > srcu, but the read lock doesn't appear to be held during the iteration:
> >
> > static void __kvm_gmem_invalidate_begin(...)
> > {
> > 	...
> > 	xa_for_each_range(&f->bindings, index, slot, start, end - 1) {
> > 		pgoff_t pgoff = slot->gmem.pgoff;
> > 	...
> >
> > When a memory slot is deleted, the code calls kvm_gmem_unbind() and
> > kfree(slot) after synchronize_srcu(&kvm->srcu). If the read lock isn't held,
> > could a concurrent memslot deletion free the slot while it is still being
> > accessed in this loop?
> >
> 
> The issue here is between __kvm_gmem_unbind() and
> __kvm_gmem_invalidate_begin(). Since f->bindings is protected by
> filemap_invalidate_lock(), I'm dividing the analysis into two cases
> where accessing slot through f->bindings with and without holding
> filemap_invalidate_lock().
> 
> ## Holding filemap_invalidate_lock()
> 
> If unbind happens before invalidate, we have
> 
>     filemap_invalidate_lock()
>     __kvm_gmem_unbind(), would have removed slot from f->bindings.
>     filemap_invalidate_unlock()
> 
>     After this, slot can be freed.
> 
>     filemap_invalidate_lock()
>     __kvm_gmem_invalidate_begin(), which iterates f->bindings and doesn't
>        see bound slot.
>     filemap_invalidate_unlock()
> 
> ## Not holding filemap_invalidate_lock()
> 
> In kvm_gmem_unbind(), __kvm_gmem_unbind() can be called without taking
> the filemap_invalidate_lock() if !file. The only places where NULL is
> written to slot->gmem.file is kvm_gmem_release() and __kvm_gmem_unbind()
> itself.
> 
> kvm_gmem_release() is prevented from racing with kvm_gmem_unbind() since
> kvm_gmem_release() holds slots_lock, so since kvm_gmem_unbind() is
> called while holding slots_lock, it should either see a guest_memfd
> memslot with a file or no slot at all? Perhaps I'm missing something
> about get_file_active().

The comments in the code pretty much say it all:

kvm_gmem_release():

	/*
	 * Prevent concurrent attempts to *unbind* a memslot.  This is the last
	 * reference to the file and thus no new bindings can be created, but
	 * dereferencing the slot for existing bindings needs to be protected
	 * against memslot updates, specifically so that unbind doesn't race
	 * and free the memslot (kvm_gmem_get_file() will return NULL).
	 *
	 * Since .release is called only when the reference count is zero,
	 * after which file_ref_get() and get_file_active() fail,
	 * kvm_gmem_get_pfn() cannot be using the file concurrently.
	 * file_ref_put() provides a full barrier, and get_file_active() the
	 * matching acquire barrier.
	 */

kvm_gmem_unbind():

	/*
	 * 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.
	 */

On a related topic, I posted a patch a while back to clarify exactly why the
release() code is safe.  I need to get back to that, but in the meantime, more
eyeballs would be appreciated.

https://lore.kernel.org/all/20251113232229.1698886-1-seanjc@google.com

> Would like Sean to check my understanding.

You're not missing anything, Sashiko struggles with scenarios where thing X
makes thing Y impossible.