From: Geert Uytterhoeven <geert@linux-m68k.org>
To: Bjorn Helgaas <helgaas@kernel.org>
Cc: "Marek Vasut" <marek.vasut+renesas@gmail.com>,
"Yoshihiro Shimoda" <yoshihiro.shimoda.uh@renesas.com>,
"Liang He" <windhl@126.com>,
"Lorenzo Pieralisi" <lpieralisi@kernel.org>,
"Rob Herring" <robh@kernel.org>,
"Krzysztof Wilczyński" <kw@linux.com>,
linux-pci <linux-pci@vger.kernel.org>,
Linux-Renesas <linux-renesas-soc@vger.kernel.org>,
"Linux Kernel Mailing List" <linux-kernel@vger.kernel.org>,
"Bjorn Helgaas" <bhelgaas@google.com>
Subject: Re: [PATCH 2/2] PCI: rcar: Resolve of_find_matching_node() reference leak
Date: Wed, 22 Jun 2022 09:45:49 +0200 [thread overview]
Message-ID: <CAMuHMdUwGLDzOQo_wwSLmzBnJXe-cOw=nqsPbFLsj-c+nHfy_w@mail.gmail.com> (raw)
In-Reply-To: <20220622025732.1359389-3-helgaas@kernel.org>
Hi Bjorn,
On Wed, Jun 22, 2022 at 4:57 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
>
> Previously, rcar_pcie_init() used of_find_matching_node() to search the
> entire device tree for compatible strings for which we need to install an
> abort handler. If we found one, we got a device_node with refcount
> incremented, but we discarded the pointer and never released that
> reference.
>
> Extend the struct rcar_variant to indicate whether each variant requires an
> abort handler. Install the handler in rcar_pcie_probe() when needed.
>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Liang He <windhl@126.com>
> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
Thanks for your patch!
> --- a/drivers/pci/controller/pcie-rcar-host.c
> +++ b/drivers/pci/controller/pcie-rcar-host.c
> @@ -964,12 +965,35 @@ static int rcar_pcie_parse_map_dma_ranges(struct rcar_pcie_host *host)
> return err;
> }
>
> +#ifdef CONFIG_ARM
> +static int rcar_pcie_aarch32_abort_handler(unsigned long addr,
> + unsigned int fsr, struct pt_regs *regs)
> +{
> + return !fixup_exception(regs);
> +}
> +#endif
> +
> +static void rcar_pcie_hook_aborts(void)
> +{
> +#ifdef CONFIG_ARM
> +#ifdef CONFIG_ARM_LPAE
> + hook_fault_code(17, rcar_pcie_aarch32_abort_handler, SIGBUS, 0,
> + "asynchronous external abort");
> +#else
> + hook_fault_code(22, rcar_pcie_aarch32_abort_handler, SIGBUS, 0,
> + "imprecise external abort");
> +#endif
> +#endif
> +}
> +
> static const struct rcar_variant rcar_h1_data = {
> .phy_init_fn = rcar_pcie_phy_init_h1,
> + .hook_aborts = true,
> };
>
> static const struct rcar_variant rcar_gen2_data = {
> .phy_init_fn = rcar_pcie_phy_init_gen2,
> + .hook_aborts = true,
> };
>
> static const struct rcar_variant rcar_gen3_data = {
> @@ -1035,6 +1059,9 @@ static int rcar_pcie_probe(struct platform_device *pdev)
> goto err_clk_disable;
> }
>
> + if (host->variant->hook_aborts)
> + rcar_pcie_hook_aborts();
I was quite sure there was a good reason why this was not done in
.probe() before...
And indeed, the original submission[1] did have a comment explaining
that:
+ /*
+ * Since probe() can be deferred we need to make sure that
+ * hook_fault_code is not called after __init memory is freed
+ * by kernel and since rcar_pcie_abort_handler() is a no-op,
+ * we can install the handler here without risking it
+ * accessing some uninitialized driver state.
+ */
No idea why it was removed in v2 and later, but the point is:
hook_fault_code() is __init, so you cannot call it from a deferred
probe.
And you should have got a section mismatch warning ;-)
[1] https://lore.kernel.org/all/20200912211853.15321-1-marek.vasut@gmail.com/
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
next prev parent reply other threads:[~2022-06-22 7:46 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-06-22 2:57 [PATCH 0/2] PCI: rcar: Fix of_find_matching_node() reference leak Bjorn Helgaas
2022-06-22 2:57 ` [PATCH 1/2] PCI: rcar: Add dev struct for of_device_get_match_data() Bjorn Helgaas
2022-06-22 2:57 ` [PATCH 2/2] PCI: rcar: Resolve of_find_matching_node() reference leak Bjorn Helgaas
2022-06-22 7:45 ` Geert Uytterhoeven [this message]
2022-06-22 11:05 ` Bjorn Helgaas
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAMuHMdUwGLDzOQo_wwSLmzBnJXe-cOw=nqsPbFLsj-c+nHfy_w@mail.gmail.com' \
--to=geert@linux-m68k.org \
--cc=bhelgaas@google.com \
--cc=helgaas@kernel.org \
--cc=kw@linux.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pci@vger.kernel.org \
--cc=linux-renesas-soc@vger.kernel.org \
--cc=lpieralisi@kernel.org \
--cc=marek.vasut+renesas@gmail.com \
--cc=robh@kernel.org \
--cc=windhl@126.com \
--cc=yoshihiro.shimoda.uh@renesas.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).