Re: [PATCH v5 02/31] KVM: Make device name configurable

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Memory allocated for `kvm_dev.name` is leaked if the subsequent `cache_name` allocation fails.
- [Medium] Mixing `goto`-based error handling and `__free()` scope-based cleanup within the same function.
--

--- Patch [2]: [PATCH v5 02/31] KVM: Make device name configurable ---
commit bb225c1b3b23a0088c5fdee909b0a0007a39479e
Author: Steffen Eiden <[email protected]>

KVM: Make device name configurable

This commit allows KVM implementations to choose alternative device names.
This is especially useful for architectures providing multiple KVM
implementations simultaneously.

> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 994cb40cf2efe..18eb3d5d04f4c 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c

[ ... ]

> @@ -6491,23 +6490,39 @@ void kvm_unregister_perf_callbacks(void)
>  }
>  #endif
>  
> -int kvm_init(unsigned vcpu_size, unsigned vcpu_align, struct module *module)
> +int kvm_init(unsigned int vcpu_size, unsigned int vcpu_align, struct module *module)
>  {
> +	return kvm_init_with_dev(vcpu_size, vcpu_align, module, "kvm", KVM_MINOR);
> +}
> +
> +int kvm_init_with_dev(unsigned int vcpu_size, unsigned int vcpu_align,
> +		      struct module *module, const char *dev_name, int minor)
> +{
> +	char *cache_name __free(kfree) = NULL;
>  	int r;
>  	int cpu;
>  
> +	kvm_dev.minor = minor;
> +	kvm_dev.name = kstrdup_const(dev_name, GFP_KERNEL);
> +	if (!kvm_dev.name)
> +		return -ENOMEM;
> +
>  	/* A kmem cache lets us meet the alignment requirements of fx_save. */
>  	if (!vcpu_align)
>  		vcpu_align = __alignof__(struct kvm_vcpu);
> +	cache_name = kasprintf(GFP_KERNEL, "%s_vcpu", dev_name);
> +	if (!cache_name)
> +		return -ENOMEM;

[Severity: Medium]
Does this direct return leak kvm_dev.name?

The kvm_dev.name pointer is allocated via kstrdup_const() just above. If
kasprintf() fails, this path returns -ENOMEM directly instead of jumping
to the err_dev_name label to free the previously allocated memory.

>  	kvm_vcpu_cache =
> -		kmem_cache_create_usercopy("kvm_vcpu", vcpu_size, vcpu_align,
> +		kmem_cache_create_usercopy(cache_name, vcpu_size, vcpu_align,
>  					   SLAB_ACCOUNT,
>  					   offsetof(struct kvm_vcpu, arch),
>  					   offsetofend(struct kvm_vcpu, stats_id)
>  					   - offsetof(struct kvm_vcpu, arch),
>  					   NULL);
> +	r = -ENOMEM;
>  	if (!kvm_vcpu_cache)
> -		return -ENOMEM;
> +		goto err_dev_name;

[Severity: Medium]
Is it intended to mix scope-based cleanup with goto error labels here?

The kvm_init_with_dev() function declares cache_name using __free(kfree):

    char *cache_name __free(kfree) = NULL;

But it also uses traditional goto-based error unwinding throughout the rest
of the function, such as the goto err_dev_name seen here. The cleanup
subsystem guidelines require that functions either convert all resources to
scope-based cleanup or use goto labels exclusively. Mixing them in the same
function can lead to confusing ownership semantics.

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