Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [High] Unchecked return value from vmbus_post_msg() leading to an infinite wait during driver unload. - [High] Destroying workqueues in vmbus_disconnect() before disabling VMBus interrupts leads to Use-After-Free if an asynchronous message is received. -- commit bf55ca432bc98fbbbbd08f5b33b12b8f32d7705a Author: Michael Kelley <[email protected]> Drivers: hv: Don't reset VMBus connection state on error/exit paths This patch fixes an issue where the VMBus connection state was forced to disconnected before calling vmbus_disconnect(). This prevented the connection from being properly unloaded since vmbus_initiate_unload() checks for the connected state. By leaving the connection state as is, the hypervisor connection can be cleanly torn down. Closes: https://lore.kernel.org/linux-hyperv/[email protected]/ Fixes: 74347a99e73ae ("x86/Hyper-V: Unload vmbus channel in hv panic callback") [ ... ] > diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c > index 5ebdbe24b5a1e..547bc731114fd 100644 > --- a/drivers/hv/vmbus_drv.c > +++ b/drivers/hv/vmbus_drv.c > @@ -3043,7 +3043,6 @@ static void __exit vmbus_exit(void) > > hv_remove_kexec_handler(); > hv_remove_crash_handler(); > - vmbus_connection.conn_state = DISCONNECTED; > hv_stimer_global_cleanup(); > vmbus_disconnect(); [Severity: High] This isn't a bug introduced by this patch, but since this patch allows vmbus_initiate_unload() to actively execute during vmbus_disconnect(), does this expose a potential infinite wait? Looking at vmbus_initiate_unload(), the code unconditionally waits for a completion event even if the message posting fails: drivers/hv/channel_mgmt.c:vmbus_initiate_unload() { ... vmbus_post_msg(&hdr, sizeof(struct vmbus_channel_message_header), !crash); if (!crash) wait_for_completion(&vmbus_connection.unload_event); } If vmbus_post_msg() fails (for example, due to insufficient buffers), the event will never be signaled by the host, causing the module unload process to hang indefinitely. Could the return value of vmbus_post_msg() be checked to prevent this? [Severity: High] This is also a pre-existing issue, but does the teardown ordering here allow a use-after-free if an asynchronous message arrives during unload? The current sequence calls vmbus_disconnect() which destroys the work queues: drivers/hv/connection.c:vmbus_disconnect() { ... if (vmbus_connection.work_queue) destroy_workqueue(vmbus_connection.work_queue); ... } However, VMBus interrupts and DPC tasklets are left active until later in vmbus_exit(): drivers/hv/vmbus_drv.c:vmbus_exit() { ... vmbus_disconnect(); if (vmbus_irq == -1) hv_remove_vmbus_handler(); else free_percpu_irq(vmbus_irq, &vmbus_evt); ... for_each_online_cpu(cpu) { struct hv_per_cpu_context *hv_cpu = ... tasklet_kill(&hv_cpu->msg_dpc); } } If the hypervisor delivers an interrupt after vmbus_disconnect() returns but before the DPC tasklets are killed, __vmbus_on_msg_dpc() can run and attempt to queue work using the destroyed workqueue pointer: drivers/hv/vmbus_drv.c:__vmbus_on_msg_dpc() { ... default: queue_work(vmbus_connection.work_queue, &ctx->work); } Should the interrupts and tasklets be disabled prior to freeing the workqueues to avoid this race condition? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
