[PATCH] of: dynamic: fix memory leak related to properties of __of_node_dup

[PATCH] of: dynamic: fix memory leak related to properties of __of_node_dup

[alawang]

It is possible a node was dynamically allocated but without any
property. The properies will be got from devices and added to the
node when devices got connected.
When release this node, all properties of which had been moved to
deadprops.
In this case, the properties in the deadprops list are never
deallocated.

Signed-off-by: alawang <alan.1.wang-FTnzSCDXTkYjo0HpFSRKWA@public.gmane.org>
---
 drivers/of/dynamic.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index 301b6db..465d43b 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -335,6 +335,10 @@ void of_node_release(struct kobject *kobj)
 	if (!of_node_check_flag(node, OF_DYNAMIC))
 		return;
 
+	if (!prop) {
+		prop = node->deadprops;
+		node->deadprops = NULL;
+	}
 	while (prop) {
 		struct property *next = prop->next;
 		kfree(prop->name);
-- 
2.6.2

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] of: dynamic: fix memory leak related to properties of __of_node_dup

[Lixin Wang]

Hello,

Sorry It was my fault that used the wrong sign off name in last email.
Here I correct it.
Thanks
---

It is possible a node was dynamically allocated but without any
property. The properies will be got from devices and added to the
node when devices got connected.
When release this node, all properties of which had been moved to
deadprops.
In this case, the properties in the deadprops list are never
deallocated.

Signed-off-by: Lixin Wang <alan.1.wang-FTnzSCDXTkYjo0HpFSRKWA@public.gmane.org>
---
 drivers/of/dynamic.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index 301b6db..465d43b 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -335,6 +335,10 @@ void of_node_release(struct kobject *kobj)
 	if (!of_node_check_flag(node, OF_DYNAMIC))
 		return;
 
+	if (!prop) {
+		prop = node->deadprops;
+		node->deadprops = NULL;
+	}
 	while (prop) {
 		struct property *next = prop->next;
 		kfree(prop->name);
-- 
2.6.2

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] of: dynamic: fix memory leak related to properties of __of_node_dup

[Rob Herring]

On Thu, Oct 12, 2017 at 8:42 PM, alawang <alan.1.wang-FTnzSCDXTkYjo0HpFSRKWA@public.gmane.org> wrote:
> It is possible a node was dynamically allocated but without any
> property. The properies will be got from devices and added to the
> node when devices got connected.
> When release this node, all properties of which had been moved to
> deadprops.
> In this case, the properties in the deadprops list are never
> deallocated.

This is not new, right? Do you actually hit this case? If so, how?

>
> Signed-off-by: alawang <alan.1.wang-FTnzSCDXTkYjo0HpFSRKWA@public.gmane.org>
> ---
>  drivers/of/dynamic.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
> index 301b6db..465d43b 100644
> --- a/drivers/of/dynamic.c
> +++ b/drivers/of/dynamic.c
> @@ -335,6 +335,10 @@ void of_node_release(struct kobject *kobj)
>         if (!of_node_check_flag(node, OF_DYNAMIC))
>                 return;
>
> +       if (!prop) {
> +               prop = node->deadprops;
> +               node->deadprops = NULL;
> +       }
>         while (prop) {
>                 struct property *next = prop->next;
>                 kfree(prop->name);
> --
> 2.6.2
>
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] of: dynamic: fix memory leak related to properties of __of_node_dup

[Frank Rowand]

