Re: [PATCH RFC v2 3/4] KVM: guest_memfd: add GUEST_MEMFD_FLAG_MIGRATABLE

On Tue, 2026-07-28 at 09:28 +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [Critical] Accessing memslots in `__kvm_gmem_invalidate_start` without holding `kvm->srcu` allows a use-after-free if the memslot is concurrently deleted while the guest_memfd is being closed.
> - [High] Concurrent execution of `kvm_gmem_unbind` on a dying file and `kvm_gmem_migrate_folio` causes asymmetric `kvm_mmu_invalidate_begin`/`end` pairing, permanently hanging the VM in page faults.
> --
> 
> --- Patch [3]: [PATCH RFC v2 3/4] KVM: guest_memfd: add GUEST_MEMFD_FLAG_MIGRATABLE ---
> commit 5a71d0799db0fa4a13e00aa0d626b30f60552d46
> Author: Shivank Garg <[email protected]>
> 
> KVM: guest_memfd: add GUEST_MEMFD_FLAG_MIGRATABLE
> 
> This commit introduces GUEST_MEMFD_FLAG_MIGRATABLE and
> kvm_arch_supports_gmem_migration() to control migration support. It allocates
> opted-in guest_memfds with GFP_HIGHUSER_MOVABLE, allowing folios to be
> migrated by the memory manager in non-confidential VMs.
> 
> [ ... ]
> 
> > diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
> > index ec881cabe8e3e..8ae455a6b38df 100644
> > --- a/virt/kvm/guest_memfd.c
> > +++ b/virt/kvm/guest_memfd.c
> 
> [ ... ]
> 
> > @@ -623,11 +628,16 @@ static int __kvm_gmem_create(struct kvm *kvm, loff_t size, u64 flags)
> >  	inode->i_mapping->a_ops = &kvm_gmem_aops;
> >  	inode->i_mode |= S_IFREG;
> >  	inode->i_size = size;
> > -	mapping_set_gfp_mask(inode->i_mapping, GFP_HIGHUSER);
> >  	mapping_set_inaccessible(inode->i_mapping);
> > -	mapping_set_unmovable(inode->i_mapping);
> > -	/* Unmovable mappings are supposed to be marked unevictable as well. */
> > -	WARN_ON_ONCE(!mapping_unevictable(inode->i_mapping));
> > +	/* guest_memfd mappings should be marked unevictable. */
> > +	mapping_set_unevictable(inode->i_mapping);
> > +
> > +	if (flags & GUEST_MEMFD_FLAG_MIGRATABLE) {
> 
> [Severity: Critical]
> This isn't a bug introduced by this patch, but enabling migration here
> exposes a pre-existing use-after-free risk in __kvm_gmem_invalidate_start().
> 
> If an unprivileged user creates a migratable guest_memfd and binds it, then
> concurrently closes the file descriptor and deletes the KVM memslot, memory
> compaction can invoke kvm_gmem_migrate_folio() which calls
> __kvm_gmem_invalidate_start().
> 
> Because kvm_gmem_unbind() bypasses filemap_invalidate_lock when the file
> is dying, it does not wait for migration. __kvm_gmem_invalidate_start() then
> dereferences the memslot without holding kvm->srcu:
> 
> virt/kvm/guest_memfd.c:__kvm_gmem_invalidate_start() {
>     xa_for_each_range(&f->bindings, index, slot, start, end - 1) {
>         pgoff_t pgoff = slot->gmem.pgoff;
>         struct kvm_gfn_range gfn_range = {
>             .start = slot->base_gfn + max(pgoff, start) - pgoff,
>         ...
> }
> 
> Is there a risk that the memslot deletion path completes synchronize_srcu()
> and calls kfree(slot) while __kvm_gmem_invalidate_start() is still accessing
> it?
> 
> 
> [Severity: High]
> This is also a pre-existing issue, but exposing the migratable flag makes it
> reachable. Could this lead to asymmetric kvm_mmu_invalidate_begin/end pairing
> and a permanent VM hang?
> 
> When kvm_gmem_unbind() executes concurrently with kvm_gmem_migrate_folio()
> on a dying file, it bypasses the lock and removes the binding from the
> xarray between migration's invalidate_start and invalidate_end:
> 
> virt/kvm/guest_memfd.c:kvm_gmem_unbind() {
>     if (!file) {
>         __kvm_gmem_unbind(slot, slot->gmem.file->private_data);
>         return;
>     }
> }
> 
> When the migration finishes, __kvm_gmem_invalidate_end() conditionally
> checks xa_find():
> 
> 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);
>     }
> }
> 
> Since the binding disappeared, doesn't this skip the _end notification,
> permanently leaking KVM's mmu_invalidate_in_progress counter and causing
> subsequent page faults to infinitely loop in mmu_invalidate_retry_gfn()?
> 
> > +		mapping_set_gfp_mask(inode->i_mapping, GFP_HIGHUSER_MOVABLE);
> > +	} else {
> > +		mapping_set_gfp_mask(inode->i_mapping, GFP_HIGHUSER);
> > +		mapping_set_unmovable(inode->i_mapping);
> > +	}
> >  
> >  	GMEM_I(inode)->flags = flags;
> 

So, these issue will also disappear if I take an invalidate lock for dying
file as well in kvm_gmem_unbind().

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

Thanks,
Shivank