| 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
| ||
|
Message-ID: <aO9ROPBGC6zF1B+i@lpieralisi>
Date: Wed, 15 Oct 2025 09:46:00 +0200
From: Lorenzo Pieralisi <lpieralisi@...nel.org>
To: Marc Zyngier <maz@...nel.org>
Cc: linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
devicetree@...r.kernel.org, linux-pci@...r.kernel.org,
Sascha Bischoff <sascha.bischoff@....com>,
Thomas Gleixner <tglx@...utronix.de>, Rob Herring <robh@...nel.org>,
Frank Li <Frank.Li@....com>, Scott Branden <sbranden@...adcom.com>,
Bjorn Helgaas <bhelgaas@...gle.com>, Ray Jui <rjui@...adcom.com>,
Manivannan Sadhasivam <mani@...nel.org>,
Krzysztof Wilczyński <kwilczynski@...nel.org>
Subject: Re: [PATCH v2 4/4] irqchip/gic-its: Rework platform MSI deviceID
detection
On Tue, Oct 14, 2025 at 06:12:11PM +0100, Marc Zyngier wrote:
> On Tue, 14 Oct 2025 10:58:45 +0100,
> Lorenzo Pieralisi <lpieralisi@...nel.org> wrote:
> >
> > Current code retrieving platform devices MSI devID in the GIC ITS MSI
> > parent helpers suffers from some minor issues:
> >
> > - It leaks a struct device_node reference
> > - It triggers an excessive WARN_ON on wrong of_phandle_args count detection
>
> Well, if your DT is that rotten, maybe you actually deserve some
> console spamming, don't you think?
Yes from that standpoint it would make sense to leave the WARN_ON there,
I can add it back.
> > - It is duplicated between GICv3 and GICv5 for no good reason
> > - It does not use the OF phandle iterator code that simplifies
> > the msi-parent property parsing
> >
> > Implement a helper function that addresses the full set of issues in one go
> > by consolidating GIC v3 and v5 code and converting the msi-parent parsing
> > loop to the more modern OF phandle iterator API, fixing the
> > struct device_node reference leak in the process.
> >
> > Signed-off-by: Lorenzo Pieralisi <lpieralisi@...nel.org>
> > Cc: Sascha Bischoff <sascha.bischoff@....com>
> > Cc: Thomas Gleixner <tglx@...utronix.de>
> > Cc: Rob Herring <robh@...nel.org>
> > Cc: Frank Li <Frank.Li@....com>
> > Cc: Marc Zyngier <maz@...nel.org>
> > ---
> > drivers/irqchip/irq-gic-its-msi-parent.c | 98 ++++++++----------------
> > 1 file changed, 33 insertions(+), 65 deletions(-)
> >
> > diff --git a/drivers/irqchip/irq-gic-its-msi-parent.c b/drivers/irqchip/irq-gic-its-msi-parent.c
> > index eb1473f1448a..a65f762b7dd4 100644
> > --- a/drivers/irqchip/irq-gic-its-msi-parent.c
> > +++ b/drivers/irqchip/irq-gic-its-msi-parent.c
> > @@ -142,83 +142,51 @@ static int its_v5_pci_msi_prepare(struct irq_domain *domain, struct device *dev,
> > #define its_v5_pci_msi_prepare NULL
> > #endif /* !CONFIG_PCI_MSI */
> >
> > -static int of_pmsi_get_dev_id(struct irq_domain *domain, struct device *dev,
> > - u32 *dev_id)
> > +static int __of_pmsi_get_dev_id(struct irq_domain *domain, struct device *dev, u32 *dev_id,
> > + phys_addr_t *pa, bool is_v5)
> > {
> > - int ret, index = 0;
> > + struct of_phandle_iterator it;
> > + uint32_t args;
>
> Use u32, this is not userspace-visible (the OF code will cope). And
> move it to where it matters instead of having such a wide scope.
Ok.
> > + int ret;
> >
> > /* Suck the DeviceID out of the msi-parent property */
> > - do {
> > - struct of_phandle_args args;
> > + of_for_each_phandle(&it, ret, dev->of_node, "msi-parent", "#msi-cells", -1) {
> > + /* GICv5 ITS domain matches the MSI controller node parent */
> > + struct device_node *np __free(device_node) = is_v5 ? of_get_parent(it.node)
> > + : of_node_get(it.node);
> >
> > - ret = of_parse_phandle_with_args(dev->of_node,
> > - "msi-parent", "#msi-cells",
> > - index, &args);
> > - if (args.np == irq_domain_get_of_node(domain)) {
> > - if (WARN_ON(args.args_count != 1))
> > - return -EINVAL;
> > - *dev_id = args.args[0];
> > - break;
> > + if (np == irq_domain_get_of_node(domain)) {
> > + if (of_phandle_iterator_args(&it, &args, 1) != 1) {
> > + dev_warn(dev, "Bogus msi-parent property\n");
> > + ret = -EINVAL;
> > + }
> > +
> > + if (!ret && is_v5)
> > + ret = its_translate_frame_address(it.node, pa);
>
> Why do you need this is_v5 hack, since the only case were you pass a
> pointer to get the translate register address is for v5?
Yep, I thought about this what you are suggesting makes sense - is_v5 is
useless (and terrible).
> > +
> > + if (!ret)
> > + *dev_id = args;
> > +
> > + of_node_put(it.node);
> > + return ret;
> > }
> > - index++;
> > - } while (!ret);
> > -
> > - if (ret) {
> > - struct device_node *np = NULL;
> > -
> > - ret = of_map_id(dev->of_node, dev->id, "msi-map", "msi-map-mask", &np, dev_id);
> > - if (np)
> > - of_node_put(np);
> > }
> >
> > - return ret;
> > + struct device_node *msi_ctrl __free(device_node) = NULL;
> > +
> > + return of_map_id(dev->of_node, dev->id, "msi-map", "msi-map-mask", &msi_ctrl, dev_id);
> > +}
> > +
> > +static int of_pmsi_get_dev_id(struct irq_domain *domain, struct device *dev,
> > + u32 *dev_id)
> > +{
> > + return __of_pmsi_get_dev_id(domain, dev, dev_id, NULL, false);
> > }
>
> At this stage, we really don't need these on-liners, as they only
> obfuscate the logic. Just use the main helper directly. Something like
> the hack below.
That makes sense.
Thanks !
Lorenzo
>
> M.
>
> diff --git a/drivers/irqchip/irq-gic-its-msi-parent.c b/drivers/irqchip/irq-gic-its-msi-parent.c
> index a65f762b7dd4d..7c82fd152655e 100644
> --- a/drivers/irqchip/irq-gic-its-msi-parent.c
> +++ b/drivers/irqchip/irq-gic-its-msi-parent.c
> @@ -142,26 +142,27 @@ static int its_v5_pci_msi_prepare(struct irq_domain *domain, struct device *dev,
> #define its_v5_pci_msi_prepare NULL
> #endif /* !CONFIG_PCI_MSI */
>
> -static int __of_pmsi_get_dev_id(struct irq_domain *domain, struct device *dev, u32 *dev_id,
> - phys_addr_t *pa, bool is_v5)
> +static int of_pmsi_get_msi_info(struct irq_domain *domain, struct device *dev, u32 *dev_id,
> + phys_addr_t *pa)
> {
> struct of_phandle_iterator it;
> - uint32_t args;
> int ret;
>
> /* Suck the DeviceID out of the msi-parent property */
> of_for_each_phandle(&it, ret, dev->of_node, "msi-parent", "#msi-cells", -1) {
> /* GICv5 ITS domain matches the MSI controller node parent */
> - struct device_node *np __free(device_node) = is_v5 ? of_get_parent(it.node)
> + struct device_node *np __free(device_node) = pa ? of_get_parent(it.node)
> : of_node_get(it.node);
>
> if (np == irq_domain_get_of_node(domain)) {
> + u32 args;
> +
> if (of_phandle_iterator_args(&it, &args, 1) != 1) {
> dev_warn(dev, "Bogus msi-parent property\n");
> ret = -EINVAL;
> }
>
> - if (!ret && is_v5)
> + if (!ret && pa)
> ret = its_translate_frame_address(it.node, pa);
>
> if (!ret)
> @@ -177,18 +178,6 @@ static int __of_pmsi_get_dev_id(struct irq_domain *domain, struct device *dev, u
> return of_map_id(dev->of_node, dev->id, "msi-map", "msi-map-mask", &msi_ctrl, dev_id);
> }
>
> -static int of_pmsi_get_dev_id(struct irq_domain *domain, struct device *dev,
> - u32 *dev_id)
> -{
> - return __of_pmsi_get_dev_id(domain, dev, dev_id, NULL, false);
> -}
> -
> -static int of_v5_pmsi_get_msi_info(struct irq_domain *domain, struct device *dev,
> - u32 *dev_id, phys_addr_t *pa)
> -{
> - return __of_pmsi_get_dev_id(domain, dev, dev_id, pa, true);
> -}
> -
> int __weak iort_pmsi_get_dev_id(struct device *dev, u32 *dev_id)
> {
> return -1;
> @@ -202,7 +191,7 @@ static int its_pmsi_prepare(struct irq_domain *domain, struct device *dev,
> int ret;
>
> if (dev->of_node)
> - ret = of_pmsi_get_dev_id(domain->parent, dev, &dev_id);
> + ret = of_pmsi_get_msi_info(domain->parent, dev, &dev_id, NULL);
> else
> ret = iort_pmsi_get_dev_id(dev, &dev_id);
> if (ret)
> @@ -230,7 +219,7 @@ static int its_v5_pmsi_prepare(struct irq_domain *domain, struct device *dev,
> if (!dev->of_node)
> return -ENODEV;
>
> - ret = of_v5_pmsi_get_msi_info(domain->parent, dev, &dev_id, &pa);
> + ret = of_pmsi_get_msi_info(domain->parent, dev, &dev_id, &pa);
> if (ret)
> return ret;
>
>
> --
> Jazz isn't dead. It just smells funny.
Powered by blists - more mailing lists