On 10/13/17 13:47, Rob Herring wrote:
> On Thu, Oct 12, 2017 at 8:42 PM, alawang <alan.1.wang@nokia-sbell.com> wrote:
>> It is possible a node was dynamically allocated but without any
>> property. The properies will be got from devices and added to the
>> node when devices got connected.
>> When release this node, all properties of which had been moved to
>> deadprops.
>> In this case, the properties in the deadprops list are never
>> deallocated.
> 
> This is not new, right? Do you actually hit this case? If so, how?
> 
>>
>> Signed-off-by: alawang <alan.1.wang@nokia-sbell.com>
>> ---
>>  drivers/of/dynamic.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
>> index 301b6db..465d43b 100644
>> --- a/drivers/of/dynamic.c
>> +++ b/drivers/of/dynamic.c
>> @@ -335,6 +335,10 @@ void of_node_release(struct kobject *kobj)
>>         if (!of_node_check_flag(node, OF_DYNAMIC))
>>                 return;
>>
>> +       if (!prop) {
>> +               prop = node->deadprops;
>> +               node->deadprops = NULL;
>> +       }
>>         while (prop) {
>>                 struct property *next = prop->next;
>>                 kfree(prop->name);
>> --
>> 2.6.2
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe devicetree" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

The code looks right to me.  The patch description is confusing to me.

If a node with no properties is dynamically added, then a property is
dynamically added to the node, then the property is dynamically removed,
the result will be node->properties == NULL and node->deadprops != NULL.

Reviewed-by: Frank Rowand <frowand.list@gmail.com>

RE: [PATCH] of: dynamic: fix memory leak related to properties of __of_node_dup

[Wang, Alan 1. (NSB - CN/Hangzhou)]

Hi Rob and Frank,

Thanks for your comments.
Frank's description is exactly what I meant. I will changed the description as yours if you don't mind.

We are using Linux v3.10.64 and a patch from https://lkml.org/lkml/2014/4/4/207.
With this patch all properties are added dynamically to the dynamic nodes. This is where I firstly found this issue using kmemleak.

And in our environment, the nodes for OCXO are kept empty in the dts file, and the properties will be generated at runtime 
according to sensor details read from ocxo EEPROM. In this case, I think memory leak remains on latest version when resetting that devices.

Thanks.
Lixin

Re: [PATCH] of: dynamic: fix memory leak related to properties of __of_node_dup

[Rob Herring]

On Fri, Oct 13, 2017 at 7:59 PM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On 10/13/17 13:47, Rob Herring wrote:
>> On Thu, Oct 12, 2017 at 8:42 PM, alawang <alan.1.wang-FTnzSCDXTkYjo0HpFSRKWA@public.gmane.org> wrote:
>>> It is possible a node was dynamically allocated but without any
>>> property. The properies will be got from devices and added to the
>>> node when devices got connected.
>>> When release this node, all properties of which had been moved to
>>> deadprops.
>>> In this case, the properties in the deadprops list are never
>>> deallocated.
>>
>> This is not new, right? Do you actually hit this case? If so, how?
>>
>>>
>>> Signed-off-by: alawang <alan.1.wang-FTnzSCDXTkYjo0HpFSRKWA@public.gmane.org>
>>> ---
>>>  drivers/of/dynamic.c | 4 ++++
>>>  1 file changed, 4 insertions(+)
>>>
>>> diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
>>> index 301b6db..465d43b 100644
>>> --- a/drivers/of/dynamic.c
>>> +++ b/drivers/of/dynamic.c
>>> @@ -335,6 +335,10 @@ void of_node_release(struct kobject *kobj)
>>>         if (!of_node_check_flag(node, OF_DYNAMIC))
>>>                 return;
>>>
>>> +       if (!prop) {
>>> +               prop = node->deadprops;
>>> +               node->deadprops = NULL;
>>> +       }
>>>         while (prop) {
>>>                 struct property *next = prop->next;
>>>                 kfree(prop->name);
>>> --
>>> 2.6.2
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe devicetree" in
>>> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>
> The code looks right to me.  The patch description is confusing to me.
>
> If a node with no properties is dynamically added, then a property is
> dynamically added to the node, then the property is dynamically removed,
> the result will be node->properties == NULL and node->deadprops != NULL.

What if you add 2 properties and then remove only 1. This would leave
both properties and deadprops !NULL. I think the while loop needs to
be a separate function so we iterate over both lists.

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4] of: dynamic: fix memory leak related to properties of __of_node_dup

[Lixin Wang]

If a node with no properties is dynamically added, then a property is
dynamically added to the node, then the property is dynamically removed,
the result will be node->properties == NULL and node->deadprops != NULL.

Add a separate function to release the properties in both lists.

Signed-off-by: Lixin Wang <alan.1.wang-FTnzSCDXTkYjo0HpFSRKWA@public.gmane.org>
Reviewed-by: Frank Rowand <frank.rowand-7U/KSKJipcs@public.gmane.org>
---
Thanks the idea from Rob Herring, that I forgot to mention at patch v3.
Thanks Frank Rowand for writing to me the example.

v3 -> v4:
 * Using the style that is more consistent with device tree 
   code, as suggested by Frank Rowand

 drivers/of/dynamic.c | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index 301b6db..f33b7c2 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -315,6 +315,17 @@ int of_detach_node(struct device_node *np)
 }
 EXPORT_SYMBOL_GPL(of_detach_node);
 
