Hi Dan Carpenter,
On Wed, Jun 11, 2025 at 11:15 PM Dan Carpenter <dan.carpenter@xxxxxxxxxx> 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
