Re: [PATCH v2 2/2] Drivers: hv: vmbus: Remove vmbus_irq_initialized

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

New issues:
- [High] Removal of vmbus_irq_initialized flag introduces a NULL pointer dereference in smpboot_unregister_percpu_thread().

Pre-existing issues:
- [High] Unconditional cleanup in vmbus_exit() leads to multiple NULL pointer dereferences if module initialization was bypassed.
--

--- Patch [2]: [PATCH v2 2/2] Drivers: hv: vmbus: Remove vmbus_irq_initialized ---
commit 5d2e18e4c080ed3a0d4cb94c834ff4580a336e21
Author: Sebastian Andrzej Siewior <[email protected]>

Drivers: hv: vmbus: Remove vmbus_irq_initialized

This commit removes the vmbus_irq_initialized flag, relying on the
assumption that if per-CPU thread registration fails, the whole
registration aborts and vmbus_exit() is never called.

> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> index 44877664d9d08..95a2c6049854d 100644
> --- a/drivers/hv/vmbus_drv.c
> +++ b/drivers/hv/vmbus_drv.c

[ ... ]

> @@ -3045,10 +3040,9 @@ static void __exit vmbus_exit(void)
>  		hv_remove_vmbus_handler();
>  	else
>  		free_percpu_irq(vmbus_irq, &vmbus_evt);
> -	if (IS_ENABLED(CONFIG_PREEMPT_RT) && vmbus_irq_initialized) {
> +	if (IS_ENABLED(CONFIG_PREEMPT_RT))
>  		smpboot_unregister_percpu_thread(&vmbus_irq_threads);
> -		vmbus_irq_initialized = false;
> -	}

[Severity: High]
Will this unconditionally unregister uninitialized threads if the module is
loaded and unloaded without executing vmbus_bus_init()?

The commit message states that vmbus_exit() is never called if registration
fails. However, hv_acpi_init() can return early and bypass vmbus_bus_init()
completely:

hv_acpi_init() {
    ...
    if (hv_root_partition() && !hv_nested)
        return 0;
    ...
}

If the module loads via this early return path and is later unloaded,
vmbus_exit() will execute. Calling smpboot_unregister_percpu_thread() on
the zero-initialized vmbus_irq_threads.list will trigger a NULL pointer
dereference in list_del().

>  
>  	for_each_online_cpu(cpu) {
>  		struct hv_per_cpu_context *hv_cpu
>  			= per_cpu_ptr(hv_context.cpu_context, cpu);
>  
>  		tasklet_kill(&hv_cpu->msg_dpc);

[Severity: High]
This is a pre-existing issue, but won't the same early return in
hv_acpi_init() cause multiple crashes during teardown as well?

If vmbus_bus_init() is completely bypassed and the module is subsequently
unloaded, vmbus_exit() performs unconditional cleanup on uninitialized
resources. 

For example, tasklet_kill() will attempt to access hv_cpu derived from an
unallocated hv_context.cpu_context. Similarly, unregister_syscore() called
at the start of vmbus_exit() will also dereference a NULL pointer since it
was never registered.

>  	}
>  	hv_debug_rm_all_dir();

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