Re: [PATCH 1/3] KVM: selftests: Fix treating KVM_CAP_MAX_VCPU_ID as inclusive

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The selftest changes assert that setting `bsp_vcpu_id` equal to `max_vcpu_ids` (the exclusive upper bound for vcpu IDs) will be rejected with `-EINVAL`, but the KVM kernel code was not fixed to enforce this and still returns success, causing the tests to fail.
--

--- Patch [1]: [PATCH 1/3] KVM: selftests: Fix treating KVM_CAP_MAX_VCPU_ID as inclusive ---
commit 985e68429c6e20e59d983752bd84f5dc599b83f0
Author: Dmytro Maluka <dmaluka@chromium.org>

KVM: selftests: Fix treating KVM_CAP_MAX_VCPU_ID as inclusive

This commit updates KVM selftests to treat KVM_CAP_MAX_VCPU_ID as
exclusive rather than inclusive. It modifies the tests to assert that
setting a boot CPU ID or vCPU ID equal to or greater than the maximum
capacity will fail.

> diff --git a/tools/testing/selftests/kvm/x86/max_vcpuid_cap_test.c b/tools/testing/selftests/kvm/x86/max_vcpuid_cap_test.c
> index 7e2bfb3c3f3ba..43f16c052de4a 100644
> --- a/tools/testing/selftests/kvm/x86/max_vcpuid_cap_test.c
> +++ b/tools/testing/selftests/kvm/x86/max_vcpuid_cap_test.c
> @@ -30,22 +30,25 @@ int main(int argc, char *argv[])
>  	if (kvm_has_cap(KVM_CAP_SET_BOOT_CPU_ID)) {
>  		vm_ioctl(vm, KVM_SET_BOOT_CPU_ID, (void *)MAX_VCPU_ID);
>  
> -		/* Try setting KVM_CAP_MAX_VCPU_ID below BOOT_CPU_ID */
> -		ret = __vm_enable_cap(vm, KVM_CAP_MAX_VCPU_ID, MAX_VCPU_ID - 1);
> +		/* Try setting KVM_CAP_MAX_VCPU_ID below or equal to BOOT_CPU_ID */
> +		ret = __vm_enable_cap(vm, KVM_CAP_MAX_VCPU_ID, MAX_VCPU_ID);
>  		TEST_ASSERT(ret < 0,
> -			    "Setting KVM_CAP_MAX_VCPU_ID below BOOT_CPU_ID should fail");
> +			    "Setting KVM_CAP_MAX_VCPU_ID <= BOOT_CPU_ID should fail");

[Severity: Medium]
Will this assertion now fail because the kernel code hasn't been updated to
enforce this behavior?

Looking at how KVM_CAP_MAX_VCPU_ID is handled in the kernel, it checks:

    if (kvm->arch.bsp_vcpu_id > cap->args[0]) {
        r = -EINVAL;
        break;
    }

If both kvm->arch.bsp_vcpu_id and cap->args[0] are set to MAX_VCPU_ID, the
greater-than condition evaluates to false. Will the ioctl return success
instead of -EINVAL, causing this test to crash?

[ ... ]

> diff --git a/tools/testing/selftests/kvm/x86/set_boot_cpu_id.c b/tools/testing/selftests/kvm/x86/set_boot_cpu_id.c
> index 8e3898646c69d..1c4ebdf8e82c5 100644
> --- a/tools/testing/selftests/kvm/x86/set_boot_cpu_id.c
> +++ b/tools/testing/selftests/kvm/x86/set_boot_cpu_id.c
> @@ -39,8 +39,8 @@ static void test_set_invalid_bsp(struct kvm_vm *vm)
>  	int r;
>  
>  	if (max_vcpu_id) {
> -		r = __vm_ioctl(vm, KVM_SET_BOOT_CPU_ID, (void *)(max_vcpu_id + 1));
> -		TEST_ASSERT(r == -1 && errno == EINVAL, "BSP with ID > MAX should fail");
> +		r = __vm_ioctl(vm, KVM_SET_BOOT_CPU_ID, (void *)max_vcpu_id);
> +		TEST_ASSERT(r == -1 && errno == EINVAL, "BSP with ID >= MAX should fail");

[Severity: Medium]
Does this change also cause the test to fail against the current kernel?

Looking at the KVM_SET_BOOT_CPU_ID handling in arch/x86/kvm/x86.c:kvm_vm_ioctl():

    else if (arg > KVM_MAX_VCPU_IDS ||
             (kvm->arch.max_vcpu_ids && arg > kvm->arch.max_vcpu_ids))
        r = -EINVAL;

Since the kernel checks if arg is greater than max_vcpu_ids instead of
greater than or equal to, will setting arg to max_vcpu_id return success
here and trigger the TEST_ASSERT failure?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260730182425.1223530-1-dmaluka@chromium.org?part=1