Re: [PATCH RFC v2 2/4] KVM: guest_memfd: implement folio migration for non-confidential VMs

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Pre-existing ABBA deadlock between `mapping->invalidate_lock` and `folio_lock` via `kvm_gmem_error_folio`.
- [High] Unpaired `kvm_mmu_invalidate_start` due to lockless `f->bindings` xarray modification during `kvm_gmem_unbind`.
--

--- Patch [2]: [PATCH RFC v2 2/4] KVM: guest_memfd: implement folio migration for non-confidential VMs ---
commit cb30b8cd0e73b5e7ad80d0285306810cbf4d963e
Author: Shivank Garg <shivankg@amd.com>
KVM: guest_memfd: implement folio migration for non-confidential VMs

Implement kvm_gmem_migrate_folio using filemap_migrate_folio for
non-confidential VMs but don't enable it yet. Guest_memfd mappings are still
marked unmovable, so MM never calls it.

> diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
> index d3b1186b03b3a..ec881cabe8e3e 100644
> --- a/virt/kvm/guest_memfd.c
> +++ b/virt/kvm/guest_memfd.c
[ ... ]
> +	start = src->index;
> +	end = start + folio_nr_pages(src);
> +
> +	kvm_gmem_invalidate_start(inode, start, end);
> +
> +	/*
> +	 * For non-confidential guests the folio is host-readable, so
> +	 * filemap_migrate_folio() can copy the contents itself via
> +	 * folio_mc_copy().
> +	 * For confidential guests, this would need firmware assistance.
> +	 */
> +	ret = filemap_migrate_folio(mapping, dst, src, mode);
> +
> +	kvm_gmem_invalidate_end(inode, start, end);

[Severity: High]
This is a pre-existing issue, but does the iteration over bindings here race
with kvm_gmem_unbind?

If a guest_memfd file is being closed and its refcount reaches zero
concurrently, kvm_gmem_unbind takes the !file branch and removes the memslot
from f->bindings without taking filemap_invalidate_lock.

Could this lockless modification happen between the calls to
kvm_gmem_invalidate_start and kvm_gmem_invalidate_end?

If f->bindings is modified during this window, __kvm_gmem_invalidate_end
might fail to find the binding and skip the slot, resulting in an unpaired
start call. This would permanently leak the mmu_invalidate_in_progress
counter and cause guest page faults to hang indefinitely.

> +
> +	filemap_invalidate_unlock_shared(mapping);
> +	return ret;
>  }
> +#else
> +#define kvm_gmem_migrate_folio NULL
> +#endif
>  
>  static int kvm_gmem_error_folio(struct address_space *mapping, struct folio *folio)
>  {

[Severity: High]
This isn't a bug introduced by this patch, but does kvm_gmem_error_folio
have the exact same ABBA deadlock that you correctly avoided above in
kvm_gmem_migrate_folio?

In kvm_gmem_error_folio, it unconditionally calls
filemap_invalidate_lock_shared while the caller (memory_failure) already
holds the folio lock.

Meanwhile, the opposing truncation path in kvm_gmem_punch_hole establishes
the strict lock ordering of acquiring mapping->invalidate_lock followed by
locking the folio.

Could a concurrent fallocate PUNCH_HOLE and a memory failure on the same
guest_memfd file deadlock the kernel here?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260728-shivank-gmem-migrate-v2-0-269ac1f84e2b@amd.com?part=2