+static void property_list_free(struct property *prop_list)
+{
+	struct property *prop;
+
+	for (prop = prop_list; prop != NULL; prop = prop->next) {
+		kfree(prop->name);
+		kfree(prop->value);
+		kfree(prop);
+	}
+}
+
 /**
  * of_node_release() - release a dynamically allocated node
  * @kref: kref element of the node to be released
@@ -324,7 +335,6 @@ EXPORT_SYMBOL_GPL(of_detach_node);
 void of_node_release(struct kobject *kobj)
 {
 	struct device_node *node = kobj_to_device_node(kobj);
-	struct property *prop = node->properties;
 
 	/* We should never be releasing nodes that haven't been detached. */
 	if (!of_node_check_flag(node, OF_DETACHED)) {
@@ -335,18 +345,9 @@ void of_node_release(struct kobject *kobj)
 	if (!of_node_check_flag(node, OF_DYNAMIC))
 		return;
 
-	while (prop) {
-		struct property *next = prop->next;
-		kfree(prop->name);
-		kfree(prop->value);
-		kfree(prop);
-		prop = next;
+	property_list_free(node->properties);
+	property_list_free(node->deadprops);
 
-		if (!prop) {
-			prop = node->deadprops;
-			node->deadprops = NULL;
-		}
-	}
 	kfree(node->full_name);
 	kfree(node->data);
 	kfree(node);
-- 
2.6.2

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4] of: dynamic: fix memory leak related to properties of __of_node_dup

[Rob Herring]

On Thu, Oct 19, 2017 at 8:59 PM, Lixin Wang <alan.1.wang-FTnzSCDXTkYjo0HpFSRKWA@public.gmane.org> wrote:
> If a node with no properties is dynamically added, then a property is
> dynamically added to the node, then the property is dynamically removed,
> the result will be node->properties == NULL and node->deadprops != NULL.
>
> Add a separate function to release the properties in both lists.
>
> Signed-off-by: Lixin Wang <alan.1.wang-FTnzSCDXTkYjo0HpFSRKWA@public.gmane.org>
> Reviewed-by: Frank Rowand <frank.rowand-7U/KSKJipcs@public.gmane.org>
> ---
> Thanks the idea from Rob Herring, that I forgot to mention at patch v3.
> Thanks Frank Rowand for writing to me the example.
>
> v3 -> v4:
>  * Using the style that is more consistent with device tree
>    code, as suggested by Frank Rowand
>
>  drivers/of/dynamic.c | 25 +++++++++++++------------
>  1 file changed, 13 insertions(+), 12 deletions(-)

Applied.

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2] of: dynamic: fix memory leak related to properties of __of_node_dup

[Lixin Wang]

If a node with no properties is dynamically added, then a property is
dynamically added to the node, then the property is dynamically removed,
the result will be node->properties == NULL and node->deadprops != NULL.

