Re: [PATCH v12 10/10] PCI: of: Avoid np->data usage for the node changeset

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

Hi Andy,

On Thu, 1 Oct 2026 22:37:16 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Thu, Oct 01, 2026 at 04:28:10PM +0200, Herve Codina wrote:
> > of_pci_remove_node() and of_pci_remove_host_bridge_node() check
> > whether the node is dynamic but not whether it has valid private data.
> > 
> > During the node creation, an OF changeset is used and this changeset is
> > stored in np->data to be available for removal functions.
> > 
> > If, for instance, a PCI host bridge is created using a device-tree
> > overlay, the related node will have the dynamic flag set but np->data
> > will be NULL. This leads to NULL pointer dereferences.
> > 
> > Checking for a non-NULL np->data pointer to determine if the node has
> > been created by the PCI node creation process is not enough. Indeed,
> > on some platforms like PowerPC, the OF_RECONFIG_ATTACH_NODE notifier
> > (e.g., in the pci_dn_reconfig_notifier() function) intercepts node
> > additions and populates np->data with its own structure, such as a
> > struct pci_dn. In that case, np->data is not NULL but it is not related
> > to our changeset stored during the PCI node process creation.
> > 
> > Avoid the usage of np->data to store the changeset used during the PCI
> > node creation. Store our changeset in a more relevant structure: either
> > struct pci_dev when the node is created for a PCI device or struct
> > pci_host_bridge when the node is created for the PCI host bridge.
> > 
> > With that done, no ambiguity remains on removal. Indeed, this changeset,
> > if non-NULL, is the one used during PCI node creation. Check and use
> > this changeset on the removal process.  
> 
> ...
> 
> >  void of_pci_remove_node(struct pci_dev *pdev)  
> 
> >  	struct device_node *np;
> >  
> >  	np = pci_device_to_OF_node(pdev);
> > -	if (!np || !of_node_check_flag(np, OF_DYNAMIC))
> > +	if (!pdev->cset || !np)
> >  		return;  
> 
> Wouldn't be better to split this conditional to two?
> 
> 	if (!pdev->cset)
> 		return;
> 
> 	np = pci_device_to_OF_node(pdev);
> 	if (!np)
> 		return;

Yes indeed, this will improve readability. I will update in the next
iteration.

Also I will perform the same kind of modification in
of_pci_remove_host_bridge_node(), the same kind of check is present.

Best regards,
Hervé