| Message ID | 20210706110647.3979002-1-maz@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | irqchip/mips: Fix RCU violation when using irqdomain lookup on interrupt entry | expand |
Hi Marc, Thanks for the fix. On Tue, Jul 06, 2021 at 12:06:47PM +0100, Marc Zyngier wrote: > Since d4a45c68dc81 ("irqdomain: Protect the linear revmap with RCU"), > any irqdomain lookup requires the RCU read lock to be held. > > This assumes that the architecture code will be structured such as > irq_enter() will be called *before* the interrupt is looked up > in the irq domain. However, this isn't the case for MIPS, and a number > of drivers are structured to do it the other way around when handling > an interrupt in their root irqchip (secondary irqchips are OK by > construction). > > This results in a RCU splat on a lockdep-enabled kernel when the kernel > takes an interrupt from idle, as reported by Guenter Roeck. Alas I am still on 5.12-rc4, so can't test it out at the moment. Soon after getting further on the modern kernel version I'll give this patch a try on my hw and send a report. Regards -Sergey > > Note that this could have fired previously if any driver had used > tree-based irqdomain, which always had the RCU requirement. > > To solve this, provide a MIPS-specific helper (do_domain_IRQ()) > as the pendent of do_IRQ() that will do thing in the right order > (and maybe save some cycles in the process). > > Ideally, MIPS would be moved over to using handle_domain_irq(), > but that's much more ambitious. > > Reported-by: Guenter Roeck <linux@roeck-us.net> > Tested-by: Guenter Roeck <linux@roeck-us.net> > Signed-off-by: Marc Zyngier <maz@kernel.org> > Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de> > Cc: Serge Semin <fancer.lancer@gmail.com> > Link: https://lore.kernel.org/r/20210705172352.GA56304@roeck-us.net > --- > arch/mips/include/asm/irq.h | 3 +++ > arch/mips/kernel/irq.c | 14 ++++++++++++++ > drivers/irqchip/irq-mips-cpu.c | 10 ++++++---- > drivers/irqchip/irq-mips-gic.c | 8 ++++---- > drivers/irqchip/irq-pic32-evic.c | 5 ++--- > 5 files changed, 29 insertions(+), 11 deletions(-) > > diff --git a/arch/mips/include/asm/irq.h b/arch/mips/include/asm/irq.h > index d1477ecb1af9..57561e0e6e8d 100644 > --- a/arch/mips/include/asm/irq.h > +++ b/arch/mips/include/asm/irq.h > @@ -57,6 +57,9 @@ asmlinkage void plat_irq_dispatch(void); > > extern void do_IRQ(unsigned int irq); > > +struct irq_domain; > +extern void do_domain_IRQ(struct irq_domain *domain, unsigned int irq); > + > extern void arch_init_irq(void); > extern void spurious_interrupt(void); > > diff --git a/arch/mips/kernel/irq.c b/arch/mips/kernel/irq.c > index 85b6c60f285d..c76005cd3b79 100644 > --- a/arch/mips/kernel/irq.c > +++ b/arch/mips/kernel/irq.c > @@ -21,6 +21,7 @@ > #include <linux/kallsyms.h> > #include <linux/kgdb.h> > #include <linux/ftrace.h> > +#include <linux/irqdomain.h> > > #include <linux/atomic.h> > #include <linux/uaccess.h> > @@ -107,3 +108,16 @@ void __irq_entry do_IRQ(unsigned int irq) > irq_exit(); > } > > +void __irq_entry do_domain_IRQ(struct irq_domain *domain, unsigned int hwirq) > +{ > + struct irq_desc *desc; > + > + irq_enter(); > + check_stack_overflow(); > + > + desc = irq_resolve_mapping(domain, hwirq); > + if (likely(desc)) > + handle_irq_desc(desc); > + > + irq_exit(); > +} > diff --git a/drivers/irqchip/irq-mips-cpu.c b/drivers/irqchip/irq-mips-cpu.c > index 0bbb0b2d0dd5..0c7ae71a0af0 100644 > --- a/drivers/irqchip/irq-mips-cpu.c > +++ b/drivers/irqchip/irq-mips-cpu.c > @@ -127,7 +127,6 @@ static struct irq_chip mips_mt_cpu_irq_controller = { > asmlinkage void __weak plat_irq_dispatch(void) > { > unsigned long pending = read_c0_cause() & read_c0_status() & ST0_IM; > - unsigned int virq; > int irq; > > if (!pending) { > @@ -137,12 +136,15 @@ asmlinkage void __weak plat_irq_dispatch(void) > > pending >>= CAUSEB_IP; > while (pending) { > + struct irq_domain *d; > + > irq = fls(pending) - 1; > if (IS_ENABLED(CONFIG_GENERIC_IRQ_IPI) && irq < 2) > - virq = irq_linear_revmap(ipi_domain, irq); > + d = ipi_domain; > else > - virq = irq_linear_revmap(irq_domain, irq); > - do_IRQ(virq); > + d = irq_domain; > + > + do_domain_IRQ(d, irq); > pending &= ~BIT(irq); > } > } > diff --git a/drivers/irqchip/irq-mips-gic.c b/drivers/irqchip/irq-mips-gic.c > index b146e069bf5b..54c7092cc61d 100644 > --- a/drivers/irqchip/irq-mips-gic.c > +++ b/drivers/irqchip/irq-mips-gic.c > @@ -169,8 +169,8 @@ static void gic_handle_shared_int(bool chained) > generic_handle_domain_irq(gic_irq_domain, > GIC_SHARED_TO_HWIRQ(intr)); > else > - do_IRQ(irq_find_mapping(gic_irq_domain, > - GIC_SHARED_TO_HWIRQ(intr))); > + do_domain_IRQ(gic_irq_domain, > + GIC_SHARED_TO_HWIRQ(intr)); > } > } > > @@ -320,8 +320,8 @@ static void gic_handle_local_int(bool chained) > generic_handle_domain_irq(gic_irq_domain, > GIC_LOCAL_TO_HWIRQ(intr)); > else > - do_IRQ(irq_find_mapping(gic_irq_domain, > - GIC_LOCAL_TO_HWIRQ(intr))); > + do_domain_IRQ(gic_irq_domain, > + GIC_LOCAL_TO_HWIRQ(intr)); > } > } > > diff --git a/drivers/irqchip/irq-pic32-evic.c b/drivers/irqchip/irq-pic32-evic.c > index 34c4b4ffacd1..1d9bb28d13e5 100644 > --- a/drivers/irqchip/irq-pic32-evic.c > +++ b/drivers/irqchip/irq-pic32-evic.c > @@ -42,11 +42,10 @@ static void __iomem *evic_base; > > asmlinkage void __weak plat_irq_dispatch(void) > { > - unsigned int irq, hwirq; > + unsigned int hwirq; > > hwirq = readl(evic_base + REG_INTSTAT) & 0xFF; > - irq = irq_linear_revmap(evic_irq_domain, hwirq); > - do_IRQ(irq); > + do_domain_IRQ(evic_irq_domain, hwirq); > } > > static struct evic_chip_data *irqd_to_priv(struct irq_data *data) > -- > 2.30.2 >
Hi Sergey, On Thu, 08 Jul 2021 10:46:08 +0100, Serge Semin <fancer.lancer@gmail.com> wrote: > > Hi Marc, > Thanks for the fix. > > On Tue, Jul 06, 2021 at 12:06:47PM +0100, Marc Zyngier wrote: > > Since d4a45c68dc81 ("irqdomain: Protect the linear revmap with RCU"), > > any irqdomain lookup requires the RCU read lock to be held. > > > > This assumes that the architecture code will be structured such as > > irq_enter() will be called *before* the interrupt is looked up > > in the irq domain. However, this isn't the case for MIPS, and a number > > of drivers are structured to do it the other way around when handling > > an interrupt in their root irqchip (secondary irqchips are OK by > > construction). > > > > This results in a RCU splat on a lockdep-enabled kernel when the kernel > > takes an interrupt from idle, as reported by Guenter Roeck. > > Alas I am still on 5.12-rc4, so can't test it out at the moment. Soon > after getting further on the modern kernel version I'll give this > patch a try on my hw and send a report. It is likely that I'll send a pull request to Thomas with this shortly, given that it affects existing systems and that this patch does address the issue (see Guenter's report). We can always amend things once you've had the time to upgrade your kernel to the latest. Thanks, M.
On Thu, Jul 08, 2021 at 05:39:28PM +0100, Marc Zyngier wrote: > Hi Sergey, > > On Thu, 08 Jul 2021 10:46:08 +0100, > Serge Semin <fancer.lancer@gmail.com> wrote: > > > > Hi Marc, > > Thanks for the fix. > > > > On Tue, Jul 06, 2021 at 12:06:47PM +0100, Marc Zyngier wrote: > > > Since d4a45c68dc81 ("irqdomain: Protect the linear revmap with RCU"), > > > any irqdomain lookup requires the RCU read lock to be held. > > > > > > This assumes that the architecture code will be structured such as > > > irq_enter() will be called *before* the interrupt is looked up > > > in the irq domain. However, this isn't the case for MIPS, and a number > > > of drivers are structured to do it the other way around when handling > > > an interrupt in their root irqchip (secondary irqchips are OK by > > > construction). > > > > > > This results in a RCU splat on a lockdep-enabled kernel when the kernel > > > takes an interrupt from idle, as reported by Guenter Roeck. > > > > Alas I am still on 5.12-rc4, so can't test it out at the moment. Soon > > after getting further on the modern kernel version I'll give this > > patch a try on my hw and send a report. > > It is likely that I'll send a pull request to Thomas with this > shortly, given that it affects existing systems and that this patch > does address the issue (see Guenter's report). We can always amend > things once you've had the time to upgrade your kernel to the latest. Ok. I'll report the test status once my working branches are rebased on the latest version. It will be done in a few weeks from now then after I finished my current activity. -Sergey > > Thanks, > > M. > > -- > Without deviation from the norm, progress is not possible.
diff --git a/arch/mips/include/asm/irq.h b/arch/mips/include/asm/irq.h index d1477ecb1af9..57561e0e6e8d 100644 --- a/arch/mips/include/asm/irq.h +++ b/arch/mips/include/asm/irq.h @@ -57,6 +57,9 @@ asmlinkage void plat_irq_dispatch(void); extern void do_IRQ(unsigned int irq); +struct irq_domain; +extern void do_domain_IRQ(struct irq_domain *domain, unsigned int irq); + extern void arch_init_irq(void); extern void spurious_interrupt(void); diff --git a/arch/mips/kernel/irq.c b/arch/mips/kernel/irq.c index 85b6c60f285d..c76005cd3b79 100644 --- a/arch/mips/kernel/irq.c +++ b/arch/mips/kernel/irq.c @@ -21,6 +21,7 @@ #include <linux/kallsyms.h> #include <linux/kgdb.h> #include <linux/ftrace.h> +#include <linux/irqdomain.h> #include <linux/atomic.h> #include <linux/uaccess.h> @@ -107,3 +108,16 @@ void __irq_entry do_IRQ(unsigned int irq) irq_exit(); } +void __irq_entry do_domain_IRQ(struct irq_domain *domain, unsigned int hwirq) +{ + struct irq_desc *desc; + + irq_enter(); + check_stack_overflow(); + + desc = irq_resolve_mapping(domain, hwirq); + if (likely(desc)) + handle_irq_desc(desc); + + irq_exit(); +} diff --git a/drivers/irqchip/irq-mips-cpu.c b/drivers/irqchip/irq-mips-cpu.c index 0bbb0b2d0dd5..0c7ae71a0af0 100644 --- a/drivers/irqchip/irq-mips-cpu.c +++ b/drivers/irqchip/irq-mips-cpu.c @@ -127,7 +127,6 @@ static struct irq_chip mips_mt_cpu_irq_controller = { asmlinkage void __weak plat_irq_dispatch(void) { unsigned long pending = read_c0_cause() & read_c0_status() & ST0_IM; - unsigned int virq; int irq; if (!pending) { @@ -137,12 +136,15 @@ asmlinkage void __weak plat_irq_dispatch(void) pending >>= CAUSEB_IP; while (pending) { + struct irq_domain *d; + irq = fls(pending) - 1; if (IS_ENABLED(CONFIG_GENERIC_IRQ_IPI) && irq < 2) - virq = irq_linear_revmap(ipi_domain, irq); + d = ipi_domain; else - virq = irq_linear_revmap(irq_domain, irq); - do_IRQ(virq); + d = irq_domain; + + do_domain_IRQ(d, irq); pending &= ~BIT(irq); } } diff --git a/drivers/irqchip/irq-mips-gic.c b/drivers/irqchip/irq-mips-gic.c index b146e069bf5b..54c7092cc61d 100644 --- a/drivers/irqchip/irq-mips-gic.c +++ b/drivers/irqchip/irq-mips-gic.c @@ -169,8 +169,8 @@ static void gic_handle_shared_int(bool chained) generic_handle_domain_irq(gic_irq_domain, GIC_SHARED_TO_HWIRQ(intr)); else - do_IRQ(irq_find_mapping(gic_irq_domain, - GIC_SHARED_TO_HWIRQ(intr))); + do_domain_IRQ(gic_irq_domain, + GIC_SHARED_TO_HWIRQ(intr)); } } @@ -320,8 +320,8 @@ static void gic_handle_local_int(bool chained) generic_handle_domain_irq(gic_irq_domain, GIC_LOCAL_TO_HWIRQ(intr)); else - do_IRQ(irq_find_mapping(gic_irq_domain, - GIC_LOCAL_TO_HWIRQ(intr))); + do_domain_IRQ(gic_irq_domain, + GIC_LOCAL_TO_HWIRQ(intr)); } } diff --git a/drivers/irqchip/irq-pic32-evic.c b/drivers/irqchip/irq-pic32-evic.c index 34c4b4ffacd1..1d9bb28d13e5 100644 --- a/drivers/irqchip/irq-pic32-evic.c +++ b/drivers/irqchip/irq-pic32-evic.c @@ -42,11 +42,10 @@ static void __iomem *evic_base; asmlinkage void __weak plat_irq_dispatch(void) { - unsigned int irq, hwirq; + unsigned int hwirq; hwirq = readl(evic_base + REG_INTSTAT) & 0xFF; - irq = irq_linear_revmap(evic_irq_domain, hwirq); - do_IRQ(irq); + do_domain_IRQ(evic_irq_domain, hwirq); } static struct evic_chip_data *irqd_to_priv(struct irq_data *data)
Since d4a45c68dc81 ("irqdomain: Protect the linear revmap with RCU"), any irqdomain lookup requires the RCU read lock to be held. This assumes that the architecture code will be structured such as irq_enter() will be called *before* the interrupt is looked up in the irq domain. However, this isn't the case for MIPS, and a number of drivers are structured to do it the other way around when handling an interrupt in their root irqchip (secondary irqchips are OK by construction). This results in a RCU splat on a lockdep-enabled kernel when the kernel takes an interrupt from idle, as reported by Guenter Roeck. Note that this could have fired previously if any driver had used tree-based irqdomain, which always had the RCU requirement. To solve this, provide a MIPS-specific helper (do_domain_IRQ()) as the pendent of do_IRQ() that will do thing in the right order (and maybe save some cycles in the process). Ideally, MIPS would be moved over to using handle_domain_irq(), but that's much more ambitious. Reported-by: Guenter Roeck <linux@roeck-us.net> Tested-by: Guenter Roeck <linux@roeck-us.net> Signed-off-by: Marc Zyngier <maz@kernel.org> Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de> Cc: Serge Semin <fancer.lancer@gmail.com> Link: https://lore.kernel.org/r/20210705172352.GA56304@roeck-us.net --- arch/mips/include/asm/irq.h | 3 +++ arch/mips/kernel/irq.c | 14 ++++++++++++++ drivers/irqchip/irq-mips-cpu.c | 10 ++++++---- drivers/irqchip/irq-mips-gic.c | 8 ++++---- drivers/irqchip/irq-pic32-evic.c | 5 ++--- 5 files changed, 29 insertions(+), 11 deletions(-)