[bug report] PCI/ACPI: Fix allocated memory release on error in pci_acpi_scan_root()

[bug report] PCI/ACPI: Fix allocated memory release on error in pci_acpi_scan_root()

[Dan Carpenter]

Hello Zhe Qiao,

Commit 631b2af2f357 ("PCI/ACPI: Fix allocated memory release on error
in pci_acpi_scan_root()") from Apr 30, 2025 (linux-next), leads to
the following Smatch static checker warning:

	drivers/pci/pci-acpi.c:1712 pci_acpi_scan_root()
	error: double free of 'root_ops' (line 1711)

drivers/pci/pci-acpi.c
    1667 struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root)
    1668 {
    1669         struct acpi_pci_generic_root_info *ri;
    1670         struct pci_bus *bus, *child;
    1671         struct acpi_pci_root_ops *root_ops;
    1672         struct pci_host_bridge *host;
    1673 
    1674         ri = kzalloc(sizeof(*ri), GFP_KERNEL);
    1675         if (!ri)
    1676                 return NULL;
    1677 
    1678         root_ops = kzalloc(sizeof(*root_ops), GFP_KERNEL);
    1679         if (!root_ops)
    1680                 goto free_ri;
    1681 
    1682         ri->cfg = pci_acpi_setup_ecam_mapping(root);
    1683         if (!ri->cfg)
    1684                 goto free_root_ops;
    1685 
    1686         root_ops->release_info = pci_acpi_generic_release_info;
    1687         root_ops->prepare_resources = pci_acpi_root_prepare_resources;
    1688         root_ops->pci_ops = (struct pci_ops *)&ri->cfg->ops->pci_ops;
    1689         bus = acpi_pci_root_create(root, root_ops, &ri->common, ri->cfg);
    1690         if (!bus)
    1691                 goto free_cfg;

The acpi_pci_root_create() function frees root_ops on error in
pci_acpi_generic_release_info().  I think there is only one error
path where it frees "ri->cfg".  I probably would advise you to re-write
the the error handling in acpi_pci_root_create().

    1692 
    1693         /* If we must preserve the resource configuration, claim now */
    1694         host = pci_find_host_bridge(bus);
    1695         if (host->preserve_config)
    1696                 pci_bus_claim_resources(bus);
    1697 
    1698         /*
    1699          * Assign whatever was left unassigned. If we didn't claim above,
    1700          * this will reassign everything.
    1701          */
    1702         pci_assign_unassigned_root_bus_resources(bus);
    1703 
    1704         list_for_each_entry(child, &bus->children, node)
    1705                 pcie_bus_configure_settings(child);
    1706 
    1707         return bus;
    1708 
    1709 free_cfg:
    1710         pci_ecam_free(ri->cfg);
    1711 free_root_ops:
--> 1712         kfree(root_ops);
    1713 free_ri:
    1714         kfree(ri);
    1715         return NULL;
    1716 }

regards,
dan carpenter

Re: [bug report] PCI/ACPI: Fix allocated memory release on error in pci_acpi_scan_root()

[Zhe Qiao]

Hi Dan Carpenter,

On Wed, Jun 11, 2025 at 11:15 PM Dan Carpenter <dan.carpenter@linaro.org> wrote:
>
> Hello Zhe Qiao,
>
> Commit 631b2af2f357 ("PCI/ACPI: Fix allocated memory release on error
> in pci_acpi_scan_root()") from Apr 30, 2025 (linux-next), leads to
> the following Smatch static checker warning:
>
>         drivers/pci/pci-acpi.c:1712 pci_acpi_scan_root()
>         error: double free of 'root_ops' (line 1711)
>
> drivers/pci/pci-acpi.c
>     1667 struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root)
>     1668 {
>     1669         struct acpi_pci_generic_root_info *ri;
>     1670         struct pci_bus *bus, *child;
>     1671         struct acpi_pci_root_ops *root_ops;
>     1672         struct pci_host_bridge *host;
>     1673
>     1674         ri = kzalloc(sizeof(*ri), GFP_KERNEL);
>     1675         if (!ri)
>     1676                 return NULL;
>     1677
>     1678         root_ops = kzalloc(sizeof(*root_ops), GFP_KERNEL);
>     1679         if (!root_ops)
>     1680                 goto free_ri;
>     1681
>     1682         ri->cfg = pci_acpi_setup_ecam_mapping(root);
>     1683         if (!ri->cfg)
>     1684                 goto free_root_ops;
>     1685
>     1686         root_ops->release_info = pci_acpi_generic_release_info;
>     1687         root_ops->prepare_resources = pci_acpi_root_prepare_resources;
>     1688         root_ops->pci_ops = (struct pci_ops *)&ri->cfg->ops->pci_ops;
>     1689         bus = acpi_pci_root_create(root, root_ops, &ri->common, ri->cfg);
>     1690         if (!bus)
>     1691                 goto free_cfg;
>
> The acpi_pci_root_create() function frees root_ops on error in
> pci_acpi_generic_release_info().  I think there is only one error
> path where it frees "ri->cfg".  I probably would advise you to re-write
> the the error handling in acpi_pci_root_create().

Thanks, this is really an unexpected gain for me. I didn't notice that
the memory
release operation has been implemented in the pci_acpi_generic_release_info
function. But I think it's a bit unclear in the code logic to release
these memories
in the pci_acpi_generic_release_info function. As you pointed out, I
want to let the
pci_acpi_generic_release_info function return directly, and put all the memory
release operations into the error handling part of the
pci_acpi_scan_root function.
What do you think of this? If you have any better suggestions, please
let me know.

>
>     1692
>     1693         /* If we must preserve the resource configuration, claim now */
>     1694         host = pci_find_host_bridge(bus);
>     1695         if (host->preserve_config)
>     1696                 pci_bus_claim_resources(bus);
>     1697
>     1698         /*
>     1699          * Assign whatever was left unassigned. If we didn't claim above,
>     1700          * this will reassign everything.
>     1701          */
>     1702         pci_assign_unassigned_root_bus_resources(bus);
>     1703
>     1704         list_for_each_entry(child, &bus->children, node)
>     1705                 pcie_bus_configure_settings(child);
>     1706
>     1707         return bus;
>     1708
>     1709 free_cfg:
>     1710         pci_ecam_free(ri->cfg);
>     1711 free_root_ops:
> --> 1712         kfree(root_ops);
>     1713 free_ri:
>     1714         kfree(ri);
>     1715         return NULL;
>     1716 }
>
> regards,
> dan carpenter

