Re: [PATCH v3 2/5] genirq: Implement synchronous disable_nmi()

[sashiko-bot@kernel.org]

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

New issues:
- [High] disable_nmi() fails to guarantee synchronous completion of running NMI handlers, violating its primary design goal.

Pre-existing issues:
- [High] __synchronize_hardirq() unconditionally enables interrupts upon unlocking, violating synchronize_hardirq() IRQ context safety.
--

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

genirq: Implement synchronous disable_nmi()
    
This commit introduces disable_nmi() to synchronously disable an NMI line,
guaranteeing any running instances of the NMI handler have completed before
returning. It implements this by wrapping disable_irq().

> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
> index a9973b61163ae..009633b5b6544 100644
> --- a/kernel/irq/manage.c
> +++ b/kernel/irq/manage.c
> @@ -767,6 +767,21 @@ void disable_nmi_nosync(unsigned int irq)
>  	disable_irq_nosync(irq);
>  }
>  
> +/**
> + * disable_nmi - disable an nmi and wait for any pending handlers
> + * @irq: Interrupt to disable
> + *
> + * Disable the selected interrupt line. Disables and enables are nested.
> + *
> + * The interrupt to disable must have been requested through request_nmi.
> + * This function ensures existing instances of the NMI handler have
> + * completed before returning.
> + */
> +void disable_nmi(unsigned int irq)
> +{
> +	disable_irq(irq);

[Severity: High]
Does this correctly guarantee synchronous completion of running NMI handlers?

This function wraps disable_irq(), which uses synchronize_irq() to wait for
completion. synchronize_irq() checks the IRQD_IRQ_INPROGRESS software flag
and the IRQCHIP_STATE_ACTIVE hardware state. 

However, NMIs handled by functions like handle_fasteoi_nmi() bypass standard
event handling and never set IRQD_IRQ_INPROGRESS:

kernel/irq/chip.c:handle_fasteoi_nmi() {
    ...
    res = action->handler(irq, action->dev_id);
    ...
}

If the irqchip doesn't support reading the active state, or if the NMI is
pending but not yet active, synchronize_irq() returns immediately. The
target CPU then processes the NMI, and since handle_fasteoi_nmi() does not
check IRQD_IRQ_DISABLED, the handler blindly executes after disable_nmi()
has returned. Can this lead to use-after-free or crashes when drivers rely
on disable_nmi() before freeing resources?

[Severity: High]
This isn't a bug introduced by this patch, but __synchronize_hardirq() is
called by synchronize_irq(), which is triggered by this code. There is a
pre-existing locking violation with how it handles locking:

kernel/irq/manage.c:__synchronize_hardirq() {
    ...
    guard(raw_spinlock_irqsave)(&desc->lock);
    ...
}

The guard is defined in include/linux/spinlock.h:

DEFINE_LOCK_GUARD_1(raw_spinlock_irqsave, raw_spinlock_t,
                    raw_spin_lock_irq_disable(_T->lock),
                    raw_spin_unlock_irq_enable(_T->lock))

This defines the guard to use raw_spin_unlock_irq_enable(), which does not
restore previously saved flags but unconditionally enables interrupts.

If a caller invokes synchronize_hardirq() (or another function using this
guard) from an IRQ handler where local interrupts are currently disabled,
it unconditionally re-enables local interrupts upon unlocking rather than
restoring the previously saved IRQ state. Could this lead to unexpected
nested interrupts or deadlocks?

> +}

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