Re: [PATCH v10 10/10] PCI: of: Guard against node removal with incorrect np->data

On Fri, Jul 17, 2026 at 04:51:45PM +0800, 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.
> 
> Further more, having a NULL np->data pointer means that the node has
> been created out of our PCI node creation process and so shouldn't be
> handled by our PCI node removal process.
> 
> Add a np->data check for NULL to bail out early of PCI node removal
> functions.
> 
> Fixes: 407d1a51921e ("PCI: Create device tree node for bridge")
> Fixes: 1f340724419e ("PCI: of: Create device tree PCI host bridge node")
> Cc: <[email protected]> # Not triggered but 'This could be a problem...'
> Signed-off-by: Herve Codina <[email protected]>
> ---
>  drivers/pci/of.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/of.c b/drivers/pci/of.c
> index 28896f748e8d..9f35e1e07cdc 100644
> --- a/drivers/pci/of.c
> +++ b/drivers/pci/of.c
> @@ -654,7 +654,7 @@ 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 (!np || !of_node_check_flag(np, OF_DYNAMIC) || !np->data)
>  		return;
>

Hi Herve,

I wonder whether "np->data != NULL" is sufficient to identify nodes
created by these PCI helpers.

struct device_node::data is a generic pointer. As my limited knowledge,
on pSeries, pci_dn_reconfig_notifier() handles OF_RECONFIG_ATTACH_NODE
and calls pci_add_device_node_info(), which stores a "struct pci_dn *"
in "dn->data"

Coud an OF_DYNAMIC PCI node from that path later reach of_pci_remove_node()
or of_pci_remove_host_bridge_node() ?

If so, the new guard would accpet the non-NULL np->data, and the removal path
would pass a "struct pci_dn *" to of_changeset_revert() and of_changeset_destroy()
as though it were the PCI-create "struct of_changeset *" ?

I have only reviewed this statically, didn't do a real pSeries setup to test it,
so I may be missing something. Could you confirm whether this scenario is possible?
If so, it seems that a stronger ownership indicator would be needed.

Best regards,
Richard Cheng.
  
>  	fw_devlink_set_device(&np->fwnode, NULL);
> @@ -750,7 +750,7 @@ void of_pci_remove_host_bridge_node(struct pci_host_bridge *bridge)
>  	struct device_node *np;
>  
>  	np = pci_bus_to_OF_node(bridge->bus);
> -	if (!np || !of_node_check_flag(np, OF_DYNAMIC))
> +	if (!np || !of_node_check_flag(np, OF_DYNAMIC) || !np->data)
>  		return;
>  
>  	fw_devlink_set_device(&np->fwnode, NULL);
> -- 
> 2.55.0
>