On Tue, Apr 4, 2017 at 9:22 PM, jeffy <jeffy.chen@xxxxxxxxxxxxxx> wrote: > Hi Bjorn, > > > On 04/05/2017 03:18 AM, Bjorn Helgaas wrote: >> >> On Thu, Mar 23, 2017 at 5:58 PM, Dmitry Torokhov <dtor@xxxxxxxxxxxx> >> wrote: >>> >>> On Thu, Mar 23, 2017 at 3:07 PM, Rob Herring <robh@xxxxxxxxxx> wrote: >>>> >>>> On Thu, Mar 23, 2017 at 3:12 AM, Jeffy Chen <jeffy.chen@xxxxxxxxxxxxxx> >>>> wrote: >>>>> >>>>> Currently we only free the allocated resource struct when error. >>>>> This would cause memory leak after pci_free_resource_list. >>>>> - pci_add_resource(resources, bus_range); >>>>> + *window->res = res; >>>> >>>> >>>> Well, now this seems racy. You add a blank resource to the list first >>>> and then fill it in. >>>> >>> >>> Huh? There is absolutely no guarantees for concurrent access here. >>> pcI_add_resource_offset() first adds a resource and then modifies >>> offset. Here we add an empty resource and then fill it in. >> >> >> I don't really like this pattern either. Even if there's no actual >> racy behavior, it takes more analysis than necessary to figure that >> out. >> >> pci_add_resource_offset() allocates a resource list entry, sets the >> offset, then adds it to the list. It doesn't update a resource entry >> that might be visible to anybody else. Here we do update a resource >> that is already visible to others because it's already on the list. > > i was following ./drivers/pnp/resource.c, but i'm agree this is not a good > way. > > i'll upload a new version to fix this in another way. more ideas: > 1/ pass a struct device to of_pci_get_host_bridge_resources and use > devm_kzalloc I would pick this one of the 3 options or... > 2/ add a new type of flags(or reuse IORESOURCE_AUTO) to tell > pci_free_resource_list to kfree them) > 3/ add new helpers of of_pci_add_resource[_offset] to alloc empty res, fill > it, add to list. 2 other options: Add a function to undo everything that of_pci_get_host_bridge_resources does. Then every caller of of_pci_get_host_bridge_resources should have a call to that function. Or maybe you can add a pci_free_resource_list_and_resources (needs a better name) to free both resources and list. Then audit all the current callers of pci_free_resource_list and determine which one's can be changed (maybe it is all of them). Rob -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
