linux-kernel - Re: [PATCH] irqchip/riscv-aplic: Fix crash when MSI domain is missing

Open Source and information security mailing list archives

Message-ID: <CAK9=C2UUq=k9JHrO9zeiPhdmakyzigm9ToQFM2S90bQs31=98w@mail.gmail.com>
Date: Mon, 18 Nov 2024 13:49:31 +0530
From: Anup Patel <apatel@...tanamicro.com>
To: Samuel Holland <samuel.holland@...ive.com>
Cc: Anup Patel <anup@...infault.org>, Thomas Gleixner <tglx@...utronix.de>, 
	Albert Ou <aou@...s.berkeley.edu>, Björn Töpel <bjorn@...osinc.com>, 
	Palmer Dabbelt <palmer@...belt.com>, Paul Walmsley <paul.walmsley@...ive.com>, 
	linux-kernel@...r.kernel.org, linux-riscv@...ts.infradead.org
Subject: Re: [PATCH] irqchip/riscv-aplic: Fix crash when MSI domain is missing

On Fri, Nov 15, 2024 at 9:27 PM Samuel Holland
<samuel.holland@...ive.com> wrote:
>
> Hi Anup,
>
> On 2024-11-15 9:42 AM, Anup Patel wrote:
> > On Fri, Nov 15, 2024 at 1:31 AM Samuel Holland
> > <samuel.holland@...ive.com> wrote:
> >>
> >> If the APLIC driver is probed before the IMSIC driver, the parent MSI
> >> domain will be missing, which causes a NULL pointer dereference in
> >> msi_create_device_irq_domain(). Avoid this by deferring probe until the
> >> parent MSI domain is available. Use dev_err_probe() to avoid printing an
> >> error message when returning -EPROBE_DEFER.
> >
> > The -EPROBE_DEFER is not needed because we expect that platforms to
> > use "msi-parent" DT property in APLIC DT node which in-turn allows Linux
> > DD framework to re-order probing based on fw_devlink dependencies. The
> > APLIC DT bindings mandates that any of "interrupt-extended" or "msi-parent"
> > DT properties MUST be present.
> >
> > Can you elaborate a bit more on how you are hitting this issue ?
>
> I agree that fw_devlink should help avoid the situation where we need to return
> -EPROBE_DEFER, but the kernel must still not crash even if fw_devlink is
> disabled (which is a perfectly valid thing to do: "fw_devlink=off" on the kernel
> command line) or if fw_devlink fails to come up with the ideal probe order.
> fw_devlink is an optimization. It should not be relied on for correctness. In my
> specific case, fw_devlink got the order wrong due to some false dependency
> cycles, which I sent a patch for separately[1].

The RISC-V kernel is heavily dependent on fw_devlink based probe ordering
and more upcoming drivers are going to increase this dependency.
For example, we also have RISC-V IOMMU driver that needs to be probed
after IMSIC since it can use MSIs.

I think we should ensure that fw_devlink can't be disabled/turned-off for the
RISC-V kernel. If this is not possible then we should have very verbose
boot-time warning when fw_devlink is disabled/turned-off.

Your other "interrupt-parent" related fix [1] looks fine to me.

Regards,
Anup

>
> Regards,
> Samuel
>
> [1]:
> https://lore.kernel.org/lkml/20241114195652.3068725-1-samuel.holland@sifive.com/
>
> >> Fixes: ca8df97fe679 ("irqchip/riscv-aplic: Add support for MSI-mode")
> >> Signed-off-by: Samuel Holland <samuel.holland@...ive.com>
> >> ---
> >>
> >>  drivers/irqchip/irq-riscv-aplic-main.c | 3 ++-
> >>  drivers/irqchip/irq-riscv-aplic-msi.c  | 3 +++
> >>  2 files changed, 5 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/irqchip/irq-riscv-aplic-main.c b/drivers/irqchip/irq-riscv-aplic-main.c
> >> index 900e72541db9..93e7c51f944a 100644
> >> --- a/drivers/irqchip/irq-riscv-aplic-main.c
> >> +++ b/drivers/irqchip/irq-riscv-aplic-main.c
> >> @@ -207,7 +207,8 @@ static int aplic_probe(struct platform_device *pdev)
> >>         else
> >>                 rc = aplic_direct_setup(dev, regs);
> >>         if (rc)
> >> -               dev_err(dev, "failed to setup APLIC in %s mode\n", msi_mode ? "MSI" : "direct");
> >> +               dev_err_probe(dev, rc, "failed to setup APLIC in %s mode\n",
> >> +                             msi_mode ? "MSI" : "direct");
> >>
> >>  #ifdef CONFIG_ACPI
> >>         if (!acpi_disabled)
> >> diff --git a/drivers/irqchip/irq-riscv-aplic-msi.c b/drivers/irqchip/irq-riscv-aplic-msi.c
> >> index 945bff28265c..fb8d1838609f 100644
> >> --- a/drivers/irqchip/irq-riscv-aplic-msi.c
> >> +++ b/drivers/irqchip/irq-riscv-aplic-msi.c
> >> @@ -266,6 +266,9 @@ int aplic_msi_setup(struct device *dev, void __iomem *regs)
> >>                         if (msi_domain)
> >>                                 dev_set_msi_domain(dev, msi_domain);
> >>                 }
> >> +
> >> +               if (!dev_get_msi_domain(dev))
> >> +                       return -EPROBE_DEFER;
> >>         }
> >>
> >>         if (!msi_create_device_irq_domain(dev, MSI_DEFAULT_DOMAIN, &aplic_msi_template,
> >> --
> >> 2.45.1
> >>
> >>
> >> _______________________________________________
> >> linux-riscv mailing list
> >> linux-riscv@...ts.infradead.org
> >> http://lists.infradead.org/mailman/listinfo/linux-riscv
>

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