Re: [PATCH v2] KVM: guest_memfd: Fix ABBA deadlock in error_remove_folio

[Sean Christopherson <seanjc@google.com>]

On Mon, Jun 15, 2026, zhanghao wrote:
> >From e7f25639c05eac51b4ac8add3dd3e3a76f6f7340 Mon Sep 17 00:00:00 2001
> From: Hao Zhang <zhanghao1@kylinos.cn>
> Date: Thu, 11 Jun 2026 15:27:27 +0800
> 
> memory_failure() calls ->error_remove_folio() while holding the poisoned
> folio lock.  guest_memfd's implementation takes mapping.invalidate_lock for
> read before zapping KVM mappings.
> 
> That lock ordering can deadlock against paths that hold
> mapping.invalidate_lock for write and then try to lock the same folio, e.g.
> guest_memfd punch-hole via truncate_inode_pages_range().
> 
> The invalidate lock is needed for page-cache invalidation, but
> ->error_remove_folio() only needs stable guest_memfd bindings while walking
> them to zap KVM mappings.  Add a guest_memfd-private rwsem to protect
> established gmem file entries and their bindings during invalidation and
> removal, and use it in ->error_remove_folio() instead of
> mapping.invalidate_lock.  Update bind, unbind, and release paths to take
> the new lock when modifying bindings or removing a gmem file entry.
> 
> Keep taking mapping.invalidate_lock before the bindings lock in paths that
> need both locks, so writers cannot queue on the bindings lock while a
> punch-hole operation is blocked on a folio lock.
> 
> Fixes: a7800aa80ea4 ("KVM: Add KVM_CREATE_GUEST_MEMFD ioctl() for guest-specific backing memory")
> Signed-off-by: Hao Zhang <zhanghao1@kylinos.cn>
> ---
> Changes in v2:
> - add a guest_memfd-private rwsem to protect the
>   gmem_file_list and each gmem_file's bindings
> 
> Link to v1: https://lore.kernel.org/all/tencent_4B051B84CDCC0D1964EC00337AA32E40DC07@qq.com/
> 
>  virt/kvm/guest_memfd.c | 38 ++++++++++++++++++++++++++++++--------
>  1 file changed, 30 insertions(+), 8 deletions(-)

Well shoot. I know I suggested this approach, but oof is it uglier than I was
expecting.  I forgot that it's not just the bindings that need to be protected,
the list of files also needs to be protected.

And thinking about this more, I'm not convinced a separate bindings lock is
sufficient, as a concurrent __kvm_gmem_populate() could establish mappings after
kvm_gmem_error_folio() completes, if kvm_gmem_error_folio() runs between
folio_unlock() and post_populate().

So for a stopgap, I'm leaning very strongly towards going with the trylock
approach you proposed in v1.  Longer term (or maybe even as an immediate fix?),
I think the ideal solution would be to rework truncate_error_folio() flows to
allow .error_remove_folio() hooks to unlock the folio.

> diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
> index 69c9d6d546b2..2ca68d09e2d3 100644
> --- a/virt/kvm/guest_memfd.c
> +++ b/virt/kvm/guest_memfd.c
> @@ -6,6 +6,7 @@
>  #include <linux/kvm_host.h>
>  #include <linux/mempolicy.h>
>  #include <linux/pseudo_fs.h>
> +#include <linux/rwsem.h>
>  #include <linux/pagemap.h>
>  
>  #include "kvm_mm.h"
> @@ -32,6 +33,13 @@ struct gmem_inode {
>  	struct inode vfs_inode;
>  	struct list_head gmem_file_list;
>  
> +	/*
> +	 * Serializes established gmem_file entries and bindings with
> +	 * invalidations that don't hold mapping->invalidate_lock, e.g.
> +	 * ->error_remove_folio().
> +	 */
> +	struct rw_semaphore bindings_lock;
> +
>  	u64 flags;
>  };
>  
> @@ -344,6 +352,7 @@ static int kvm_gmem_release(struct inode *inode, struct file *file)
>  
>  	filemap_invalidate_lock(inode->i_mapping);
>  
> +	down_write(&GMEM_I(inode)->bindings_lock);
>  	xa_for_each(&f->bindings, index, slot)
>  		WRITE_ONCE(slot->gmem.file, NULL);
>  
> @@ -357,6 +366,7 @@ static int kvm_gmem_release(struct inode *inode, struct file *file)
>  	__kvm_gmem_invalidate_end(f, 0, -1ul);
>  
>  	list_del(&f->entry);
> +	up_write(&GMEM_I(inode)->bindings_lock);
>  
>  	filemap_invalidate_unlock(inode->i_mapping);
>  
> @@ -499,11 +509,10 @@ static int kvm_gmem_error_folio(struct address_space *mapping, struct folio *fol
>  {
>  	pgoff_t start, end;
>  
> -	filemap_invalidate_lock_shared(mapping);
> -
>  	start = folio->index;
>  	end = start + folio_nr_pages(folio);
>  
> +	down_read(&GMEM_I(mapping->host)->bindings_lock);
>  	kvm_gmem_invalidate_begin(mapping->host, start, end);
>  
>  	/*
> @@ -516,8 +525,7 @@ static int kvm_gmem_error_folio(struct address_space *mapping, struct folio *fol
>  	 */
>  
>  	kvm_gmem_invalidate_end(mapping->host, start, end);
> -
> -	filemap_invalidate_unlock_shared(mapping);
> +	up_read(&GMEM_I(mapping->host)->bindings_lock);
>  
>  	return MF_DELAYED;
>  }
> @@ -673,10 +681,11 @@ int kvm_gmem_bind(struct kvm *kvm, struct kvm_memory_slot *slot,
>  	start = offset >> PAGE_SHIFT;
>  	end = start + slot->npages;
>  
> +	down_write(&GMEM_I(inode)->bindings_lock);
>  	if (!xa_empty(&f->bindings) &&
>  	    xa_find(&f->bindings, &start, end - 1, XA_PRESENT)) {
> -		filemap_invalidate_unlock(inode->i_mapping);
> -		goto err;
> +		r = -EINVAL;
> +		goto err_unlock;
>  	}
>  
>  	/*
> @@ -690,6 +699,10 @@ int kvm_gmem_bind(struct kvm *kvm, struct kvm_memory_slot *slot,
>  		slot->flags |= KVM_MEMSLOT_GMEM_ONLY;
>  
>  	xa_store_range(&f->bindings, start, end - 1, slot, GFP_KERNEL);
> +	r = 0;
> +
> +err_unlock:
> +	up_write(&GMEM_I(inode)->bindings_lock);
>  	filemap_invalidate_unlock(inode->i_mapping);
>  
>  	/*
> @@ -697,7 +710,6 @@ int kvm_gmem_bind(struct kvm *kvm, struct kvm_memory_slot *slot,
>  	 * not the other way 'round.  Active bindings are invalidated if the
>  	 * file is closed before memslots are destroyed.
>  	 */
> -	r = 0;
>  err:
>  	fput(file);
>  	return r;
> @@ -739,12 +751,21 @@ void kvm_gmem_unbind(struct kvm_memory_slot *slot)
>  	 * until the caller drops slots_lock.
>  	 */
>  	if (!file) {
> -		__kvm_gmem_unbind(slot, slot->gmem.file->private_data);
> +		struct file *slot_file = slot->gmem.file;
> +		struct inode *inode = file_inode(slot_file);
> +
> +		filemap_invalidate_lock(inode->i_mapping);
> +		down_write(&GMEM_I(inode)->bindings_lock);
> +		__kvm_gmem_unbind(slot, slot_file->private_data);
> +		up_write(&GMEM_I(inode)->bindings_lock);
> +		filemap_invalidate_unlock(inode->i_mapping);
>  		return;
>  	}
>  
>  	filemap_invalidate_lock(file->f_mapping);
> +	down_write(&GMEM_I(file_inode(file))->bindings_lock);
>  	__kvm_gmem_unbind(slot, file->private_data);
> +	up_write(&GMEM_I(file_inode(file))->bindings_lock);
>  	filemap_invalidate_unlock(file->f_mapping);
>  }
>  
> @@ -946,6 +967,7 @@ static struct inode *kvm_gmem_alloc_inode(struct super_block *sb)
>  
>  	gi->flags = 0;
>  	INIT_LIST_HEAD(&gi->gmem_file_list);
> +	init_rwsem(&gi->bindings_lock);
>  	return &gi->vfs_inode;
>  }
>  
> 
> base-commit: 9716c086c8e8b141d35aa61f2e96a2e83de212a7
> -- 
> 2.15.0
>