| 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
| ||
|
Date: Thu, 28 Jul 2022 08:44:58 +0100
From: Marc Zyngier <maz@...nel.org>
To: Vladimir Oltean <vladimir.oltean@....com>
Cc: linux-kernel@...r.kernel.org, Lee Jones <lee.jones@...aro.org>,
Thomas Gleixner <tglx@...utronix.de>,
Rasmus Villemoes <linux@...musvillemoes.dk>,
Arnd Bergmann <arnd@...db.de>,
Hou Zhiqiang <Zhiqiang.Hou@....com>,
Biwen Li <biwen.li@....com>,
Sean Anderson <sean.anderson@...o.com>
Subject: Re: [PATCH v3] irqchip/ls-extirq: fix invalid wait context by avoiding to use regmap
On Wed, 27 Jul 2022 17:09:15 +0100,
Vladimir Oltean <vladimir.oltean@....com> wrote:
>
> The irqchip->irq_set_type method is called by __irq_set_trigger() under
> the desc->lock raw spinlock.
>
> The ls-extirq implementation, ls_extirq_irq_set_type(), uses an MMIO
> regmap created by of_syscon_register(), which uses plain spinlocks
> (the kind that are sleepable on RT).
>
> Therefore, this is an invalid locking scheme for which we get a kernel
> splat stating just that ("[ BUG: Invalid wait context ]"), because the
> context in which the plain spinlock may sleep is atomic due to the raw
> spinlock. We need to go raw spinlocks all the way.
Interesting you say that...
>
> Make this driver ioremap its INTPCR register on its own, and stop
> relying on syscon to provide a regmap. Since the regmap we got from
> syscon belonged to the parent and the newly ioremapped region belongs
> just to us, the offset to the INTPCR register is now 0, because of the
> address translation that takes place through the device tree.
>
> One complication, due to the fact that this driver uses IRQCHIP_DECLARE
> rather than traditional platform devices with probe and remove methods,
> is that we cannot use devres, so we need to implement a full-blown
> cleanup procedure on the error path.
>
> Fixes: 0dcd9f872769 ("irqchip: Add support for Layerscape external interrupt lines")
> Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>
> ---
> v2->v3:
> - stop using regmap, do the rmw manually using function pointers for BE/LE
> - adapt comment style to the subsystem
> - use of_device_is_big_endian
> - reword commit message
>
> v1->v2:
> - create a separate regmap for the ls-extirq driver rather than relying
> on the one provided by syscon or modifying that.
>
> For reference, v1 is at:
> https://lore.kernel.org/lkml/20210825205041.927788-3-vladimir.oltean@nxp.com/
> and v2 is at:
> https://lore.kernel.org/lkml/20220722204019.969272-1-vladimir.oltean@nxp.com/
>
> For extra reviewer convenience, the ls-extirq appears in the following
> SoC device trees:
> https://elixir.bootlin.com/linux/v5.18.13/source/arch/arm64/boot/dts/freescale/fsl-ls208xa.dtsi#L289
> https://elixir.bootlin.com/linux/v5.18.13/source/arch/arm64/boot/dts/freescale/fsl-ls1088a.dtsi#L249
> https://elixir.bootlin.com/linux/v5.18.13/source/arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi#L319
> https://elixir.bootlin.com/linux/v5.18.13/source/arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi#L325
> https://elixir.bootlin.com/linux/v5.18.13/source/arch/arm64/boot/dts/freescale/fsl-lx2160a.dtsi#L682
> https://elixir.bootlin.com/linux/v5.18.13/source/arch/arm/boot/dts/ls1021a.dtsi#L182
>
> Patch tested on LX2160A and LS1021A.
>
> drivers/irqchip/irq-ls-extirq.c | 91 ++++++++++++++++++++++++---------
> 1 file changed, 66 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/irqchip/irq-ls-extirq.c b/drivers/irqchip/irq-ls-extirq.c
> index 853b3972dbe7..cfbbe5959c8e 100644
> --- a/drivers/irqchip/irq-ls-extirq.c
> +++ b/drivers/irqchip/irq-ls-extirq.c
> @@ -6,8 +6,7 @@
> #include <linux/irqchip.h>
> #include <linux/irqdomain.h>
> #include <linux/of.h>
> -#include <linux/mfd/syscon.h>
> -#include <linux/regmap.h>
> +#include <linux/of_address.h>
> #include <linux/slab.h>
>
> #include <dt-bindings/interrupt-controller/arm-gic.h>
> @@ -16,19 +15,40 @@
> #define LS1021A_SCFGREVCR 0x200
>
> struct ls_extirq_data {
> - struct regmap *syscon;
> - u32 intpcr;
> + void __iomem *intpcr;
> + u32 (*read)(void __iomem *addr);
> + void (*write)(void __iomem *addr, u32 val);
> bool is_ls1021a_or_ls1043a;
> u32 nirq;
> struct irq_fwspec map[MAXIRQ];
> };
>
> +static u32 ls_extirq_read_be(void __iomem *addr)
> +{
> + return ioread32be(addr);
> +}
> +
> +static u32 ls_extirq_read(void __iomem *addr)
> +{
> + return ioread32(addr);
> +}
> +
> +static void ls_extirq_write_be(void __iomem *addr, u32 val)
> +{
> + iowrite32be(val, addr);
> +}
> +
> +static void ls_extirq_write(void __iomem *addr, u32 val)
> +{
> + iowrite32(val, addr);
> +}
> +
> static int
> ls_extirq_set_type(struct irq_data *data, unsigned int type)
> {
> struct ls_extirq_data *priv = data->chip_data;
> irq_hw_number_t hwirq = data->hwirq;
> - u32 value, mask;
> + u32 intpcr, value, mask;
>
> if (priv->is_ls1021a_or_ls1043a)
> mask = 1U << (31 - hwirq);
> @@ -51,7 +71,11 @@ ls_extirq_set_type(struct irq_data *data, unsigned int type)
> default:
> return -EINVAL;
> }
> - regmap_update_bits(priv->syscon, priv->intpcr, mask, value);
> +
> + intpcr = priv->read(priv->intpcr);
> + intpcr &= ~mask;
> + intpcr |= value;
> + priv->write(priv->intpcr, intpcr);
Which really begs a few questions:
- Where is the locking gone? Sure, the warning is gone. But the driver
is now broken for *all* configurations, and not only RT. Result!
- Is it *really* worth it to have 4 dumb helpers that bring nothing in
terms of abstraction, and two indirections for something that could
equally be expressed with a conditional?
M.
--
Without deviation from the norm, progress is not possible.
Powered by blists - more mailing lists