[PATCH 0/2] of: unittest: re-implement overlay tracking

[PATCH 0/2] of: unittest: re-implement overlay tracking

[frowand.list]

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

Some overlays are tracked when they are applied.  The tracked overlays
are later removed after the overlay tests are completed.  The old
implementation makes assumptions about the expected values for
overlay changeset id created by the overlay apply which result
in fragile code.  The new code removes the assumptions.

A symptom that exposes a problem with the tracking code is a
warning "UBSAN: shift-out-of-bounds in drivers/of/unittest.c:1933:36",
Kernel Version: 5.15-rc7, PPC-64, Talos II.  This results from variable
"id" value of -1 in the final line of of_unittest_untrack_overlay().

The first patch in the series cleans up the inconsistent use of overlay
changeset id and the obsolete overlay id.  The id is a core concept in
the overlay tracking that is re-implemented in the second patch in
the series.

Frank Rowand (2):
  of: unittest: change references to obsolete overlay id
  of: unittest: re-implement overlay tracking

 drivers/of/unittest.c | 154 +++++++++++++++++++-----------------------
 1 file changed, 71 insertions(+), 83 deletions(-)

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

[PATCH 1/2] of: unittest: change references to obsolete overlay id

[frowand.list]

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

Unittest inconsistently interchanges overlay changeset id and
overlay id.  Change variable names of overlay id to overlay
changeset id.

Do not fix variable names in the overlay tracking functions
of_unittest_overlay_tracked(), of_unittest_track_overlay(), and
of_unittest_destroy_tracked_overlays() which will be replaced in
a following commit.

Signed-off-by: Frank Rowand <frank.rowand@sony.com>
---
 drivers/of/unittest.c | 54 +++++++++++++++++++++----------------------
 1 file changed, 27 insertions(+), 27 deletions(-)

diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
index 481ba8682ebf..33c458044600 100644
--- a/drivers/of/unittest.c
+++ b/drivers/of/unittest.c
@@ -1492,7 +1492,7 @@ static int __init unittest_data_add(void)
 }
 
 #ifdef CONFIG_OF_OVERLAY
-static int __init overlay_data_apply(const char *overlay_name, int *overlay_id);
+static int __init overlay_data_apply(const char *overlay_name, int *ovcs_id);
 
 static int unittest_probe(struct platform_device *pdev)
 {
@@ -1973,18 +1973,18 @@ static void of_unittest_destroy_tracked_overlays(void)
 	} while (defers > 0);
 }
 
