Re: [PATCH v8 07/29] KVM: Make device name configurable

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] kvm: KVM_DEV_STRING macro evaluates to literal "KVM_DEV_NAME_vcpu"
- [Low] kvm: hardcoded "kvm" strings bypass the configurable device name
--

commit d598c93c97a669f28341c88d4d371abfd9c3b335
Author: Steffen Eiden <seiden@linux.ibm.com>

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/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 2b56fb4e85557..5926478563e88 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -45,6 +45,9 @@
>  #include <asm/kvm_host.h>
>  #include <linux/kvm_dirty_ring.h>
>  
> +#define __KVM_DEV_STRING(name, x) __stringify(name ## _ ## x)
> +#define KVM_DEV_STRING(x) __KVM_DEV_STRING(KVM_DEV_NAME, x)

[Severity: Medium]
Does KVM_DEV_STRING(vcpu) expand to the literal string "KVM_DEV_NAME_vcpu"
instead of the expected configured name?

Because the name parameter in __KVM_DEV_STRING is adjacent to the ##
token-pasting operator, the C preprocessor does not macro-expand it before
pasting. The tokens KVM_DEV_NAME, _, and vcpu are pasted into
KVM_DEV_NAME_vcpu, which __stringify converts to the literal string.

Does this need an extra level of macro indirection to evaluate the arguments
before pasting?

[ ... ]
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 891a67cb365ab..4f25e170b542c 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -5596,8 +5596,8 @@ static struct file_operations kvm_chardev_ops = {
>  };
>  
>  static struct miscdevice kvm_dev = {
> -	KVM_MINOR,
> -	"kvm",
> +	KVM_DEV_MINOR,
> +	__stringify(KVM_DEV_NAME),

[Severity: Low]
Are there other hardcoded "kvm" strings that will bypass this configurable
device name when creating a VM or interacting with KVM infrastructure?

For instance, processes utilizing different KVM implementations will have
indistinguishable workqueues and anonymous inodes, reducing observability:

virt/kvm/eventfd.c:kvm_irqfd_init() {
    ...
    irqfd_cleanup_wq = alloc_workqueue("kvm-irqfd-cleanup", WQ_PERCPU, 0);
    ...
}

virt/kvm/kvm_main.c:kvm_create_vm() {
    ...
    file = anon_inode_getfile("kvm-vm", &kvm_vm_fops, kvm, O_RDWR);
    ...
}

And also hardcoded log prefixes:

virt/kvm/kvm_main.c:kvm_shutdown() {
    ...
    pr_info("kvm: exiting hardware virtualization\n");
    ...
}

Do these strings also need to use the configured device name to ensure
concurrent KVM instances are cleanly segregated?

[Severity: Low]
This is a pre-existing issue, but does generic multiple KVM support conflict
with hardcoded core kernel singletons?

If architectures using CONFIG_KVM_GENERIC_HARDWARE_ENABLING attempt to load
multiple KVM modules simultaneously, the second module will fail to initialize.
The core hotplug subsystem returns -EBUSY when a second implementation invokes
it since the hotplug state is statically assigned:

virt/kvm/kvm_main.c:kvm_enable_virtualization() {
    ...
    r = cpuhp_setup_state(CPUHP_AP_KVM_ONLINE, "kvm/cpu:online",
                          kvm_online_cpu, kvm_offline_cpu);
    ...
}

Also, concurrent registrations of guest info callbacks trigger a WARN_ON_ONCE
since the core kernel only maintains a single pointer:

virt/kvm/kvm_main.c:__kvm_register_perf_callbacks() {
    ...
    perf_register_guest_info_callbacks(&kvm_guest_cbs);
    ...
}

Can these singletons be adjusted to support concurrent implementations?

>  	&kvm_chardev_ops,
>  };
>  
[ ... ]
> @@ -6521,7 +6521,7 @@ int kvm_init(unsigned vcpu_size, unsigned vcpu_align, struct module *module)
>  	if (!vcpu_align)
>  		vcpu_align = __alignof__(struct kvm_vcpu);
>  	kvm_vcpu_cache =
> -		kmem_cache_create_usercopy("kvm_vcpu", vcpu_size, vcpu_align,
> +		kmem_cache_create_usercopy(KVM_DEV_STRING(vcpu), vcpu_size, vcpu_align,

[Severity: Medium]
With the KVM_DEV_STRING() macro evaluating to the literal string
"KVM_DEV_NAME_vcpu", will all KVM modules unconditionally attempt to create a
kmem_cache named literally "KVM_DEV_NAME_vcpu" during module initialization?

This causes duplicate slab names across multiple KVM implementations, leading
to sysfs creation warnings (SLUB: Unable to add cache KVM_DEV_NAME_vcpu to
sysfs) and broken observability in /proc/slabinfo. Although SLUB permits
duplicate names and proceeds with cache creation, this seems to break the
intended uniqueness.

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260918133107.1042730-1-seiden@linux.ibm.com?part=7