Re: [PATCH] of/irq: Fix device node refcount leak in of_check_msi_parent()

[Lorenzo Pieralisi <lpieralisi@kernel.org>]

On Fri, Sep 04, 2026 at 01:48:11PM +0100, Fuad Tabba wrote:
> On Fri, 4 Sept 2026 at 12:11, Lorenzo Pieralisi <lpieralisi@kernel.org> wrote:
> ...
> > > It does continue. -EINVAL also covers #msi-cells != 0, so
> > > of_msi_xlate() cannot tell a phandle mismatch from an msi-parent that
> > > is not a 1:1 mapping, and breaking on it would stop the walk for both.
> > >
> > > Lorenzo, Rob, should the walk stop at the first node declaring an msi-parent?
> >
> > First off, thank you for fixing this.
> >
> > I think we must stop at the first msi-parent and apply the same (convoluted)
> > logic that of_msi_xlate() executes upon of_map_msi_id()'s return to make
> > sure ref counts are kosher (we already have an msi_spec scratch variable,
> > we can pass that to of_check_msi_parent() - that is, always drop the
> > msi_spec.np reference if !NULL and get one if !*msi_np to initialize it).
> 
> Thanks for the clarification.

I can send a patch myself - I understand this is a nuisance for you - I think
though that since you are in the process of fixing it I don't want to get
in the way either.

Let me know.

Thanks,
Lorenzo

> 
> Cheers,
> /fuad
> 
> > >
> > > >
> > > > [Severity: High]
> > > > This is a pre-existing issue, but does of_msi_xlate() fail to traverse the
> > > > device hierarchy when msi_np is NULL?
> > > >
> > > > When of_map_msi_id() (which calls of_map_id()) processes an absent map
> > > > property while filter_np is NULL, it returns 0 instead of an error:
> > > >
> > > > drivers/of/base.c:of_map_id() {
> > > >         ...
> > > >         map = of_get_property(np, map_name, &map_bytes);
> > > >         if (!map) {
> > > >                 if (filter_np)
> > > >                         return -ENODEV;
> > > >                 /* Otherwise, no map implies no translation */
> > > >                 arg->args[0] = id;
> > > >                 arg->args_count = 1;
> > > >                 return 0;
> > > >         }
> > > >         ...
> > > > }
> > > >
> > > > In of_msi_xlate(), this return value of 0 triggers a break:
> > > >
> > > > drivers/of/irq.c:of_msi_xlate() {
> > > >                 if (!of_map_msi_id(parent_dev->of_node, id_in, msi_np, &msi_spec)) {
> > > >                         ...
> > > >                         /* msi-map present but no match -> stop walking */
> > > >                         break;
> > > >                 }
> > > > }
> > > >
> > > > Because of_map_id() returns 0 when the map is absent, wouldn't this early
> > > > break erroneously abort the parent traversal loop on the first iteration,
> > > > breaking MSI ID mapping for callers passing a NULL msi_np?
> > >
> > > It does break early there, but not for fsl-mc, the only NULL caller:
> > > AFAICT, an fsl_mc_device never gets an of_node, so the walk carries on
> > > up to the fsl-mc platform device, which is where fsl,qoriq-mc puts
> > > msi-map.
> > >
> > > > [Severity: High]
> > > > This isn't a bug introduced by this patch, but is there a fwnode reference
> > > > leak on the error path in its_v5_pci_msi_prepare()?
> > > >
> > > > drivers/irqchip/irq-gic-its-msi-parent.c:its_v5_pci_msi_prepare() {
> > > >         rid = pci_msi_map_rid_ctlr_node(domain->parent, pdev, &msi_node);
> > > >         if (!msi_node)
> > > >                 return -ENODEV;
> > > >
> > > >         ret = its_translate_frame_address(msi_node, &pa);
> > > >         if (ret)
> > > >                 return -ENODEV;
> > > >
> > > >         fwnode_handle_put(msi_node);
> > > >         ...
> > > > }
> > > >
> > > > If its_translate_frame_address() fails, it returns -ENODEV without dropping
> > > > the acquired msi_node reference. Can this leak the fwnode_handle if the
> > > > frame address translation fails repeatedly during driver bind or unbind?
> > >
> > > Yes, every failed translation leaks one. I have a patch written,
> > > moving the put above the error check. It goes with the irqchip fixes
> > > rather than here.
> >
> > Yep.
> >
> > Thanks,
> > Lorenzo