Re: [PATCH] KVM: selftests: Check guest memfd validity with flags

[Sean Christopherson <seanjc@google.com>]

+Ackerley and Fuad

On Wed, May 13, 2026, Bibo Mao wrote:
> On 2026/5/13 上午7:41, Sean Christopherson wrote:
> > On Fri, May 08, 2026, Bibo Mao wrote:
> > > The type of guest_memfd in structure kvm_userspace_memory_region2
> > > is __u32, it is not correct to assign it with -1 and check whether
> > > it is smaller than 0. Here check flags with KVM_MEM_GUEST_MEMFD
> > > set.
> > > 
> > > Signed-off-by: Bibo Mao <maobibo@loongson.cn>
> > > ---
> > >   tools/testing/selftests/kvm/lib/kvm_util.c | 4 +---
> > >   1 file changed, 1 insertion(+), 3 deletions(-)
> > > 
> > > diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
> > > index 2a76eca7029d..9d3553f7e6a5 100644
> > > --- a/tools/testing/selftests/kvm/lib/kvm_util.c
> > > +++ b/tools/testing/selftests/kvm/lib/kvm_util.c
> > > @@ -817,7 +817,7 @@ static void __vm_mem_region_delete(struct kvm_vm *vm,
> > >   		kvm_munmap(region->mmap_alias, region->mmap_size);
> > >   		close(region->fd);
> > >   	}
> > > -	if (region->region.guest_memfd >= 0)
> > > +	if (region->region.flags & KVM_MEM_GUEST_MEMFD)
> > 
> > Hmm, it's a bit gross, but this is probably more robust?
> > 
> > 	if ((int)region->region.guest_memfd < 0)
> yes, this is more direct, only that some guys in the community do not like
> type conversion. Both are ok for me.
> 
> > 
> > E.g. if we somehow end up in a state where KVM_MEM_GUEST_MEMFD is either stale
> > or the guest_memfd file was already closed.  I highly doubt either of those things
> > will happen, but logically it's the correct fix (the only reason guest_memfd is
> > a u32 is being it's part of the kernel's uAPI).
> Actually it probably will happen, how about something like this:
> -       if (region->region.guest_memfd >= 0)
> +       if ((int)region->region.guest_memfd >= 0) {

LOL, doh.  Yeah, that's what I meant.

>                 close(region->region.guest_memfd);
> +               region->region.guest_memfd = -1;

It's funny how these sorts of things seem to come in bunches.  Can you hold off
on this specific change, and just send a v2 for the fix?  Invalidating guest_memfd
isn't at all necessary here, because region itself is freed shortly thereafter.

But, Ackerley and Fuad want give kvm_vm_release() the same treatment[*], at which
point there's no good reason not to be paranoid.  I want to do that in a dedicated
patch though, and harden "everything" in one shot.  I'll send something like the
below.

[*] https://lore.kernel.org/all/20260511113759.610924-3-tabba@google.com

diff --git tools/testing/selftests/kvm/lib/kvm_util.c tools/testing/selftests/kvm/lib/kvm_util.c
index 2a76eca7029d..2476167252a1 100644
--- tools/testing/selftests/kvm/lib/kvm_util.c
+++ tools/testing/selftests/kvm/lib/kvm_util.c
@@ -737,6 +737,12 @@ userspace_mem_region_find(struct kvm_vm *vm, u64 start, u64 end)
        return NULL;
 }
 
+static void kvm_free_fd(int *fd)
+{
+       kvm_close(*fd);
+       *fd = -1;
+}
+
 static void kvm_stats_release(struct kvm_binary_stats *stats)
 {
        if (stats->fd < 0)
@@ -747,8 +753,7 @@ static void kvm_stats_release(struct kvm_binary_stats *stats)
                stats->desc = NULL;
        }
 
-       kvm_close(stats->fd);
-       stats->fd = -1;
+       kvm_free_fd(&stats->fd);
 }
 
 __weak void vcpu_arch_free(struct kvm_vcpu *vcpu)
@@ -777,7 +782,7 @@ static void vm_vcpu_rm(struct kvm_vm *vm, struct kvm_vcpu *vcpu)
 
        kvm_munmap(vcpu->run, vcpu_mmap_sz());
 
-       kvm_close(vcpu->fd);
+       kvm_free_fd(&vcpu->fd);
        kvm_stats_release(&vcpu->stats);
 
        list_del(&vcpu->list);
@@ -793,8 +798,8 @@ void kvm_vm_release(struct kvm_vm *vmp)
        list_for_each_entry_safe(vcpu, tmp, &vmp->vcpus, list)
                vm_vcpu_rm(vmp, vcpu);
 
-       kvm_close(vmp->fd);
-       kvm_close(vmp->kvm_fd);
+       kvm_free_fd(&vmp->fd);
+       kvm_free_fd(&vmp->kvm_fd);
 
        /* Free cached stats metadata and close FD */
        kvm_stats_release(&vmp->stats);
@@ -815,10 +820,10 @@ static void __vm_mem_region_delete(struct kvm_vm *vm,
        if (region->fd >= 0) {
                /* There's an extra map when using shared memory. */
                kvm_munmap(region->mmap_alias, region->mmap_size);
-               close(region->fd);
+               kvm_free_fd(&region->fd);
        }
        if (region->region.guest_memfd >= 0)
-               close(region->region.guest_memfd);
+               kvm_free_fd((int *)&region->region.guest_memfd);
 
        free(region);
 }