Re: [PATCH v2] PCI/ACPI: Fix double free bug in pci_acpi_scan_root() function

[Bjorn Helgaas <helgaas@kernel.org>]

[+cc Dan]

On Tue, Jun 17, 2025 at 10:37:38AM +0800, Zhe Qiao wrote:
> The patch "PCI/ACPI: Fix allocated memory release on error in
> pci_acpi_scan_root()" introduces a dual release issue. When
> acpi_pci_root_creat() fails, the pci_cpi_can_root() function
> will release 'ri ->cfg' and 'root_ops' in the error handling
> path.However, acpi_pci_root_creat() will also call
> __acpi_pci_root_release_info(), which in turn will call the
> release_info hook, causing the same block of memory to be
> released again.

These are all nits, but would have to be fixed before applying:

  - 'The patch "PCI/ACPI: Fix ..."' is not the usual way to identify a
    commit.  Use the same style as in the Fixes: tag below.

  - Typo in "acpi_pci_root_creat" (twice)

  - Typo in "pci_cpi_can_root"

  - Add space after the period in "path.However, ..."

  - Add "Reported-by: Dan Carpenter <dan.carpenter@linaro.org>" and
    "Closes: https://lore.kernel.org/all/aEmdnuw715btq7Q5@stanley.mountain/"
    and cc: Dan.

  - 631b2af2f357 appeared in v6.16-rc1, so we should try to get the
    fix into v6.16.  A hint after the "---" would be helpful to make
    sure that happens.

Wait a few days before reposting in case other folks have comments.

> Fixes: 631b2af2f357 ("PCI/ACPI: Fix allocated memory release on error in pci_acpi_scan_root()")
> Signed-off-by: Zhe Qiao <qiaozhe@iscas.ac.cn>
> ---
> v1 -> v2:
>  - Restore all changes from the first version.
>  - Remove unnecessary release info hooks.
>  - Add a NULL check before calling info->ops->release_info().
>  - Delete the currently unused pci_api_geneic_delease_info () function.
> ---
>  drivers/acpi/pci_root.c |  3 ++-
>  drivers/pci/pci-acpi.c  | 12 ------------
>  2 files changed, 2 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> index 74ade4160314..83628adbc56b 100644
> --- a/drivers/acpi/pci_root.c
> +++ b/drivers/acpi/pci_root.c
> @@ -974,7 +974,8 @@ static void __acpi_pci_root_release_info(struct acpi_pci_root_info *info)
>  		resource_list_destroy_entry(entry);
>  	}
>  
> -	info->ops->release_info(info);
> +	if (info->ops && info->ops->release_info)
> +		info->ops->release_info(info);
>  }
>  
>  static void acpi_pci_root_release_info(struct pci_host_bridge *bridge)
> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> index b78e0e417324..6e85816ee1c3 100644
> --- a/drivers/pci/pci-acpi.c
> +++ b/drivers/pci/pci-acpi.c
> @@ -1652,17 +1652,6 @@ pci_acpi_setup_ecam_mapping(struct acpi_pci_root *root)
>  	return cfg;
>  }
>  
> -/* release_info: free resources allocated by init_info */
> -static void pci_acpi_generic_release_info(struct acpi_pci_root_info *ci)
> -{
> -	struct acpi_pci_generic_root_info *ri;
> -
> -	ri = container_of(ci, struct acpi_pci_generic_root_info, common);
> -	pci_ecam_free(ri->cfg);
> -	kfree(ci->ops);
> -	kfree(ri);
> -}
> -
>  /* Interface called from ACPI code to setup PCI host controller */
>  struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root)
>  {
> @@ -1683,7 +1672,6 @@ struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root)
>  	if (!ri->cfg)
>  		goto free_root_ops;
>  
> -	root_ops->release_info = pci_acpi_generic_release_info;
>  	root_ops->prepare_resources = pci_acpi_root_prepare_resources;
>  	root_ops->pci_ops = (struct pci_ops *)&ri->cfg->ops->pci_ops;
>  	bus = acpi_pci_root_create(root, root_ops, &ri->common, ri->cfg);
> -- 
> 2.43.0
>