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

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

[Bjorn Helgaas]

On Mon, Oct 12, 2026 at 04:28:24PM +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.
> 
> Fixes: 407d1a51921e ("PCI: Create device tree node for bridge")
> Fixes: 1f340724419e ("PCI: of: Create device tree PCI host bridge node")
> Cc: <stable+noautosel@kernel.org> # Issue not triggered but could be a problem
> Signed-off-by: Herve Codina <herve.codina@bootlin.com>

Acked-by: Bjorn Helgaas <bhelgaas@google.com>

> ---
>  drivers/pci/of.c    | 32 ++++++++++++++++++++------------
>  include/linux/pci.h | 11 +++++++++++
>  2 files changed, 31 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/pci/of.c b/drivers/pci/of.c
> index 0a5797652e18..fbed24a244d0 100644
> --- a/drivers/pci/of.c
> +++ b/drivers/pci/of.c
> @@ -732,15 +732,20 @@ void of_pci_remove_node(struct pci_dev *pdev)
>  {
>  	struct device_node *np;
>  
> +	if (!pdev->cset)
> +		return;
> +
>  	np = pci_device_to_OF_node(pdev);
> -	if (!np || !of_node_check_flag(np, OF_DYNAMIC))
> +	if (!np)
>  		return;
>  
>  	fw_devlink_set_device(&np->fwnode, NULL);
>  	device_remove_of_node(&pdev->dev);
> -	of_changeset_revert(np->data);
> -	of_changeset_destroy(np->data);
> +	of_changeset_revert(pdev->cset);
> +	of_changeset_destroy(pdev->cset);
>  	of_node_put(np);
> +	kfree(pdev->cset);
> +	pdev->cset = NULL;
>  }
>  
>  void of_pci_make_dev_node(struct pci_dev *pdev)
> @@ -800,18 +805,17 @@ void of_pci_make_dev_node(struct pci_dev *pdev)
>  	if (ret)
>  		goto out_clear_devlink_dev;
>  
> -	np->data = cset;
> -
>  	ret = device_add_of_node(&pdev->dev, np);
>  	if (ret)
>  		goto out_revert_cset;
>  
> +	pdev->cset = cset;
> +
>  	kfree(name);
>  
>  	return;
>  
>  out_revert_cset:
> -	np->data = NULL;
>  	of_changeset_revert(cset);
>  out_clear_devlink_dev:
>  	fw_devlink_set_device(&np->fwnode, NULL);
> @@ -828,16 +832,21 @@ void of_pci_remove_host_bridge_node(struct pci_host_bridge *bridge)
>  {
>  	struct device_node *np;
>  
> +	if (!bridge->cset)
> +		return;
> +
>  	np = pci_bus_to_OF_node(bridge->bus);
> -	if (!np || !of_node_check_flag(np, OF_DYNAMIC))
> +	if (!np)
>  		return;
>  
>  	fw_devlink_set_device(&np->fwnode, NULL);
>  	device_remove_of_node(&bridge->bus->dev);
>  	device_remove_of_node(&bridge->dev);
> -	of_changeset_revert(np->data);
> -	of_changeset_destroy(np->data);
> +	of_changeset_revert(bridge->cset);
> +	of_changeset_destroy(bridge->cset);
>  	of_node_put(np);
> +	kfree(bridge->cset);
> +	bridge->cset = NULL;
>  }
>  
>  void of_pci_make_host_bridge_node(struct pci_host_bridge *bridge)
> @@ -899,8 +908,6 @@ void of_pci_make_host_bridge_node(struct pci_host_bridge *bridge)
>  	if (ret)
>  		goto out_clear_devlink_dev;
>  
> -	np->data = cset;
> -
>  	/* Add the of_node to host bridge and the root bus */
>  	ret = device_add_of_node(&bridge->dev, np);
>  	if (ret)
> @@ -910,6 +917,8 @@ void of_pci_make_host_bridge_node(struct pci_host_bridge *bridge)
>  	if (ret)
>  		goto out_remove_bridge_dev_of_node;
>  
> +	bridge->cset = cset;
> +
>  	kfree(name);
>  
>  	return;
> @@ -917,7 +926,6 @@ void of_pci_make_host_bridge_node(struct pci_host_bridge *bridge)
>  out_remove_bridge_dev_of_node:
>  	device_remove_of_node(&bridge->dev);
>  out_revert_cset:
> -	np->data = NULL;
>  	of_changeset_revert(cset);
>  out_clear_devlink_dev:
>  	fw_devlink_set_device(&np->fwnode, NULL);
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index d31a8d107b1e..7b0ba9ec7b5c 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -339,6 +339,9 @@ struct pcie_link_state;
>  struct pci_sriov;
>  struct pci_p2pdma;
>  struct rcec_ea;
> +#ifdef CONFIG_PCI_DYNAMIC_OF_NODES
> +struct of_changeset;
> +#endif
>  
>  /* struct pci_dev - describes a PCI device
>   *
> @@ -598,6 +601,10 @@ struct pci_dev {
>  	u8		tph_mode;	/* TPH mode */
>  	u8		tph_req_type;	/* TPH requester type */
>  #endif
> +
> +#ifdef CONFIG_PCI_DYNAMIC_OF_NODES
> +	struct of_changeset *cset; /* Changeset used for OF node creation */
> +#endif
>  };
>  
>  static inline struct pci_dev *pci_physfn(struct pci_dev *dev)
> @@ -670,6 +677,10 @@ struct pci_host_bridge {
>  	unsigned int	broken_l1ss_resume:1;	/* Resuming from L1SS during
>  						   system suspend is broken */
>  
> +#ifdef CONFIG_PCI_DYNAMIC_OF_NODES
> +	struct of_changeset *cset; /* Changeset used for OF node creation */
> +#endif
> +
>  	/* Resource alignment requirements */
>  	resource_size_t (*align_resource)(struct pci_dev *dev,
>  			const struct resource *res,
> -- 
> 2.55.0
>