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

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The patch modifies a KVM selftest to avoid an invalid configuration but sweeps an underlying kernel bug under the rug by failing to fix the kernel and masking the issue in the test suite.
--

--- Patch [1]: [PATCH v2 1/4] KVM: selftests: Fix treating KVM_CAP_MAX_VCPU_ID as inclusive ---
commit 5e2afeaca4e83d31005f9aa5caf94dbbc9f20b8c
Author: Dmytro Maluka <[email protected]>

KVM: selftests: Fix treating KVM_CAP_MAX_VCPU_ID as inclusive

This patch modifies the max_vcpuid_cap_test selftest to account for
KVM_CAP_MAX_VCPU_ID being exclusive (maximum allowed vcpu_id plus one).
It changes the test to use MAX_VCPU_ID + 1 when enabling the capability,
avoiding the boundary condition.

> 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..47ee14967873f 100644
> --- a/tools/testing/selftests/kvm/x86/max_vcpuid_cap_test.c
> +++ b/tools/testing/selftests/kvm/x86/max_vcpuid_cap_test.c
[ ... ]
> @@ -36,16 +36,19 @@ int main(int argc, char *argv[])
>  			    "Setting KVM_CAP_MAX_VCPU_ID below BOOT_CPU_ID should fail");
>  	}
>  
> -	/* Set KVM_CAP_MAX_VCPU_ID */
> -	vm_enable_cap(vm, KVM_CAP_MAX_VCPU_ID, MAX_VCPU_ID);
> +	/*
> +	 * Set KVM_CAP_MAX_VCPU_ID. Note: KVM_CAP_MAX_VCPU_ID is a misnomer,
> +	 * it actually represents maximum vcpu_id plus one.
> +	 */
> +	vm_enable_cap(vm, KVM_CAP_MAX_VCPU_ID, MAX_VCPU_ID + 1);

[Severity: Medium]
Does this patch mask an underlying kernel bug rather than fixing it?

The commit message notes that setting bsp_vcpu_id equal to KVM_CAP_MAX_VCPU_ID
is invalid. However, by changing the test to avoid this exact boundary
condition, it seems we are leaving the kernel's off-by-one bug unfixed.

Looking at the kernel side in arch/x86/kvm/x86.c:kvm_vm_ioctl_enable_cap():

    if (kvm->arch.bsp_vcpu_id > cap->args[0]) {

This bounds check uses ">" instead of ">=", which appears to incorrectly allow
bsp_vcpu_id to equal max_vcpu_ids. 

Should the kernel be fixed to reject this invalid configuration, and the
selftest be updated with a negative test to ensure it actually fails?

>  
>  	/* Try to set KVM_CAP_MAX_VCPU_ID again */
> -	ret = __vm_enable_cap(vm, KVM_CAP_MAX_VCPU_ID, MAX_VCPU_ID + 1);
> +	ret = __vm_enable_cap(vm, KVM_CAP_MAX_VCPU_ID, MAX_VCPU_ID + 2);
>  	TEST_ASSERT(ret < 0,
>  		    "Setting KVM_CAP_MAX_VCPU_ID multiple times should fail");
>  
>  	/* Create vCPU with id beyond KVM_CAP_MAX_VCPU_ID cap */
> -	ret = __vm_ioctl(vm, KVM_CREATE_VCPU, (void *)MAX_VCPU_ID);
> +	ret = __vm_ioctl(vm, KVM_CREATE_VCPU, (void *)(MAX_VCPU_ID + 1));
>  	TEST_ASSERT(ret < 0, "Creating vCPU with ID > MAX_VCPU_ID should fail");

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