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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] genirq: inverted active state check order in
  __synchronize_nmi()
--

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

genirq: Synchronize in-flight handlers during NMI teardown

This commit tracks in-flight NMI handlers with an atomic counter and adds
__synchronize_nmi() to wait for running NMI handlers during NMI teardown
and synchronization.

> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
> index ec3ecb0e48cec..3fb77a2d954d9 100644
> --- a/kernel/irq/manage.c
> +++ b/kernel/irq/manage.c
> @@ -48,11 +48,72 @@ static inline void synchronize_irqwork(struct irq_desc *desc) { }
>  
>  static int __irq_get_irqchip_state(struct irq_data *d, enum irqchip_irq_state which, bool *state);
>  
> +static void __synchronize_nmi(struct irq_desc *desc, bool sync_chip)
> +{
> +	struct irq_data *irqd = irq_desc_get_irq_data(desc);
> +	bool inprogress;
> +
> +	do {
> +		/*
> +		 * Wait until we're out of the NMI handler before taking
> +		 * desc->lock or polling the irqchip. This might give the
> +		 * wrong answer due to the lack of memory barriers.
> +		 */
> +		while (atomic_read(&desc->nmi_active))
> +			cpu_relax();
> +
> +		guard(raw_spinlock_irqsave)(&desc->lock);
> +		/*
> +		 * Ok, that indicated we're done: double-check carefully.
> +		 *
> +		 * Holding desc->lock serializes with prior state updates (e.g.
> +		 * IRQD_IRQ_DISABLED set under desc->lock) and protects
> +		 * __irq_get_irqchip_state(). Together with
> +		 * smp_mb__after_spinlock(), this provides a full memory
> +		 * barrier pairing with smp_mb__after_atomic() in
> +		 * handle_fasteoi_nmi() so those updates are visible before
> +		 * reading nmi_active.
> +		 */
> +		smp_mb__after_spinlock();
> +
> +		/*
> +		 * Pair with smp_mb__before_atomic() + atomic_dec() in
> +		 * handle_fasteoi_nmi() so all handler memory accesses complete
> +		 * before __synchronize_nmi() returns.
> +		 */
> +		inprogress = atomic_read_acquire(&desc->nmi_active);
> +
> +		/*
> +		 * Also check hardware active state when supported to bridge the
> +		 * window between hardware acknowledge and atomic_inc() in
> +		 * handle_fasteoi_nmi() on non-percpu_devid SPIs. Ignore the
> +		 * return code; inprogress is only updated when the chip
> +		 * supports it. Note that if a chip does not implement this,
> +		 * disable_nmi() is still safe, and free_nmi() is as safe as we
> +		 * can make it (though an NMI acknowledged just before masking
> +		 * that has not yet entered handle_fasteoi_nmi() could race with
> +		 * irq_nmi_teardown() resetting desc->handle_irq).
> +		 */
> +		if (!inprogress && sync_chip)
> +			__irq_get_irqchip_state(irqd, IRQCHIP_STATE_ACTIVE,
> +						&inprogress);
> +	} while (inprogress);
> +}

[Severity: Medium]
Can __synchronize_nmi() return while handle_fasteoi_nmi() is still running
on another CPU because it checks desc->nmi_active before querying
IRQCHIP_STATE_ACTIVE?

Unlike regular IRQ flow handlers, which acquire desc->lock before setting
IRQD_IRQ_INPROGRESS and calling chip->irq_eoi(), handle_fasteoi_nmi() runs
without desc->lock.  On the NMI CPU, hardware acknowledge sets
IRQCHIP_STATE_ACTIVE first, then handle_fasteoi_nmi() increments
desc->nmi_active, then chip->irq_eoi() clears IRQCHIP_STATE_ACTIVE, and
finally desc->nmi_active is decremented:

CPU0 (NMI)                        CPU1
<NMI acknowledged in hw>
  (IRQCHIP_STATE_ACTIVE = true)
                                  __synchronize_nmi()
                                    inprogress =
                                      atomic_read_acquire(&desc->nmi_active);
                                      -> 0
  handle_fasteoi_nmi()
    atomic_inc(&desc->nmi_active)
    action->handler(...)
    chip->irq_eoi()
      (IRQCHIP_STATE_ACTIVE = false)
                                    __irq_get_irqchip_state(...,
                                      IRQCHIP_STATE_ACTIVE, &inprogress);
                                      -> false
                                  /* __synchronize_nmi() returns */
                                  __cleanup_nmi()
                                    irq_nmi_teardown()
    atomic_dec(&desc->nmi_active)  <-- NMI handler still in flight

Querying IRQCHIP_STATE_ACTIVE before reading desc->nmi_active (or
re-checking desc->nmi_active after observing IRQCHIP_STATE_ACTIVE is
false) would prevent missing the handover between hardware-active and
software-active states.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112031629.396819-1-mrungta@google.com?part=1