Re: [PATCH 25/60] kvm: Move kvm_vcpu->dirty_ring to struct kvm_vcpu_common

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Time-of-Check to Time-of-Use (TOCTOU) race condition in vCPU creation allows exceeding `KVM_MAX_VCPUS`.
- [Low] Mixing goto-based error handling with scope-based `__free(kfree)` cleanup in `kvm_vcpu_init_common`.
--

commit 384dc0ded9d6fc5bf32fefdca7e2933c00f7cb54
Author: Joerg Roedel <joerg.roedel@amd.com>

kvm: Move kvm_vcpu->dirty_ring to struct kvm_vcpu_common

The dirty tracking should happen across all planes of a given VCPU, so
move the dirty_ring to struct kvm_vcpu_common.

> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index f1d528f392e5e..d37e9ee24f0a6 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -483,6 +483,13 @@ static int kvm_vcpu_init_common(struct kvm_vcpu *vcpu, struct kvm *kvm, unsigned
>  	common->ready = false;
>  	preempt_notifier_init(&common->preempt_notifier, &kvm_preempt_ops);
>  
> +	if (kvm->dirty_ring_size) {
> +		r = kvm_dirty_ring_alloc(kvm, &common->dirty_ring,
> +					 id, kvm->dirty_ring_size);
> +		if (r)
> +			goto out_drop_counter;
> +	}
> +

[Severity: Low]
Does this introduce a mix of goto-based error handling and scope-based
cleanup?

Looking at kvm_vcpu_init_common(), common is allocated with __free(kfree):

	struct kvm_vcpu_common *common __free(kfree) =
		kzalloc(sizeof(*common), GFP_KERNEL_ACCOUNT);

But this patch adds a manual goto label (goto out_drop_counter) to handle
error paths. Our subsystem guidelines state that mixing goto-based cleanup
labels and scope-based __free() declarations within the same function
should be avoided.

[Severity: High]
Can this lead to a time-of-check to time-of-use race condition that allows
exceeding the maximum allowed vCPUs?

The bounds check for maximum vCPUs happens under kvm->lock, but then the
lock is dropped:

kvm_vm_ioctl_create_vcpu() {
	mutex_lock(&kvm->lock);
	if (kvm->created_vcpus >= kvm->max_vcpus) {
		mutex_unlock(&kvm->lock);
		return -EINVAL;
	}
	r = kvm_arch_vcpu_precreate(kvm, id);
	mutex_unlock(&kvm->lock);
	...
}

Later, the counter is incremented inside kvm_vcpu_init_common() under a newly
acquired lock:

kvm_vcpu_init_common() {
	...
	mutex_lock(&kvm->lock);
	kvm->created_vcpus++;
	mutex_unlock(&kvm->lock);
	...
}

Does this sequence allow concurrent threads to pass the bounds check before any
of them increment the counter, potentially creating an unbounded number of
vCPUs and bypassing the limit?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260608144252.351443-1-joro@8bytes.org?part=25