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

[sashiko-bot@kernel.org]

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?

>  	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?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260708100302.517792-1-herve.codina@bootlin.com?part=7