Re: [PATCH v2 2/2] of/pci: Fix memory leak in of_pci_get_host_bridge_resources

[Rob Herring]

On Tue, Apr 4, 2017 at 9:22 PM, jeffy  wrote:
> Hi Bjorn,
>
>
> On 04/05/2017 03:18 AM, Bjorn Helgaas wrote:
>>
>> On Thu, Mar 23, 2017 at 5:58 PM, Dmitry Torokhov 
>> wrote:
>>>
>>> On Thu, Mar 23, 2017 at 3:07 PM, Rob Herring  wrote:

 On Thu, Mar 23, 2017 at 3:12 AM, Jeffy Chen 
 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

Re: [PATCH v2 2/2] of/pci: Fix memory leak in of_pci_get_host_bridge_resources

[Rob Herring]

On Tue, Apr 4, 2017 at 9:22 PM, jeffy  wrote:
> Hi Bjorn,
>
>
> On 04/05/2017 03:18 AM, Bjorn Helgaas wrote:
>>
>> On Thu, Mar 23, 2017 at 5:58 PM, Dmitry Torokhov 
>> wrote:
>>>
>>> On Thu, Mar 23, 2017 at 3:07 PM, Rob Herring  wrote:

 On Thu, Mar 23, 2017 at 3:12 AM, Jeffy Chen 
 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

Re: [PATCH v2 2/2] of/pci: Fix memory leak in of_pci_get_host_bridge_resources

[jeffy]

Hi Bjorn,

On 04/05/2017 03:18 AM, Bjorn Helgaas wrote:

On Thu, Mar 23, 2017 at 5:58 PM, Dmitry Torokhov  wrote:

On Thu, Mar 23, 2017 at 3:07 PM, Rob Herring  wrote:

On Thu, Mar 23, 2017 at 3:12 AM, Jeffy Chen  wrote:

Currently we only free the allocated resource struct when error.
This would cause memory leak after pci_free_resource_list.

Signed-off-by: Jeffy Chen 
---

Changes in v2:
Don't change the resource_list_create_entry's behavior.

  drivers/of/of_pci.c | 57 +++--
  1 file changed, 25 insertions(+), 32 deletions(-)