-static int __init of_unittest_apply_overlay(int overlay_nr, int *overlay_id)
+static int __init of_unittest_apply_overlay(int overlay_nr, int *ovcs_id)
 {
 	const char *overlay_name;
 
 	overlay_name = overlay_name_from_nr(overlay_nr);
 
-	if (!overlay_data_apply(overlay_name, overlay_id)) {
+	if (!overlay_data_apply(overlay_name, ovcs_id)) {
 		unittest(0, "could not apply overlay \"%s\"\n",
 				overlay_name);
 		return -EFAULT;
 	}
-	of_unittest_track_overlay(*overlay_id);
+	of_unittest_track_overlay(*ovcs_id);
 
 	return 0;
 }
@@ -2029,7 +2029,7 @@ static int __init of_unittest_apply_revert_overlay_check(int overlay_nr,
 		int unittest_nr, int before, int after,
 		enum overlay_type ovtype)
 {
-	int ret, ovcs_id, save_id;
+	int ret, ovcs_id, save_ovcs_id;
 
 	/* unittest device must be in before state */
 	if (of_unittest_device_exists(unittest_nr, ovtype) != before) {
@@ -2057,7 +2057,7 @@ static int __init of_unittest_apply_revert_overlay_check(int overlay_nr,
 		return -EINVAL;
 	}
 
-	save_id = ovcs_id;
+	save_ovcs_id = ovcs_id;
 	ret = of_overlay_remove(&ovcs_id);
 	if (ret != 0) {
 		unittest(0, "%s failed to be destroyed @\"%s\"\n",
@@ -2065,7 +2065,7 @@ static int __init of_unittest_apply_revert_overlay_check(int overlay_nr,
 				unittest_path(unittest_nr, ovtype));
 		return ret;
 	}
-	of_unittest_untrack_overlay(save_id);
+	of_unittest_untrack_overlay(save_ovcs_id);
 
 	/* unittest device must be again in before state */
 	if (of_unittest_device_exists(unittest_nr, PDEV_OVERLAY) != before) {
@@ -2192,7 +2192,7 @@ static void __init of_unittest_overlay_5(void)
 /* test overlay application in sequence */
 static void __init of_unittest_overlay_6(void)
 {
-	int i, ov_id[2], ovcs_id;
+	int i, save_ovcs_id[2], ovcs_id;
 	int overlay_nr = 6, unittest_nr = 6;
 	int before = 0, after = 1;
 	const char *overlay_name;
@@ -2225,8 +2225,8 @@ static void __init of_unittest_overlay_6(void)
 		unittest(0, "could not apply overlay \"%s\"\n", overlay_name);
 			return;
 	}
-	ov_id[0] = ovcs_id;
-	of_unittest_track_overlay(ov_id[0]);
+	save_ovcs_id[0] = ovcs_id;
+	of_unittest_track_overlay(ovcs_id);
 
 	EXPECT_END(KERN_INFO,
 		   "OF: overlay: WARNING: memory leak will occur if overlay removed, property: /testcase-data/overlay-node/test-bus/test-unittest6/status");
@@ -2242,8 +2242,8 @@ static void __init of_unittest_overlay_6(void)
 		unittest(0, "could not apply overlay \"%s\"\n", overlay_name);
 			return;
 	}
-	ov_id[1] = ovcs_id;
-	of_unittest_track_overlay(ov_id[1]);
+	save_ovcs_id[1] = ovcs_id;
+	of_unittest_track_overlay(ovcs_id);
 
 	EXPECT_END(KERN_INFO,
 		   "OF: overlay: WARNING: memory leak will occur if overlay removed, property: /testcase-data/overlay-node/test-bus/test-unittest7/status");
@@ -2263,7 +2263,7 @@ static void __init of_unittest_overlay_6(void)
 	}
 
 	for (i = 1; i >= 0; i--) {
-		ovcs_id = ov_id[i];
+		ovcs_id = save_ovcs_id[i];
 		if (of_overlay_remove(&ovcs_id)) {
 			unittest(0, "%s failed destroy @\"%s\"\n",
 					overlay_name_from_nr(overlay_nr + i),
@@ -2271,7 +2271,7 @@ static void __init of_unittest_overlay_6(void)
 						PDEV_OVERLAY));
 			return;
 		}
-		of_unittest_untrack_overlay(ov_id[i]);
+		of_unittest_untrack_overlay(save_ovcs_id[i]);
 	}
 
 	for (i = 0; i < 2; i++) {
@@ -2294,7 +2294,7 @@ static void __init of_unittest_overlay_6(void)
 /* test overlay application in sequence */
 static void __init of_unittest_overlay_8(void)
 {
-	int i, ov_id[2], ovcs_id;
+	int i, save_ovcs_id[2], ovcs_id;
 	int overlay_nr = 8, unittest_nr = 8;
 	const char *overlay_name;
 	int ret;
@@ -2316,8 +2316,8 @@ static void __init of_unittest_overlay_8(void)
 	if (!ret)
 		return;
 
-	ov_id[0] = ovcs_id;
-	of_unittest_track_overlay(ov_id[0]);
+	save_ovcs_id[0] = ovcs_id;
+	of_unittest_track_overlay(ovcs_id);
 
 	overlay_name = overlay_name_from_nr(overlay_nr + 1);
 
@@ -2335,11 +2335,11 @@ static void __init of_unittest_overlay_8(void)
 		return;
 	}
 
-	ov_id[1] = ovcs_id;
-	of_unittest_track_overlay(ov_id[1]);
+	save_ovcs_id[1] = ovcs_id;
+	of_unittest_track_overlay(ovcs_id);
 
 	/* now try to remove first overlay (it should fail) */
-	ovcs_id = ov_id[0];
+	ovcs_id = save_ovcs_id[0];
 
 	EXPECT_BEGIN(KERN_INFO,
 		     "OF: overlay: node_overlaps_later_cs: #6 overlaps with #7 @/testcase-data/overlay-node/test-bus/test-unittest8");
@@ -2365,7 +2365,7 @@ static void __init of_unittest_overlay_8(void)
 
 	/* removing them in order should work */
 	for (i = 1; i >= 0; i--) {
-		ovcs_id = ov_id[i];
+		ovcs_id = save_ovcs_id[i];
 		if (of_overlay_remove(&ovcs_id)) {
 			unittest(0, "%s not destroyed @\"%s\"\n",
 					overlay_name_from_nr(overlay_nr + i),
@@ -2373,7 +2373,7 @@ static void __init of_unittest_overlay_8(void)
 						PDEV_OVERLAY));
 			return;
 		}
-		of_unittest_untrack_overlay(ov_id[i]);
+		of_unittest_untrack_overlay(save_ovcs_id[i]);
 	}
 
 	unittest(1, "overlay test %d passed\n", 8);
@@ -2837,7 +2837,7 @@ struct overlay_info {
 	uint8_t		*dtb_begin;
 	uint8_t		*dtb_end;
 	int		expected_result;
-	int		overlay_id;
+	int		ovcs_id;
 	char		*name;
 };
 
@@ -2991,7 +2991,7 @@ void __init unittest_unflatten_overlay_base(void)
  *
  * Return 0 on unexpected error.
  */
-static int __init overlay_data_apply(const char *overlay_name, int *overlay_id)
+static int __init overlay_data_apply(const char *overlay_name, int *ovcs_id)
 {
 	struct overlay_info *info;
 	int found = 0;
@@ -3013,9 +3013,9 @@ static int __init overlay_data_apply(const char *overlay_name, int *overlay_id)
 	if (!size)
 		pr_err("no overlay data for %s\n", overlay_name);
 
-	ret = of_overlay_fdt_apply(info->dtb_begin, size, &info->overlay_id);
-	if (overlay_id)
-		*overlay_id = info->overlay_id;
+	ret = of_overlay_fdt_apply(info->dtb_begin, size, &info->ovcs_id);
+	if (ovcs_id)
+		*ovcs_id = info->ovcs_id;
 	if (ret < 0)
 		goto out;
 
-- 
Frank Rowand <frank.rowand@sony.com>

[PATCH 2/2] of: unittest: re-implement overlay tracking

[frowand.list]

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

Some overlays are tracked when they are applied.  The tracked overlays
are later removed after the overlay tests are completed.  The old
implementation makes assumptions about the expected values for
overlay changeset id created by the overlay apply which result
in fragile code.  The new code removes the assumptions.

A symptom that exposes a problem with the tracking code is a
warning "UBSAN: shift-out-of-bounds in drivers/of/unittest.c:1933:36",
Kernel Version: 5.15-rc7, PPC-64, Talos II.  This results from variable
"id" value of -1 in the final line of of_unittest_untrack_overlay().

Reported-by: erhard_f@mailbox.org
Signed-off-by: Frank Rowand <frank.rowand@sony.com>
---
 drivers/of/unittest.c | 110 +++++++++++++++++++-----------------------
 1 file changed, 49 insertions(+), 61 deletions(-)

diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
index 33c458044600..e2807014cbe7 100644
--- a/drivers/of/unittest.c
+++ b/drivers/of/unittest.c
@@ -1657,7 +1657,7 @@ static void __init of_unittest_overlay_gpio(void)
 	 * The overlays are applied by overlay_data_apply()
 	 * instead of of_unittest_apply_overlay() so that they
 	 * will not be tracked.  Thus they will not be removed
-	 * by of_unittest_destroy_tracked_overlays().
+	 * by of_unittest_remove_tracked_overlays().
 	 *
 	 * - apply overlay_gpio_01
 	 * - apply overlay_gpio_02a
@@ -1905,86 +1905,70 @@ static const char *overlay_name_from_nr(int nr)
 
 static const char *bus_path = "/testcase-data/overlay-node/test-bus";
 
-/* FIXME: it is NOT guaranteed that overlay ids are assigned in sequence */
+#define MAX_TRACK_OVCS_IDS 256
 
-#define MAX_UNITTEST_OVERLAYS	256
-static unsigned long overlay_id_bits[BITS_TO_LONGS(MAX_UNITTEST_OVERLAYS)];
-static int overlay_first_id = -1;
+static int track_ovcs_id[MAX_TRACK_OVCS_IDS];
+static int track_ovcs_id_overlay_nr[MAX_TRACK_OVCS_IDS];
+static int track_ovcs_id_cnt;
 
-static long of_unittest_overlay_tracked(int id)
+static void of_unittest_track_overlay(int ovcs_id, int overlay_nr)
 {
-	if (WARN_ON(id >= MAX_UNITTEST_OVERLAYS))
-		return 0;
-	return overlay_id_bits[BIT_WORD(id)] & BIT_MASK(id);
-}
-
-static void of_unittest_track_overlay(int id)
-{
-	if (overlay_first_id < 0)
-		overlay_first_id = id;
-	id -= overlay_first_id;
-
-	if (WARN_ON(id >= MAX_UNITTEST_OVERLAYS))
+	if (WARN_ON(track_ovcs_id_cnt >= MAX_TRACK_OVCS_IDS))
 		return;
-	overlay_id_bits[BIT_WORD(id)] |= BIT_MASK(id);
+
+	track_ovcs_id[track_ovcs_id_cnt] = ovcs_id;
+	track_ovcs_id_overlay_nr[track_ovcs_id_cnt] = overlay_nr;
+	track_ovcs_id_cnt++;
 }
 
-static void of_unittest_untrack_overlay(int id)
+static void of_unittest_untrack_overlay(int ovcs_id)
 {
-	if (overlay_first_id < 0)
+	if (WARN_ON(track_ovcs_id_cnt < 1))
 		return;
-	id -= overlay_first_id;
-	if (WARN_ON(id >= MAX_UNITTEST_OVERLAYS))
-		return;
-	overlay_id_bits[BIT_WORD(id)] &= ~BIT_MASK(id);
-}
 
-static void of_unittest_destroy_tracked_overlays(void)
-{
-	int id, ret, defers, ovcs_id;
+	track_ovcs_id_cnt--;
 
-	if (overlay_first_id < 0)
-		return;
+	/* If out of synch then test is broken.  Do not try to recover. */
+	WARN_ON(track_ovcs_id[track_ovcs_id_cnt] != ovcs_id);
+}
 
-	/* try until no defers */
-	do {
-		defers = 0;
-		/* remove in reverse order */
-		for (id = MAX_UNITTEST_OVERLAYS - 1; id >= 0; id--) {
-			if (!of_unittest_overlay_tracked(id))
-				continue;
+static void of_unittest_remove_tracked_overlays(void)
+{
+	int ret, ovcs_id, overlay_nr, save_ovcs_id;
+	const char *overlay_name;
 
-			ovcs_id = id + overlay_first_id;
-			ret = of_overlay_remove(&ovcs_id);
-			if (ret == -ENODEV) {
-				pr_warn("%s: no overlay to destroy for #%d\n",
-					__func__, id + overlay_first_id);
-				continue;
-			}
-			if (ret != 0) {
-				defers++;
-				pr_warn("%s: overlay destroy failed for #%d\n",
-					__func__, id + overlay_first_id);
-				continue;
-			}
+	while (track_ovcs_id_cnt > 0) {
 
-			of_unittest_untrack_overlay(id);
+		ovcs_id = track_ovcs_id[track_ovcs_id_cnt - 1];
+		overlay_nr = track_ovcs_id_overlay_nr[track_ovcs_id_cnt - 1];
+		save_ovcs_id = ovcs_id;
+		ret = of_overlay_remove(&ovcs_id);
+		if (ret == -ENODEV) {
+			overlay_name = overlay_name_from_nr(overlay_nr);
+			pr_warn("%s: of_overlay_remove() for overlay \"%s\" failed, ret = %d\n",
+				__func__, overlay_name, ret);
 		}
-	} while (defers > 0);
+		of_unittest_untrack_overlay(save_ovcs_id);
+	};
+
 }
 
 static int __init of_unittest_apply_overlay(int overlay_nr, int *ovcs_id)
 {
+	/*
+	 * The overlay will be tracked, thus it will be removed
+	 * by of_unittest_remove_tracked_overlays().
+	 */
+
 	const char *overlay_name;
 
 	overlay_name = overlay_name_from_nr(overlay_nr);
 
 	if (!overlay_data_apply(overlay_name, ovcs_id)) {
-		unittest(0, "could not apply overlay \"%s\"\n",
-				overlay_name);
+		unittest(0, "could not apply overlay \"%s\"\n", overlay_name);
 		return -EFAULT;
 	}
-	of_unittest_track_overlay(*ovcs_id);
+	of_unittest_track_overlay(*ovcs_id, overlay_nr);
 
 	return 0;
 }
@@ -2226,7 +2210,7 @@ static void __init of_unittest_overlay_6(void)
 			return;
 	}
 	save_ovcs_id[0] = ovcs_id;
-	of_unittest_track_overlay(ovcs_id);
+	of_unittest_track_overlay(ovcs_id, overlay_nr + 0);
 
 	EXPECT_END(KERN_INFO,
 		   "OF: overlay: WARNING: memory leak will occur if overlay removed, property: /testcase-data/overlay-node/test-bus/test-unittest6/status");
@@ -2243,7 +2227,7 @@ static void __init of_unittest_overlay_6(void)
 			return;
 	}
 	save_ovcs_id[1] = ovcs_id;
-	of_unittest_track_overlay(ovcs_id);
+	of_unittest_track_overlay(ovcs_id, overlay_nr + 1);
 
 	EXPECT_END(KERN_INFO,
 		   "OF: overlay: WARNING: memory leak will occur if overlay removed, property: /testcase-data/overlay-node/test-bus/test-unittest7/status");
@@ -2317,7 +2301,7 @@ static void __init of_unittest_overlay_8(void)
 		return;
 
 	save_ovcs_id[0] = ovcs_id;
-	of_unittest_track_overlay(ovcs_id);
+	of_unittest_track_overlay(ovcs_id, overlay_nr + 0);
 
 	overlay_name = overlay_name_from_nr(overlay_nr + 1);
 
@@ -2336,7 +2320,7 @@ static void __init of_unittest_overlay_8(void)
 	}
 
 	save_ovcs_id[1] = ovcs_id;
-	of_unittest_track_overlay(ovcs_id);
+	of_unittest_track_overlay(ovcs_id, overlay_nr + 1);
 
 	/* now try to remove first overlay (it should fail) */
 	ovcs_id = save_ovcs_id[0];
@@ -2356,6 +2340,10 @@ static void __init of_unittest_overlay_8(void)
 		   "OF: overlay: node_overlaps_later_cs: #6 overlaps with #7 @/testcase-data/overlay-node/test-bus/test-unittest8");
 
 	if (!ret) {
+		/*
+		 * Should never get here.  If we do, expect a lot of
+		 * subsequent tracking and overlay removal related errors.
+		 */
 		unittest(0, "%s was destroyed @\"%s\"\n",
 				overlay_name_from_nr(overlay_nr + 0),
 				unittest_path(unittest_nr,
@@ -2805,7 +2793,7 @@ static void __init of_unittest_overlay(void)
 
 	of_unittest_overlay_gpio();
 
-	of_unittest_destroy_tracked_overlays();
+	of_unittest_remove_tracked_overlays();
 
 out:
 	of_node_put(bus_np);
-- 
Frank Rowand <frank.rowand@sony.com>

Re: [PATCH 0/2] of: unittest: re-implement overlay tracking

[Frank Rowand]

Hi Erhard,

Can you please check whether this patch series fixes the shift-out-of-bounds
problem you reported?

-Frank


On 12/31/21 9:33 PM, frowand.list@gmail.com wrote:
> From: Frank Rowand <frank.rowand@sony.com>
> 
> Some overlays are tracked when they are applied.  The tracked overlays
> are later removed after the overlay tests are completed.  The old
> implementation makes assumptions about the expected values for
> overlay changeset id created by the overlay apply which result
> in fragile code.  The new code removes the assumptions.
> 
> A symptom that exposes a problem with the tracking code is a
> warning "UBSAN: shift-out-of-bounds in drivers/of/unittest.c:1933:36",
> Kernel Version: 5.15-rc7, PPC-64, Talos II.  This results from variable
> "id" value of -1 in the final line of of_unittest_untrack_overlay().
> 
> The first patch in the series cleans up the inconsistent use of overlay
> changeset id and the obsolete overlay id.  The id is a core concept in
> the overlay tracking that is re-implemented in the second patch in
> the series.
> 
> Frank Rowand (2):
>   of: unittest: change references to obsolete overlay id
>   of: unittest: re-implement overlay tracking
> 
>  drivers/of/unittest.c | 154 +++++++++++++++++++-----------------------
>  1 file changed, 71 insertions(+), 83 deletions(-)
> 

Re: [PATCH 1/2] of: unittest: change references to obsolete overlay id

[Rob Herring]

On Fri, 31 Dec 2021 21:33:28 -0600, frowand.list@gmail.com wrote:
> From: Frank Rowand <frank.rowand@sony.com>
> 
> Unittest inconsistently interchanges overlay changeset id and
> overlay id.  Change variable names of overlay id to overlay
> changeset id.
> 
> Do not fix variable names in the overlay tracking functions
> of_unittest_overlay_tracked(), of_unittest_track_overlay(), and
> of_unittest_destroy_tracked_overlays() which will be replaced in
> a following commit.
> 
> Signed-off-by: Frank Rowand <frank.rowand@sony.com>
> ---
>  drivers/of/unittest.c | 54 +++++++++++++++++++++----------------------
>  1 file changed, 27 insertions(+), 27 deletions(-)
> 

Applied, thanks!

Re: [PATCH 2/2] of: unittest: re-implement overlay tracking

[Rob Herring]

On Fri, 31 Dec 2021 21:33:29 -0600, frowand.list@gmail.com wrote:
> From: Frank Rowand <frank.rowand@sony.com>
> 
> Some overlays are tracked when they are applied.  The tracked overlays
> are later removed after the overlay tests are completed.  The old
> implementation makes assumptions about the expected values for
> overlay changeset id created by the overlay apply which result
> in fragile code.  The new code removes the assumptions.
> 
> A symptom that exposes a problem with the tracking code is a
> warning "UBSAN: shift-out-of-bounds in drivers/of/unittest.c:1933:36",
> Kernel Version: 5.15-rc7, PPC-64, Talos II.  This results from variable
> "id" value of -1 in the final line of of_unittest_untrack_overlay().
> 
> Reported-by: erhard_f@mailbox.org
> Signed-off-by: Frank Rowand <frank.rowand@sony.com>
> ---
>  drivers/of/unittest.c | 110 +++++++++++++++++++-----------------------
>  1 file changed, 49 insertions(+), 61 deletions(-)
> 

Applied, thanks!