Re: [PATCH v9 7/9] PCI: of: Clear fwnode->dev during root bridge node removal

[Herve Codina <herve.codina@bootlin.com>]

On Wed, 08 Jul 2026 10:34:34 +0000
sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] Missing fw_devlink cleanup in the error path of of_pci_make_host_bridge_node(), leaving a dangling fwnode->dev pointer.
> - [High] Kernel panic on removal of overlay-provided PCI host bridge nodes due to unsafe changeset revert.
> --
> 
> --- Patch [7]: [PATCH v9 7/9] PCI: of: Clear fwnode->dev during root bridge node removal ---
> commit 9e6873c21d6442ce4222fe6e9ac417348c8b3fbd
> Author: Herve Codina <herve.codina@bootlin.com>
> 
> PCI: of: Clear fwnode->dev during root bridge node removal
> 
> This commit clears fwnode->dev during of_pci_remove_host_bridge_node() to
> prevent a dangling pointer. It ensures fw_devlink_set_device() is called
> with a NULL device before the fwnode is cleared.
> 
> > diff --git a/drivers/pci/of.c b/drivers/pci/of.c
> > index ee9eb384b377a..8dd558a490bbd 100644
> > --- a/drivers/pci/of.c
> > +++ b/drivers/pci/of.c
> > @@ -743,6 +743,7 @@ void of_pci_remove_host_bridge_node(struct pci_host_bridge *bridge)
> >  	if (!np || !of_node_check_flag(np, OF_DYNAMIC))
> >  		return;
> >  
> > +	fw_devlink_set_device(&np->fwnode, NULL);  
> 
> [Severity: High]
> This is a pre-existing issue, but does of_pci_make_host_bridge_node()
> also need to clear the fwnode->dev pointer on its error path?
> 
> If device_add_of_node() fails in of_pci_make_host_bridge_node(), it jumps
> to the error paths but leaves np->fwnode.dev pointing to the bridge device:
> 
> drivers/pci/of.c:of_pci_make_host_bridge_node() {
> 	...
> 	fw_devlink_set_device(&np->fwnode, &bridge->dev);
> 	...
> 	/* Add the of_node to host bridge and the root bus */
> 	ret = device_add_of_node(&bridge->dev, np);
> 	if (ret)
> 		goto out_revert_cset;
> 
> 	ret = device_add_of_node(&bridge->bus->dev, np);
> 	if (ret)
> 		goto out_remove_bridge_dev_of_node;
> 	...
> out_remove_bridge_dev_of_node:
> 	device_remove_of_node(&bridge->dev);
> out_revert_cset:
> 	np->data = NULL;
> 	of_changeset_revert(cset);
> out_free_node:
> 	of_node_put(np);
> 	...
> }
> 
> If a concurrent thread acquired a reference to np, could the node outlive
> the error path and result in a use-after-free where fwnode->dev points to
> freed memory?

This issue is relevant and clearing np->fwnode.dev in the error path will
be take into account in the next iteration.

> 
> >  	device_remove_of_node(&bridge->bus->dev);
> >  	device_remove_of_node(&bridge->dev);
> >  	of_changeset_revert(np->data);  
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but does calling
> of_changeset_revert(np->data) unconditionally cause a crash when removing
> an overlay-provided PCI host bridge node?
> 
> When an overlay provides the device tree node for a PCI host bridge, the
> node inherently has the OF_DYNAMIC flag set. If the bridge is removed,
> of_pci_remove_host_bridge_node() assumes the node was created by
> of_pci_make_host_bridge_node() and executes of_changeset_revert(np->data).
> 
> Since np->data is NULL for standard overlay nodes, does this result in a
> NULL pointer dereference in __of_changeset_revert_entries() when it tries
> to evaluate &ocs->entries?
> 

This scenario should not exist but anyway, I will add a new patch in the
series in order to have a more robust code checking for a NULL np->data in
both of_pci_remove_host_bridge_node() and of_pci_remove_node().

Hervé