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

[Bibo Mao]

On 2026/5/13 下午9:52, Sean Christopherson wrote:

+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 <maob...@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-ta...@google.com

yes, it is better as a whole. Waiting for it.

Regards
Bibo Mao

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);

  }