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

[Herve Codina]

Hi Richard,

On Thu, 2 Jul 2026 12:02:35 +0800
Richard Cheng <icheng@xxxxxxxxxx> wrote:

> > @@ -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);
> > +
> >  	ret = of_changeset_apply(cset);
> >  	if (ret)
> >  		goto out_free_node;
> > -- 
> > 2.54.0
> > 
> >  
> 
> Hi Herve,
> 
> I wonder if this part has some issue, it sets np->fwnode.dev = &pdev->dev,
> but I don't see am matching clear on removal path, I doubt the back-pointer
> can outlive the pci_dev.
> 
> device_del() do the check
> """
> if (dev->fwnode && dev->fwnode->dev == dev)
>     fw_devlink_set_device(dev->fwnode, NULL);
> """
> 
> On removal, pci_stop_dev() calls of_pci_remove_node() before pci_destroy_dev()
> calls device_del(), and of_pci_remove_node() -> device_remove_of_node() has already NULLed pdev->dev.fwnode by then, so the "dev->fwnode" guard is false, and
> of_pci_remove_node() itself never clears np->fwnode.dev
> 
> If something holds an extra ref on np past removal, e.g. a DT overlay applied via configfs that pins np through its gragment targets,
> np survives, the pci_dev is freed, and np->fwnode.dev dnalges into freed memory.
> Then fw_devlink walker that resolve it via get_dev_from_fwnode() -> get_device() would hit a use-after-free .
> 
> I think of_pci_remove_node() should cleaer the back-pointer it set,
> before dropping the node's ref, e.g.
> 
> """
> np = pci_device_to_OF_node(pdev);
> if (!np || !of_node_check_flag(np, OF_DYNAMIC))
>     return;
> 
> fw_devlink_set_device(&np->fwnode, NULL);
> device_remove_of_node(&pdev->dev);
> of_changeset_revert(np->data);
> """
> 
> Does that make sense to you ?
> 

Thanks for pointed out this issue.

I am not sure that the scenario you proposed using configfs which can lead
to the use-after-free is relevant but anyway this use-after-free is possible.

The fwnode->dev is set by of_pci_make_dev_node() and so it is consistent 
to unset it (set to NULL) in of_pci_remove_node().

I will update this patch to add the fw_devlink_set_device(&np->fwnode, NULL);
call in the next iteration.

Also I will had a new patch in the next iteration to perform same operation
for PCI root bridge. Same kind of code path, and so same issue but with
of_pci_make_host_bridge_node() and of_pci_remove_host_bridge_node().

Best regards,
Hervé