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

On Tue, 2026-07-28 at 09:20 +0000, [email protected] wrote:
> 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 <[email protected]>
> 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.

This is valid point.
So, IIUC, I can remove !file branch and take an invalidate lock for slots-
>gmem.file, if I can't get reference on gmem.file.

	CLASS(gmem_get_file, file)(slot);
	struct file* f = file ? file: slot->gmem.file;

	filemap_invalidate_lock(f->f_mapping);
	__kvm_gmem_unbind(slot, f->private_data);
	filemap_invalidate_unlock(f->f_mapping);

This would prevent it from removing the memslot.

> > +
> > +	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?
> 

Hao Zhang is actively working on this issue. [1]

[1] https://lore.kernel.org/all/[email protected]

Thanks,
Shivank