Signed-off-by: Lixin Wang <alan.1.wang@nokia-sbell.com>
---
v1:
 * Change the description of this patch as suggested by Frank Rowand.

 drivers/of/dynamic.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index 301b6db..465d43b 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -335,6 +335,10 @@ void of_node_release(struct kobject *kobj)
 	if (!of_node_check_flag(node, OF_DYNAMIC))
 		return;
 
+	if (!prop) {
+		prop = node->deadprops;
+		node->deadprops = NULL;
+	}
 	while (prop) {
 		struct property *next = prop->next;
 		kfree(prop->name);
-- 
2.6.2

[PATCH v3] of: dynamic: fix memory leak related to properties of __of_node_dup

[Lixin Wang]

If a node with no properties is dynamically added, then a property is
dynamically added to the node, then the property is dynamically removed,
the result will be node->properties == NULL and node->deadprops != NULL.

Add a separate function to release the properties in both lists.

Signed-off-by: Lixin Wang <alan.1.wang@nokia-sbell.com>
---
v2 -> v3:
 * Add a separate function to release the properties in both lists.
 * Change the patch description for this change.

 drivers/of/dynamic.c | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index 301b6db..b28511f 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -315,6 +315,19 @@ int of_detach_node(struct device_node *np)
 }
 EXPORT_SYMBOL_GPL(of_detach_node);
 
+static void __of_property_list_release(struct property *prop)
+{
+	struct property *next;
+
+	while (prop) {
+		next = prop->next;
+		kfree(prop->name);
+		kfree(prop->value);
+		kfree(prop);
+		prop = next;
+	}
+}
+
 /**
  * of_node_release() - release a dynamically allocated node
  * @kref: kref element of the node to be released
@@ -324,7 +337,6 @@ EXPORT_SYMBOL_GPL(of_detach_node);
 void of_node_release(struct kobject *kobj)
 {
 	struct device_node *node = kobj_to_device_node(kobj);
-	struct property *prop = node->properties;
 
 	/* We should never be releasing nodes that haven't been detached. */
 	if (!of_node_check_flag(node, OF_DETACHED)) {
@@ -335,18 +347,9 @@ void of_node_release(struct kobject *kobj)
 	if (!of_node_check_flag(node, OF_DYNAMIC))
 		return;
 
-	while (prop) {
-		struct property *next = prop->next;
-		kfree(prop->name);
-		kfree(prop->value);
-		kfree(prop);
-		prop = next;
+	__of_property_list_release(node->properties);
+	__of_property_list_release(node->deadprops);
 
-		if (!prop) {
-			prop = node->deadprops;
-			node->deadprops = NULL;
-		}
-	}
 	kfree(node->full_name);
 	kfree(node->data);
 	kfree(node);
-- 
2.6.2

Re: [PATCH v3] of: dynamic: fix memory leak related to properties of __of_node_dup

[Frank Rowand]

Hi Lixin,

On 10/19/17 02:40, Lixin Wang wrote:
> If a node with no properties is dynamically added, then a property is
> dynamically added to the node, then the property is dynamically removed,
> the result will be node->properties == NULL and node->deadprops != NULL.
> 
> Add a separate function to release the properties in both lists.
> 
> Signed-off-by: Lixin Wang <alan.1.wang@nokia-sbell.com>
> ---
> v2 -> v3:
>  * Add a separate function to release the properties in both lists.
>  * Change the patch description for this change.
> 
>  drivers/of/dynamic.c | 27 +++++++++++++++------------
>  1 file changed, 15 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
> index 301b6db..b28511f 100644
> --- a/drivers/of/dynamic.c
> +++ b/drivers/of/dynamic.c
> @@ -315,6 +315,19 @@ int of_detach_node(struct device_node *np)
>  }
>  EXPORT_SYMBOL_GPL(of_detach_node);
>  
> +static void __of_property_list_release(struct property *prop)
> +{
> +	struct property *next;
> +
> +	while (prop) {
> +		next = prop->next;
> +		kfree(prop->name);
> +		kfree(prop->value);
> +		kfree(prop);
> +		prop = next;
> +	}
> +}
> +
>  /**
>   * of_node_release() - release a dynamically allocated node
>   * @kref: kref element of the node to be released
> @@ -324,7 +337,6 @@ EXPORT_SYMBOL_GPL(of_detach_node);
>  void of_node_release(struct kobject *kobj)
>  {
>  	struct device_node *node = kobj_to_device_node(kobj);
> -	struct property *prop = node->properties;
>  
>  	/* We should never be releasing nodes that haven't been detached. */
>  	if (!of_node_check_flag(node, OF_DETACHED)) {
> @@ -335,18 +347,9 @@ void of_node_release(struct kobject *kobj)
>  	if (!of_node_check_flag(node, OF_DYNAMIC))
>  		return;
>  
> -	while (prop) {
> -		struct property *next = prop->next;
> -		kfree(prop->name);
> -		kfree(prop->value);
> -		kfree(prop);
> -		prop = next;
> +	__of_property_list_release(node->properties);
> +	__of_property_list_release(node->deadprops);
>  
> -		if (!prop) {
> -			prop = node->deadprops;
> -			node->deadprops = NULL;
> -		}
> -	}
>  	kfree(node->full_name);
>  	kfree(node->data);
>  	kfree(node);
> 

The code is correct, but I would prefer some style issues to be more
consistent with device tree code.  I will reply to this email with
an example patch that you can use.

-Frank

Re: [PATCH v3] of: dynamic: fix memory leak related to properties of __of_node_dup

[frowand.list-Re5JQEeQqe8AvxtiuMwx3w]

From: Frank Rowand <frank.rowand-7U/KSKJipcs@public.gmane.org>

This is Lixin's patch v3, reworded for my preferred style.

---
If Lixin agrees,

   Reviewed-by: Frank Rowand <frank.rowand-7U/KSKJipcs@public.gmane.org>


 drivers/of/dynamic.c | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index 301b6db2b48d..f33b7c2e703e 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -315,6 +315,17 @@ int of_detach_node(struct device_node *np)
 }
 EXPORT_SYMBOL_GPL(of_detach_node);
 
