[PATCH 0/5] of: unittest: kmleak detected memory leaks

[PATCH 0/5] of: unittest: kmleak detected memory leaks

[frowand.list]

From: Frank Rowand <frank.rowand@sony.com>

Original bug report:
  https://bugzilla.kernel.org/show_bug.cgi?id=206203
  https://lore.kernel.org/r/877dyqlles.fsf@mpe.ellerman.id.au

kmemleak detected many memory leaks originating if devicetree
unittests.

Five separate causes were found.  Four causes were bugs and
one was expected.

Fix the four bugs and modify the test that led to the expected
memory leak to no longer have a memory leak.


Frank Rowand (5):
  of: unittest: kmemleak on changeset destroy
  of: unittest: kmemleak in of_unittest_platform_populate()
  of: unittest: kmemleak in of_unittest_overlay_high_level()
  of: overlay: kmemleak in dup_and_fixup_symbol_prop()
  of: unittest: kmemleak in duplicate property update

 drivers/of/overlay.c                               |  2 ++
 .../of/unittest-data/overlay_bad_add_dup_prop.dts  | 23 ++++++++++++++----
 drivers/of/unittest.c                              | 28 +++++++++++++++-------
 3 files changed, 40 insertions(+), 13 deletions(-)

-- 
Frank Rowand <frank.rowand@sony.com>

[PATCH 1/5] of: unittest: kmemleak on changeset destroy

[frowand.list]

From: Frank Rowand <frank.rowand@sony.com>

kmemleak reports several memory leaks from devicetree unittest.
This is the fix for problem 1 of 5.

of_unittest_changeset() reaches deeply into the dynamic devicetree
functions.  Several nodes were left with an elevated reference
count and thus were not properly cleaned up.  Fix the reference
counts so that the memory will be freed.

Fixes: 201c910bd689 ("of: Transactional DT support.")
Reported-by: Erhard F. <erhard_f@mailbox.org>
Signed-off-by: Frank Rowand <frank.rowand@sony.com>
---
 drivers/of/unittest.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
index 7e27670c3616..20ff2dfc3143 100644
--- a/drivers/of/unittest.c
+++ b/drivers/of/unittest.c
@@ -861,6 +861,10 @@ static void __init of_unittest_changeset(void)
 	unittest(!of_changeset_revert(&chgset), "revert failed\n");
 
 	of_changeset_destroy(&chgset);
+
+	of_node_put(n1);
+	of_node_put(n2);
+	of_node_put(n21);
 #endif
 }
 
-- 
Frank Rowand <frank.rowand@sony.com>

[PATCH 2/5] of: unittest: kmemleak in of_unittest_platform_populate()

[frowand.list]

From: Frank Rowand <frank.rowand@sony.com>

kmemleak reports several memory leaks from devicetree unittest.
This is the fix for problem 2 of 5.

of_unittest_platform_populate() left an elevated reference count for
grandchild nodes (which are platform devices).  Fix the platform
device reference counts so that the memory will be freed.

Fixes: fb2caa50fbac ("of/selftest: add testcase for nodes with same name and address")
Reported-by: Erhard F. <erhard_f@mailbox.org>
Signed-off-by: Frank Rowand <frank.rowand@sony.com>
---
 drivers/of/unittest.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
