Re: [PATCH v3 1/5] genirq: Synchronize in-flight handlers during NMI teardown

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Pending NMIs can cause a NULL pointer dereference in lockless NMI handlers after desc->action is cleared.
- [High] __cleanup_nmi() does not abort when desc->action is NULL, leading to double-free of resources and refcount underflows if invoked on an already freed NMI.
--

commit cba735e452b4b54ed5512df243c59c9e7de9a13b
Author: Mayank Rungta <mrungta@google.com>

genirq: Synchronize in-flight handlers during NMI teardown

This patch modifies __cleanup_nmi() to synchronize in-flight handlers on
other CPUs before tearing down the NMI. It serializes teardown, shuts down
the line, and queries the hardware state using __irq_get_irqchip_state().

> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
> index 2fbff2618a1e2..a9973b61163ae 100644
> --- a/kernel/irq/manage.c
> +++ b/kernel/irq/manage.c
[ ... ]
> @@ -2034,11 +2042,20 @@ static const void *__cleanup_nmi(unsigned int irq, struct irq_desc *desc)
[ ... ]
> +	/*
> +	 * Ensure all in-flight NMI handlers on other CPUs complete before
> +	 * clearing desc->action or tearing down NMI state.
> +	 */
> +	__synchronize_hardirq(desc, true);
>  
> +	scoped_guard(raw_spinlock_irqsave, &desc->lock) {
>  		if (!WARN_ON(desc->action == NULL)) {
>  			action = desc->action;
>  			irq_pm_remove_action(desc, action);
>  			devname = action->name;
>  		}

[Severity: High]
This isn't a bug introduced by this patch, but does this function fail to
abort when it detects that the NMI was already cleaned up?

If a race condition occurs where desc->action is already NULL, this block
issues a warning but proceeds with the rest of the teardown unconditionally.
Could this lead to double-freeing irqchip resources, unbalanced PM usage, and
module refcount underflows if invoked on an already freed NMI?

>  		desc->action = NULL;

[Severity: High]
This is a pre-existing issue, but does this synchronization step fail to wait
for pending interrupts?

If an NMI is asserted by the hardware but the CPU has not yet taken the
exception, __synchronize_hardirq() queries IRQCHIP_STATE_ACTIVE and will
return immediately. The teardown path then clears desc->action to NULL.

Could the CPU taking the pending NMI exception then cause handle_fasteoi_nmi()
in kernel/irq/chip.c to locklessly dereference action->handler, resulting in a
NULL pointer dereference?

> -		irq_settings_clr_disable_unlazy(desc);
> -		irq_shutdown_and_deactivate(desc);
> +		irq_nmi_teardown(desc);
> +		desc->istate &= ~IRQS_NMI;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260902-qcom-wdt-nmi-series-v3-0-f3999362a9ea@google.com?part=1