Re: [bug report] PCI/ACPI: Fix allocated memory release on error in pci_acpi_scan_root()

[Dan Carpenter]

On Thu, Jun 12, 2025 at 01:12:43PM +0800, Zhe Qiao wrote:
> Hi Dan Carpenter,
> 
> On Wed, Jun 11, 2025 at 11:15 PM Dan Carpenter <dan.carpenter@linaro.org> wrote:
> >
> > Hello Zhe Qiao,
> >
> > Commit 631b2af2f357 ("PCI/ACPI: Fix allocated memory release on error
> > in pci_acpi_scan_root()") from Apr 30, 2025 (linux-next), leads to
> > the following Smatch static checker warning:
> >
> >         drivers/pci/pci-acpi.c:1712 pci_acpi_scan_root()
> >         error: double free of 'root_ops' (line 1711)
> >
> > drivers/pci/pci-acpi.c
> >     1667 struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root)
> >     1668 {
> >     1669         struct acpi_pci_generic_root_info *ri;
> >     1670         struct pci_bus *bus, *child;
> >     1671         struct acpi_pci_root_ops *root_ops;
> >     1672         struct pci_host_bridge *host;
> >     1673
> >     1674         ri = kzalloc(sizeof(*ri), GFP_KERNEL);
> >     1675         if (!ri)
> >     1676                 return NULL;
> >     1677
> >     1678         root_ops = kzalloc(sizeof(*root_ops), GFP_KERNEL);
> >     1679         if (!root_ops)
> >     1680                 goto free_ri;
> >     1681
> >     1682         ri->cfg = pci_acpi_setup_ecam_mapping(root);
> >     1683         if (!ri->cfg)
> >     1684                 goto free_root_ops;
> >     1685
> >     1686         root_ops->release_info = pci_acpi_generic_release_info;
> >     1687         root_ops->prepare_resources = pci_acpi_root_prepare_resources;
> >     1688         root_ops->pci_ops = (struct pci_ops *)&ri->cfg->ops->pci_ops;
> >     1689         bus = acpi_pci_root_create(root, root_ops, &ri->common, ri->cfg);
> >     1690         if (!bus)
> >     1691                 goto free_cfg;
> >
> > The acpi_pci_root_create() function frees root_ops on error in
> > pci_acpi_generic_release_info().  I think there is only one error
> > path where it frees "ri->cfg".  I probably would advise you to re-write
> > the the error handling in acpi_pci_root_create().
> 
> Thanks, this is really an unexpected gain for me. I didn't notice that
> the memory
> release operation has been implemented in the pci_acpi_generic_release_info
> function. But I think it's a bit unclear in the code logic to release
> these memories
> in the pci_acpi_generic_release_info function. As you pointed out, I
> want to let the
> pci_acpi_generic_release_info function return directly, and put all the memory
> release operations into the error handling part of the
> pci_acpi_scan_root function.
> What do you think of this? If you have any better suggestions, please
> let me know.
> 

Either way is fine with me.  I understand why we tried to do the
free in acpi_pci_root_create() but that approached didn't free
"sysdata" reliably so it's not like we can just revert your commit.

If you take your approach you'll have to change pci_acpi_scan_root() as
well.

regards,
dan carpenter