index 20ff2dfc3143..4c7818276857 100644
--- a/drivers/of/unittest.c
+++ b/drivers/of/unittest.c
@@ -1247,10 +1247,13 @@ static void __init of_unittest_platform_populate(void)
 
 	of_platform_populate(np, match, NULL, &test_bus->dev);
 	for_each_child_of_node(np, child) {
-		for_each_child_of_node(child, grandchild)
-			unittest(of_find_device_by_node(grandchild),
+		for_each_child_of_node(child, grandchild) {
+			pdev = of_find_device_by_node(grandchild);
+			unittest(pdev,
 				 "Could not create device for node '%pOFn'\n",
 				 grandchild);
+			of_dev_put(pdev);
+		}
 	}
 
 	of_platform_depopulate(&test_bus->dev);
-- 
Frank Rowand <frank.rowand@sony.com>

[PATCH 3/5] of: unittest: kmemleak in of_unittest_overlay_high_level()

[frowand.list]

From: Frank Rowand <frank.rowand@sony.com>

kmemleak reports several memory leaks from devicetree unittest.
This is the fix for problem 3 of 5.

of_unittest_overlay_high_level() failed to kfree the newly created
property when the property named 'name' is skipped.

Fixes: 39a751a4cb7e ("of: change overlay apply input data from unflattened to FDT")
Reported-by: Erhard F. <erhard_f@mailbox.org>
Signed-off-by: Frank Rowand <frank.rowand@sony.com>
---
 drivers/of/unittest.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
index 4c7818276857..f238b7a3865d 100644
--- a/drivers/of/unittest.c
+++ b/drivers/of/unittest.c
@@ -3094,8 +3094,11 @@ static __init void of_unittest_overlay_high_level(void)
 				goto err_unlock;
 			}
 			if (__of_add_property(of_symbols, new_prop)) {
+				kfree(new_prop->name);
+				kfree(new_prop->value);
+				kfree(new_prop);
 				/* "name" auto-generated by unflatten */
-				if (!strcmp(new_prop->name, "name"))
+				if (!strcmp(prop->name, "name"))
 					continue;
 				unittest(0, "duplicate property '%s' in overlay_base node __symbols__",
 					 prop->name);
-- 
Frank Rowand <frank.rowand@sony.com>

[PATCH 4/5] of: overlay: kmemleak in dup_and_fixup_symbol_prop()

[frowand.list]

From: Frank Rowand <frank.rowand@sony.com>

kmemleak reports several memory leaks from devicetree unittest.
This is the fix for problem 4 of 5.

target_path was not freed in the non-error path.

Fixes: e0a58f3e08d4 ("of: overlay: remove a dependency on device node full_name")
Reported-by: Erhard F. <erhard_f@mailbox.org>
Signed-off-by: Frank Rowand <frank.rowand@sony.com>
---
 drivers/of/overlay.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index c9219fddf44b..50bbe0edf538 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -261,6 +261,8 @@ static struct property *dup_and_fixup_symbol_prop(
 
 	of_property_set_flag(new_prop, OF_DYNAMIC);
 
+	kfree(target_path);
+
 	return new_prop;
 
 err_free_new_prop:
-- 
Frank Rowand <frank.rowand@sony.com>

[PATCH 5/5] of: unittest: kmemleak in duplicate property update

[frowand.list]

From: Frank Rowand <frank.rowand@sony.com>

kmemleak reports several memory leaks from devicetree unittest.
This is the fix for problem 5 of 5.

When overlay 'overlay_bad_add_dup_prop' is applied, the apply code
properly detects that a memory leak will occur if the overlay is removed
since the duplicate property is located in a base devicetree node and
reports via printk():

  OF: overlay: WARNING: memory leak will occur if overlay removed, property: /testcase-data-2/substation@100/motor-1/rpm_avail
  OF: overlay: WARNING: memory leak will occur if overlay removed, property: /testcase-data-2/substation@100/motor-1/rpm_avail

The overlay is removed when the apply code detects multiple changesets
modifying the same property.  This is reported via printk():

  OF: overlay: ERROR: multiple fragments add, update, and/or delete property /testcase-data-2/substation@100/motor-1/rpm_avail

As a result of this error, the overlay is removed resulting in the
expected memory leak.

Add another device node level to the overlay so that the duplicate
property is located in a node added by the overlay, thus no memory
leak will occur when the overlay is removed.

Thus users of kmemleak will not have to debug this leak in the future.

Fixes: 2fe0e8769df9 ("of: overlay: check prevents multiple fragments touching same property")
Reported-by: Erhard F. <erhard_f@mailbox.org>
Signed-off-by: Frank Rowand <frank.rowand@sony.com>
---
 .../of/unittest-data/overlay_bad_add_dup_prop.dts  | 23 ++++++++++++++++++----
 drivers/of/unittest.c                              | 12 +++++------
 2 files changed, 25 insertions(+), 10 deletions(-)

diff --git a/drivers/of/unittest-data/overlay_bad_add_dup_prop.dts b/drivers/of/unittest-data/overlay_bad_add_dup_prop.dts
index c190da54f175..6327d1ffb963 100644
--- a/drivers/of/unittest-data/overlay_bad_add_dup_prop.dts
+++ b/drivers/of/unittest-data/overlay_bad_add_dup_prop.dts
@@ -3,22 +3,37 @@
 /plugin/;
 
 /*
- * &electric_1/motor-1 and &spin_ctrl_1 are the same node:
- *   /testcase-data-2/substation@100/motor-1
+ * &electric_1/motor-1/electric and &spin_ctrl_1/electric are the same node:
+ *   /testcase-data-2/substation@100/motor-1/electric
  *
  * Thus the property "rpm_avail" in each fragment will
  * result in an attempt to update the same property twice.
  * This will result in an error and the overlay apply
  * will fail.
+ *
+ * The previous version of this test did not include the extra
+ * level of node 'electric'.  That resulted in the 'rpm_avail'
+ * property being located in the pre-existing node 'motor-1'.
+ * Modifying a property results in a WARNING that a memory leak
+ * will occur if the overlay is removed.  Since the overlay apply
+ * fails, the memory leak does actually occur, and kmemleak will
+ * further report the memory leak if CONFIG_DEBUG_KMEMLEAK is
+ * enabled.  Adding the overlay node 'electric' avoids the
+ * memory leak and thus people who use kmemleak will not
+ * have to debug this non-problem again.
  */
 
 &electric_1 {
 
 	motor-1 {
-		rpm_avail = < 100 >;
+		electric {
+			rpm_avail = < 100 >;
+		};
 	};
 };
 
 &spin_ctrl_1 {
-		rpm_avail = < 100 200 >;
+		electric {
+			rpm_avail = < 100 200 >;
+		};
 };
diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
index f238b7a3865d..398de04fd19c 100644
--- a/drivers/of/unittest.c
+++ b/drivers/of/unittest.c
@@ -3181,21 +3181,21 @@ static __init void of_unittest_overlay_high_level(void)
 		   "OF: overlay: ERROR: multiple fragments add and/or delete node /testcase-data-2/substation@100/motor-1/controller");
 
 	EXPECT_BEGIN(KERN_ERR,
-		     "OF: overlay: WARNING: memory leak will occur if overlay removed, property: /testcase-data-2/substation@100/motor-1/rpm_avail");
+		     "OF: overlay: ERROR: multiple fragments add and/or delete node /testcase-data-2/substation@100/motor-1/electric");
 	EXPECT_BEGIN(KERN_ERR,
-		     "OF: overlay: WARNING: memory leak will occur if overlay removed, property: /testcase-data-2/substation@100/motor-1/rpm_avail");
+		     "OF: overlay: ERROR: multiple fragments add, update, and/or delete property /testcase-data-2/substation@100/motor-1/electric/rpm_avail");
 	EXPECT_BEGIN(KERN_ERR,
-		     "OF: overlay: ERROR: multiple fragments add, update, and/or delete property /testcase-data-2/substation@100/motor-1/rpm_avail");
+		     "OF: overlay: ERROR: multiple fragments add, update, and/or delete property /testcase-data-2/substation@100/motor-1/electric/name");
 
 	unittest(overlay_data_apply("overlay_bad_add_dup_prop", NULL),
 		 "Adding overlay 'overlay_bad_add_dup_prop' failed\n");
 
 	EXPECT_END(KERN_ERR,
-		   "OF: overlay: ERROR: multiple fragments add, update, and/or delete property /testcase-data-2/substation@100/motor-1/rpm_avail");
+		     "OF: overlay: ERROR: multiple fragments add, update, and/or delete property /testcase-data-2/substation@100/motor-1/electric/name");
 	EXPECT_END(KERN_ERR,
-		   "OF: overlay: WARNING: memory leak will occur if overlay removed, property: /testcase-data-2/substation@100/motor-1/rpm_avail");
+		     "OF: overlay: ERROR: multiple fragments add, update, and/or delete property /testcase-data-2/substation@100/motor-1/electric/rpm_avail");
 	EXPECT_END(KERN_ERR,
-		   "OF: overlay: WARNING: memory leak will occur if overlay removed, property: /testcase-data-2/substation@100/motor-1/rpm_avail");
+		     "OF: overlay: ERROR: multiple fragments add and/or delete node /testcase-data-2/substation@100/motor-1/electric");
 
 	unittest(overlay_data_apply("overlay_bad_phandle", NULL),
 		 "Adding overlay 'overlay_bad_phandle' failed\n");
