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

[Zhe Qiao]

Hi Bjorn

On Tue, Jun 17, 2025 at 10:40 PM Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote:
>
> [+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@xxxxxxxxxx>" 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.
>

Thank you for your detailed review and pointing out these issues.
I will wait a few days before resending.

Thanks again!

> > Fixes: 631b2af2f357 ("PCI/ACPI: Fix allocated memory release on error in pci_acpi_scan_root()")
> > Signed-off-by: Zhe Qiao <qiaozhe@xxxxxxxxxxx>
> > ---
> > 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
> >

regards,
Zhe