Date:   Tue, 21 Mar 2017 11:55:31 +0800
From:   jeffy <jeffy.chen@...k-chips.com>
To:     Brian Norris <briannorris@...omium.org>,
        Shawn Lin <shawn.lin@...k-chips.com>
CC:     Bjorn Helgaas <bhelgaas@...gle.com>, linux-kernel@...r.kernel.org,
        Wenrui Li <wenrui.li@...k-chips.com>,
        linux-pci@...r.kernel.org, linux-rockchip@...ts.infradead.org
Subject: Re: [PATCH] PCI: rockchip: don't leak the PCI resource list

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@...omium.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);
>>>
>>
>
>
>

Powered by blists - more mailing lists