-- 
Frank Rowand <frank.rowand@sony.com>

Re: [PATCH 0/5] of: unittest: kmleak detected memory leaks

[Rob Herring]

On Thu, Apr 16, 2020 at 04:42:45PM -0500, frowand.list@gmail.com wrote:
> From: Frank Rowand <frank.rowand@sony.com>
> 
> Original bug report:
>   https://bugzilla.kernel.org/show_bug.cgi?id=206203
>   https://lore.kernel.org/r/877dyqlles.fsf@mpe.ellerman.id.au
> 
> kmemleak detected many memory leaks originating if devicetree
> unittests.
> 
> Five separate causes were found.  Four causes were bugs and
> one was expected.
> 
> Fix the four bugs and modify the test that led to the expected
> memory leak to no longer have a memory leak.
> 
> 
> Frank Rowand (5):
>   of: unittest: kmemleak on changeset destroy
>   of: unittest: kmemleak in of_unittest_platform_populate()
>   of: unittest: kmemleak in of_unittest_overlay_high_level()
>   of: overlay: kmemleak in dup_and_fixup_symbol_prop()
>   of: unittest: kmemleak in duplicate property update

Series applied. (To get into today's -next and I plan to send to Linus 
tomorrow.)

Rob

Re: [PATCH 5/5] of: unittest: kmemleak in duplicate property update

