[PATCH v3] KVM: guest_memfd: Avoid deadlock in error_remove_folio()

[PATCH v3] KVM: guest_memfd: Avoid deadlock in error_remove_folio()

[Hao Zhang]

From: Hao Zhang <zhanghao1@kylinos.cn>

memory_failure() invokes ->error_remove_folio() while holding the
poisoned folio lock.  KVM's guest_memfd callback then takes
mapping->invalidate_lock for read before invalidating guest mappings.

This can deadlock with paths that hold mapping->invalidate_lock for write
and then try to lock the same folio, e.g. truncation from MADV_REMOVE or
FALLOC_FL_PUNCH_HOLE.  Because rwsems are fair, the problem is not limited
to guest_memfd punch-hole: a pending writer can also block future readers,
so ->error_remove_folio() must not block on the invalidate lock while
holding the poisoned folio lock.

Use filemap_invalidate_trylock_shared() in kvm_gmem_error_folio().  If the
invalidate lock is contended, fail the recovery instead of potentially
deadlocking the memory-failure path.

Fixes: a7800aa80ea4 ("KVM: Add KVM_CREATE_GUEST_MEMFD ioctl() for guest-specific backing memory")
Signed-off-by: Hao Zhang <zhanghao1@kylinos.cn>
---
Changes in v3:
 - Drop the guest_memfd-private bindings_lock approach from v2 and go back
   to the trylock stopgap from v1, as suggested by Sean.
 - Rewrite the changelog and code comment to describe the problematic lock
   dependency as the poisoned folio lock vs. mapping->invalidate_lock, and
   avoid implying that mf_mutex is part of the actual deadlock.
 - Account for rwsem fairness: a pending writer can block future readers, so
   the issue is not limited to guest_memfd punch-hole.

Link to v2: https://lore.kernel.org/all/tencent_7DA064CAC9B463EB9B651801011E2CE4230A@qq.com/

 virt/kvm/guest_memfd.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
index db57c5766ab6..6476ab4a2be7 100644
--- a/virt/kvm/guest_memfd.c
+++ b/virt/kvm/guest_memfd.c
@@ -500,7 +500,17 @@ static int kvm_gmem_error_folio(struct address_space *mapping, struct folio *fol
 {
 	pgoff_t start, end;
 
-	filemap_invalidate_lock_shared(mapping);
+	/*
+	 * memory_failure() invokes ->error_remove_folio() while holding the
+	 * poisoned folio lock.  Do not block on mapping->invalidate_lock, as
+	 * a writer that already holds the invalidate lock can be waiting for
+	 * the same folio, e.g. truncation from MADV_REMOVE/FALLOC_FL_PUNCH_HOLE.
+	 *
+	 * If the invalidate lock is contended, fail the recovery instead of
+	 * risking a deadlock in the memory-failure path.
+	 */
+	if (!filemap_invalidate_trylock_shared(mapping))
+		return -EBUSY;
 
 	start = folio->index;
 	end = start + folio_nr_pages(folio);

base-commit: 58717b2a1365d06c8c64b72aa948541b53fe31eb
-- 
2.39.2

Re: [PATCH v3] KVM: guest_memfd: Avoid deadlock in error_remove_folio()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Asynchronous memslot unbinding can leak kvm->mmu_invalidate_in_progress when racing with kvm_gmem_error_folio(), causing a permanent KVM page fault hang.
--

--- Patch [1]: [PATCH v3] KVM: guest_memfd: Avoid deadlock in error_remove_folio() ---
commit db14f829c7993aef911f52935c9113e37c1a49ea
Author: Hao Zhang <zhanghao1@kylinos.cn>

KVM: guest_memfd: Avoid deadlock in error_remove_folio()

memory_failure() invokes ->error_remove_folio() while holding the
poisoned folio lock. KVM's guest_memfd callback then takes
mapping->invalidate_lock for read before invalidating guest mappings.
This can deadlock with paths that hold mapping->invalidate_lock for write.

Link: https://lore.kernel.org/all/tencent_7DA064CAC9B463EB9B651801011E2CE4230A@qq.com/

> diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
> index db57c5766ab60..6476ab4a2be79 100644
> --- a/virt/kvm/guest_memfd.c
> +++ b/virt/kvm/guest_memfd.c
> @@ -500,7 +500,17 @@ static int kvm_gmem_error_folio(struct address_space *mapping, struct folio *fol
>  {
>  	pgoff_t start, end;
>  
> -	filemap_invalidate_lock_shared(mapping);
> +	/*
> +	 * memory_failure() invokes ->error_remove_folio() while holding the
> +	 * poisoned folio lock.  Do not block on mapping->invalidate_lock, as
> +	 * a writer that already holds the invalidate lock can be waiting for
> +	 * the same folio, e.g. truncation from MADV_REMOVE/FALLOC_FL_PUNCH_HOLE.
> +	 *
> +	 * If the invalidate lock is contended, fail the recovery instead of
> +	 * risking a deadlock in the memory-failure path.
> +	 */
> +	if (!filemap_invalidate_trylock_shared(mapping))
> +		return -EBUSY;
>  
>  	start = folio->index;
>  	end = start + folio_nr_pages(folio);

[Severity: High]
This is a pre-existing issue, but can this function race with memslot
unbinding and permanently leak kvm->mmu_invalidate_in_progress, causing a
KVM page fault hang?

If userspace closes the guest_memfd file descriptor, dropping its refcount
to 0, and concurrently deletes the memslot, kvm_gmem_unbind() gets called.
Because the file refcount is 0, it takes the lockless path and modifies
f->bindings without holding mapping->invalidate_lock:

virt/kvm/guest_memfd.c:kvm_gmem_unbind() {
    ...
	if (!file) {
		__kvm_gmem_unbind(slot, slot->gmem.file->private_data);
		return;
	}
    ...
}

Concurrently, if a hardware memory failure triggers kvm_gmem_error_folio(),
it acquires the invalidate_lock via filemap_invalidate_trylock_shared().
It then calls kvm_gmem_invalidate_start(), which finds the memslot and
increments the invalidation counter:

virt/kvm/guest_memfd.c:__kvm_gmem_invalidate_start() {
    ...
		if (!found_memslot) {
			found_memslot = true;

			KVM_MMU_LOCK(kvm);
			kvm_mmu_invalidate_start(kvm);
		}
    ...
}

But because __kvm_gmem_unbind() concurrently removes the binding from
f->bindings without synchronization against invalidate_lock readers,
__kvm_gmem_invalidate_end() then fails to find the binding and skips the
decrement:

virt/kvm/guest_memfd.c:__kvm_gmem_invalidate_end() {
    ...
	if (xa_find(&f->bindings, &start, end - 1, XA_PRESENT)) {
		KVM_MMU_LOCK(kvm);
		kvm_mmu_invalidate_end(kvm);
		KVM_MMU_UNLOCK(kvm);
	}
    ...
}

Could this permanent leak cause the KVM page fault retry protocol to
unconditionally return true, throwing the vCPU into an infinite page fault
loop?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/ambEdSPjerZIVN0b@192.168.1.215?part=1