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

[sashiko-bot@kernel.org]

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