From: Frank Rowand <[email protected]>
New unittests for overlay notifiers reveal a memory leak in
of_overlay_apply() when a notifier returns an error for action
OF_OVERLAY_POST_APPLY. The pr_err() message is:
OF: ERROR: memory leak, expected refcount 1 instead of 3,
of_node_get()/of_node_put() unbalanced - destroy cset entry: attach
overlay node /testcase-data/overlay-node/test-bus/test-unittest17
Change the error path to no longer call free_overlay_changeset(),
and document that the caller of of_overlay_fdt_apply() may choose
to remove the overlay.
Update the unittest that triggered the error to expect the changed
return values and to call of_overlay_remove().
Signed-off-by: Frank Rowand <[email protected]>
---
Changes since version 1:
- patch 1/1 v1 did not apply on Rob's dt/next branch, rebase on top of
5f756a2eaa44 of: overlay: do not break notify on NOTIFY_{OK|STOP}
Output of the new overlay notifier unittests, as filtered by
scripts/dtc/of_unittest_expect:
### dt-test ### pass of_unittest_overlay_notify():2825
ok OF: overlay: overlay changeset pre-apply notifier error -16, target: /testcase-data/overlay-node/test-bus
### dt-test ### pass of_unittest_overlay_notify():2846
### dt-test ### pass of_unittest_overlay_notify():2851
ok OF: overlay: overlay changeset post-apply notifier error -17, target: /testcase-data/overlay-node/test-bus
### dt-test ### pass of_unittest_overlay_notify():2857
### dt-test ### pass of_unittest_overlay_notify():2862
### dt-test ### pass of_unittest_overlay_notify():2866
### dt-test ### pass of_unittest_overlay_notify():2872
### dt-test ### pass of_unittest_overlay_notify():2875
ok OF: overlay: overlay changeset pre-remove notifier error -18, target: /testcase-data/overlay-node/test-bus
### dt-test ### pass of_unittest_overlay_notify():2886
### dt-test ### pass of_unittest_overlay_notify():2894
### dt-test ### pass of_unittest_overlay_notify():2898
### dt-test ### pass of_unittest_overlay_notify():2901
ok OF: overlay: overlay changeset post-remove notifier error -19, target: /testcase-data/overlay-node/test-bus
### dt-test ### pass of_unittest_overlay_notify():2908
### dt-test ### pass of_unittest_overlay_notify():2915
### dt-test ### pass of_unittest_overlay_notify():2920
### dt-test ### pass of_unittest_overlay_notify():2932
drivers/of/overlay.c | 29 ++++++++++++++++++++++++++---
drivers/of/unittest.c | 10 ++++++++--
2 files changed, 34 insertions(+), 5 deletions(-)
diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index ae5ea5b1079b..4044ddcb02c6 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -952,6 +952,25 @@ static int of_overlay_apply(struct overlay_changeset *ovcs)
return ret;
}
+/*
+ * of_overlay_fdt_apply() - Create and apply an overlay changeset
+ * @overlay_fdt: pointer to overlay FDT
+ * @overlay_fdt_size: number of bytes in @overlay_fdt
+ * @ret_ovcs_id: pointer for returning created changeset id
+ *
+ * Creates and applies an overlay changeset.
+ *
+ * See of_overlay_apply() for important behavior information.
+ *
+ * Return: 0 on success, or a negative error number. *@ret_ovcs_id is set to
+ * the value of overlay changeset id, which can be passed to of_overlay_remove()
+ * to remove the overlay.
+ *
+ * On error return, the changeset may be partially applied. This is especially
+ * likely if an OF_OVERLAY_POST_APPLY notifier returns an error. In this case
+ * the caller should call of_overlay_remove() with the value in *@ret_ovcs_id.
+ */
+
int of_overlay_fdt_apply(const void *overlay_fdt, u32 overlay_fdt_size,
int *ret_ovcs_id)
{
@@ -1019,15 +1038,19 @@ int of_overlay_fdt_apply(const void *overlay_fdt, u32 overlay_fdt_size,
ovcs->overlay_mem = overlay_mem;
ret = of_overlay_apply(ovcs);
- if (ret < 0)
- goto err_free_ovcs;
+ /*
+ * If of_overlay_apply() error, calling free_overlay_changeset() may
+ * result in a memory leak if the apply partly succeeded, so do NOT
+ * goto err_free_ovcs. Instead, the caller of of_overlay_fdt_apply()
+ * can call of_overlay_remove();
+ */
mutex_unlock(&of_mutex);
of_overlay_mutex_unlock();
*ret_ovcs_id = ovcs->id;
- return 0;
+ return ret;
err_free_ovcs:
free_overlay_changeset(ovcs);
diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
index e28c3df2c4c2..dff55ae09d97 100644
--- a/drivers/of/unittest.c
+++ b/drivers/of/unittest.c
@@ -2848,7 +2848,7 @@ static void __init of_unittest_overlay_notify(void)
EXPECT_END(KERN_INFO, "OF: overlay: overlay changeset pre-apply notifier error -16, target: /testcase-data/overlay-node/test-bus");
- unittest(!ovcs_id, "ovcs_id created for overlay_16\n");
+ unittest(ovcs_id, "ovcs_id not created for overlay_16\n");
/* --- overlay 17 --- */
@@ -2859,7 +2859,13 @@ static void __init of_unittest_overlay_notify(void)
EXPECT_END(KERN_INFO, "OF: overlay: overlay changeset post-apply notifier error -17, target: /testcase-data/overlay-node/test-bus");
- unittest(!ovcs_id, "ovcs_id created for overlay_17\n");
+ unittest(ovcs_id, "ovcs_id not created for overlay_17\n");
+
+ if (ovcs_id) {
+ ret = of_overlay_remove(&ovcs_id);
+ unittest(!ret,
+ "overlay_17 of_overlay_remove(), ret = %d\n", ret);
+ }
/* --- overlay 18 --- */
--
Frank Rowand <[email protected]>
On Mon, 02 May 2022 13:17:42 -0500, [email protected] wrote:
> From: Frank Rowand <[email protected]>
>
> New unittests for overlay notifiers reveal a memory leak in
> of_overlay_apply() when a notifier returns an error for action
> OF_OVERLAY_POST_APPLY. The pr_err() message is:
>
> OF: ERROR: memory leak, expected refcount 1 instead of 3,
> of_node_get()/of_node_put() unbalanced - destroy cset entry: attach
> overlay node /testcase-data/overlay-node/test-bus/test-unittest17
>
> Change the error path to no longer call free_overlay_changeset(),
> and document that the caller of of_overlay_fdt_apply() may choose
> to remove the overlay.
>
> Update the unittest that triggered the error to expect the changed
> return values and to call of_overlay_remove().
>
> Signed-off-by: Frank Rowand <[email protected]>
> ---
> Changes since version 1:
> - patch 1/1 v1 did not apply on Rob's dt/next branch, rebase on top of
> 5f756a2eaa44 of: overlay: do not break notify on NOTIFY_{OK|STOP}
>
> Output of the new overlay notifier unittests, as filtered by
> scripts/dtc/of_unittest_expect:
>
> ### dt-test ### pass of_unittest_overlay_notify():2825
> ok OF: overlay: overlay changeset pre-apply notifier error -16, target: /testcase-data/overlay-node/test-bus
> ### dt-test ### pass of_unittest_overlay_notify():2846
> ### dt-test ### pass of_unittest_overlay_notify():2851
> ok OF: overlay: overlay changeset post-apply notifier error -17, target: /testcase-data/overlay-node/test-bus
> ### dt-test ### pass of_unittest_overlay_notify():2857
> ### dt-test ### pass of_unittest_overlay_notify():2862
> ### dt-test ### pass of_unittest_overlay_notify():2866
> ### dt-test ### pass of_unittest_overlay_notify():2872
> ### dt-test ### pass of_unittest_overlay_notify():2875
> ok OF: overlay: overlay changeset pre-remove notifier error -18, target: /testcase-data/overlay-node/test-bus
> ### dt-test ### pass of_unittest_overlay_notify():2886
> ### dt-test ### pass of_unittest_overlay_notify():2894
> ### dt-test ### pass of_unittest_overlay_notify():2898
> ### dt-test ### pass of_unittest_overlay_notify():2901
> ok OF: overlay: overlay changeset post-remove notifier error -19, target: /testcase-data/overlay-node/test-bus
> ### dt-test ### pass of_unittest_overlay_notify():2908
> ### dt-test ### pass of_unittest_overlay_notify():2915
> ### dt-test ### pass of_unittest_overlay_notify():2920
> ### dt-test ### pass of_unittest_overlay_notify():2932
>
>
> drivers/of/overlay.c | 29 ++++++++++++++++++++++++++---
> drivers/of/unittest.c | 10 ++++++++--
> 2 files changed, 34 insertions(+), 5 deletions(-)
>
Applied, thanks!