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

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

[Bibo Mao]

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)
 		close(region->region.guest_memfd);
 
 	free(region);
@@ -1101,8 +1101,6 @@ void vm_mem_add(struct kvm_vm *vm, enum vm_mem_backing_src_type src_type,
 
 		region->region.guest_memfd = guest_memfd;
 		region->region.guest_memfd_offset = guest_memfd_offset;
-	} else {
-		region->region.guest_memfd = -1;
 	}
 
 	region->unused_phy_pages = sparsebit_alloc();

base-commit: 74fe02ce122a6103f207d29fafc8b3a53de6abaf
-- 
2.39.3

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

[Sean Christopherson]

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)

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

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

[Bibo Mao]

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) {
                 close(region->region.guest_memfd);
+               region->region.guest_memfd = -1;
+       }

Regards
Bibo Mao

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

[Sean Christopherson]

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

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

[Ackerley Tng]

Sean Christopherson <seanjc@google.com> writes:

>
> [...snip...]
>
> 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.
>

Thanks!

> [*] 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)

Not where the line is drawn between "free" vs "release" in the
selftests, just wanted to draw your attention to the two terms we can
pick the correct term.

> +{
> +       kvm_close(*fd);
> +       *fd = -1;
> +}
> +
>
> [...snip...]
>