+static void property_list_free(struct property *prop_list)
+{
+	struct property *prop;
+
+	for (prop = prop_list; prop != NULL; prop = prop->next) {
+		kfree(prop->name);
+		kfree(prop->value);
+		kfree(prop);
+	}
+}
+
 /**
  * of_node_release() - release a dynamically allocated node
  * @kref: kref element of the node to be released
@@ -324,7 +335,6 @@ int of_detach_node(struct device_node *np)
 void of_node_release(struct kobject *kobj)
 {
 	struct device_node *node = kobj_to_device_node(kobj);
-	struct property *prop = node->properties;
 
 	/* We should never be releasing nodes that haven't been detached. */
 	if (!of_node_check_flag(node, OF_DETACHED)) {
@@ -335,18 +345,9 @@ void of_node_release(struct kobject *kobj)
 	if (!of_node_check_flag(node, OF_DYNAMIC))
 		return;
 
-	while (prop) {
-		struct property *next = prop->next;
-		kfree(prop->name);
-		kfree(prop->value);
-		kfree(prop);
-		prop = next;
+	property_list_free(node->properties);
+	property_list_free(node->deadprops);
 
-		if (!prop) {
-			prop = node->deadprops;
-			node->deadprops = NULL;
-		}
-	}
 	kfree(node->full_name);
 	kfree(node->data);
 	kfree(node);
-- 
Frank Rowand <frank.rowand-7U/KSKJipcs@public.gmane.org>

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v5] of: dynamic: fix memory leak related to properties of __of_node_dup

[Lixin Wang]

