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@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.
>
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
