Re: [PATCH v6 02/33] KVM: Make device name configurable

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Calling `kvm_init_with_dev()` multiple times within the same module to support multiple simultaneous KVM implementations corrupts KVM's static singleton state and guarantees list corruption.
- [High] `kvm_init_with_dev` is not exported, making it unusable by independent loadable KVM architecture modules.
- [Medium] The patch fails to parameterize the `kvm_gmem_inode_cache` name, causing a sysfs kmem_cache name collision when multiple KVM implementations are loaded simultaneously.
- [Medium] The patch silently adds the `SLAB_ACCOUNT` flag to the async page fault kmem_cache without justification in the commit message.
--

--- Patch [2]: [PATCH v6 02/33] KVM: Make device name configurable ---
commit fddf3fec3ead355cf194e06551ca60993f2aec03
Author: Steffen Eiden <[email protected]>

KVM: Make device name configurable

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

> diff --git a/virt/kvm/async_pf.c b/virt/kvm/async_pf.c
> index b8aaa96b799bf..0157612491070 100644
> --- a/virt/kvm/async_pf.c
> +++ b/virt/kvm/async_pf.c
[ ... ]
> -int kvm_async_pf_init(void)
> +int kvm_async_pf_init(const char *cache_name)
>  {
> -	async_pf_cache = KMEM_CACHE(kvm_async_pf, 0);
> +	char *full_cache_name;
> +
> +	full_cache_name = kasprintf(GFP_KERNEL, "%s_async_pf", cache_name);
> +	if (!full_cache_name)
> +		return -ENOMEM;
> +
> +	async_pf_cache = kmem_cache_create(full_cache_name,
> +					   sizeof(struct kvm_async_pf), 0,
> +					   SLAB_ACCOUNT, NULL);

[Severity: Medium]
Does this silently introduce a behavior change? The previous KMEM_CACHE call
passed 0 for flags, but this adds SLAB_ACCOUNT. While accounting these
to the vCPU's memory cgroup is likely correct, shouldn't this functional
change be documented in the commit message?

> +	kfree(full_cache_name);
>  
>  	if (!async_pf_cache)
>  		return -ENOMEM;

[ ... ]

> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 0d80730e84240..0b34477bf325b 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
[ ... ]
> +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;
>  	int r;
>  	int cpu;
>  
> +	kvm_dev.minor = minor;
> +	kvm_dev.name = kstrdup_const(dev_name, GFP_KERNEL);

[Severity: High]
Can calling kvm_init_with_dev() multiple times corrupt the static miscdevice
and list state?

If multiple KVM implementations share the same kvm_main.o core and call this
initialization, they will unconditionally overwrite the shared static kvm_dev
struct. Calling misc_register(&kvm_dev) a second time with the exact same
static struct would overwrite its list_head pointers, which structurally
guarantees misc_list corruption and a fatal kernel panic on the next
traversal.

> +	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);
> +
> +	r = -ENOMEM;
> +	cache_name = kasprintf(GFP_KERNEL, "%s_vcpu", dev_name);
> +	if (!cache_name)
> +		goto err_dev_name;
>  	kvm_vcpu_cache =
> -		kmem_cache_create_usercopy("kvm_vcpu", vcpu_size, vcpu_align,
> +		kmem_cache_create_usercopy(cache_name, vcpu_size, vcpu_align,

[Severity: High]
Doesn't this also unconditionally overwrite the global kvm_vcpu_cache pointer
and leak the previous cache when multiple implementations initialize?

>  					   SLAB_ACCOUNT,
>  					   offsetof(struct kvm_vcpu, arch),
>  					   offsetofend(struct kvm_vcpu, stats_id)

[ ... ]

> @@ -6597,6 +6615,8 @@ int kvm_init(unsigned vcpu_size, unsigned vcpu_align, struct module *module)
>  	for_each_possible_cpu(cpu)
>  		free_cpumask_var(per_cpu(cpu_kick_mask, cpu));
>  	kmem_cache_destroy(kvm_vcpu_cache);
> +err_dev_name:
> +	kfree_const(kvm_dev.name);
>  	return r;
>  }
>  EXPORT_SYMBOL_FOR_KVM_INTERNAL(kvm_init);

[Severity: High]
Should kvm_init_with_dev() be exported? The commit states it is meant for
architectures providing multiple KVM implementations simultaneously, but
without an export macro like EXPORT_SYMBOL_FOR_KVM_INTERNAL, external
loadable architecture modules will fail to link against it.

[Severity: Medium]
Also, if multiple implementations are statically linked to avoid the missing
export, wouldn't kvm_gmem_init() (called earlier in this function) cause a
sysfs naming collision?

It does not take a dev_name parameter and hardcodes the name
"kvm_gmem_inode_cache". Because SLAB_ACCOUNT caches are unmergeable, the
second creation attempt will fail sysfs registration and emit a SLUB error
in dmesg.

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