If a node with no properties is dynamically added, then a property is
dynamically added to the node, then the property is dynamically removed,
the result will be node->properties == NULL and node->deadprops != NULL.

Add a separate function to release the properties in both lists.

Signed-off-by: Lixin Wang <alan.1.wang@nokia-sbell.com>
---
 v4 -> v5:
 * fix the bug in v4, that the prop->next should be saved before release
   the prop.

 drivers/of/dynamic.c | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index 301b6db..39d69d3 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -315,6 +315,18 @@ int of_detach_node(struct device_node *np)
 }
 EXPORT_SYMBOL_GPL(of_detach_node);
 
+static void property_list_free(struct property *prop_list)
+{
+	struct property *prop, *next;
+
+	for (prop = prop_list; prop != NULL; prop = next) {
+		next = prop->next;
+		kfree(prop->name);
+		kfree(prop->value);
+		kfree(prop);
+	}
+}
+
 /**
  * of_node_release() - release a dynamically allocated node
  * @kref: kref element of the node to be released
@@ -324,7 +336,6 @@ EXPORT_SYMBOL_GPL(of_detach_node);
 void of_node_release(struct kobject *kobj)
 {
 	struct device_node *node = kobj_to_device_node(kobj);
-	struct property *prop = node->properties;
 
 	/* We should never be releasing nodes that haven't been detached. */
 	if (!of_node_check_flag(node, OF_DETACHED)) {
@@ -335,18 +346,9 @@ void of_node_release(struct kobject *kobj)
 	if (!of_node_check_flag(node, OF_DYNAMIC))
 		return;
 
-	while (prop) {
-		struct property *next = prop->next;
-		kfree(prop->name);
-		kfree(prop->value);
-		kfree(prop);
-		prop = next;
+	property_list_free(node->properties);
+	property_list_free(node->deadprops);
 
-		if (!prop) {
-			prop = node->deadprops;
-			node->deadprops = NULL;
-		}
-	}
 	kfree(node->full_name);
 	kfree(node->data);
 	kfree(node);
-- 
2.6.2

Re: [PATCH v5] of: dynamic: fix memory leak related to properties of __of_node_dup

[Frank Rowand]

On 10/22/17 20:19, Lixin Wang wrote:
> If a node with no properties is dynamically added, then a property is
> dynamically added to the node, then the property is dynamically removed,
> the result will be node->properties == NULL and node->deadprops != NULL.
> 
> Add a separate function to release the properties in both lists.
> 
> Signed-off-by: Lixin Wang <alan.1.wang@nokia-sbell.com>
> ---
>  v4 -> v5:
>  * fix the bug in v4, that the prop->next should be saved before release
>    the prop.
> 
>  drivers/of/dynamic.c | 26 ++++++++++++++------------
>  1 file changed, 14 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
> index 301b6db..39d69d3 100644
> --- a/drivers/of/dynamic.c
> +++ b/drivers/of/dynamic.c
> @@ -315,6 +315,18 @@ int of_detach_node(struct device_node *np)
>  }
>  EXPORT_SYMBOL_GPL(of_detach_node);
>  
> +static void property_list_free(struct property *prop_list)
> +{
> +	struct property *prop, *next;
> +
> +	for (prop = prop_list; prop != NULL; prop = next) {
> +		next = prop->next;
> +		kfree(prop->name);
> +		kfree(prop->value);
> +		kfree(prop);
> +	}
> +}
> +
>  /**
>   * of_node_release() - release a dynamically allocated node
>   * @kref: kref element of the node to be released
> @@ -324,7 +336,6 @@ EXPORT_SYMBOL_GPL(of_detach_node);
>  void of_node_release(struct kobject *kobj)
>  {
>  	struct device_node *node = kobj_to_device_node(kobj);
> -	struct property *prop = node->properties;
>  
>  	/* We should never be releasing nodes that haven't been detached. */
>  	if (!of_node_check_flag(node, OF_DETACHED)) {
> @@ -335,18 +346,9 @@ void of_node_release(struct kobject *kobj)
>  	if (!of_node_check_flag(node, OF_DYNAMIC))
>  		return;
>  
> -	while (prop) {
> -		struct property *next = prop->next;
> -		kfree(prop->name);
> -		kfree(prop->value);
> -		kfree(prop);
> -		prop = next;
> +	property_list_free(node->properties);
> +	property_list_free(node->deadprops);
>  
> -		if (!prop) {
> -			prop = node->deadprops;
> -			node->deadprops = NULL;
> -		}
> -	}
>  	kfree(node->full_name);
>  	kfree(node->data);
>  	kfree(node);
> 

Hi Lixin,
 
My bad...  Thanks for the fix.

Reviewed-by: Frank Rowand <frank.rowand@sony.com>

Re: [PATCH v5] of: dynamic: fix memory leak related to properties of __of_node_dup

[Rob Herring]

On Mon, Oct 23, 2017 at 12:16 AM, Frank Rowand <frowand.list@gmail.com> wrote:
> On 10/22/17 20:19, Lixin Wang wrote:
>> If a node with no properties is dynamically added, then a property is
>> dynamically added to the node, then the property is dynamically removed,
>> the result will be node->properties == NULL and node->deadprops != NULL.
>>
>> Add a separate function to release the properties in both lists.
>>
>> Signed-off-by: Lixin Wang <alan.1.wang@nokia-sbell.com>
>> ---
>>  v4 -> v5:
>>  * fix the bug in v4, that the prop->next should be saved before release
>>    the prop.
>>
>>  drivers/of/dynamic.c | 26 ++++++++++++++------------
>>  1 file changed, 14 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
>> index 301b6db..39d69d3 100644
>> --- a/drivers/of/dynamic.c
>> +++ b/drivers/of/dynamic.c
>> @@ -315,6 +315,18 @@ int of_detach_node(struct device_node *np)
>>  }
>>  EXPORT_SYMBOL_GPL(of_detach_node);
>>
>> +static void property_list_free(struct property *prop_list)
>> +{
>> +     struct property *prop, *next;
>> +
>> +     for (prop = prop_list; prop != NULL; prop = next) {
>> +             next = prop->next;
>> +             kfree(prop->name);
>> +             kfree(prop->value);
>> +             kfree(prop);
>> +     }
>> +}
>> +
>>  /**
>>   * of_node_release() - release a dynamically allocated node
>>   * @kref: kref element of the node to be released
>> @@ -324,7 +336,6 @@ EXPORT_SYMBOL_GPL(of_detach_node);
>>  void of_node_release(struct kobject *kobj)
>>  {
>>       struct device_node *node = kobj_to_device_node(kobj);
>> -     struct property *prop = node->properties;
>>
>>       /* We should never be releasing nodes that haven't been detached. */
>>       if (!of_node_check_flag(node, OF_DETACHED)) {
>> @@ -335,18 +346,9 @@ void of_node_release(struct kobject *kobj)
>>       if (!of_node_check_flag(node, OF_DYNAMIC))
>>               return;
>>
>> -     while (prop) {
>> -             struct property *next = prop->next;
>> -             kfree(prop->name);
>> -             kfree(prop->value);
>> -             kfree(prop);
>> -             prop = next;
>> +     property_list_free(node->properties);
>> +     property_list_free(node->deadprops);
>>
>> -             if (!prop) {
>> -                     prop = node->deadprops;
>> -                     node->deadprops = NULL;
>> -             }
>> -     }
>>       kfree(node->full_name);
>>       kfree(node->data);
>>       kfree(node);
>>
>
> Hi Lixin,
>
> My bad...  Thanks for the fix.
>
> Reviewed-by: Frank Rowand <frank.rowand@sony.com>

I've dropped v4 and applied v5.

Rob