Re: [PATCH v7 7/8] PCI: of: Set fwnode device of newly created PCI device nodes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The patch fails to achieve its stated goal because `of_changeset_apply()` unconditionally sets the `FWNODE_FLAG_NOT_DEVICE` flag, causing `fw_devlink` to ignore the explicitly set fwnode device.
- [High] Dangling `fwnode->dev` pointer leads to a Use-After-Free when the PCI device is removed.
--

commit 3981405050778ad9837921511772531243255e6a
Author: Herve Codina <herve.codina@bootlin.com>

PCI: of: Set fwnode device of newly created PCI device nodes

This commit sets the fwnode device field of dynamically created PCI device
nodes to the corresponding PCI device. This aims to allow fw_devlink to
properly use the PCI device as a supplier, instead of falling back to the
PCI root bridge.

> diff --git a/drivers/pci/of.c b/drivers/pci/of.c
> index 7506208815a47..25fb02c3dc720 100644
> --- a/drivers/pci/of.c
> +++ b/drivers/pci/of.c
> @@ -709,6 +709,13 @@ void of_pci_make_dev_node(struct pci_dev *pdev)
>  	if (ret)
>  		goto out_free_node;
>  
> +	/*
> +	 * Set the fwnode device in order to have fw_devlink creating links
> +	 * pointing to this PCI device instead of walking up to the PCI host
> +	 * bridge.
> +	 */
> +	fw_devlink_set_device(&np->fwnode, &pdev->dev);

Can this lead to a use-after-free regression when the PCI device is removed?

During PCI device removal, of_pci_remove_node() calls
device_remove_of_node(&pdev->dev) which explicitly sets pdev->dev.fwnode
to NULL.

Later, when device_del(&pdev->dev) is executed, its built-in fwnode cleanup
mechanism is completely bypassed because pdev->dev.fwnode was already cleared:

    if (dev->fwnode && dev->fwnode->dev == dev)
        fw_devlink_set_device(dev->fwnode, NULL);

This leaves np->fwnode.dev as a dangling pointer to &pdev->dev.

If the OF node outlives the PCI device, any subsequent operation that accesses
the node through fw_devlink will dereference the freed pdev->dev memory.

> +
>  	ret = of_changeset_apply(cset);

Does this code actually prevent fw_devlink from walking up to the PCI host
bridge?

of_changeset_apply() invokes __of_attach_node(), which unconditionally
executes fwnode_set_flag(&np->fwnode, FWNODE_FLAG_NOT_DEVICE).

When fw_devlink attempts to resolve a supplier, it checks for this flag:

    if (fwnode_test_flag(sup_handle, FWNODE_FLAG_NOT_DEVICE))
        sup_dev = fwnode_get_next_parent_dev(sup_handle);

Because FWNODE_FLAG_NOT_DEVICE is set, fw_devlink bypasses the explicitly
mapped np->fwnode.dev and walks up the tree to the parent device anyway.

>  	if (ret)
>  		goto out_free_node;

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