Re: [PATCH] KVM: selftests: verify toggling KVM_MEM_GUEST_MEMFD on an existing slot is rejected

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The commit message incorrectly claims that both directions of toggling KVM_MEM_GUEST_MEMFD are enforced by the XOR flags mask.
- [Low] The second test case reuses a memfd and offset that are already bound, which could lead to shadowed errors if KVM validation ordering changes.
--

--- Patch [1]: [PATCH] KVM: selftests: verify toggling KVM_MEM_GUEST_MEMFD on an existing slot is rejected ---
commit bf15eec5cbe6f11d42dcd195f6bac07b5502de7d
Author: Valery Borovsky <[email protected]>

KVM: selftests: verify toggling KVM_MEM_GUEST_MEMFD on an existing slot is rejected

This commit adds a new test case, test_toggle_private_memory_region(), to
the set_memory_region_test. It verifies that KVM rejects attempts to both
clear KVM_MEM_GUEST_MEMFD on a private slot and set it on a slot created
without it.

> Add test_toggle_private_memory_region() to close that gap. It asserts
> both directions are rejected with EINVAL:
> 
>   1. clearing KVM_MEM_GUEST_MEMFD on a private slot;
>   2. setting KVM_MEM_GUEST_MEMFD on a slot created without it.
> 
> Both are enforced by the flags mask in kvm_set_memory_region():
> 
>   (mem->flags ^ old->flags) & (KVM_MEM_READONLY | KVM_MEM_GUEST_MEMFD)

[Severity: Medium]
Is this explanation entirely accurate for direction 2 (setting the flag)?

Looking at kvm_set_memory_region() in virt/kvm/kvm_main.c, setting
KVM_MEM_GUEST_MEMFD on an existing slot seems to be explicitly rejected
earlier in the function:

if (mem->flags & KVM_MEM_GUEST_MEMFD)
        return -EINVAL;

Could the commit message be updated to clarify that direction 2 is caught by
this early check before the XOR flags mask is evaluated?

> diff --git a/tools/testing/selftests/kvm/set_memory_region_test.c b/tools/testing/selftests/kvm/set_memory_region_test.c
> index a152ab65c6577..8fb2e7a96f6cd 100644
> --- a/tools/testing/selftests/kvm/set_memory_region_test.c
> +++ b/tools/testing/selftests/kvm/set_memory_region_test.c

[ ... ]

> @@ -500,6 +500,50 @@ static void test_add_private_memory_region(void)
>  	kvm_vm_free(vm);
>  }
>  
> +static void test_toggle_private_memory_region(void)
> +{

[ ... ]

> +	/*
> +	 * Symmetrically, attempting to set KVM_MEM_GUEST_MEMFD on a slot
> +	 * that was created without it must also be rejected.
> +	 */
> +	vm_set_user_memory_region(vm, MEM_REGION_SLOT + 1, 0,
> +				  MEM_REGION_GPA * 2, MEM_REGION_SIZE, NULL);
> +	r = __vm_set_user_memory_region2(vm, MEM_REGION_SLOT + 1,
> +					 KVM_MEM_GUEST_MEMFD,
> +					 MEM_REGION_GPA * 2, MEM_REGION_SIZE,
> +					 0, memfd, 0);

[Severity: Low]
Does reusing the same memfd and offset 0 make this test unnecessarily brittle?

This call uses a memfd and offset that were already successfully bound to
MEM_REGION_SLOT earlier in the test function. If KVM were to ever reorder
its internal validation and evaluate kvm_gmem_bind() before rejecting the
immutable flag toggle, wouldn't this return -EEXIST due to the already-bound
range, causing a false test failure instead of the expected -EINVAL?

Would it be safer to use a fresh memfd or a non-overlapping offset to
strictly isolate the flag toggle validation?

> +	TEST_ASSERT(r == -1 && errno == EINVAL,
> +		    "Setting KVM_MEM_GUEST_MEMFD should have failed with EINVAL, got r=%d errno=%d", r, errno);
> +
> +	close(memfd);
> +	kvm_vm_free(vm);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1