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

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

[Herve Codina]

Hi All,

On Tue, 12 May 2026 22:46:21 +0000
sashiko-bot@kernel.org wrote:

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

It will not. In device_remove_of_node(), the OF node is destroyed when the
of_changeset_revert() and of_changeset_destroy() are called.

> 
> > +
> >  	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;  
> 

Well, an other series fixes this FWNODE_FLAG_NOT_DEVICE issue [1].

In this one, with or without the fw_devlink_set_device() call the issue is
present.


[1] patches 1 and 2 in https://lore.kernel.org/all/20260511155755.34428-1-herve.codina@bootlin.com/

Best regards,
Hervé