Re: [PATCH v5 09/13] KVM: s390: Free the mmu cache when kvm_arch_vcpu_create() fails

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

Pre-existing issues:
- [High] Memory leak of `vcpu->arch.pv.stor_base` and `sida_addr` in `kvm_s390_pv_create_cpu` error path.
--

--- Patch [9]: [PATCH v5 09/13] KVM: s390: Free the mmu cache when kvm_arch_vcpu_create() fails ---
commit b7924aec9fd280e9673810b13295e30b7a659f81
Author: Claudio Imbrenda <[email protected]>

KVM: s390: Free the mmu cache when kvm_arch_vcpu_create() fails

The mmu cache is the first thing that is allocated in
kvm_arch_vcpu_create(), but in case of failure it was not freed.
This patch refactors the function to use scope-based cleanup to ensure
proper cleanup on failure.

> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index c04b7e84c4346..40f33cdcdf84d 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
[ ... ]
> @@ -3861,22 +3860,19 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
>  	trace_kvm_s390_create_vcpu(vcpu->vcpu_id, vcpu, vcpu->arch.sie_block);
>  
>  	rc = kvm_s390_vcpu_setup(vcpu);
> -	if (rc)
> -		goto out_ucontrol_uninit;
> +	if (rc) {

[Severity: High]
This isn't a bug introduced by this patch, but does the error path in
kvm_s390_pv_create_cpu() leak vcpu->arch.pv.stor_base and sida_addr when
called from kvm_s390_vcpu_setup() here?

If uv_call() fails during kvm_s390_pv_create_cpu(), it attempts cleanup
by calling kvm_s390_pv_destroy_cpu():

arch/s390/kvm/pv.c:kvm_s390_pv_create_cpu() {
	...
	if (cc) {
		u16 dummy;

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

However, kvm_s390_pv_destroy_cpu() returns early because the PV handle
is not yet set:

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 assigned only after a successful uv_call(),
it is 0 during this error path. As a result, kvm_s390_pv_destroy_cpu()
does not free the previously allocated stor_base and sida_addr. 

The caller then propagates the error back here, and generic KVM code skips
kvm_arch_vcpu_destroy(), making the memory leak permanent.

> +		if (kvm_is_ucontrol(vcpu->kvm)) {
> +			scoped_guard(spinlock, &vcpu->kvm->arch.gmap->children_lock)
> +				gmap_remove_child(vcpu->arch.gmap);
> +			vcpu->arch.gmap = gmap_put(vcpu->arch.gmap);
> +		}
> +		return rc;
> +	}

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