Re: [PATCH 2/5] KVM: Protect all of kvm_vm_ioctl_create_vcpu() with kvm->lock

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Self-deadlock in KVM vCPU creation due to recursive `kvm->lock` acquisition on PowerPC and s390.
- [Low] Mixing goto-based error handling and guard() cleanup in the same function.
--

commit de58d2836963a7ea1b99cd6c2ad5be4f18984b33
Author: Sean Christopherson <seanjc@google.com>
KVM: Protect all of kvm_vm_ioctl_create_vcpu() with kvm->lock

This commit serializes vCPU creation by holding kvm->lock throughout
the entirety of kvm_vm_ioctl_create_vcpu(). It prevents concurrent
vCPU creations to simplify the code and prevent race conditions.

> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 78cc090435be5..c17cc8dd371b6 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -4165,6 +4165,8 @@ static int kvm_vm_ioctl_create_vcpu(struct kvm *kvm, unsigned long id)
>  	struct kvm_vcpu *vcpu;
>  	struct page *page;
>  
> +	guard(mutex)(&kvm->lock);
> +

[Severity: Critical]
Could this cause a self-deadlock on PowerPC and s390? The guard acquires
kvm->lock for the entire scope of kvm_vm_ioctl_create_vcpu(), but this
function subsequently calls architecture-specific hooks like
kvm_arch_vcpu_create() and kvm_arch_vcpu_postcreate().

On PowerPC, kvm_arch_vcpu_create() eventually calls
kvmppc_core_vcpu_create_hv() in arch/powerpc/kvm/book3s_hv.c, which
explicitly attempts to acquire kvm->lock via mutex_lock(&kvm->lock).

Similarly, on s390, both kvm_s390_vcpu_setup() and
kvm_arch_vcpu_postcreate() in arch/s390/kvm/s390/s390.c call
mutex_lock(&vcpu->kvm->lock).

Since standard Linux mutexes are not recursive, it appears any userspace
process invoking the KVM_CREATE_VCPU ioctl on these architectures will
indefinitely hang the creating thread.

[ ... ]

> @@ -4267,7 +4259,6 @@ static int kvm_vm_ioctl_create_vcpu(struct kvm *kvm, unsigned long id)
>  	atomic_inc(&kvm->online_vcpus);
>  	mutex_unlock(&vcpu->mutex);
>  
> -	mutex_unlock(&kvm->lock);
>  	kvm_arch_vcpu_postcreate(vcpu);
>  	kvm_create_vcpu_debugfs(vcpu);
>  	return r;
> @@ -4278,7 +4269,6 @@ static int kvm_vm_ioctl_create_vcpu(struct kvm *kvm, unsigned long id)
>  	xa_erase(&kvm->vcpu_array, vcpu->vcpu_idx);
>  unlock_vcpu_destroy:
>  	vcpu->vcpu_idx = -1;
> -	mutex_unlock(&kvm->lock);
>  	kvm_dirty_ring_free(&vcpu->dirty_ring);
>  arch_vcpu_destroy:

[Severity: Low]
Does this mix goto-based error handling and scope-based cleanup within
the same function? The cleanup subsystem guidelines specify that a function
should either use goto for cleanup or scope-based cleanup helpers, but
never both.

While the lock is safely released on return here, introducing
guard(mutex)(&kvm->lock) at the top of kvm_vm_ioctl_create_vcpu()
while retaining manual goto cleanup labels like unlock_vcpu_destroy
creates confusing ownership semantics.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260914181223.289061-1-seanjc@google.com?part=2