[PATCH v2 0/2] Drivers: hv: vmbus: Small cleanups

[PATCH v2 0/2] Drivers: hv: vmbus: Small cleanups

[Sebastian Andrzej Siewior]

Replacing the lockdep_hardirq_threaded() annotation which does not
belong in drivers and removing the vmbus_irq_initialized which seems
redundant.

v1…v2: https://lore.kernel.org/all/20260401151517.1743555-1-bigeasy@linutronix.de/
  - Added a comment next to the invocation of __vmbus_isr()
  - Added the "Drivers:" prefix to the subject line

Sebastian Andrzej Siewior (2):
  Drivers: hv: vmbus: Replace lockdep_hardirq_threaded() with lockdep
    annotation
  Drivers: hv: vmbus: Remove vmbus_irq_initialized

 drivers/hv/vmbus_drv.c | 27 ++++++++++++++++-----------
 1 file changed, 16 insertions(+), 11 deletions(-)

-- 
2.53.0

[PATCH v2 1/2] Drivers: hv: vmbus: Replace lockdep_hardirq_threaded() with lockdep annotation

[Sebastian Andrzej Siewior]

lockdep_hardirq_threaded() is supposed to be used within IRQ core code
and not within drivers. It is not obvious from within the driver, that
this is the only interrupt service routing and that it is not shared
handler.

Replace lockdep_hardirq_threaded() with a lockdep annotation limiting
threaded context on PREEMPT_RT to __vmbus_isr().

Fixes: f8e6343b7a89c ("Drivers: hv: vmbus: Use kthread for vmbus interrupts on PREEMPT_RT")
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/hv/vmbus_drv.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 23206640c6139..44877664d9d08 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -1384,8 +1384,19 @@ void vmbus_isr(void)
 	if (IS_ENABLED(CONFIG_PREEMPT_RT)) {
 		vmbus_irqd_wake();
 	} else {
-		lockdep_hardirq_threaded();
+		static DEFINE_WAIT_OVERRIDE_MAP(vmbus_map, LD_WAIT_CONFIG);
+
+		/*
+		 * vmbus_isr is never force-threaded and always invoked at hard
+		 * IRQ level. __vmbus_isr() below can acquire a spinlock_t
+		 * which becomes a sleeping lock and must not be acquired in
+		 * this context. Therefore on PREEMPT_RT this will be threaded
+		 * via vmbus_irqd_wake(). On non-PREEMPT the annotation lets
+		 * lockdep know that acquiring a spinlock_t is not an issue.
+		 */
+		lock_map_acquire_try(&vmbus_map);
 		__vmbus_isr();
+		lock_map_release(&vmbus_map);
 	}
 }
 EXPORT_SYMBOL_FOR_MODULES(vmbus_isr, "mshv_vtl");
-- 
2.53.0

Re: [PATCH v2 1/2] Drivers: hv: vmbus: Replace lockdep_hardirq_threaded() with lockdep annotation

[sashiko-bot]

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

Pre-existing issues:
- [Critical] The clock event timer interrupt (HVMSG_TIMER_EXPIRED) is incorrectly processed in a kthread on PREEMPT_RT, violating the architectural requirement that the system tick must run in hardirq context.
--

--- Patch [1]: [PATCH v2 1/2] Drivers: hv: vmbus: Replace lockdep_hardirq_threaded() with lockdep annotation ---
commit f939a4b9727e456e689d5d7488e6095b1358e5b5
Author: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

Drivers: hv: vmbus: Replace lockdep_hardirq_threaded() with lockdep annotation

This commit replaces the use of lockdep_hardirq_threaded() with a lockdep
annotation. This limits the threaded context on PREEMPT_RT to __vmbus_isr()
and clarifies that the ISR is not a shared handler.

> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> index 23206640c6139..44877664d9d08 100644
> --- a/drivers/hv/vmbus_drv.c
> +++ b/drivers/hv/vmbus_drv.c
> @@ -1384,8 +1384,19 @@ void vmbus_isr(void)
>  	if (IS_ENABLED(CONFIG_PREEMPT_RT)) {
>  		vmbus_irqd_wake();

[Severity: Critical]
This isn't a bug introduced by this patch, but is it safe to defer all
VMbus interrupt processing to the vmbus_irqd kthread on PREEMPT_RT?

When booting a PREEMPT_RT kernel on a Hyper-V host using legacy timer mode,
the kthread handles the hypervisor timer message and invokes the timer ISR:

vmbus_message_sched()
    if (msg->header.message_type == HVMSG_TIMER_EXPIRED) {
        hv_stimer0_isr();

This executes the clock event handler in thread context, which breaks the
requirement that the system tick must run in hardirq context:

hv_stimer0_isr()
    ce = this_cpu_ptr(hv_clock_event);
    ce->event_handler(ce);

Because get_irq_regs() returns NULL when called from a kthread, calling
tick_handle_periodic() and then
update_process_times(user_mode(get_irq_regs())) will dereference a NULL
pointer and panic the kernel.

Alternatively, if high-resolution timers are used, tick_nohz_handler() sees
a NULL regs pointer and skips tick_sched_handle(), completely stalling the
scheduler tick and RCU updates, freezing the system.

>  	} else {
> -		lockdep_hardirq_threaded();
> +		static DEFINE_WAIT_OVERRIDE_MAP(vmbus_map, LD_WAIT_CONFIG);
> +
> +		/*
> +		 * vmbus_isr is never force-threaded and always invoked at hard

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260721153216.382536-1-bigeasy@linutronix.de?part=1

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

[Sebastian Andrzej Siewior]

vmbus_irq_initialized is only true if the registration of the per-CPU
threads succeeded. If it failed, the whole registration aborts and the
vmbus_exit() path is never called.

Remove vmbus_irq_initialized.

Reviewed-by: Michael Kelley <mhklinux@outlook.com>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/hv/vmbus_drv.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

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
@@ -1369,8 +1369,6 @@ static void run_vmbus_irqd(unsigned int cpu)
 	__vmbus_isr();
 }
 
-static bool vmbus_irq_initialized;
-
 static struct smp_hotplug_thread vmbus_irq_threads = {
 	.store                  = &vmbus_irqd,
 	.setup			= vmbus_irqd_setup,
@@ -1497,11 +1495,10 @@ static int vmbus_bus_init(void)
 	 * the VMbus interrupt handler.
 	 */
 
-	if (IS_ENABLED(CONFIG_PREEMPT_RT) && !vmbus_irq_initialized) {
+	if (IS_ENABLED(CONFIG_PREEMPT_RT)) {
 		ret = smpboot_register_percpu_thread(&vmbus_irq_threads);
 		if (ret)
 			goto err_kthread;
-		vmbus_irq_initialized = true;
 	}
 
 	if (vmbus_irq == -1) {
@@ -1545,10 +1542,8 @@ static int vmbus_bus_init(void)
 	else
 		free_percpu_irq(vmbus_irq, &vmbus_evt);
 err_setup:
-	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;
-	}
 err_kthread:
 	bus_unregister(&hv_bus);
 	return ret;
@@ -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;
-	}
+
 	for_each_online_cpu(cpu) {
 		struct hv_per_cpu_context *hv_cpu
 			= per_cpu_ptr(hv_context.cpu_context, cpu);
-- 
2.53.0

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

[sashiko-bot]

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 <bigeasy@linutronix.de>

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/20260721153216.382536-1-bigeasy@linutronix.de?part=2