diff --git a/drivers/of/of_pci.c b/drivers/of/of_pci.c
index 0ee42c3..a0ec246 100644
--- a/drivers/of/of_pci.c
+++ b/drivers/of/of_pci.c
@@ -190,8 +190,7 @@ int of_pci_get_host_bridge_resources(struct device_node 
*dev,
 struct list_head *resources, resource_size_t *io_base)
  {
 struct resource_entry *window;
-   struct resource *res;
-   struct resource *bus_range;
+   struct resource res;
 struct of_pci_range range;
 struct of_pci_range_parser parser;
 char range_type[4];
@@ -200,24 +199,24 @@ int of_pci_get_host_bridge_resources(struct device_node 
*dev,
 if (io_base)
 *io_base = (resource_size_t)OF_BAD_ADDR;

-   bus_range = kzalloc(sizeof(*bus_range), GFP_KERNEL);
-   if (!bus_range)
-   return -ENOMEM;
-
 pr_info("host bridge %s ranges:\n", dev->full_name);

-   err = of_pci_parse_bus_range(dev, bus_range);
+   err = of_pci_parse_bus_range(dev, );
 if (err) {
-   bus_range->start = busno;
-   bus_range->end = bus_max;
-   bus_range->flags = IORESOURCE_BUS;
-   pr_info("  No bus range found for %s, using %pR\n",
-   dev->full_name, bus_range);
+   res.start = busno;
+   res.end = bus_max;
+   res.flags = IORESOURCE_BUS;
+   pr_info("  No bus range found for %s\n", dev->full_name);
 } else {
-   if (bus_range->end > bus_range->start + bus_max)
-   bus_range->end = bus_range->start + bus_max;
+   if (res.end > res.start + bus_max)
+   res.end = res.start + bus_max;
+   }
+   window = pci_add_resource(resources, NULL);
+   if (!window) {
+   err = -ENOMEM;
+   goto parse_failed;
 }
-   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
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.


Bjorn

BTW, please CC linux-pci on the entire series so it's easier to
review.  I don't know where you envision having this applied, but I
only apply things to the PCI tree after they appear on linux-pci.


oh, sorry, didn't notice that, will do in next version.

Re: [PATCH v2 2/2] of/pci: Fix memory leak in of_pci_get_host_bridge_resources

[jeffy]

Hi Bjorn,

On 04/05/2017 03:18 AM, Bjorn Helgaas wrote:

On Thu, Mar 23, 2017 at 5:58 PM, Dmitry Torokhov  wrote:

On Thu, Mar 23, 2017 at 3:07 PM, Rob Herring  wrote:

On Thu, Mar 23, 2017 at 3:12 AM, Jeffy Chen  wrote:

Currently we only free the allocated resource struct when error.
This would cause memory leak after pci_free_resource_list.

Signed-off-by: Jeffy Chen 
---

Changes in v2:
Don't change the resource_list_create_entry's behavior.

  drivers/of/of_pci.c | 57 +++--
  1 file changed, 25 insertions(+), 32 deletions(-)

diff --git a/drivers/of/of_pci.c b/drivers/of/of_pci.c
index 0ee42c3..a0ec246 100644
--- a/drivers/of/of_pci.c
+++ b/drivers/of/of_pci.c
@@ -190,8 +190,7 @@ int of_pci_get_host_bridge_resources(struct device_node 
*dev,
 struct list_head *resources, resource_size_t *io_base)
  {
 struct resource_entry *window;
-   struct resource *res;
-   struct resource *bus_range;
+   struct resource res;
 struct of_pci_range range;
 struct of_pci_range_parser parser;
 char range_type[4];
@@ -200,24 +199,24 @@ int of_pci_get_host_bridge_resources(struct device_node 
*dev,
 if (io_base)
 *io_base = (resource_size_t)OF_BAD_ADDR;

-   bus_range = kzalloc(sizeof(*bus_range), GFP_KERNEL);
-   if (!bus_range)
-   return -ENOMEM;
-
 pr_info("host bridge %s ranges:\n", dev->full_name);

-   err = of_pci_parse_bus_range(dev, bus_range);
+   err = of_pci_parse_bus_range(dev, );
 if (err) {
-   bus_range->start = busno;
-   bus_range->end = bus_max;
-   bus_range->flags = IORESOURCE_BUS;
-   pr_info("  No bus range found for %s, using %pR\n",
-   dev->full_name, bus_range);
+   res.start = busno;
+   res.end = bus_max;
+   res.flags = IORESOURCE_BUS;
+   pr_info("  No bus range found for %s\n", dev->full_name);
 } else {
-   if (bus_range->end > bus_range->start + bus_max)
-   bus_range->end = bus_range->start + bus_max;
+   if (res.end > res.start + bus_max)
+   res.end = res.start + bus_max;
+   }
+   window = pci_add_resource(resources, NULL);
+   if (!window) {
+   err = -ENOMEM;
+   goto parse_failed;
 }
-   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
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.


Bjorn

BTW, please CC linux-pci on the entire series so it's easier to
review.  I don't know where you envision having this applied, but I
only apply things to the PCI tree after they appear on linux-pci.


oh, sorry, didn't notice that, will do in next version.

Re: [PATCH v2 2/2] of/pci: Fix memory leak in of_pci_get_host_bridge_resources

[Bjorn Helgaas]

On Thu, Mar 23, 2017 at 5:58 PM, Dmitry Torokhov  wrote:
> On Thu, Mar 23, 2017 at 3:07 PM, Rob Herring  wrote:
>> On Thu, Mar 23, 2017 at 3:12 AM, Jeffy Chen  
>> wrote:
>>> Currently we only free the allocated resource struct when error.
>>> This would cause memory leak after pci_free_resource_list.
>>>
>>> Signed-off-by: Jeffy Chen 
>>> ---
>>>
>>> Changes in v2:
>>> Don't change the resource_list_create_entry's behavior.
>>>
>>>  drivers/of/of_pci.c | 57 
>>> +++--
>>>  1 file changed, 25 insertions(+), 32 deletions(-)
>>>
>>> diff --git a/drivers/of/of_pci.c b/drivers/of/of_pci.c
>>> index 0ee42c3..a0ec246 100644
>>> --- a/drivers/of/of_pci.c
>>> +++ b/drivers/of/of_pci.c
>>> @@ -190,8 +190,7 @@ int of_pci_get_host_bridge_resources(struct device_node 
>>> *dev,
>>> struct list_head *resources, resource_size_t 
>>> *io_base)
>>>  {
>>> struct resource_entry *window;
>>> -   struct resource *res;
>>> -   struct resource *bus_range;
>>> +   struct resource res;
>>> struct of_pci_range range;
>>> struct of_pci_range_parser parser;
>>> char range_type[4];
>>> @@ -200,24 +199,24 @@ int of_pci_get_host_bridge_resources(struct 
>>> device_node *dev,
>>> if (io_base)
>>> *io_base = (resource_size_t)OF_BAD_ADDR;
>>>
>>> -   bus_range = kzalloc(sizeof(*bus_range), GFP_KERNEL);
>>> -   if (!bus_range)
>>> -   return -ENOMEM;
>>> -
>>> pr_info("host bridge %s ranges:\n", dev->full_name);
>>>
>>> -   err = of_pci_parse_bus_range(dev, bus_range);
>>> +   err = of_pci_parse_bus_range(dev, );
>>> if (err) {
>>> -   bus_range->start = busno;
>>> -   bus_range->end = bus_max;
>>> -   bus_range->flags = IORESOURCE_BUS;
>>> -   pr_info("  No bus range found for %s, using %pR\n",
>>> -   dev->full_name, bus_range);
>>> +   res.start = busno;
>>> +   res.end = bus_max;
>>> +   res.flags = IORESOURCE_BUS;
>>> +   pr_info("  No bus range found for %s\n", dev->full_name);
>>> } else {
>>> -   if (bus_range->end > bus_range->start + bus_max)
>>> -   bus_range->end = bus_range->start + bus_max;
>>> +   if (res.end > res.start + bus_max)
>>> +   res.end = res.start + bus_max;
>>> +   }
>>> +   window = pci_add_resource(resources, NULL);
>>> +   if (!window) {
>>> +   err = -ENOMEM;
>>> +   goto parse_failed;
>>> }
>>> -   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.

Bjorn

BTW, please CC linux-pci on the entire series so it's easier to
review.  I don't know where you envision having this applied, but I
only apply things to the PCI tree after they appear on linux-pci.

Re: [PATCH v2 2/2] of/pci: Fix memory leak in of_pci_get_host_bridge_resources

[Bjorn Helgaas]

On Thu, Mar 23, 2017 at 5:58 PM, Dmitry Torokhov  wrote:
> On Thu, Mar 23, 2017 at 3:07 PM, Rob Herring  wrote:
>> On Thu, Mar 23, 2017 at 3:12 AM, Jeffy Chen  
>> wrote:
>>> Currently we only free the allocated resource struct when error.
>>> This would cause memory leak after pci_free_resource_list.
>>>
>>> Signed-off-by: Jeffy Chen 
>>> ---
>>>
>>> Changes in v2:
>>> Don't change the resource_list_create_entry's behavior.
>>>
>>>  drivers/of/of_pci.c | 57 
>>> +++--
>>>  1 file changed, 25 insertions(+), 32 deletions(-)
>>>
>>> diff --git a/drivers/of/of_pci.c b/drivers/of/of_pci.c
>>> index 0ee42c3..a0ec246 100644
>>> --- a/drivers/of/of_pci.c
>>> +++ b/drivers/of/of_pci.c
>>> @@ -190,8 +190,7 @@ int of_pci_get_host_bridge_resources(struct device_node 
>>> *dev,
>>> struct list_head *resources, resource_size_t 
>>> *io_base)
>>>  {
>>> struct resource_entry *window;
>>> -   struct resource *res;
>>> -   struct resource *bus_range;
>>> +   struct resource res;
>>> struct of_pci_range range;
>>> struct of_pci_range_parser parser;
>>> char range_type[4];
>>> @@ -200,24 +199,24 @@ int of_pci_get_host_bridge_resources(struct 
>>> device_node *dev,
>>> if (io_base)
>>> *io_base = (resource_size_t)OF_BAD_ADDR;
>>>
>>> -   bus_range = kzalloc(sizeof(*bus_range), GFP_KERNEL);
>>> -   if (!bus_range)
>>> -   return -ENOMEM;
>>> -
>>> pr_info("host bridge %s ranges:\n", dev->full_name);
>>>
>>> -   err = of_pci_parse_bus_range(dev, bus_range);
>>> +   err = of_pci_parse_bus_range(dev, );
>>> if (err) {
>>> -   bus_range->start = busno;
>>> -   bus_range->end = bus_max;
>>> -   bus_range->flags = IORESOURCE_BUS;
>>> -   pr_info("  No bus range found for %s, using %pR\n",
>>> -   dev->full_name, bus_range);
>>> +   res.start = busno;
>>> +   res.end = bus_max;
>>> +   res.flags = IORESOURCE_BUS;
>>> +   pr_info("  No bus range found for %s\n", dev->full_name);
>>> } else {
>>> -   if (bus_range->end > bus_range->start + bus_max)
>>> -   bus_range->end = bus_range->start + bus_max;
>>> +   if (res.end > res.start + bus_max)
>>> +   res.end = res.start + bus_max;
>>> +   }
>>> +   window = pci_add_resource(resources, NULL);
>>> +   if (!window) {
>>> +   err = -ENOMEM;
>>> +   goto parse_failed;
>>> }
>>> -   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.

Bjorn

BTW, please CC linux-pci on the entire series so it's easier to
review.  I don't know where you envision having this applied, but I
only apply things to the PCI tree after they appear on linux-pci.

Re: [PATCH v2 2/2] of/pci: Fix memory leak in of_pci_get_host_bridge_resources

[jeffy]

Hi Rob & Dmitry,

On 03/24/2017 06:58 AM, Dmitry Torokhov wrote:

On Thu, Mar 23, 2017 at 3:07 PM, Rob Herring  wrote:

On Thu, Mar 23, 2017 at 3:12 AM, Jeffy Chen  wrote:

Currently we only free the allocated resource struct when error.
This would cause memory leak after pci_free_resource_list.

Signed-off-by: Jeffy Chen 
---

Changes in v2:
Don't change the resource_list_create_entry's behavior.

  drivers/of/of_pci.c | 57 +++--
  1 file changed, 25 insertions(+), 32 deletions(-)

diff --git a/drivers/of/of_pci.c b/drivers/of/of_pci.c
index 0ee42c3..a0ec246 100644
--- a/drivers/of/of_pci.c
+++ b/drivers/of/of_pci.c
@@ -190,8 +190,7 @@ int of_pci_get_host_bridge_resources(struct device_node 
*dev,
 struct list_head *resources, resource_size_t *io_base)
  {
 struct resource_entry *window;
-   struct resource *res;
-   struct resource *bus_range;
+   struct resource res;
 struct of_pci_range range;
 struct of_pci_range_parser parser;
 char range_type[4];
@@ -200,24 +199,24 @@ int of_pci_get_host_bridge_resources(struct device_node 
*dev,
 if (io_base)
 *io_base = (resource_size_t)OF_BAD_ADDR;

-   bus_range = kzalloc(sizeof(*bus_range), GFP_KERNEL);
-   if (!bus_range)
-   return -ENOMEM;
-
 pr_info("host bridge %s ranges:\n", dev->full_name);

-   err = of_pci_parse_bus_range(dev, bus_range);
+   err = of_pci_parse_bus_range(dev, );
 if (err) {
-   bus_range->start = busno;
-   bus_range->end = bus_max;
-   bus_range->flags = IORESOURCE_BUS;
-   pr_info("  No bus range found for %s, using %pR\n",
-   dev->full_name, bus_range);
+   res.start = busno;
+   res.end = bus_max;
+   res.flags = IORESOURCE_BUS;
+   pr_info("  No bus range found for %s\n", dev->full_name);
 } else {
-   if (bus_range->end > bus_range->start + bus_max)
-   bus_range->end = bus_range->start + bus_max;
+   if (res.end > res.start + bus_max)
+   res.end = res.start + bus_max;
+   }
+   window = pci_add_resource(resources, NULL);
+   if (!window) {
+   err = -ENOMEM;
+   goto parse_failed;
 }
-   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.

currently, we are using of_pci_get_host_bridge_resources in this pattern:

create resource list:
LIST_HEAD(res);
...
add resources into the list:
err = of_pci_get_host_bridge_resources(dev->of_node, 0, 0xff,
   , _base);
...
walk over the list:
/* Get the I/O and memory ranges from DT */
resource_list_for_each_entry(win, ) {

so only of_pci_get_host_bridge_resources is accessing this list at that 
time.


and an empty resource is harmless i think(with zero size and flags) ;)

maybe i should add some comments in the patch



Thanks.

Re: [PATCH v2 2/2] of/pci: Fix memory leak in of_pci_get_host_bridge_resources

[jeffy]

Hi Rob & Dmitry,

On 03/24/2017 06:58 AM, Dmitry Torokhov wrote:

On Thu, Mar 23, 2017 at 3:07 PM, Rob Herring  wrote:

On Thu, Mar 23, 2017 at 3:12 AM, Jeffy Chen  wrote:

Currently we only free the allocated resource struct when error.
This would cause memory leak after pci_free_resource_list.

Signed-off-by: Jeffy Chen 
---

Changes in v2:
Don't change the resource_list_create_entry's behavior.

  drivers/of/of_pci.c | 57 +++--
  1 file changed, 25 insertions(+), 32 deletions(-)

diff --git a/drivers/of/of_pci.c b/drivers/of/of_pci.c
index 0ee42c3..a0ec246 100644
--- a/drivers/of/of_pci.c
+++ b/drivers/of/of_pci.c
@@ -190,8 +190,7 @@ int of_pci_get_host_bridge_resources(struct device_node 
*dev,
 struct list_head *resources, resource_size_t *io_base)
  {
 struct resource_entry *window;
-   struct resource *res;
-   struct resource *bus_range;
+   struct resource res;
 struct of_pci_range range;
 struct of_pci_range_parser parser;
 char range_type[4];
@@ -200,24 +199,24 @@ int of_pci_get_host_bridge_resources(struct device_node 
*dev,
 if (io_base)
 *io_base = (resource_size_t)OF_BAD_ADDR;

-   bus_range = kzalloc(sizeof(*bus_range), GFP_KERNEL);
-   if (!bus_range)
-   return -ENOMEM;
-
 pr_info("host bridge %s ranges:\n", dev->full_name);

-   err = of_pci_parse_bus_range(dev, bus_range);
+   err = of_pci_parse_bus_range(dev, );
 if (err) {
-   bus_range->start = busno;
-   bus_range->end = bus_max;
-   bus_range->flags = IORESOURCE_BUS;
-   pr_info("  No bus range found for %s, using %pR\n",
-   dev->full_name, bus_range);
+   res.start = busno;
+   res.end = bus_max;
+   res.flags = IORESOURCE_BUS;
+   pr_info("  No bus range found for %s\n", dev->full_name);
 } else {
-   if (bus_range->end > bus_range->start + bus_max)
-   bus_range->end = bus_range->start + bus_max;
+   if (res.end > res.start + bus_max)
+   res.end = res.start + bus_max;
+   }
+   window = pci_add_resource(resources, NULL);
+   if (!window) {
+   err = -ENOMEM;
+   goto parse_failed;
 }
-   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.

currently, we are using of_pci_get_host_bridge_resources in this pattern:

create resource list:
LIST_HEAD(res);
...
add resources into the list:
err = of_pci_get_host_bridge_resources(dev->of_node, 0, 0xff,
   , _base);
...
walk over the list:
/* Get the I/O and memory ranges from DT */
resource_list_for_each_entry(win, ) {

so only of_pci_get_host_bridge_resources is accessing this list at that 
time.


and an empty resource is harmless i think(with zero size and flags) ;)

maybe i should add some comments in the patch



Thanks.

Re: [PATCH v2 2/2] of/pci: Fix memory leak in of_pci_get_host_bridge_resources

[Dmitry Torokhov]

On Thu, Mar 23, 2017 at 3:07 PM, Rob Herring  wrote:
> On Thu, Mar 23, 2017 at 3:12 AM, Jeffy Chen  wrote:
>> Currently we only free the allocated resource struct when error.
>> This would cause memory leak after pci_free_resource_list.
>>
>> Signed-off-by: Jeffy Chen 
>> ---
>>
>> Changes in v2:
>> Don't change the resource_list_create_entry's behavior.
>>
>>  drivers/of/of_pci.c | 57 
>> +++--
>>  1 file changed, 25 insertions(+), 32 deletions(-)
>>
>> diff --git a/drivers/of/of_pci.c b/drivers/of/of_pci.c
>> index 0ee42c3..a0ec246 100644
>> --- a/drivers/of/of_pci.c
>> +++ b/drivers/of/of_pci.c
>> @@ -190,8 +190,7 @@ int of_pci_get_host_bridge_resources(struct device_node 
>> *dev,
>> struct list_head *resources, resource_size_t 
>> *io_base)
>>  {
>> struct resource_entry *window;
>> -   struct resource *res;
>> -   struct resource *bus_range;
>> +   struct resource res;
>> struct of_pci_range range;
>> struct of_pci_range_parser parser;
>> char range_type[4];
>> @@ -200,24 +199,24 @@ int of_pci_get_host_bridge_resources(struct 
>> device_node *dev,
>> if (io_base)
>> *io_base = (resource_size_t)OF_BAD_ADDR;
>>
>> -   bus_range = kzalloc(sizeof(*bus_range), GFP_KERNEL);
>> -   if (!bus_range)
>> -   return -ENOMEM;
>> -
>> pr_info("host bridge %s ranges:\n", dev->full_name);
>>
>> -   err = of_pci_parse_bus_range(dev, bus_range);
>> +   err = of_pci_parse_bus_range(dev, );
>> if (err) {
>> -   bus_range->start = busno;
>> -   bus_range->end = bus_max;
>> -   bus_range->flags = IORESOURCE_BUS;
>> -   pr_info("  No bus range found for %s, using %pR\n",
>> -   dev->full_name, bus_range);
>> +   res.start = busno;
>> +   res.end = bus_max;
>> +   res.flags = IORESOURCE_BUS;
>> +   pr_info("  No bus range found for %s\n", dev->full_name);
>> } else {
>> -   if (bus_range->end > bus_range->start + bus_max)
>> -   bus_range->end = bus_range->start + bus_max;
>> +   if (res.end > res.start + bus_max)
>> +   res.end = res.start + bus_max;
>> +   }
>> +   window = pci_add_resource(resources, NULL);
>> +   if (!window) {
>> +   err = -ENOMEM;
>> +   goto parse_failed;
>> }
>> -   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.

Thanks.

-- 
Dmitry

Re: [PATCH v2 2/2] of/pci: Fix memory leak in of_pci_get_host_bridge_resources

[Dmitry Torokhov]

On Thu, Mar 23, 2017 at 3:07 PM, Rob Herring  wrote:
> On Thu, Mar 23, 2017 at 3:12 AM, Jeffy Chen  wrote:
>> Currently we only free the allocated resource struct when error.
>> This would cause memory leak after pci_free_resource_list.
>>
>> Signed-off-by: Jeffy Chen 
>> ---
>>
>> Changes in v2:
>> Don't change the resource_list_create_entry's behavior.
>>
>>  drivers/of/of_pci.c | 57 
>> +++--
>>  1 file changed, 25 insertions(+), 32 deletions(-)
>>
>> diff --git a/drivers/of/of_pci.c b/drivers/of/of_pci.c
>> index 0ee42c3..a0ec246 100644
>> --- a/drivers/of/of_pci.c
>> +++ b/drivers/of/of_pci.c
>> @@ -190,8 +190,7 @@ int of_pci_get_host_bridge_resources(struct device_node 
>> *dev,
>> struct list_head *resources, resource_size_t 
>> *io_base)
>>  {
>> struct resource_entry *window;
>> -   struct resource *res;
>> -   struct resource *bus_range;
>> +   struct resource res;
>> struct of_pci_range range;
>> struct of_pci_range_parser parser;
>> char range_type[4];
>> @@ -200,24 +199,24 @@ int of_pci_get_host_bridge_resources(struct 
>> device_node *dev,
>> if (io_base)
>> *io_base = (resource_size_t)OF_BAD_ADDR;
>>
>> -   bus_range = kzalloc(sizeof(*bus_range), GFP_KERNEL);
>> -   if (!bus_range)
>> -   return -ENOMEM;
>> -
>> pr_info("host bridge %s ranges:\n", dev->full_name);
>>
>> -   err = of_pci_parse_bus_range(dev, bus_range);
>> +   err = of_pci_parse_bus_range(dev, );
>> if (err) {
>> -   bus_range->start = busno;
>> -   bus_range->end = bus_max;
>> -   bus_range->flags = IORESOURCE_BUS;
>> -   pr_info("  No bus range found for %s, using %pR\n",
>> -   dev->full_name, bus_range);
>> +   res.start = busno;
>> +   res.end = bus_max;
>> +   res.flags = IORESOURCE_BUS;
>> +   pr_info("  No bus range found for %s\n", dev->full_name);
>> } else {
>> -   if (bus_range->end > bus_range->start + bus_max)
>> -   bus_range->end = bus_range->start + bus_max;
>> +   if (res.end > res.start + bus_max)
>> +   res.end = res.start + bus_max;
>> +   }
>> +   window = pci_add_resource(resources, NULL);
>> +   if (!window) {
>> +   err = -ENOMEM;
>> +   goto parse_failed;
>> }
>> -   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.

Thanks.

-- 
Dmitry

Re: [PATCH v2 2/2] of/pci: Fix memory leak in of_pci_get_host_bridge_resources

[Rob Herring]

On Thu, Mar 23, 2017 at 3:12 AM, Jeffy Chen  wrote:
> Currently we only free the allocated resource struct when error.
> This would cause memory leak after pci_free_resource_list.
>
> Signed-off-by: Jeffy Chen 
> ---
>
> Changes in v2:
> Don't change the resource_list_create_entry's behavior.
>
>  drivers/of/of_pci.c | 57 
> +++--
>  1 file changed, 25 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/of/of_pci.c b/drivers/of/of_pci.c
> index 0ee42c3..a0ec246 100644
> --- a/drivers/of/of_pci.c
> +++ b/drivers/of/of_pci.c
> @@ -190,8 +190,7 @@ int of_pci_get_host_bridge_resources(struct device_node 
> *dev,
> struct list_head *resources, resource_size_t *io_base)
>  {
> struct resource_entry *window;
> -   struct resource *res;
> -   struct resource *bus_range;
> +   struct resource res;
> struct of_pci_range range;
> struct of_pci_range_parser parser;
> char range_type[4];
> @@ -200,24 +199,24 @@ int of_pci_get_host_bridge_resources(struct device_node 
> *dev,
> if (io_base)
> *io_base = (resource_size_t)OF_BAD_ADDR;
>
> -   bus_range = kzalloc(sizeof(*bus_range), GFP_KERNEL);
> -   if (!bus_range)
> -   return -ENOMEM;
> -
> pr_info("host bridge %s ranges:\n", dev->full_name);
>
> -   err = of_pci_parse_bus_range(dev, bus_range);
> +   err = of_pci_parse_bus_range(dev, );
> if (err) {
> -   bus_range->start = busno;
> -   bus_range->end = bus_max;
> -   bus_range->flags = IORESOURCE_BUS;
> -   pr_info("  No bus range found for %s, using %pR\n",
> -   dev->full_name, bus_range);
> +   res.start = busno;
> +   res.end = bus_max;
> +   res.flags = IORESOURCE_BUS;
> +   pr_info("  No bus range found for %s\n", dev->full_name);
> } else {
> -   if (bus_range->end > bus_range->start + bus_max)
> -   bus_range->end = bus_range->start + bus_max;
> +   if (res.end > res.start + bus_max)
> +   res.end = res.start + bus_max;
> +   }
> +   window = pci_add_resource(resources, NULL);
> +   if (!window) {
> +   err = -ENOMEM;
> +   goto parse_failed;
> }
> -   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.

Rob

Re: [PATCH v2 2/2] of/pci: Fix memory leak in of_pci_get_host_bridge_resources

[Rob Herring]

On Thu, Mar 23, 2017 at 3:12 AM, Jeffy Chen  wrote:
> Currently we only free the allocated resource struct when error.
> This would cause memory leak after pci_free_resource_list.
>
> Signed-off-by: Jeffy Chen 
> ---
>
> Changes in v2:
> Don't change the resource_list_create_entry's behavior.
>
>  drivers/of/of_pci.c | 57 
> +++--
>  1 file changed, 25 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/of/of_pci.c b/drivers/of/of_pci.c
> index 0ee42c3..a0ec246 100644
> --- a/drivers/of/of_pci.c
> +++ b/drivers/of/of_pci.c
> @@ -190,8 +190,7 @@ int of_pci_get_host_bridge_resources(struct device_node 
> *dev,
> struct list_head *resources, resource_size_t *io_base)
>  {
> struct resource_entry *window;
> -   struct resource *res;
> -   struct resource *bus_range;
> +   struct resource res;
> struct of_pci_range range;
> struct of_pci_range_parser parser;
> char range_type[4];
> @@ -200,24 +199,24 @@ int of_pci_get_host_bridge_resources(struct device_node 
> *dev,
> if (io_base)
> *io_base = (resource_size_t)OF_BAD_ADDR;
>
> -   bus_range = kzalloc(sizeof(*bus_range), GFP_KERNEL);
> -   if (!bus_range)
> -   return -ENOMEM;
> -
> pr_info("host bridge %s ranges:\n", dev->full_name);
>
> -   err = of_pci_parse_bus_range(dev, bus_range);
> +   err = of_pci_parse_bus_range(dev, );
> if (err) {
> -   bus_range->start = busno;
> -   bus_range->end = bus_max;
> -   bus_range->flags = IORESOURCE_BUS;
> -   pr_info("  No bus range found for %s, using %pR\n",
> -   dev->full_name, bus_range);
> +   res.start = busno;
> +   res.end = bus_max;
> +   res.flags = IORESOURCE_BUS;
> +   pr_info("  No bus range found for %s\n", dev->full_name);
> } else {
> -   if (bus_range->end > bus_range->start + bus_max)
> -   bus_range->end = bus_range->start + bus_max;
> +   if (res.end > res.start + bus_max)
> +   res.end = res.start + bus_max;
> +   }
> +   window = pci_add_resource(resources, NULL);
> +   if (!window) {
> +   err = -ENOMEM;
> +   goto parse_failed;
> }
> -   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.

Rob