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
