| Message ID | 20170320224936.28605-1-briannorris@chromium.org |
|---|---|
| State | Changes Requested |
| Headers | show |
Hi Brian, On 2017/3/21 6:49, Brian Norris wrote: > This list is local to the probe() function. We should free it up in both > the success case and the error case, but currently we're only freeing it > in the error case (see commit f1d722b607d6 ("PCI: rockchip: Fix > rockchip_pcie_probe() error path to free resource list")). > > Caught by kmemleak, when doing repeated bind/unbind tests. > It doesn't look natural to free it in probe, instead it looks more like we should do the cleanup work when calling .remove I didn't know if there is something that still use this resource , for instance, hotplug? But I noticed it from Bjron's statement[1] that "The struct resource for each host bridge window must live as long as the host bridge itself". So I didn't free it when finishing probe. I guess the proper fix is to do it in pci_remove_root_bus or somewhere of the cleanup code if I undertand it correctly. [1]: https://lkml.org/lkml/2017/2/8/802 > Signed-off-by: Brian Norris <briannorris@chromium.org> > --- > drivers/pci/host/pcie-rockchip.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/pci/host/pcie-rockchip.c b/drivers/pci/host/pcie-rockchip.c > index bd6df7254de4..8087a0698d65 100644 > --- a/drivers/pci/host/pcie-rockchip.c > +++ b/drivers/pci/host/pcie-rockchip.c > @@ -1396,6 +1396,7 @@ static int rockchip_pcie_probe(struct platform_device *pdev) > goto err_free_res; > } > rockchip->root_bus = bus; > + pci_free_resource_list(&res); > > pci_bus_size_bridges(bus); > pci_bus_assign_resources(bus); >
On Tue, Mar 21, 2017 at 09:25:16AM +0800, Shawn Lin wrote: > On 2017/3/21 6:49, Brian Norris wrote: > >This list is local to the probe() function. We should free it up in both > >the success case and the error case, but currently we're only freeing it > >in the error case (see commit f1d722b607d6 ("PCI: rockchip: Fix > >rockchip_pcie_probe() error path to free resource list")). > > > >Caught by kmemleak, when doing repeated bind/unbind tests. > > > > It doesn't look natural to free it in probe, instead it looks more > like we should do the cleanup work when calling .remove Hmm, I did write this one up pretty quickly, so I might have gotten it a bit wrong. It's not clear there's a *real* problem with this patch, since the bridge's window list doesn't actually get used much later, but I agree this isn't correct. And actually, I think my patch is a red herring; the leak is still there. I notice that the resource list is actually freed in pci_release_host_bridge_dev() already anyway. > I didn't know if there is something that still use this resource > , for instance, hotplug? But I noticed it from Bjron's statement[1] > that "The struct resource for each host bridge window must live as long > as the host bridge itself". So I didn't free it when finishing probe. > I guess the proper fix is to do it in pci_remove_root_bus or somewhere > of the cleanup code if I undertand it correctly. That thread is talking about a different issue; in that driver, the 'struct resource' is on the stack. That's not good. In our case, it's just the resource list head that's on the stack, which is fine. The relevant entries get spliced onto a longer-lived list in the end. But then, we have these to consider for leaks: (a) the 'resource_entry's allocated in of_pci_get_host_bridge_resources() (via pci_add_resource() and pci_add_resource_offset()) (b) the 'resource's allocated in of_pci_get_host_bridge_resources(), to go with each of the 'resource_entry's from (b) As noted above, I think (a) is actually taken care of (and so my current patch is not needed). The problem is only with (b). (I think?) I'll double check this all tomorrow. Brian > [1]: https://lkml.org/lkml/2017/2/8/802 > > >Signed-off-by: Brian Norris <briannorris@chromium.org> > >--- > > drivers/pci/host/pcie-rockchip.c | 1 + > > 1 file changed, 1 insertion(+) > > > >diff --git a/drivers/pci/host/pcie-rockchip.c b/drivers/pci/host/pcie-rockchip.c > >index bd6df7254de4..8087a0698d65 100644 > >--- a/drivers/pci/host/pcie-rockchip.c > >+++ b/drivers/pci/host/pcie-rockchip.c > >@@ -1396,6 +1396,7 @@ static int rockchip_pcie_probe(struct platform_device *pdev) > > goto err_free_res; > > } > > rockchip->root_bus = bus; > >+ pci_free_resource_list(&res); > > > > pci_bus_size_bridges(bus); > > pci_bus_assign_resources(bus); > > >
Hi brian, On 03/21/2017 10:21 AM, Brian Norris wrote: > On Tue, Mar 21, 2017 at 09:25:16AM +0800, Shawn Lin wrote: >> On 2017/3/21 6:49, Brian Norris wrote: >>> This list is local to the probe() function. We should free it up in both >>> the success case and the error case, but currently we're only freeing it >>> in the error case (see commit f1d722b607d6 ("PCI: rockchip: Fix >>> rockchip_pcie_probe() error path to free resource list")). >>> >>> Caught by kmemleak, when doing repeated bind/unbind tests. >>> >> >> It doesn't look natural to free it in probe, instead it looks more >> like we should do the cleanup work when calling .remove > > Hmm, I did write this one up pretty quickly, so I might have gotten it a > bit wrong. It's not clear there's a *real* problem with this patch, > since the bridge's window list doesn't actually get used much later, but > I agree this isn't correct. > > And actually, I think my patch is a red herring; the leak is still > there. I notice that the resource list is actually freed in > pci_release_host_bridge_dev() already anyway. > >> I didn't know if there is something that still use this resource >> , for instance, hotplug? But I noticed it from Bjron's statement[1] >> that "The struct resource for each host bridge window must live as long >> as the host bridge itself". So I didn't free it when finishing probe. >> I guess the proper fix is to do it in pci_remove_root_bus or somewhere >> of the cleanup code if I undertand it correctly. > > That thread is talking about a different issue; in that driver, the > 'struct resource' is on the stack. That's not good. In our case, it's > just the resource list head that's on the stack, which is fine. The > relevant entries get spliced onto a longer-lived list in the end. > > But then, we have these to consider for leaks: > > (a) the 'resource_entry's allocated in > of_pci_get_host_bridge_resources() (via pci_add_resource() and > pci_add_resource_offset()) > > (b) the 'resource's allocated in > of_pci_get_host_bridge_resources(), to go with each of the > 'resource_entry's from (b) > > As noted above, I think (a) is actually taken care of (and so my current > patch is not needed). The problem is only with (b). (I think?) yes, it looks like there's no way to free resources allocated in of_pci_get_host_bridge_resources, expect for the err path: kfree(window->res). i think it's a common issue, which may not get fix in rockchip pci driver. maybe we should modify of_pci_get_host_bridge_resources, pass the struct device to it, and change all kzalloc to devm_kzalloc(also remove the kfree) > > I'll double check this all tomorrow. > > Brian > >> [1]: https://lkml.org/lkml/2017/2/8/802 >> >>> Signed-off-by: Brian Norris <briannorris@chromium.org> >>> --- >>> drivers/pci/host/pcie-rockchip.c | 1 + >>> 1 file changed, 1 insertion(+) >>> >>> diff --git a/drivers/pci/host/pcie-rockchip.c b/drivers/pci/host/pcie-rockchip.c >>> index bd6df7254de4..8087a0698d65 100644 >>> --- a/drivers/pci/host/pcie-rockchip.c >>> +++ b/drivers/pci/host/pcie-rockchip.c >>> @@ -1396,6 +1396,7 @@ static int rockchip_pcie_probe(struct platform_device *pdev) >>> goto err_free_res; >>> } >>> rockchip->root_bus = bus; >>> + pci_free_resource_list(&res); >>> >>> pci_bus_size_bridges(bus); >>> pci_bus_assign_resources(bus); >>> >> > > >
diff --git a/drivers/pci/host/pcie-rockchip.c b/drivers/pci/host/pcie-rockchip.c index bd6df7254de4..8087a0698d65 100644 --- a/drivers/pci/host/pcie-rockchip.c +++ b/drivers/pci/host/pcie-rockchip.c @@ -1396,6 +1396,7 @@ static int rockchip_pcie_probe(struct platform_device *pdev) goto err_free_res; } rockchip->root_bus = bus; + pci_free_resource_list(&res); pci_bus_size_bridges(bus); pci_bus_assign_resources(bus);
This list is local to the probe() function. We should free it up in both the success case and the error case, but currently we're only freeing it in the error case (see commit f1d722b607d6 ("PCI: rockchip: Fix rockchip_pcie_probe() error path to free resource list")). Caught by kmemleak, when doing repeated bind/unbind tests. Signed-off-by: Brian Norris <briannorris@chromium.org> --- drivers/pci/host/pcie-rockchip.c | 1 + 1 file changed, 1 insertion(+)