Re: [PATCH 1/1] Drivers: hv: Don't reset VMBus connection state on error/exit paths

[sashiko-bot@kernel.org]

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 <mhkelley58@gmail.com>

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/20260805205806.B5AC31F000E9@smtp.kernel.org/
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/20260901034514.457089-1-mhklinux@outlook.com?part=1