Re: [PATCH RFC] genirq/kexec: Do not take the bus lock of an interrupt chip on the crash path

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] irq/kexec: incomplete slow bus check allows hang via irq_bus_sync_unlock
--

commit a12343156827b9035af6e3982ac65b3437921502
Author: Igor Velkov <iav@iav.lv>

genirq/kexec: Do not take the bus lock of an interrupt chip on the crash path

machine_kexec_mask_interrupts() calls irq_set_irqchip_state() for every
started interrupt. That function takes the bus lock of the interrupt
chip and syncs the chip when it drops the lock. On the crash path
interrupts are off and the other CPUs are stopped, so a chip that needs
its bus for the sync never returns.

> diff --git a/kernel/irq/kexec.c b/kernel/irq/kexec.c
> index 1a3deffe6b5b6..c6c745cc76a25 100644
> --- a/kernel/irq/kexec.c
> +++ b/kernel/irq/kexec.c
> @@ -20,7 +20,11 @@ void machine_kexec_mask_interrupts(void)
>  		if (!chip || !irqd_is_started(&desc->irq_data))
>  			continue;
>  
> -		if (IS_ENABLED(CONFIG_GENERIC_IRQ_KEXEC_CLEAR_VM_FORWARD)) {
> +		/*
> +		 * irq_set_irqchip_state() takes the bus lock, and a chip that
> +		 * has one may sleep or talk to its bus when the lock is dropped.
> +		 */
> +		if (IS_ENABLED(CONFIG_GENERIC_IRQ_KEXEC_CLEAR_VM_FORWARD) && !chip->irq_bus_lock) {

[Severity: Medium]
Does this check comprehensively protect against slow bus chips?

If an irqchip implements irq_bus_sync_unlock but omits irq_bus_lock, this
condition evaluates to true and irq_set_irqchip_state() is called.
Since irq_set_irqchip_state() ultimately unlocks the descriptor via
__irq_put_desc_unlock(), it unconditionally invokes the sync callback:

kernel/irq/irqdesc.c:__irq_put_desc_unlock() {
    ...
    if (bus)
        chip_bus_sync_unlock(desc);
    ...
}

Executing the sync callback on the crash path without interrupts could hang the
system, defeating the purpose of this patch.

Other places in the subsystem check for both callbacks to identify slow bus
locking/syncing, for instance:

kernel/irq/manage.c:irq_supports_nmi() {
    ...
    if (d->chip->irq_bus_lock || d->chip->irq_bus_sync_unlock)
        return false;
    ...
}

Should this check also ensure !chip->irq_bus_sync_unlock?

>  			/*
>  			 * First try to remove the active state from an interrupt which is forwarded
>  			 * to a VM. If the interrupt is not forwarded, try to EOI the interrupt.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261005-kexec-irq-bus-lock-v1-1-09f6b5323460@iav.lv?part=1