linux-kernel - Re: [PATCH] irqchip/sifive-plic: Unmask interrupt in plic_irq

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <8734ley58q.ffs@tglx>
Date: Wed, 02 Oct 2024 16:00:21 +0200
From: Thomas Gleixner <tglx@...utronix.de>
To: Nam Cao <namcao@...utronix.de>, Paul Walmsley
 <paul.walmsley@...ive.com>, Samuel Holland <samuel.holland@...ive.com>,
 Marc Zyngier <maz@...nel.org>, linux-kernel@...r.kernel.org,
 linux-riscv@...ts.infradead.org
Cc: Nam Cao <namcao@...utronix.de>, stable@...r.kernel.org
Subject: Re: [PATCH] irqchip/sifive-plic: Unmask interrupt in plic_irq_enable()

On Thu, Sep 26 2024 at 17:43, Nam Cao wrote:
> If another task disables the interrupt in the middle of the above steps,
> the interrupt will not get unmasked, and will remain masked when it is
> enabled in the future.
>
> The problem is occasionally observed when PREEMPT_RT is enabled, because
> PREEMPT_RT add the IRQS_ONESHOT flag. But PREEMPT_RT only makes the
> problem more likely to appear, the bug has been around since
> commit a1706a1c5062 ("irqchip/sifive-plic: Separate the enable and mask
> operations").

Correct. It's a general problem independent of RT.

> Fix it by unmasking interrupt in plic_irq_enable().
>
> Fixes: a1706a1c5062 ("irqchip/sifive-plic: Separate the enable and mask operations").
> Signed-off-by: Nam Cao <namcao@...utronix.de>
> Cc: stable@...r.kernel.org
> ---
>  drivers/irqchip/irq-sifive-plic.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
> index 2f6ef5c495bd..0efbf14ec9fa 100644
> --- a/drivers/irqchip/irq-sifive-plic.c
> +++ b/drivers/irqchip/irq-sifive-plic.c
> @@ -128,6 +128,9 @@ static inline void plic_irq_toggle(const struct cpumask *mask,
>  
>  static void plic_irq_enable(struct irq_data *d)
>  {
> +	struct plic_priv *priv = irq_data_get_irq_chip_data(d);
> +
> +	writel(1, priv->regs + PRIORITY_BASE + d->hwirq * PRIORITY_PER_ID);
>  	plic_irq_toggle(irq_data_get_effective_affinity_mask(d), d, 1);

Can you please move plic_irq_enable() below plic_irq_unmask() and invoke
the latter instead of duplicating the code?

Also usually unmask() is done after enable(), but the ordering probably
does not matter here.

Thanks,

        tglx