Re: [PATCH v3 3/9] KVM: guest_memfd: implement folio migration for non-confidential VMs

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

Pre-existing issues:
- [High] Pre-existing ABBA deadlock between folio lock and filemap_invalidate_lock in kvm_gmem_error_folio
--

--- Patch [3]: [PATCH v3 3/9] KVM: guest_memfd: implement folio migration for non-confidential VMs ---
commit 7a6778b9d1b7f5633978063ee1c7ee906f8f31c4
Author: Shivank Garg <[email protected]>

KVM: guest_memfd: implement folio migration for non-confidential VMs

This commit implements the folio migration callback for guest_memfd using
filemap_migrate_folio(). It explicitly uses a trylock for the invalidate lock
to avoid an ABBA deadlock with the truncation path.

> diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
> index 169f75f954337..583341d593c93 100644
> --- a/virt/kvm/guest_memfd.c
> +++ b/virt/kvm/guest_memfd.c
[ ... ]
> @@ -488,13 +488,45 @@ static struct file_operations kvm_gmem_fops = {
[ ... ]
> +	/*
> +	 * Migration invokes ->migrate_folio() while holding the folio lock.
> +	 * Use a non-blocking trylock to avoid inverting the lock order with
> +	 * truncation, which takes the invalidate lock before locking the
> +	 * folios.
> +	 */
> +	if (!filemap_invalidate_trylock_shared(mapping))
> +		return -EAGAIN;
[ ... ]
>  static int kvm_gmem_error_folio(struct address_space *mapping, struct folio *folio)
>  {
>  	pgoff_t start, end;
>  
>  	filemap_invalidate_lock_shared(mapping);

[Severity: High]
This is a pre-existing issue, but does kvm_gmem_error_folio() suffer from the
same ABBA deadlock that you correctly avoided in the new migration path?

The truncation path in kvm_gmem_punch_hole() acquires the invalidate_lock for 
write, and then acquires folio locks during truncate_inode_pages_range().

Concurrently, if a hardware memory error occurs, memory_failure() acquires the
folio lock before calling the error_remove_folio callback (which is routed to
kvm_gmem_error_folio()). This callback then unconditionally calls 
filemap_invalidate_lock_shared(), resulting in an inversion:

Thread 1 (Truncation):
  kvm_gmem_punch_hole()
    filemap_invalidate_lock(inode->i_mapping);
    truncate_inode_pages_range(...)
      folio_lock(folio);

Thread 2 (Memory Failure):
  memory_failure()
    folio_lock(folio);
    kvm_gmem_error_folio(...)
      filemap_invalidate_lock_shared(mapping);

Should this error path also use filemap_invalidate_trylock_shared() or another
mechanism to avoid the deadlock?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3