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

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

[zhanghao]

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

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

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

[Sean Christopherson]

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
> 

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

[Ackerley Tng]

Sean Christopherson <seanjc@google.com> writes:

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

I think we can go with the stopgap for now, with guest_memfd
conversions, the filemap_invalidate_lock() is relied on for even more.

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

Actually it seems like having the folio locked is part of the contract
of removing it from the filemap. IIUC filemap lookup relies on folio
locking too. If that's correct, then it kind of makes sense that
the .error_remove_folio() callback, which is meant to let filesystems
take folios out of the filemap, is called under the folio lock. Or
perhaps that's an assumption that all filesystems use a filemap?

The folio lock also prevents concurrent faults from happening. At the
point of calling truncate_error_folio() the folio is supposed to already
be unmapped. If we drop the folio lock for a short while, then
truncation can't rely on the folio already being unmapped.

Since guest_memfd kind of monopolizes the folios it owns (on behalf of
the guest), and with the restructuring that will probably come with huge
page support, maybe memory failure for guest_memfd should be specially
handled?

>>
>> [...snip...]
>>