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

[sashiko-bot@kernel.org]

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 <shivankg@amd.com>

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;

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