Re: [PATCH v2 5/7] KVM: s390: ucontrol: Add missing locking around gmap_remove_child()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [Low] The patch introduces a scoped_guard() into a function that already uses goto-based error handling.

Pre-existing issues:
- [High] Missing cleanup for vcpu->arch.mc in the error paths of kvm_arch_vcpu_create().
- [High] Preexisting memory leak of vcpu->arch.pv.stor_base and sida_addr in kvm_s390_pv_create_cpu() on UV call failure.
--

--- Patch [5]: [PATCH v2 5/7] KVM: s390: ucontrol: Add missing locking around gmap_remove_child() ---
commit 08ba9ce959b1a049a33ef520bb8230d16cec1943
Author: Claudio Imbrenda <imbrenda@linux.ibm.com>

KVM: s390: ucontrol: Add missing locking around gmap_remove_child()

This commit adds missing locking for the parent gmap's children_lock when
calling gmap_remove_child(). This fixes an issue in the error handling path
of kvm_arch_vcpu_create() for UCONTROL guests.

> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
[ ... ]
> @@ -3866,7 +3866,8 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
>  
>  out_ucontrol_uninit:
>  	if (kvm_is_ucontrol(vcpu->kvm)) {
> -		gmap_remove_child(vcpu->arch.gmap);
> +		scoped_guard(spinlock, &vcpu->kvm->arch.gmap->children_lock)
> +			gmap_remove_child(vcpu->arch.gmap);

[Severity: Low]
Does this mix scoped_guard() with goto-based error handling?

The cleanup guidelines suggest converting all resources that need a goto 
cleanup to scope-based cleanup, or converting none of them, rather than 
mixing them in the same function.

>  		vcpu->arch.gmap = gmap_put(vcpu->arch.gmap);
>  	}
>  out_free_sie_block:
>  	free_page((unsigned long)(vcpu->arch.sie_block));
>  	return rc;
>  }

[Severity: High]
This is a pre-existing issue, but does this error path leak vcpu->arch.mc?

When kvm_arch_vcpu_create() allocates vcpu->arch.mc via 
kvm_s390_new_mmu_cache(), does the out_free_sie_block path miss freeing it 
before returning rc? 

Since kvm_arch_vcpu_destroy() isn't invoked on creation failure, it seems 
vcpu->arch.mc might remain permanently allocated.


[Severity: High]
This is a pre-existing issue, but does kvm_s390_pv_create_cpu() leak 
vcpu->arch.pv.stor_base and sida_addr if uv_call() fails?

In arch/s390/kvm/pv.c:kvm_s390_pv_create_cpu():

    if (cc) {
        ...
        kvm_s390_pv_destroy_cpu(vcpu, &dummy, &dummy);
        return -EIO;
    }

If this fails, it calls kvm_s390_pv_destroy_cpu():

arch/s390/kvm/pv.c:kvm_s390_pv_destroy_cpu():

    if (!kvm_s390_pv_cpu_get_handle(vcpu))
        return 0;

Since vcpu->arch.pv.handle is only populated on the uv_call() success path, 
will this return early and skip cleaning up stor_base and sida_addr?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260713150857.269954-1-imbrenda@linux.ibm.com?part=5