[Rob Herring]

On Thu, 16 Apr 2020 16:42:50 -0500, frowand.list@gmail.com wrote:
> From: Frank Rowand <frank.rowand@sony.com>
> 
> kmemleak reports several memory leaks from devicetree unittest.
> This is the fix for problem 5 of 5.
> 
> When overlay 'overlay_bad_add_dup_prop' is applied, the apply code
> properly detects that a memory leak will occur if the overlay is removed
> since the duplicate property is located in a base devicetree node and
> reports via printk():
> 
>   OF: overlay: WARNING: memory leak will occur if overlay removed, property: /testcase-data-2/substation@100/motor-1/rpm_avail
>   OF: overlay: WARNING: memory leak will occur if overlay removed, property: /testcase-data-2/substation@100/motor-1/rpm_avail
> 
> The overlay is removed when the apply code detects multiple changesets
> modifying the same property.  This is reported via printk():
> 
>   OF: overlay: ERROR: multiple fragments add, update, and/or delete property /testcase-data-2/substation@100/motor-1/rpm_avail
> 
> As a result of this error, the overlay is removed resulting in the
> expected memory leak.
> 
> Add another device node level to the overlay so that the duplicate
> property is located in a node added by the overlay, thus no memory
> leak will occur when the overlay is removed.
> 
> Thus users of kmemleak will not have to debug this leak in the future.
> 
> Fixes: 2fe0e8769df9 ("of: overlay: check prevents multiple fragments touching same property")
> Reported-by: Erhard F. <erhard_f@mailbox.org>
> Signed-off-by: Frank Rowand <frank.rowand@sony.com>
> ---
>  .../of/unittest-data/overlay_bad_add_dup_prop.dts  | 23 ++++++++++++++++++----
>  drivers/of/unittest.c                              | 12 +++++------
>  2 files changed, 25 insertions(+), 10 deletions(-)
> 

Applied, thanks.

Rob