[PATCH v2 0/2] of: overlay: rework overlay apply and remove kfree()s

[PATCH v2 0/2] of: overlay: rework overlay apply and remove kfree()s

[frowand.list]

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

Fix various kfree() issues related to of_overlay_apply().

The fixes revealed inconsist variable names for the same variable
across functions, resulting in difficulty understanding the code
that was being modified.  Doing both variable renaming and the
fixes results in a hard to review patch, so split into two patches.

The first patch in the series contains only variable renaming.
The second patch contains the kfree() related fixes.

Frank Rowand (2):
  of: overlay: rename variables to be consistent
  of: overlay: rework overlay apply and remove kfree()s

 Documentation/devicetree/overlay-notes.rst |  23 ++-
 drivers/of/overlay.c                       | 175 +++++++++++----------
 2 files changed, 115 insertions(+), 83 deletions(-)

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

[PATCH v2 1/2] of: overlay: rename variables to be consistent

[frowand.list]

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

Variables change name across function calls when there is not a good
reason to do so.  Fix by changing "fdt" to "new_fdt" and "tree" to
"overlay_tree".

The name disparity was confusing when creating the following commit.
The name changes are in this separate commit to make review of the
following commmit less complex.

Signed-off-by: Frank Rowand <frank.rowand@sony.com>
---
Changes since v1:
  - This patch is added to the series.

 drivers/of/overlay.c | 94 ++++++++++++++++++++++----------------------
 1 file changed, 47 insertions(+), 47 deletions(-)

diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index d80160cf34bb..f74aa9ff67aa 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -57,7 +57,7 @@ struct fragment {
  * struct overlay_changeset
  * @id:			changeset identifier
  * @ovcs_list:		list on which we are located
- * @fdt:		base of memory allocated to hold aligned FDT that was unflattened to create @overlay_tree
+ * @new_fdt:		Memory allocated to hold unflattened aligned FDT
  * @overlay_tree:	expanded device tree that contains the fragment nodes
  * @count:		count of fragment structures
  * @fragments:		fragment nodes in the overlay expanded device tree
@@ -67,7 +67,7 @@ struct fragment {
 struct overlay_changeset {
 	int id;
 	struct list_head ovcs_list;
-	const void *fdt;
+	const void *new_fdt;
 	struct device_node *overlay_tree;
 	int count;
 	struct fragment *fragments;
@@ -718,19 +718,20 @@ static struct device_node *find_target(struct device_node *info_node)
 
 /**
  * init_overlay_changeset() - initialize overlay changeset from overlay tree
- * @ovcs:	Overlay changeset to build
- * @fdt:	base of memory allocated to hold aligned FDT that was unflattened to create @tree
- * @tree:	Contains the overlay fragments and overlay fixup nodes
+ * @ovcs:		Overlay changeset to build
+ * @new_fdt:		Memory allocated to hold unflattened aligned FDT
+ * @overlay_tree:	Contains the overlay fragments and overlay fixup nodes
  *
  * Initialize @ovcs.  Populate @ovcs->fragments with node information from
- * the top level of @tree.  The relevant top level nodes are the fragment
- * nodes and the __symbols__ node.  Any other top level node will be ignored.
+ * the top level of @overlay_tree.  The relevant top level nodes are the
+ * fragment nodes and the __symbols__ node.  Any other top level node will
+ * be ignored.
  *
  * Return: 0 on success, -ENOMEM if memory allocation failure, -EINVAL if error
- * detected in @tree, or -ENOSPC if idr_alloc() error.
+ * detected in @overlay_tree, or -ENOSPC if idr_alloc() error.
  */
 static int init_overlay_changeset(struct overlay_changeset *ovcs,
-		const void *fdt, struct device_node *tree)
+		const void *new_fdt, struct device_node *overlay_tree)
 {
 	struct device_node *node, *overlay_node;
 	struct fragment *fragment;
@@ -741,17 +742,17 @@ static int init_overlay_changeset(struct overlay_changeset *ovcs,
 	 * Warn for some issues.  Can not return -EINVAL for these until
 	 * of_unittest_apply_overlay() is fixed to pass these checks.
 	 */
-	if (!of_node_check_flag(tree, OF_DYNAMIC))
-		pr_debug("%s() tree is not dynamic\n", __func__);
+	if (!of_node_check_flag(overlay_tree, OF_DYNAMIC))
+		pr_debug("%s() overlay_tree is not dynamic\n", __func__);
 
-	if (!of_node_check_flag(tree, OF_DETACHED))
-		pr_debug("%s() tree is not detached\n", __func__);
+	if (!of_node_check_flag(overlay_tree, OF_DETACHED))
+		pr_debug("%s() overlay_tree is not detached\n", __func__);
 
-	if (!of_node_is_root(tree))
-		pr_debug("%s() tree is not root\n", __func__);
+	if (!of_node_is_root(overlay_tree))
+		pr_debug("%s() overlay_tree is not root\n", __func__);
 
-	ovcs->overlay_tree = tree;
-	ovcs->fdt = fdt;
+	ovcs->overlay_tree = overlay_tree;
+	ovcs->new_fdt = new_fdt;
 
 	INIT_LIST_HEAD(&ovcs->ovcs_list);
 
@@ -764,7 +765,7 @@ static int init_overlay_changeset(struct overlay_changeset *ovcs,
 	cnt = 0;
 
 	/* fragment nodes */
-	for_each_child_of_node(tree, node) {
+	for_each_child_of_node(overlay_tree, node) {
 		overlay_node = of_get_child_by_name(node, "__overlay__");
 		if (overlay_node) {
 			cnt++;
@@ -772,7 +773,7 @@ static int init_overlay_changeset(struct overlay_changeset *ovcs,
 		}
 	}
 
-	node = of_get_child_by_name(tree, "__symbols__");
+	node = of_get_child_by_name(overlay_tree, "__symbols__");
 	if (node) {
 		cnt++;
 		of_node_put(node);
@@ -785,7 +786,7 @@ static int init_overlay_changeset(struct overlay_changeset *ovcs,
 	}
 
 	cnt = 0;
-	for_each_child_of_node(tree, node) {
+	for_each_child_of_node(overlay_tree, node) {
 		overlay_node = of_get_child_by_name(node, "__overlay__");
 		if (!overlay_node)
 			continue;
@@ -807,7 +808,7 @@ static int init_overlay_changeset(struct overlay_changeset *ovcs,
 	 * if there is a symbols fragment in ovcs->fragments[i] it is
 	 * the final element in the array
 	 */
-	node = of_get_child_by_name(tree, "__symbols__");
+	node = of_get_child_by_name(overlay_tree, "__symbols__");
 	if (node) {
 		ovcs->symbols_fragment = 1;
 		fragment = &fragments[cnt];
@@ -862,11 +863,11 @@ static void free_overlay_changeset(struct overlay_changeset *ovcs)
 	kfree(ovcs->fragments);
 	/*
 	 * There should be no live pointers into ovcs->overlay_tree and
-	 * ovcs->fdt due to the policy that overlay notifiers are not allowed
-	 * to retain pointers into the overlay devicetree.
+	 * ovcs->new_fdt due to the policy that overlay notifiers are not
+	 * allowed to retain pointers into the overlay devicetree.
 	 */
 	kfree(ovcs->overlay_tree);
-	kfree(ovcs->fdt);
+	kfree(ovcs->new_fdt);
 	kfree(ovcs);
 }
 
@@ -874,16 +875,15 @@ static void free_overlay_changeset(struct overlay_changeset *ovcs)
  * internal documentation
  *
  * of_overlay_apply() - Create and apply an overlay changeset
- * @fdt:	base of memory allocated to hold the aligned FDT
- * @tree:	Expanded overlay device tree
- * @ovcs_id:	Pointer to overlay changeset id
+ * @new_fdt:		Memory allocated to hold the aligned FDT
+ * @overlay_tree:	Expanded overlay device tree
+ * @ovcs_id:		Pointer to overlay changeset id
  *
  * Creates and applies an overlay changeset.
  *
  * If an error occurs in a pre-apply notifier, then no changes are made
  * to the device tree.
  *
-
  * A non-zero return value will not have created the changeset if error is from:
  *   - parameter checks
  *   - building the changeset
@@ -913,29 +913,29 @@ static void free_overlay_changeset(struct overlay_changeset *ovcs)
  * id is returned to *ovcs_id.
  */
 
-static int of_overlay_apply(const void *fdt, struct device_node *tree,
-		int *ovcs_id)
+static int of_overlay_apply(const void *new_fdt,
+		struct device_node *overlay_tree, int *ovcs_id)
 {
 	struct overlay_changeset *ovcs;
 	int ret = 0, ret_revert, ret_tmp;
 
 	/*
-	 * As of this point, fdt and tree belong to the overlay changeset.
-	 * overlay changeset code is responsible for freeing them.
+	 * As of this point, new_fdt and overlay_tree belong to the overlay
+	 * changeset.  overlay changeset code is responsible for freeing them.
 	 */
 
 	if (devicetree_corrupt()) {
 		pr_err("devicetree state suspect, refuse to apply overlay\n");
-		kfree(fdt);
-		kfree(tree);
+		kfree(new_fdt);
+		kfree(overlay_tree);
 		ret = -EBUSY;
 		goto out;
 	}
 
 	ovcs = kzalloc(sizeof(*ovcs), GFP_KERNEL);
 	if (!ovcs) {
-		kfree(fdt);
-		kfree(tree);
+		kfree(new_fdt);
+		kfree(overlay_tree);
 		ret = -ENOMEM;
 		goto out;
 	}
@@ -943,20 +943,20 @@ static int of_overlay_apply(const void *fdt, struct device_node *tree,
 	of_overlay_mutex_lock();
 	mutex_lock(&of_mutex);
 
-	ret = of_resolve_phandles(tree);
+	ret = of_resolve_phandles(overlay_tree);
 	if (ret)
-		goto err_free_tree;
+		goto err_free_overlay_tree;
 
-	ret = init_overlay_changeset(ovcs, fdt, tree);
+	ret = init_overlay_changeset(ovcs, new_fdt, overlay_tree);
 	if (ret)
-		goto err_free_tree;
+		goto err_free_overlay_tree;
 
 	/*
 	 * after overlay_notify(), ovcs->overlay_tree related pointers may have
-	 * leaked to drivers, so can not kfree() tree, aka ovcs->overlay_tree;
-	 * and can not free memory containing aligned fdt.  The aligned fdt
-	 * is contained within the memory at ovcs->fdt, possibly at an offset
-	 * from ovcs->fdt.
+	 * leaked to drivers, so can not kfree() overlay_tree,
+	 * aka ovcs->overlay_tree; and can not free memory containing aligned
+	 * fdt.  The aligned fdt is contained within the memory at
+	 * ovcs->new_fdt, possibly at an offset from ovcs->new_fdt.
 	 */
 	ret = overlay_notify(ovcs, OF_OVERLAY_PRE_APPLY);
 	if (ret) {
@@ -997,9 +997,9 @@ static int of_overlay_apply(const void *fdt, struct device_node *tree,
 
 	goto out_unlock;
 
-err_free_tree:
-	kfree(fdt);
-	kfree(tree);
+err_free_overlay_tree:
+	kfree(new_fdt);
+	kfree(overlay_tree);
 
 err_free_overlay_changeset:
 	free_overlay_changeset(ovcs);
-- 
Frank Rowand <frank.rowand@sony.com>

[PATCH v2 2/2] of: overlay: rework overlay apply and remove kfree()s

[frowand.list]

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

Fix various kfree() issues related to of_overlay_apply().
  - Double kfree() of fdt and tree when init_overlay_changeset()
    returns an error.
  - free_overlay_changeset() free the root of the unflattened
    overlay (variable tree) instead of the memory that contains
    the unflattened overlay.
  - For the case of a failure during applying an overlay, move kfree()
    of new_fdt and overlay_mem into the function that allocated them.
    For the case of removing an overlay, the kfree() remains in
    free_overlay_changeset().
  - Check return value of of_fdt_unflatten_tree() for error instead
    of checking the returnded value of overlay_root.

More clearly document policy related to lifetime of pointers into
overlay memory.

Double kfree()
Reported-by: Slawomir Stepien <slawomir.stepien@nokia.com>

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

Changes since v1:
  - Move kfree()s from init_overlay_changeset() to of_overlay_fdt_apply()
  - Better document lifetime of pointers into overlay, both in overlay.c
    and Documentation/devicetree/overlay-notes.rst

 Documentation/devicetree/overlay-notes.rst |  23 +++-
 drivers/of/overlay.c                       | 127 ++++++++++++---------
 2 files changed, 91 insertions(+), 59 deletions(-)

diff --git a/Documentation/devicetree/overlay-notes.rst b/Documentation/devicetree/overlay-notes.rst
index b2b8db765b8c..7a6e85f75567 100644
--- a/Documentation/devicetree/overlay-notes.rst
+++ b/Documentation/devicetree/overlay-notes.rst
@@ -119,10 +119,25 @@ Finally, if you need to remove all overlays in one-go, just call
 of_overlay_remove_all() which will remove every single one in the correct
 order.
 
-In addition, there is the option to register notifiers that get called on
+There is the option to register notifiers that get called on
 overlay operations. See of_overlay_notifier_register/unregister and
 enum of_overlay_notify_action for details.
 
-Note that a notifier callback is not supposed to store pointers to a device
-tree node or its content beyond OF_OVERLAY_POST_REMOVE corresponding to the
-respective node it received.
+A notifier callback for OF_OVERLAY_PRE_APPLY, OF_OVERLAY_POST_APPLY, or
+OF_OVERLAY_PRE_REMOVE may store pointers to a device tree node in the overlay
+or its content but these pointers must not persist past the notifier callback
+for OF_OVERLAY_POST_REMOVE.  The memory containing the overlay will be
+kfree()ed after OF_OVERLAY_POST_REMOVE notifiers are called.  Note that the
+memory will be kfree()ed even if the notifier for OF_OVERLAY_POST_REMOVE
+returns an error.
+
+The changeset notifiers in drivers/of/dynamic.c are a second type of notifier
+that could be triggered by applying or removing an overlay.  These notifiers
+are not allowed to store pointers to a device tree node in the overlay
+or its content.  The overlay code does not protect against such pointers
+remaining active when the memory containing the overlay is freed as a result
+of removing the overlay.
+
+Any other code that retains a pointer to the overlay nodes or data is
+considered to be a bug because after removing the overlay the pointer
+will refer to freed memory.
diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index f74aa9ff67aa..c8e999518f2f 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -58,6 +58,7 @@ struct fragment {
  * @id:			changeset identifier
  * @ovcs_list:		list on which we are located
  * @new_fdt:		Memory allocated to hold unflattened aligned FDT
+ * @overlay_mem:	the memory chunk that contains @overlay_tree
  * @overlay_tree:	expanded device tree that contains the fragment nodes
  * @count:		count of fragment structures
  * @fragments:		fragment nodes in the overlay expanded device tree
@@ -68,6 +69,7 @@ struct overlay_changeset {
 	int id;
 	struct list_head ovcs_list;
 	const void *new_fdt;
+	const void *overlay_mem;
 	struct device_node *overlay_tree;
 	int count;
 	struct fragment *fragments;
@@ -720,18 +722,20 @@ static struct device_node *find_target(struct device_node *info_node)
  * init_overlay_changeset() - initialize overlay changeset from overlay tree
  * @ovcs:		Overlay changeset to build
  * @new_fdt:		Memory allocated to hold unflattened aligned FDT
+ * @tree_mem:		Memory that contains @overlay_tree
  * @overlay_tree:	Contains the overlay fragments and overlay fixup nodes
  *
  * Initialize @ovcs.  Populate @ovcs->fragments with node information from
  * the top level of @overlay_tree.  The relevant top level nodes are the
  * fragment nodes and the __symbols__ node.  Any other top level node will
- * be ignored.
+ * be ignored.  Populate other @ovcs fields.
  *
  * Return: 0 on success, -ENOMEM if memory allocation failure, -EINVAL if error
  * detected in @overlay_tree, or -ENOSPC if idr_alloc() error.
  */
 static int init_overlay_changeset(struct overlay_changeset *ovcs,
-		const void *new_fdt, struct device_node *overlay_tree)
+		const void *new_fdt, const void *tree_mem,
+		struct device_node *overlay_tree)
 {
 	struct device_node *node, *overlay_node;
 	struct fragment *fragment;
@@ -751,9 +755,6 @@ static int init_overlay_changeset(struct overlay_changeset *ovcs,
 	if (!of_node_is_root(overlay_tree))
 		pr_debug("%s() overlay_tree is not root\n", __func__);
 
-	ovcs->overlay_tree = overlay_tree;
-	ovcs->new_fdt = new_fdt;
-
 	INIT_LIST_HEAD(&ovcs->ovcs_list);
 
 	of_changeset_init(&ovcs->cset);
@@ -832,6 +833,9 @@ static int init_overlay_changeset(struct overlay_changeset *ovcs,
 
 	ovcs->id = id;
 	ovcs->count = cnt;
+	ovcs->new_fdt = new_fdt;
+	ovcs->overlay_mem = tree_mem;
+	ovcs->overlay_tree = overlay_tree;
 	ovcs->fragments = fragments;
 
 	return 0;
@@ -846,7 +850,7 @@ static int init_overlay_changeset(struct overlay_changeset *ovcs,
 	return ret;
 }
 
-static void free_overlay_changeset(struct overlay_changeset *ovcs)
+static void free_overlay_changeset_contents(struct overlay_changeset *ovcs)
 {
 	int i;
 
@@ -861,12 +865,20 @@ static void free_overlay_changeset(struct overlay_changeset *ovcs)
 		of_node_put(ovcs->fragments[i].overlay);
 	}
 	kfree(ovcs->fragments);
+}
+static void free_overlay_changeset(struct overlay_changeset *ovcs)
+{
+
+	free_overlay_changeset_contents(ovcs);
+
 	/*
-	 * There should be no live pointers into ovcs->overlay_tree and
+	 * There should be no live pointers into ovcs->overlay_mem and
 	 * ovcs->new_fdt due to the policy that overlay notifiers are not
-	 * allowed to retain pointers into the overlay devicetree.
+	 * allowed to retain pointers into the overlay devicetree other
+	 * than the window between OF_OVERLAY_PRE_APPLY overlay notifiers
+	 * and the OF_OVERLAY_POST_REMOVE overlay notifiers.
 	 */
-	kfree(ovcs->overlay_tree);
+	kfree(ovcs->overlay_mem);
 	kfree(ovcs->new_fdt);
 	kfree(ovcs);
 }
@@ -876,8 +888,10 @@ static void free_overlay_changeset(struct overlay_changeset *ovcs)
  *
  * of_overlay_apply() - Create and apply an overlay changeset
  * @new_fdt:		Memory allocated to hold the aligned FDT
+ * @tree_mem:		Memory that contains @overlay_tree
  * @overlay_tree:	Expanded overlay device tree
  * @ovcs_id:		Pointer to overlay changeset id
+ * @kfree_unsafe:	Pointer to flag to not kfree() @new_fdt and @overlay_tree
  *
  * Creates and applies an overlay changeset.
  *
@@ -910,34 +924,25 @@ static void free_overlay_changeset(struct overlay_changeset *ovcs)
  * refused.
  *
  * Returns 0 on success, or a negative error number.  Overlay changeset
- * id is returned to *ovcs_id.
+ * id is returned to *ovcs_id.  When references to @new_fdt and @overlay_tree
+ * may exist, *kfree_unsafe is set to true.
  */
 
-static int of_overlay_apply(const void *new_fdt,
-		struct device_node *overlay_tree, int *ovcs_id)
+static int of_overlay_apply(const void *new_fdt, void *tree_mem,
+		struct device_node *overlay_tree, int *ovcs_id,
+		bool *kfree_unsafe)
 {
 	struct overlay_changeset *ovcs;
 	int ret = 0, ret_revert, ret_tmp;
 
-	/*
-	 * As of this point, new_fdt and overlay_tree belong to the overlay
-	 * changeset.  overlay changeset code is responsible for freeing them.
-	 */
-
 	if (devicetree_corrupt()) {
 		pr_err("devicetree state suspect, refuse to apply overlay\n");
-		kfree(new_fdt);
-		kfree(overlay_tree);
-		ret = -EBUSY;
-		goto out;
+		return -EBUSY;
 	}
 
 	ovcs = kzalloc(sizeof(*ovcs), GFP_KERNEL);
 	if (!ovcs) {
-		kfree(new_fdt);
-		kfree(overlay_tree);
-		ret = -ENOMEM;
-		goto out;
+		return -ENOMEM;
 	}
 
 	of_overlay_mutex_lock();
@@ -945,28 +950,27 @@ static int of_overlay_apply(const void *new_fdt,
 
 	ret = of_resolve_phandles(overlay_tree);
 	if (ret)
-		goto err_free_overlay_tree;
+		goto err_free_ovcs;
 
-	ret = init_overlay_changeset(ovcs, new_fdt, overlay_tree);
+	ret = init_overlay_changeset(ovcs, new_fdt, tree_mem, overlay_tree);
 	if (ret)
-		goto err_free_overlay_tree;
+		goto err_free_ovcs_contents;
 
 	/*
-	 * after overlay_notify(), ovcs->overlay_tree related pointers may have
-	 * leaked to drivers, so can not kfree() overlay_tree,
-	 * aka ovcs->overlay_tree; and can not free memory containing aligned
-	 * fdt.  The aligned fdt is contained within the memory at
-	 * ovcs->new_fdt, possibly at an offset from ovcs->new_fdt.
+	 * After overlay_notify(), ovcs->overlay_tree related pointers may have
+	 * leaked to drivers, so can not kfree() ovcs->overlay_mem and
+	 * ovcs->new_fdt until after OF_OVERLAY_POST_REMOVE notifiers.
 	 */
+	*kfree_unsafe = true;
 	ret = overlay_notify(ovcs, OF_OVERLAY_PRE_APPLY);
 	if (ret) {
 		pr_err("overlay changeset pre-apply notify error %d\n", ret);
-		goto err_free_overlay_changeset;
+		goto err_free_ovcs_contents;
 	}
 
 	ret = build_changeset(ovcs);
 	if (ret)
-		goto err_free_overlay_changeset;
+		goto err_free_ovcs_contents;
 
 	ret_revert = 0;
 	ret = __of_changeset_apply_entries(&ovcs->cset, &ret_revert);
@@ -976,7 +980,7 @@ static int of_overlay_apply(const void *new_fdt,
 				 ret_revert);
 			devicetree_state_flags |= DTSF_APPLY_FAIL;
 		}
-		goto err_free_overlay_changeset;
+		goto err_free_ovcs_contents;
 	}
 
 	ret = __of_changeset_apply_notify(&ovcs->cset);
@@ -997,18 +1001,16 @@ static int of_overlay_apply(const void *new_fdt,
 
 	goto out_unlock;
 
-err_free_overlay_tree:
-	kfree(new_fdt);
-	kfree(overlay_tree);
+err_free_ovcs_contents:
+	free_overlay_changeset_contents(ovcs);
 
-err_free_overlay_changeset:
-	free_overlay_changeset(ovcs);
+err_free_ovcs:
+	kfree(ovcs);
 
 out_unlock:
 	mutex_unlock(&of_mutex);
 	of_overlay_mutex_unlock();
 
-out:
 	pr_debug("%s() err=%d\n", __func__, ret);
 
 	return ret;
@@ -1019,11 +1021,14 @@ int of_overlay_fdt_apply(const void *overlay_fdt, u32 overlay_fdt_size,
 {
 	void *new_fdt;
 	void *new_fdt_align;
+	void *overlay_mem;
+	bool kfree_unsafe;
 	int ret;
 	u32 size;
 	struct device_node *overlay_root = NULL;
 
 	*ovcs_id = 0;
+	kfree_unsafe = false;
 
 	if (overlay_fdt_size < sizeof(struct fdt_header) ||
 	    fdt_check_header(overlay_fdt)) {
@@ -1046,30 +1051,37 @@ int of_overlay_fdt_apply(const void *overlay_fdt, u32 overlay_fdt_size,
 	new_fdt_align = PTR_ALIGN(new_fdt, FDT_ALIGN_SIZE);
 	memcpy(new_fdt_align, overlay_fdt, size);
 
-	of_fdt_unflatten_tree(new_fdt_align, NULL, &overlay_root);
-	if (!overlay_root) {
+	overlay_mem = of_fdt_unflatten_tree(new_fdt_align, NULL, &overlay_root);
+	if (!overlay_mem) {
 		pr_err("unable to unflatten overlay_fdt\n");
 		ret = -EINVAL;
 		goto out_free_new_fdt;
 	}
 
-	ret = of_overlay_apply(new_fdt, overlay_root, ovcs_id);
-	if (ret < 0) {
-		/*
-		 * new_fdt and overlay_root now belong to the overlay
-		 * changeset.
-		 * overlay changeset code is responsible for freeing them.
-		 */
-		goto out;
-	}
+	ret = of_overlay_apply(new_fdt, overlay_mem, overlay_root, ovcs_id,
+			&kfree_unsafe);
+	if (ret < 0)
+		goto out_free_overlay_mem;
 
+	/*
+	 * new_fdt and overlay_mem now belong to the overlay changeset.
+	 * free_overlay_changeset() is responsible for freeing them.
+	 */
 	return 0;
 
+	/*
+	 * After overlay_notify(), ovcs->overlay_tree related pointers may have
+	 * leaked to drivers, so can not kfree() overlay_mem and new_fdt.  This
+	 * will result in a memory leak.
+	 */
+out_free_overlay_mem:
+	if (!kfree_unsafe)
+		kfree(overlay_mem);
 
 out_free_new_fdt:
-	kfree(new_fdt);
+	if (!kfree_unsafe)
+		kfree(new_fdt);
 
-out:
 	return ret;
 }
 EXPORT_SYMBOL_GPL(of_overlay_fdt_apply);
@@ -1237,6 +1249,11 @@ int of_overlay_remove(int *ovcs_id)
 
 	*ovcs_id = 0;
 
+	/*
+	 * Note that the overlay memory will be kfree()ed by
+	 * free_overlay_changeset() even if the notifier for
+	 * OF_OVERLAY_POST_REMOVE returns an error.
+	 */
 	ret_tmp = overlay_notify(ovcs, OF_OVERLAY_POST_REMOVE);
 	if (ret_tmp) {
 		pr_err("overlay changeset post-remove notify error %d\n",
-- 
Frank Rowand <frank.rowand@sony.com>

Re: [PATCH v2 2/2] of: overlay: rework overlay apply and remove kfree()s

[Frank Rowand]

On 4/10/22 16:08, frowand.list@gmail.com wrote:
> From: Frank Rowand <frank.rowand@sony.com>
> 
> Fix various kfree() issues related to of_overlay_apply().
>   - Double kfree() of fdt and tree when init_overlay_changeset()
>     returns an error.
>   - free_overlay_changeset() free the root of the unflattened
>     overlay (variable tree) instead of the memory that contains
>     the unflattened overlay.
>   - For the case of a failure during applying an overlay, move kfree()
>     of new_fdt and overlay_mem into the function that allocated them.
>     For the case of removing an overlay, the kfree() remains in
>     free_overlay_changeset().
>   - Check return value of of_fdt_unflatten_tree() for error instead
>     of checking the returnded value of overlay_root.
> 
> More clearly document policy related to lifetime of pointers into
> overlay memory.
> 
> Double kfree()
> Reported-by: Slawomir Stepien <slawomir.stepien@nokia.com>

I should have also added:

Fixes: 83ef4777f5ff ("of: overlay: Stop leaking resources on overlay removal")

-Frank

> 
> Signed-off-by: Frank Rowand <frank.rowand@sony.com>
> ---
> 
> Changes since v1:
>   - Move kfree()s from init_overlay_changeset() to of_overlay_fdt_apply()
>   - Better document lifetime of pointers into overlay, both in overlay.c
>     and Documentation/devicetree/overlay-notes.rst
> 
>  Documentation/devicetree/overlay-notes.rst |  23 +++-
>  drivers/of/overlay.c                       | 127 ++++++++++++---------
>  2 files changed, 91 insertions(+), 59 deletions(-)
> 
> diff --git a/Documentation/devicetree/overlay-notes.rst b/Documentation/devicetree/overlay-notes.rst
> index b2b8db765b8c..7a6e85f75567 100644
> --- a/Documentation/devicetree/overlay-notes.rst
> +++ b/Documentation/devicetree/overlay-notes.rst
> @@ -119,10 +119,25 @@ Finally, if you need to remove all overlays in one-go, just call
>  of_overlay_remove_all() which will remove every single one in the correct
>  order.
>  
> -In addition, there is the option to register notifiers that get called on
> +There is the option to register notifiers that get called on
>  overlay operations. See of_overlay_notifier_register/unregister and
>  enum of_overlay_notify_action for details.
>  
> -Note that a notifier callback is not supposed to store pointers to a device
> -tree node or its content beyond OF_OVERLAY_POST_REMOVE corresponding to the
> -respective node it received.
> +A notifier callback for OF_OVERLAY_PRE_APPLY, OF_OVERLAY_POST_APPLY, or
> +OF_OVERLAY_PRE_REMOVE may store pointers to a device tree node in the overlay
> +or its content but these pointers must not persist past the notifier callback
> +for OF_OVERLAY_POST_REMOVE.  The memory containing the overlay will be
> +kfree()ed after OF_OVERLAY_POST_REMOVE notifiers are called.  Note that the
> +memory will be kfree()ed even if the notifier for OF_OVERLAY_POST_REMOVE
> +returns an error.
> +
> +The changeset notifiers in drivers/of/dynamic.c are a second type of notifier
> +that could be triggered by applying or removing an overlay.  These notifiers
> +are not allowed to store pointers to a device tree node in the overlay
> +or its content.  The overlay code does not protect against such pointers
> +remaining active when the memory containing the overlay is freed as a result
> +of removing the overlay.
> +
> +Any other code that retains a pointer to the overlay nodes or data is
> +considered to be a bug because after removing the overlay the pointer
> +will refer to freed memory.
> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
> index f74aa9ff67aa..c8e999518f2f 100644
> --- a/drivers/of/overlay.c
> +++ b/drivers/of/overlay.c
> @@ -58,6 +58,7 @@ struct fragment {
>   * @id:			changeset identifier
>   * @ovcs_list:		list on which we are located
>   * @new_fdt:		Memory allocated to hold unflattened aligned FDT
> + * @overlay_mem:	the memory chunk that contains @overlay_tree
>   * @overlay_tree:	expanded device tree that contains the fragment nodes
>   * @count:		count of fragment structures
>   * @fragments:		fragment nodes in the overlay expanded device tree
> @@ -68,6 +69,7 @@ struct overlay_changeset {
>  	int id;
>  	struct list_head ovcs_list;
>  	const void *new_fdt;
> +	const void *overlay_mem;
>  	struct device_node *overlay_tree;
>  	int count;
>  	struct fragment *fragments;
> @@ -720,18 +722,20 @@ static struct device_node *find_target(struct device_node *info_node)
>   * init_overlay_changeset() - initialize overlay changeset from overlay tree
>   * @ovcs:		Overlay changeset to build
>   * @new_fdt:		Memory allocated to hold unflattened aligned FDT
> + * @tree_mem:		Memory that contains @overlay_tree
>   * @overlay_tree:	Contains the overlay fragments and overlay fixup nodes
>   *
>   * Initialize @ovcs.  Populate @ovcs->fragments with node information from
>   * the top level of @overlay_tree.  The relevant top level nodes are the
>   * fragment nodes and the __symbols__ node.  Any other top level node will
> - * be ignored.
> + * be ignored.  Populate other @ovcs fields.
>   *
>   * Return: 0 on success, -ENOMEM if memory allocation failure, -EINVAL if error
>   * detected in @overlay_tree, or -ENOSPC if idr_alloc() error.
>   */
>  static int init_overlay_changeset(struct overlay_changeset *ovcs,
> -		const void *new_fdt, struct device_node *overlay_tree)
> +		const void *new_fdt, const void *tree_mem,
> +		struct device_node *overlay_tree)
>  {
>  	struct device_node *node, *overlay_node;
>  	struct fragment *fragment;
> @@ -751,9 +755,6 @@ static int init_overlay_changeset(struct overlay_changeset *ovcs,
>  	if (!of_node_is_root(overlay_tree))
>  		pr_debug("%s() overlay_tree is not root\n", __func__);
>  
> -	ovcs->overlay_tree = overlay_tree;
> -	ovcs->new_fdt = new_fdt;
> -
>  	INIT_LIST_HEAD(&ovcs->ovcs_list);
>  
>  	of_changeset_init(&ovcs->cset);
> @@ -832,6 +833,9 @@ static int init_overlay_changeset(struct overlay_changeset *ovcs,
>  
>  	ovcs->id = id;
>  	ovcs->count = cnt;
> +	ovcs->new_fdt = new_fdt;
> +	ovcs->overlay_mem = tree_mem;
> +	ovcs->overlay_tree = overlay_tree;
>  	ovcs->fragments = fragments;
>  
>  	return 0;
> @@ -846,7 +850,7 @@ static int init_overlay_changeset(struct overlay_changeset *ovcs,
>  	return ret;
>  }
>  
> -static void free_overlay_changeset(struct overlay_changeset *ovcs)
> +static void free_overlay_changeset_contents(struct overlay_changeset *ovcs)
>  {
>  	int i;
>  
> @@ -861,12 +865,20 @@ static void free_overlay_changeset(struct overlay_changeset *ovcs)
>  		of_node_put(ovcs->fragments[i].overlay);
>  	}
>  	kfree(ovcs->fragments);
> +}
> +static void free_overlay_changeset(struct overlay_changeset *ovcs)
> +{
> +
> +	free_overlay_changeset_contents(ovcs);
> +
>  	/*
> -	 * There should be no live pointers into ovcs->overlay_tree and
> +	 * There should be no live pointers into ovcs->overlay_mem and
>  	 * ovcs->new_fdt due to the policy that overlay notifiers are not
> -	 * allowed to retain pointers into the overlay devicetree.
> +	 * allowed to retain pointers into the overlay devicetree other
> +	 * than the window between OF_OVERLAY_PRE_APPLY overlay notifiers
> +	 * and the OF_OVERLAY_POST_REMOVE overlay notifiers.
>  	 */
> -	kfree(ovcs->overlay_tree);
> +	kfree(ovcs->overlay_mem);
>  	kfree(ovcs->new_fdt);
>  	kfree(ovcs);
>  }
> @@ -876,8 +888,10 @@ static void free_overlay_changeset(struct overlay_changeset *ovcs)
>   *
>   * of_overlay_apply() - Create and apply an overlay changeset
>   * @new_fdt:		Memory allocated to hold the aligned FDT
> + * @tree_mem:		Memory that contains @overlay_tree
>   * @overlay_tree:	Expanded overlay device tree
>   * @ovcs_id:		Pointer to overlay changeset id
> + * @kfree_unsafe:	Pointer to flag to not kfree() @new_fdt and @overlay_tree
>   *
>   * Creates and applies an overlay changeset.
>   *
> @@ -910,34 +924,25 @@ static void free_overlay_changeset(struct overlay_changeset *ovcs)
>   * refused.
>   *
>   * Returns 0 on success, or a negative error number.  Overlay changeset
> - * id is returned to *ovcs_id.
> + * id is returned to *ovcs_id.  When references to @new_fdt and @overlay_tree
> + * may exist, *kfree_unsafe is set to true.
>   */
>  
> -static int of_overlay_apply(const void *new_fdt,
> -		struct device_node *overlay_tree, int *ovcs_id)
> +static int of_overlay_apply(const void *new_fdt, void *tree_mem,
> +		struct device_node *overlay_tree, int *ovcs_id,
> +		bool *kfree_unsafe)
>  {
>  	struct overlay_changeset *ovcs;
>  	int ret = 0, ret_revert, ret_tmp;
>  
> -	/*
> -	 * As of this point, new_fdt and overlay_tree belong to the overlay
> -	 * changeset.  overlay changeset code is responsible for freeing them.
> -	 */
> -
>  	if (devicetree_corrupt()) {
>  		pr_err("devicetree state suspect, refuse to apply overlay\n");
> -		kfree(new_fdt);
> -		kfree(overlay_tree);
> -		ret = -EBUSY;
> -		goto out;
> +		return -EBUSY;
>  	}
>  
>  	ovcs = kzalloc(sizeof(*ovcs), GFP_KERNEL);
>  	if (!ovcs) {
> -		kfree(new_fdt);
> -		kfree(overlay_tree);
> -		ret = -ENOMEM;
> -		goto out;
> +		return -ENOMEM;
>  	}
>  
>  	of_overlay_mutex_lock();
> @@ -945,28 +950,27 @@ static int of_overlay_apply(const void *new_fdt,
>  
>  	ret = of_resolve_phandles(overlay_tree);
>  	if (ret)
> -		goto err_free_overlay_tree;
> +		goto err_free_ovcs;
>  
> -	ret = init_overlay_changeset(ovcs, new_fdt, overlay_tree);
> +	ret = init_overlay_changeset(ovcs, new_fdt, tree_mem, overlay_tree);
>  	if (ret)
> -		goto err_free_overlay_tree;
> +		goto err_free_ovcs_contents;
>  
>  	/*
> -	 * after overlay_notify(), ovcs->overlay_tree related pointers may have
> -	 * leaked to drivers, so can not kfree() overlay_tree,
> -	 * aka ovcs->overlay_tree; and can not free memory containing aligned
> -	 * fdt.  The aligned fdt is contained within the memory at
> -	 * ovcs->new_fdt, possibly at an offset from ovcs->new_fdt.
> +	 * After overlay_notify(), ovcs->overlay_tree related pointers may have
> +	 * leaked to drivers, so can not kfree() ovcs->overlay_mem and
> +	 * ovcs->new_fdt until after OF_OVERLAY_POST_REMOVE notifiers.
>  	 */
> +	*kfree_unsafe = true;
>  	ret = overlay_notify(ovcs, OF_OVERLAY_PRE_APPLY);
>  	if (ret) {
>  		pr_err("overlay changeset pre-apply notify error %d\n", ret);
> -		goto err_free_overlay_changeset;
> +		goto err_free_ovcs_contents;
>  	}
>  
>  	ret = build_changeset(ovcs);
>  	if (ret)
> -		goto err_free_overlay_changeset;
> +		goto err_free_ovcs_contents;
>  
>  	ret_revert = 0;
>  	ret = __of_changeset_apply_entries(&ovcs->cset, &ret_revert);
> @@ -976,7 +980,7 @@ static int of_overlay_apply(const void *new_fdt,
>  				 ret_revert);
>  			devicetree_state_flags |= DTSF_APPLY_FAIL;
>  		}
> -		goto err_free_overlay_changeset;
> +		goto err_free_ovcs_contents;
>  	}
>  
>  	ret = __of_changeset_apply_notify(&ovcs->cset);
> @@ -997,18 +1001,16 @@ static int of_overlay_apply(const void *new_fdt,
>  
>  	goto out_unlock;
>  
> -err_free_overlay_tree:
> -	kfree(new_fdt);
> -	kfree(overlay_tree);
> +err_free_ovcs_contents:
> +	free_overlay_changeset_contents(ovcs);
>  
> -err_free_overlay_changeset:
> -	free_overlay_changeset(ovcs);
> +err_free_ovcs:
> +	kfree(ovcs);
>  
>  out_unlock:
>  	mutex_unlock(&of_mutex);
>  	of_overlay_mutex_unlock();
>  
> -out:
>  	pr_debug("%s() err=%d\n", __func__, ret);
>  
>  	return ret;
> @@ -1019,11 +1021,14 @@ int of_overlay_fdt_apply(const void *overlay_fdt, u32 overlay_fdt_size,
>  {
>  	void *new_fdt;
>  	void *new_fdt_align;
> +	void *overlay_mem;
> +	bool kfree_unsafe;
>  	int ret;
>  	u32 size;
>  	struct device_node *overlay_root = NULL;
>  
>  	*ovcs_id = 0;
> +	kfree_unsafe = false;
>  
>  	if (overlay_fdt_size < sizeof(struct fdt_header) ||
>  	    fdt_check_header(overlay_fdt)) {
> @@ -1046,30 +1051,37 @@ int of_overlay_fdt_apply(const void *overlay_fdt, u32 overlay_fdt_size,
>  	new_fdt_align = PTR_ALIGN(new_fdt, FDT_ALIGN_SIZE);
>  	memcpy(new_fdt_align, overlay_fdt, size);
>  
> -	of_fdt_unflatten_tree(new_fdt_align, NULL, &overlay_root);
> -	if (!overlay_root) {
> +	overlay_mem = of_fdt_unflatten_tree(new_fdt_align, NULL, &overlay_root);
> +	if (!overlay_mem) {
>  		pr_err("unable to unflatten overlay_fdt\n");
>  		ret = -EINVAL;
>  		goto out_free_new_fdt;
>  	}
>  
> -	ret = of_overlay_apply(new_fdt, overlay_root, ovcs_id);
> -	if (ret < 0) {
> -		/*
> -		 * new_fdt and overlay_root now belong to the overlay
> -		 * changeset.
> -		 * overlay changeset code is responsible for freeing them.
> -		 */
> -		goto out;
> -	}
> +	ret = of_overlay_apply(new_fdt, overlay_mem, overlay_root, ovcs_id,
> +			&kfree_unsafe);
> +	if (ret < 0)
> +		goto out_free_overlay_mem;
>  
> +	/*
> +	 * new_fdt and overlay_mem now belong to the overlay changeset.
> +	 * free_overlay_changeset() is responsible for freeing them.
> +	 */
>  	return 0;
>  
> +	/*
> +	 * After overlay_notify(), ovcs->overlay_tree related pointers may have
> +	 * leaked to drivers, so can not kfree() overlay_mem and new_fdt.  This
> +	 * will result in a memory leak.
> +	 */
> +out_free_overlay_mem:
> +	if (!kfree_unsafe)
> +		kfree(overlay_mem);
>  
>  out_free_new_fdt:
> -	kfree(new_fdt);
> +	if (!kfree_unsafe)
> +		kfree(new_fdt);
>  
> -out:
>  	return ret;
>  }
>  EXPORT_SYMBOL_GPL(of_overlay_fdt_apply);
> @@ -1237,6 +1249,11 @@ int of_overlay_remove(int *ovcs_id)
>  
>  	*ovcs_id = 0;
>  
> +	/*
> +	 * Note that the overlay memory will be kfree()ed by
> +	 * free_overlay_changeset() even if the notifier for
> +	 * OF_OVERLAY_POST_REMOVE returns an error.
> +	 */
>  	ret_tmp = overlay_notify(ovcs, OF_OVERLAY_POST_REMOVE);
>  	if (ret_tmp) {
>  		pr_err("overlay changeset post-remove notify error %d\n",

Re: [PATCH v2 0/2] of: overlay: rework overlay apply and remove kfree()s

[Frank Rowand]

adding cc: Jan Kiszka <jan.kiszka@siemens.com>

On 4/10/22 16:08, frowand.list@gmail.com wrote:
> From: Frank Rowand <frank.rowand@sony.com>
> 
> Fix various kfree() issues related to of_overlay_apply().
> 
> The fixes revealed inconsist variable names for the same variable
> across functions, resulting in difficulty understanding the code
> that was being modified.  Doing both variable renaming and the
> fixes results in a hard to review patch, so split into two patches.
> 
> The first patch in the series contains only variable renaming.
> The second patch contains the kfree() related fixes.
> 
> Frank Rowand (2):
>   of: overlay: rename variables to be consistent
>   of: overlay: rework overlay apply and remove kfree()s
> 
>  Documentation/devicetree/overlay-notes.rst |  23 ++-
>  drivers/of/overlay.c                       | 175 +++++++++++----------
>  2 files changed, 115 insertions(+), 83 deletions(-)
> 

Re: [PATCH v2 1/2] of: overlay: rename variables to be consistent

[Frank Rowand]

adding cc: Jan Kiszka <jan.kiszka@siemens.com>

On 4/10/22 16:08, frowand.list@gmail.com wrote:
> From: Frank Rowand <frank.rowand@sony.com>
> 
> Variables change name across function calls when there is not a good
> reason to do so.  Fix by changing "fdt" to "new_fdt" and "tree" to
> "overlay_tree".
> 
> The name disparity was confusing when creating the following commit.
> The name changes are in this separate commit to make review of the
> following commmit less complex.
> 
> Signed-off-by: Frank Rowand <frank.rowand@sony.com>
> ---
> Changes since v1:
>   - This patch is added to the series.
> 
>  drivers/of/overlay.c | 94 ++++++++++++++++++++++----------------------
>  1 file changed, 47 insertions(+), 47 deletions(-)
> 
> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
> index d80160cf34bb..f74aa9ff67aa 100644
> --- a/drivers/of/overlay.c
> +++ b/drivers/of/overlay.c
> @@ -57,7 +57,7 @@ struct fragment {
>   * struct overlay_changeset
>   * @id:			changeset identifier
>   * @ovcs_list:		list on which we are located
> - * @fdt:		base of memory allocated to hold aligned FDT that was unflattened to create @overlay_tree
> + * @new_fdt:		Memory allocated to hold unflattened aligned FDT
>   * @overlay_tree:	expanded device tree that contains the fragment nodes
>   * @count:		count of fragment structures
>   * @fragments:		fragment nodes in the overlay expanded device tree
> @@ -67,7 +67,7 @@ struct fragment {
>  struct overlay_changeset {
>  	int id;
>  	struct list_head ovcs_list;
> -	const void *fdt;
> +	const void *new_fdt;
>  	struct device_node *overlay_tree;
>  	int count;
>  	struct fragment *fragments;
> @@ -718,19 +718,20 @@ static struct device_node *find_target(struct device_node *info_node)
>  
>  /**
>   * init_overlay_changeset() - initialize overlay changeset from overlay tree
> - * @ovcs:	Overlay changeset to build
> - * @fdt:	base of memory allocated to hold aligned FDT that was unflattened to create @tree
> - * @tree:	Contains the overlay fragments and overlay fixup nodes
> + * @ovcs:		Overlay changeset to build
> + * @new_fdt:		Memory allocated to hold unflattened aligned FDT
> + * @overlay_tree:	Contains the overlay fragments and overlay fixup nodes
>   *
>   * Initialize @ovcs.  Populate @ovcs->fragments with node information from
> - * the top level of @tree.  The relevant top level nodes are the fragment
> - * nodes and the __symbols__ node.  Any other top level node will be ignored.
> + * the top level of @overlay_tree.  The relevant top level nodes are the
> + * fragment nodes and the __symbols__ node.  Any other top level node will
> + * be ignored.
>   *
>   * Return: 0 on success, -ENOMEM if memory allocation failure, -EINVAL if error
> - * detected in @tree, or -ENOSPC if idr_alloc() error.
> + * detected in @overlay_tree, or -ENOSPC if idr_alloc() error.
>   */
>  static int init_overlay_changeset(struct overlay_changeset *ovcs,
> -		const void *fdt, struct device_node *tree)
> +		const void *new_fdt, struct device_node *overlay_tree)
>  {
>  	struct device_node *node, *overlay_node;
>  	struct fragment *fragment;
> @@ -741,17 +742,17 @@ static int init_overlay_changeset(struct overlay_changeset *ovcs,
>  	 * Warn for some issues.  Can not return -EINVAL for these until
>  	 * of_unittest_apply_overlay() is fixed to pass these checks.
>  	 */
> -	if (!of_node_check_flag(tree, OF_DYNAMIC))
> -		pr_debug("%s() tree is not dynamic\n", __func__);
> +	if (!of_node_check_flag(overlay_tree, OF_DYNAMIC))
> +		pr_debug("%s() overlay_tree is not dynamic\n", __func__);
>  
> -	if (!of_node_check_flag(tree, OF_DETACHED))
> -		pr_debug("%s() tree is not detached\n", __func__);
> +	if (!of_node_check_flag(overlay_tree, OF_DETACHED))
> +		pr_debug("%s() overlay_tree is not detached\n", __func__);
>  
> -	if (!of_node_is_root(tree))
> -		pr_debug("%s() tree is not root\n", __func__);
> +	if (!of_node_is_root(overlay_tree))
> +		pr_debug("%s() overlay_tree is not root\n", __func__);
>  
> -	ovcs->overlay_tree = tree;
> -	ovcs->fdt = fdt;
> +	ovcs->overlay_tree = overlay_tree;
> +	ovcs->new_fdt = new_fdt;
>  
>  	INIT_LIST_HEAD(&ovcs->ovcs_list);
>  
> @@ -764,7 +765,7 @@ static int init_overlay_changeset(struct overlay_changeset *ovcs,
>  	cnt = 0;
>  
>  	/* fragment nodes */
> -	for_each_child_of_node(tree, node) {
> +	for_each_child_of_node(overlay_tree, node) {
>  		overlay_node = of_get_child_by_name(node, "__overlay__");
>  		if (overlay_node) {
>  			cnt++;
> @@ -772,7 +773,7 @@ static int init_overlay_changeset(struct overlay_changeset *ovcs,
>  		}
>  	}
>  
> -	node = of_get_child_by_name(tree, "__symbols__");
> +	node = of_get_child_by_name(overlay_tree, "__symbols__");
>  	if (node) {
>  		cnt++;
>  		of_node_put(node);
> @@ -785,7 +786,7 @@ static int init_overlay_changeset(struct overlay_changeset *ovcs,
>  	}
>  
>  	cnt = 0;
> -	for_each_child_of_node(tree, node) {
> +	for_each_child_of_node(overlay_tree, node) {
>  		overlay_node = of_get_child_by_name(node, "__overlay__");
>  		if (!overlay_node)
>  			continue;
> @@ -807,7 +808,7 @@ static int init_overlay_changeset(struct overlay_changeset *ovcs,
>  	 * if there is a symbols fragment in ovcs->fragments[i] it is
>  	 * the final element in the array
>  	 */
> -	node = of_get_child_by_name(tree, "__symbols__");
> +	node = of_get_child_by_name(overlay_tree, "__symbols__");
>  	if (node) {
>  		ovcs->symbols_fragment = 1;
>  		fragment = &fragments[cnt];
> @@ -862,11 +863,11 @@ static void free_overlay_changeset(struct overlay_changeset *ovcs)
>  	kfree(ovcs->fragments);
>  	/*
>  	 * There should be no live pointers into ovcs->overlay_tree and
> -	 * ovcs->fdt due to the policy that overlay notifiers are not allowed
> -	 * to retain pointers into the overlay devicetree.
> +	 * ovcs->new_fdt due to the policy that overlay notifiers are not
> +	 * allowed to retain pointers into the overlay devicetree.
>  	 */
>  	kfree(ovcs->overlay_tree);
> -	kfree(ovcs->fdt);
> +	kfree(ovcs->new_fdt);
>  	kfree(ovcs);
>  }
>  
> @@ -874,16 +875,15 @@ static void free_overlay_changeset(struct overlay_changeset *ovcs)
>   * internal documentation
>   *
>   * of_overlay_apply() - Create and apply an overlay changeset
> - * @fdt:	base of memory allocated to hold the aligned FDT
> - * @tree:	Expanded overlay device tree
> - * @ovcs_id:	Pointer to overlay changeset id
> + * @new_fdt:		Memory allocated to hold the aligned FDT
> + * @overlay_tree:	Expanded overlay device tree
> + * @ovcs_id:		Pointer to overlay changeset id
>   *
>   * Creates and applies an overlay changeset.
>   *
>   * If an error occurs in a pre-apply notifier, then no changes are made
>   * to the device tree.
>   *
> -
>   * A non-zero return value will not have created the changeset if error is from:
>   *   - parameter checks
>   *   - building the changeset
> @@ -913,29 +913,29 @@ static void free_overlay_changeset(struct overlay_changeset *ovcs)
>   * id is returned to *ovcs_id.
>   */
>  
> -static int of_overlay_apply(const void *fdt, struct device_node *tree,
> -		int *ovcs_id)
> +static int of_overlay_apply(const void *new_fdt,
> +		struct device_node *overlay_tree, int *ovcs_id)
>  {
>  	struct overlay_changeset *ovcs;
>  	int ret = 0, ret_revert, ret_tmp;
>  
>  	/*
> -	 * As of this point, fdt and tree belong to the overlay changeset.
> -	 * overlay changeset code is responsible for freeing them.
> +	 * As of this point, new_fdt and overlay_tree belong to the overlay
> +	 * changeset.  overlay changeset code is responsible for freeing them.
>  	 */
>  
>  	if (devicetree_corrupt()) {
>  		pr_err("devicetree state suspect, refuse to apply overlay\n");
> -		kfree(fdt);
> -		kfree(tree);
> +		kfree(new_fdt);
> +		kfree(overlay_tree);
>  		ret = -EBUSY;
>  		goto out;
>  	}
>  
>  	ovcs = kzalloc(sizeof(*ovcs), GFP_KERNEL);
>  	if (!ovcs) {
> -		kfree(fdt);
> -		kfree(tree);
> +		kfree(new_fdt);
> +		kfree(overlay_tree);
>  		ret = -ENOMEM;
>  		goto out;
>  	}
> @@ -943,20 +943,20 @@ static int of_overlay_apply(const void *fdt, struct device_node *tree,
>  	of_overlay_mutex_lock();
>  	mutex_lock(&of_mutex);
>  
> -	ret = of_resolve_phandles(tree);
> +	ret = of_resolve_phandles(overlay_tree);
>  	if (ret)
> -		goto err_free_tree;
> +		goto err_free_overlay_tree;
>  
> -	ret = init_overlay_changeset(ovcs, fdt, tree);
> +	ret = init_overlay_changeset(ovcs, new_fdt, overlay_tree);
>  	if (ret)
> -		goto err_free_tree;
> +		goto err_free_overlay_tree;
>  
>  	/*
>  	 * after overlay_notify(), ovcs->overlay_tree related pointers may have
> -	 * leaked to drivers, so can not kfree() tree, aka ovcs->overlay_tree;
> -	 * and can not free memory containing aligned fdt.  The aligned fdt
> -	 * is contained within the memory at ovcs->fdt, possibly at an offset
> -	 * from ovcs->fdt.
> +	 * leaked to drivers, so can not kfree() overlay_tree,
> +	 * aka ovcs->overlay_tree; and can not free memory containing aligned
> +	 * fdt.  The aligned fdt is contained within the memory at
> +	 * ovcs->new_fdt, possibly at an offset from ovcs->new_fdt.
>  	 */
>  	ret = overlay_notify(ovcs, OF_OVERLAY_PRE_APPLY);
>  	if (ret) {
> @@ -997,9 +997,9 @@ static int of_overlay_apply(const void *fdt, struct device_node *tree,
>  
>  	goto out_unlock;
>  
> -err_free_tree:
> -	kfree(fdt);
> -	kfree(tree);
> +err_free_overlay_tree:
> +	kfree(new_fdt);
> +	kfree(overlay_tree);
>  
>  err_free_overlay_changeset:
>  	free_overlay_changeset(ovcs);

Re: [PATCH v2 2/2] of: overlay: rework overlay apply and remove kfree()s

[Frank Rowand]

adding cc: Jan Kiszka <jan.kiszka@siemens.com>

On 4/10/22 16:08, frowand.list@gmail.com wrote:
> From: Frank Rowand <frank.rowand@sony.com>
> 
> Fix various kfree() issues related to of_overlay_apply().
>   - Double kfree() of fdt and tree when init_overlay_changeset()
>     returns an error.
>   - free_overlay_changeset() free the root of the unflattened
>     overlay (variable tree) instead of the memory that contains
>     the unflattened overlay.
>   - For the case of a failure during applying an overlay, move kfree()
>     of new_fdt and overlay_mem into the function that allocated them.
>     For the case of removing an overlay, the kfree() remains in
>     free_overlay_changeset().
>   - Check return value of of_fdt_unflatten_tree() for error instead
>     of checking the returnded value of overlay_root.
> 
> More clearly document policy related to lifetime of pointers into
> overlay memory.
> 
> Double kfree()
> Reported-by: Slawomir Stepien <slawomir.stepien@nokia.com>
> 
> Signed-off-by: Frank Rowand <frank.rowand@sony.com>
> ---
> 
> Changes since v1:
>   - Move kfree()s from init_overlay_changeset() to of_overlay_fdt_apply()
>   - Better document lifetime of pointers into overlay, both in overlay.c
>     and Documentation/devicetree/overlay-notes.rst
> 
>  Documentation/devicetree/overlay-notes.rst |  23 +++-
>  drivers/of/overlay.c                       | 127 ++++++++++++---------
>  2 files changed, 91 insertions(+), 59 deletions(-)
> 
> diff --git a/Documentation/devicetree/overlay-notes.rst b/Documentation/devicetree/overlay-notes.rst
> index b2b8db765b8c..7a6e85f75567 100644
> --- a/Documentation/devicetree/overlay-notes.rst
> +++ b/Documentation/devicetree/overlay-notes.rst
> @@ -119,10 +119,25 @@ Finally, if you need to remove all overlays in one-go, just call
>  of_overlay_remove_all() which will remove every single one in the correct
>  order.
>  
> -In addition, there is the option to register notifiers that get called on
> +There is the option to register notifiers that get called on
>  overlay operations. See of_overlay_notifier_register/unregister and
>  enum of_overlay_notify_action for details.
>  
> -Note that a notifier callback is not supposed to store pointers to a device
> -tree node or its content beyond OF_OVERLAY_POST_REMOVE corresponding to the
> -respective node it received.
> +A notifier callback for OF_OVERLAY_PRE_APPLY, OF_OVERLAY_POST_APPLY, or
> +OF_OVERLAY_PRE_REMOVE may store pointers to a device tree node in the overlay
> +or its content but these pointers must not persist past the notifier callback
> +for OF_OVERLAY_POST_REMOVE.  The memory containing the overlay will be
> +kfree()ed after OF_OVERLAY_POST_REMOVE notifiers are called.  Note that the
> +memory will be kfree()ed even if the notifier for OF_OVERLAY_POST_REMOVE
> +returns an error.
> +
> +The changeset notifiers in drivers/of/dynamic.c are a second type of notifier
> +that could be triggered by applying or removing an overlay.  These notifiers
> +are not allowed to store pointers to a device tree node in the overlay
> +or its content.  The overlay code does not protect against such pointers
> +remaining active when the memory containing the overlay is freed as a result
> +of removing the overlay.
> +
> +Any other code that retains a pointer to the overlay nodes or data is
> +considered to be a bug because after removing the overlay the pointer
> +will refer to freed memory.
> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
> index f74aa9ff67aa..c8e999518f2f 100644
> --- a/drivers/of/overlay.c
> +++ b/drivers/of/overlay.c
> @@ -58,6 +58,7 @@ struct fragment {
>   * @id:			changeset identifier
>   * @ovcs_list:		list on which we are located
>   * @new_fdt:		Memory allocated to hold unflattened aligned FDT
> + * @overlay_mem:	the memory chunk that contains @overlay_tree
>   * @overlay_tree:	expanded device tree that contains the fragment nodes
>   * @count:		count of fragment structures
>   * @fragments:		fragment nodes in the overlay expanded device tree
> @@ -68,6 +69,7 @@ struct overlay_changeset {
>  	int id;
>  	struct list_head ovcs_list;
>  	const void *new_fdt;
> +	const void *overlay_mem;
>  	struct device_node *overlay_tree;
>  	int count;
>  	struct fragment *fragments;
> @@ -720,18 +722,20 @@ static struct device_node *find_target(struct device_node *info_node)
>   * init_overlay_changeset() - initialize overlay changeset from overlay tree
>   * @ovcs:		Overlay changeset to build
>   * @new_fdt:		Memory allocated to hold unflattened aligned FDT
> + * @tree_mem:		Memory that contains @overlay_tree
>   * @overlay_tree:	Contains the overlay fragments and overlay fixup nodes
>   *
>   * Initialize @ovcs.  Populate @ovcs->fragments with node information from
>   * the top level of @overlay_tree.  The relevant top level nodes are the
>   * fragment nodes and the __symbols__ node.  Any other top level node will
> - * be ignored.
> + * be ignored.  Populate other @ovcs fields.
>   *
>   * Return: 0 on success, -ENOMEM if memory allocation failure, -EINVAL if error
>   * detected in @overlay_tree, or -ENOSPC if idr_alloc() error.
>   */
>  static int init_overlay_changeset(struct overlay_changeset *ovcs,
> -		const void *new_fdt, struct device_node *overlay_tree)
> +		const void *new_fdt, const void *tree_mem,
> +		struct device_node *overlay_tree)
>  {
>  	struct device_node *node, *overlay_node;
>  	struct fragment *fragment;
> @@ -751,9 +755,6 @@ static int init_overlay_changeset(struct overlay_changeset *ovcs,
>  	if (!of_node_is_root(overlay_tree))
>  		pr_debug("%s() overlay_tree is not root\n", __func__);
>  
> -	ovcs->overlay_tree = overlay_tree;
> -	ovcs->new_fdt = new_fdt;
> -
>  	INIT_LIST_HEAD(&ovcs->ovcs_list);
>  
>  	of_changeset_init(&ovcs->cset);
> @@ -832,6 +833,9 @@ static int init_overlay_changeset(struct overlay_changeset *ovcs,
>  
>  	ovcs->id = id;
>  	ovcs->count = cnt;
> +	ovcs->new_fdt = new_fdt;
> +	ovcs->overlay_mem = tree_mem;
> +	ovcs->overlay_tree = overlay_tree;
>  	ovcs->fragments = fragments;
>  
>  	return 0;
> @@ -846,7 +850,7 @@ static int init_overlay_changeset(struct overlay_changeset *ovcs,
>  	return ret;
>  }
>  
> -static void free_overlay_changeset(struct overlay_changeset *ovcs)
> +static void free_overlay_changeset_contents(struct overlay_changeset *ovcs)
>  {
>  	int i;
>  
> @@ -861,12 +865,20 @@ static void free_overlay_changeset(struct overlay_changeset *ovcs)
>  		of_node_put(ovcs->fragments[i].overlay);
>  	}
>  	kfree(ovcs->fragments);
> +}
> +static void free_overlay_changeset(struct overlay_changeset *ovcs)
> +{
> +
> +	free_overlay_changeset_contents(ovcs);
> +
>  	/*
> -	 * There should be no live pointers into ovcs->overlay_tree and
> +	 * There should be no live pointers into ovcs->overlay_mem and
>  	 * ovcs->new_fdt due to the policy that overlay notifiers are not
> -	 * allowed to retain pointers into the overlay devicetree.
> +	 * allowed to retain pointers into the overlay devicetree other
> +	 * than the window between OF_OVERLAY_PRE_APPLY overlay notifiers
> +	 * and the OF_OVERLAY_POST_REMOVE overlay notifiers.
>  	 */
> -	kfree(ovcs->overlay_tree);
> +	kfree(ovcs->overlay_mem);
>  	kfree(ovcs->new_fdt);
>  	kfree(ovcs);
>  }
> @@ -876,8 +888,10 @@ static void free_overlay_changeset(struct overlay_changeset *ovcs)
>   *
>   * of_overlay_apply() - Create and apply an overlay changeset
>   * @new_fdt:		Memory allocated to hold the aligned FDT
> + * @tree_mem:		Memory that contains @overlay_tree
>   * @overlay_tree:	Expanded overlay device tree
>   * @ovcs_id:		Pointer to overlay changeset id
> + * @kfree_unsafe:	Pointer to flag to not kfree() @new_fdt and @overlay_tree
>   *
>   * Creates and applies an overlay changeset.
>   *
> @@ -910,34 +924,25 @@ static void free_overlay_changeset(struct overlay_changeset *ovcs)
>   * refused.
>   *
>   * Returns 0 on success, or a negative error number.  Overlay changeset
> - * id is returned to *ovcs_id.
> + * id is returned to *ovcs_id.  When references to @new_fdt and @overlay_tree
> + * may exist, *kfree_unsafe is set to true.
>   */
>  
> -static int of_overlay_apply(const void *new_fdt,
> -		struct device_node *overlay_tree, int *ovcs_id)
> +static int of_overlay_apply(const void *new_fdt, void *tree_mem,
> +		struct device_node *overlay_tree, int *ovcs_id,
> +		bool *kfree_unsafe)
>  {
>  	struct overlay_changeset *ovcs;
>  	int ret = 0, ret_revert, ret_tmp;
>  
> -	/*
> -	 * As of this point, new_fdt and overlay_tree belong to the overlay
> -	 * changeset.  overlay changeset code is responsible for freeing them.
> -	 */
> -
>  	if (devicetree_corrupt()) {
>  		pr_err("devicetree state suspect, refuse to apply overlay\n");
> -		kfree(new_fdt);
> -		kfree(overlay_tree);
> -		ret = -EBUSY;
> -		goto out;
> +		return -EBUSY;
>  	}
>  
>  	ovcs = kzalloc(sizeof(*ovcs), GFP_KERNEL);
>  	if (!ovcs) {
> -		kfree(new_fdt);
> -		kfree(overlay_tree);
> -		ret = -ENOMEM;
> -		goto out;
> +		return -ENOMEM;
>  	}
>  
>  	of_overlay_mutex_lock();
> @@ -945,28 +950,27 @@ static int of_overlay_apply(const void *new_fdt,
>  
>  	ret = of_resolve_phandles(overlay_tree);
>  	if (ret)
> -		goto err_free_overlay_tree;
> +		goto err_free_ovcs;
>  
> -	ret = init_overlay_changeset(ovcs, new_fdt, overlay_tree);
> +	ret = init_overlay_changeset(ovcs, new_fdt, tree_mem, overlay_tree);
>  	if (ret)
> -		goto err_free_overlay_tree;
> +		goto err_free_ovcs_contents;
>  
>  	/*
> -	 * after overlay_notify(), ovcs->overlay_tree related pointers may have
> -	 * leaked to drivers, so can not kfree() overlay_tree,
> -	 * aka ovcs->overlay_tree; and can not free memory containing aligned
> -	 * fdt.  The aligned fdt is contained within the memory at
> -	 * ovcs->new_fdt, possibly at an offset from ovcs->new_fdt.
> +	 * After overlay_notify(), ovcs->overlay_tree related pointers may have
> +	 * leaked to drivers, so can not kfree() ovcs->overlay_mem and
> +	 * ovcs->new_fdt until after OF_OVERLAY_POST_REMOVE notifiers.
>  	 */
> +	*kfree_unsafe = true;
>  	ret = overlay_notify(ovcs, OF_OVERLAY_PRE_APPLY);
>  	if (ret) {
>  		pr_err("overlay changeset pre-apply notify error %d\n", ret);
> -		goto err_free_overlay_changeset;
> +		goto err_free_ovcs_contents;
>  	}
>  
>  	ret = build_changeset(ovcs);
>  	if (ret)
> -		goto err_free_overlay_changeset;
> +		goto err_free_ovcs_contents;
>  
>  	ret_revert = 0;
>  	ret = __of_changeset_apply_entries(&ovcs->cset, &ret_revert);
> @@ -976,7 +980,7 @@ static int of_overlay_apply(const void *new_fdt,
>  				 ret_revert);
>  			devicetree_state_flags |= DTSF_APPLY_FAIL;
>  		}
> -		goto err_free_overlay_changeset;
> +		goto err_free_ovcs_contents;
>  	}
>  
>  	ret = __of_changeset_apply_notify(&ovcs->cset);
> @@ -997,18 +1001,16 @@ static int of_overlay_apply(const void *new_fdt,
>  
>  	goto out_unlock;
>  
> -err_free_overlay_tree:
> -	kfree(new_fdt);
> -	kfree(overlay_tree);
> +err_free_ovcs_contents:
> +	free_overlay_changeset_contents(ovcs);
>  
> -err_free_overlay_changeset:
> -	free_overlay_changeset(ovcs);
> +err_free_ovcs:
> +	kfree(ovcs);
>  
>  out_unlock:
>  	mutex_unlock(&of_mutex);
>  	of_overlay_mutex_unlock();
>  
> -out:
>  	pr_debug("%s() err=%d\n", __func__, ret);
>  
>  	return ret;
> @@ -1019,11 +1021,14 @@ int of_overlay_fdt_apply(const void *overlay_fdt, u32 overlay_fdt_size,
>  {
>  	void *new_fdt;
>  	void *new_fdt_align;
> +	void *overlay_mem;
> +	bool kfree_unsafe;
>  	int ret;
>  	u32 size;
>  	struct device_node *overlay_root = NULL;
>  
>  	*ovcs_id = 0;
> +	kfree_unsafe = false;
>  
>  	if (overlay_fdt_size < sizeof(struct fdt_header) ||
>  	    fdt_check_header(overlay_fdt)) {
> @@ -1046,30 +1051,37 @@ int of_overlay_fdt_apply(const void *overlay_fdt, u32 overlay_fdt_size,
>  	new_fdt_align = PTR_ALIGN(new_fdt, FDT_ALIGN_SIZE);
>  	memcpy(new_fdt_align, overlay_fdt, size);
>  
> -	of_fdt_unflatten_tree(new_fdt_align, NULL, &overlay_root);
> -	if (!overlay_root) {
> +	overlay_mem = of_fdt_unflatten_tree(new_fdt_align, NULL, &overlay_root);
> +	if (!overlay_mem) {
>  		pr_err("unable to unflatten overlay_fdt\n");
>  		ret = -EINVAL;
>  		goto out_free_new_fdt;
>  	}
>  
> -	ret = of_overlay_apply(new_fdt, overlay_root, ovcs_id);
> -	if (ret < 0) {
> -		/*
> -		 * new_fdt and overlay_root now belong to the overlay
> -		 * changeset.
> -		 * overlay changeset code is responsible for freeing them.
> -		 */
> -		goto out;
> -	}
> +	ret = of_overlay_apply(new_fdt, overlay_mem, overlay_root, ovcs_id,
> +			&kfree_unsafe);
> +	if (ret < 0)
> +		goto out_free_overlay_mem;
>  
> +	/*
> +	 * new_fdt and overlay_mem now belong to the overlay changeset.
> +	 * free_overlay_changeset() is responsible for freeing them.
> +	 */
>  	return 0;
>  
> +	/*
> +	 * After overlay_notify(), ovcs->overlay_tree related pointers may have
> +	 * leaked to drivers, so can not kfree() overlay_mem and new_fdt.  This
> +	 * will result in a memory leak.
> +	 */
> +out_free_overlay_mem:
> +	if (!kfree_unsafe)
> +		kfree(overlay_mem);
>  
>  out_free_new_fdt:
> -	kfree(new_fdt);
> +	if (!kfree_unsafe)
> +		kfree(new_fdt);
>  
> -out:
>  	return ret;
>  }
>  EXPORT_SYMBOL_GPL(of_overlay_fdt_apply);
> @@ -1237,6 +1249,11 @@ int of_overlay_remove(int *ovcs_id)
>  
>  	*ovcs_id = 0;
>  
> +	/*
> +	 * Note that the overlay memory will be kfree()ed by
> +	 * free_overlay_changeset() even if the notifier for
> +	 * OF_OVERLAY_POST_REMOVE returns an error.
> +	 */
>  	ret_tmp = overlay_notify(ovcs, OF_OVERLAY_POST_REMOVE);
>  	if (ret_tmp) {
>  		pr_err("overlay changeset post-remove notify error %d\n",

Re: [PATCH v2 2/2] of: overlay: rework overlay apply and remove kfree()s

[Slawomir Stepien]

On kwi 10, 2022 23:11, Frank Rowand wrote:
> adding cc: Jan Kiszka <jan.kiszka@siemens.com>
> 
> On 4/10/22 16:08, frowand.list@gmail.com wrote:
> > From: Frank Rowand <frank.rowand@sony.com>
> > 
> > Fix various kfree() issues related to of_overlay_apply().
> >   - Double kfree() of fdt and tree when init_overlay_changeset()
> >     returns an error.
> >   - free_overlay_changeset() free the root of the unflattened
> >     overlay (variable tree) instead of the memory that contains
> >     the unflattened overlay.
> >   - For the case of a failure during applying an overlay, move kfree()
> >     of new_fdt and overlay_mem into the function that allocated them.
> >     For the case of removing an overlay, the kfree() remains in
> >     free_overlay_changeset().
> >   - Check return value of of_fdt_unflatten_tree() for error instead
> >     of checking the returnded value of overlay_root.
> > 
> > More clearly document policy related to lifetime of pointers into
> > overlay memory.
> > 
> > Double kfree()
> > Reported-by: Slawomir Stepien <slawomir.stepien@nokia.com>
> > 
> > Signed-off-by: Frank Rowand <frank.rowand@sony.com>

Hi Frank

It looks good to me.

Reviewed-by: Slawomir Stepien <slawomir.stepien@nokia.com>

> > ---
> > 
> > Changes since v1:
> >   - Move kfree()s from init_overlay_changeset() to of_overlay_fdt_apply()
> >   - Better document lifetime of pointers into overlay, both in overlay.c
> >     and Documentation/devicetree/overlay-notes.rst
> > 
> >  Documentation/devicetree/overlay-notes.rst |  23 +++-
> >  drivers/of/overlay.c                       | 127 ++++++++++++---------
> >  2 files changed, 91 insertions(+), 59 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/overlay-notes.rst b/Documentation/devicetree/overlay-notes.rst
> > index b2b8db765b8c..7a6e85f75567 100644
> > --- a/Documentation/devicetree/overlay-notes.rst
> > +++ b/Documentation/devicetree/overlay-notes.rst
> > @@ -119,10 +119,25 @@ Finally, if you need to remove all overlays in one-go, just call
> >  of_overlay_remove_all() which will remove every single one in the correct
> >  order.
> >  
> > -In addition, there is the option to register notifiers that get called on
> > +There is the option to register notifiers that get called on
> >  overlay operations. See of_overlay_notifier_register/unregister and
> >  enum of_overlay_notify_action for details.
> >  
> > -Note that a notifier callback is not supposed to store pointers to a device
> > -tree node or its content beyond OF_OVERLAY_POST_REMOVE corresponding to the
> > -respective node it received.
> > +A notifier callback for OF_OVERLAY_PRE_APPLY, OF_OVERLAY_POST_APPLY, or
> > +OF_OVERLAY_PRE_REMOVE may store pointers to a device tree node in the overlay
> > +or its content but these pointers must not persist past the notifier callback
> > +for OF_OVERLAY_POST_REMOVE.  The memory containing the overlay will be
> > +kfree()ed after OF_OVERLAY_POST_REMOVE notifiers are called.  Note that the
> > +memory will be kfree()ed even if the notifier for OF_OVERLAY_POST_REMOVE
> > +returns an error.
> > +
> > +The changeset notifiers in drivers/of/dynamic.c are a second type of notifier
> > +that could be triggered by applying or removing an overlay.  These notifiers
> > +are not allowed to store pointers to a device tree node in the overlay
> > +or its content.  The overlay code does not protect against such pointers
> > +remaining active when the memory containing the overlay is freed as a result
> > +of removing the overlay.
> > +
> > +Any other code that retains a pointer to the overlay nodes or data is
> > +considered to be a bug because after removing the overlay the pointer
> > +will refer to freed memory.
> > diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
> > index f74aa9ff67aa..c8e999518f2f 100644
> > --- a/drivers/of/overlay.c
> > +++ b/drivers/of/overlay.c
> > @@ -58,6 +58,7 @@ struct fragment {
> >   * @id:			changeset identifier
> >   * @ovcs_list:		list on which we are located
> >   * @new_fdt:		Memory allocated to hold unflattened aligned FDT
> > + * @overlay_mem:	the memory chunk that contains @overlay_tree
> >   * @overlay_tree:	expanded device tree that contains the fragment nodes
> >   * @count:		count of fragment structures
> >   * @fragments:		fragment nodes in the overlay expanded device tree
> > @@ -68,6 +69,7 @@ struct overlay_changeset {
> >  	int id;
> >  	struct list_head ovcs_list;
> >  	const void *new_fdt;
> > +	const void *overlay_mem;
> >  	struct device_node *overlay_tree;
> >  	int count;
> >  	struct fragment *fragments;
> > @@ -720,18 +722,20 @@ static struct device_node *find_target(struct device_node *info_node)
> >   * init_overlay_changeset() - initialize overlay changeset from overlay tree
> >   * @ovcs:		Overlay changeset to build
> >   * @new_fdt:		Memory allocated to hold unflattened aligned FDT
> > + * @tree_mem:		Memory that contains @overlay_tree
> >   * @overlay_tree:	Contains the overlay fragments and overlay fixup nodes
> >   *
> >   * Initialize @ovcs.  Populate @ovcs->fragments with node information from
> >   * the top level of @overlay_tree.  The relevant top level nodes are the
> >   * fragment nodes and the __symbols__ node.  Any other top level node will
> > - * be ignored.
> > + * be ignored.  Populate other @ovcs fields.
> >   *
> >   * Return: 0 on success, -ENOMEM if memory allocation failure, -EINVAL if error
> >   * detected in @overlay_tree, or -ENOSPC if idr_alloc() error.
> >   */
> >  static int init_overlay_changeset(struct overlay_changeset *ovcs,
> > -		const void *new_fdt, struct device_node *overlay_tree)
> > +		const void *new_fdt, const void *tree_mem,
> > +		struct device_node *overlay_tree)
> >  {
> >  	struct device_node *node, *overlay_node;
> >  	struct fragment *fragment;
> > @@ -751,9 +755,6 @@ static int init_overlay_changeset(struct overlay_changeset *ovcs,
> >  	if (!of_node_is_root(overlay_tree))
> >  		pr_debug("%s() overlay_tree is not root\n", __func__);
> >  
> > -	ovcs->overlay_tree = overlay_tree;
> > -	ovcs->new_fdt = new_fdt;
> > -
> >  	INIT_LIST_HEAD(&ovcs->ovcs_list);
> >  
> >  	of_changeset_init(&ovcs->cset);
> > @@ -832,6 +833,9 @@ static int init_overlay_changeset(struct overlay_changeset *ovcs,
> >  
> >  	ovcs->id = id;
> >  	ovcs->count = cnt;
> > +	ovcs->new_fdt = new_fdt;
> > +	ovcs->overlay_mem = tree_mem;
> > +	ovcs->overlay_tree = overlay_tree;
> >  	ovcs->fragments = fragments;
> >  
> >  	return 0;
> > @@ -846,7 +850,7 @@ static int init_overlay_changeset(struct overlay_changeset *ovcs,
> >  	return ret;
> >  }
> >  
> > -static void free_overlay_changeset(struct overlay_changeset *ovcs)
> > +static void free_overlay_changeset_contents(struct overlay_changeset *ovcs)
> >  {
> >  	int i;
> >  
> > @@ -861,12 +865,20 @@ static void free_overlay_changeset(struct overlay_changeset *ovcs)
> >  		of_node_put(ovcs->fragments[i].overlay);
> >  	}
> >  	kfree(ovcs->fragments);
> > +}
> > +static void free_overlay_changeset(struct overlay_changeset *ovcs)
> > +{
> > +
> > +	free_overlay_changeset_contents(ovcs);
> > +
> >  	/*
> > -	 * There should be no live pointers into ovcs->overlay_tree and
> > +	 * There should be no live pointers into ovcs->overlay_mem and
> >  	 * ovcs->new_fdt due to the policy that overlay notifiers are not
> > -	 * allowed to retain pointers into the overlay devicetree.
> > +	 * allowed to retain pointers into the overlay devicetree other
> > +	 * than the window between OF_OVERLAY_PRE_APPLY overlay notifiers
> > +	 * and the OF_OVERLAY_POST_REMOVE overlay notifiers.
> >  	 */
> > -	kfree(ovcs->overlay_tree);
> > +	kfree(ovcs->overlay_mem);
> >  	kfree(ovcs->new_fdt);
> >  	kfree(ovcs);
> >  }
> > @@ -876,8 +888,10 @@ static void free_overlay_changeset(struct overlay_changeset *ovcs)
> >   *
> >   * of_overlay_apply() - Create and apply an overlay changeset
> >   * @new_fdt:		Memory allocated to hold the aligned FDT
> > + * @tree_mem:		Memory that contains @overlay_tree
> >   * @overlay_tree:	Expanded overlay device tree
> >   * @ovcs_id:		Pointer to overlay changeset id
> > + * @kfree_unsafe:	Pointer to flag to not kfree() @new_fdt and @overlay_tree
> >   *
> >   * Creates and applies an overlay changeset.
> >   *
> > @@ -910,34 +924,25 @@ static void free_overlay_changeset(struct overlay_changeset *ovcs)
> >   * refused.
> >   *
> >   * Returns 0 on success, or a negative error number.  Overlay changeset
> > - * id is returned to *ovcs_id.
> > + * id is returned to *ovcs_id.  When references to @new_fdt and @overlay_tree
> > + * may exist, *kfree_unsafe is set to true.
> >   */
> >  
> > -static int of_overlay_apply(const void *new_fdt,
> > -		struct device_node *overlay_tree, int *ovcs_id)
> > +static int of_overlay_apply(const void *new_fdt, void *tree_mem,
> > +		struct device_node *overlay_tree, int *ovcs_id,
> > +		bool *kfree_unsafe)
> >  {
> >  	struct overlay_changeset *ovcs;
> >  	int ret = 0, ret_revert, ret_tmp;
> >  
> > -	/*
> > -	 * As of this point, new_fdt and overlay_tree belong to the overlay
> > -	 * changeset.  overlay changeset code is responsible for freeing them.
> > -	 */
> > -
> >  	if (devicetree_corrupt()) {
> >  		pr_err("devicetree state suspect, refuse to apply overlay\n");
> > -		kfree(new_fdt);
> > -		kfree(overlay_tree);
> > -		ret = -EBUSY;
> > -		goto out;
> > +		return -EBUSY;
> >  	}
> >  
> >  	ovcs = kzalloc(sizeof(*ovcs), GFP_KERNEL);
> >  	if (!ovcs) {
> > -		kfree(new_fdt);
> > -		kfree(overlay_tree);
> > -		ret = -ENOMEM;
> > -		goto out;
> > +		return -ENOMEM;
> >  	}
> >  
> >  	of_overlay_mutex_lock();
> > @@ -945,28 +950,27 @@ static int of_overlay_apply(const void *new_fdt,
> >  
> >  	ret = of_resolve_phandles(overlay_tree);
> >  	if (ret)
> > -		goto err_free_overlay_tree;
> > +		goto err_free_ovcs;
> >  
> > -	ret = init_overlay_changeset(ovcs, new_fdt, overlay_tree);
> > +	ret = init_overlay_changeset(ovcs, new_fdt, tree_mem, overlay_tree);
> >  	if (ret)
> > -		goto err_free_overlay_tree;
> > +		goto err_free_ovcs_contents;
> >  
> >  	/*
> > -	 * after overlay_notify(), ovcs->overlay_tree related pointers may have
> > -	 * leaked to drivers, so can not kfree() overlay_tree,
> > -	 * aka ovcs->overlay_tree; and can not free memory containing aligned
> > -	 * fdt.  The aligned fdt is contained within the memory at
> > -	 * ovcs->new_fdt, possibly at an offset from ovcs->new_fdt.
> > +	 * After overlay_notify(), ovcs->overlay_tree related pointers may have
> > +	 * leaked to drivers, so can not kfree() ovcs->overlay_mem and
> > +	 * ovcs->new_fdt until after OF_OVERLAY_POST_REMOVE notifiers.
> >  	 */
> > +	*kfree_unsafe = true;
> >  	ret = overlay_notify(ovcs, OF_OVERLAY_PRE_APPLY);
> >  	if (ret) {
> >  		pr_err("overlay changeset pre-apply notify error %d\n", ret);
> > -		goto err_free_overlay_changeset;
> > +		goto err_free_ovcs_contents;
> >  	}
> >  
> >  	ret = build_changeset(ovcs);
> >  	if (ret)
> > -		goto err_free_overlay_changeset;
> > +		goto err_free_ovcs_contents;
> >  
> >  	ret_revert = 0;
> >  	ret = __of_changeset_apply_entries(&ovcs->cset, &ret_revert);
> > @@ -976,7 +980,7 @@ static int of_overlay_apply(const void *new_fdt,
> >  				 ret_revert);
> >  			devicetree_state_flags |= DTSF_APPLY_FAIL;
> >  		}
> > -		goto err_free_overlay_changeset;
> > +		goto err_free_ovcs_contents;
> >  	}
> >  
> >  	ret = __of_changeset_apply_notify(&ovcs->cset);
> > @@ -997,18 +1001,16 @@ static int of_overlay_apply(const void *new_fdt,
> >  
> >  	goto out_unlock;
> >  
> > -err_free_overlay_tree:
> > -	kfree(new_fdt);
> > -	kfree(overlay_tree);
> > +err_free_ovcs_contents:
> > +	free_overlay_changeset_contents(ovcs);
> >  
> > -err_free_overlay_changeset:
> > -	free_overlay_changeset(ovcs);
> > +err_free_ovcs:
> > +	kfree(ovcs);
> >  
> >  out_unlock:
> >  	mutex_unlock(&of_mutex);
> >  	of_overlay_mutex_unlock();
> >  
> > -out:
> >  	pr_debug("%s() err=%d\n", __func__, ret);
> >  
> >  	return ret;
> > @@ -1019,11 +1021,14 @@ int of_overlay_fdt_apply(const void *overlay_fdt, u32 overlay_fdt_size,
> >  {
> >  	void *new_fdt;
> >  	void *new_fdt_align;
> > +	void *overlay_mem;
> > +	bool kfree_unsafe;
> >  	int ret;
> >  	u32 size;
> >  	struct device_node *overlay_root = NULL;
> >  
> >  	*ovcs_id = 0;
> > +	kfree_unsafe = false;
> >  
> >  	if (overlay_fdt_size < sizeof(struct fdt_header) ||
> >  	    fdt_check_header(overlay_fdt)) {
> > @@ -1046,30 +1051,37 @@ int of_overlay_fdt_apply(const void *overlay_fdt, u32 overlay_fdt_size,
> >  	new_fdt_align = PTR_ALIGN(new_fdt, FDT_ALIGN_SIZE);
> >  	memcpy(new_fdt_align, overlay_fdt, size);
> >  
> > -	of_fdt_unflatten_tree(new_fdt_align, NULL, &overlay_root);
> > -	if (!overlay_root) {
> > +	overlay_mem = of_fdt_unflatten_tree(new_fdt_align, NULL, &overlay_root);
> > +	if (!overlay_mem) {
> >  		pr_err("unable to unflatten overlay_fdt\n");
> >  		ret = -EINVAL;
> >  		goto out_free_new_fdt;
> >  	}
> >  
> > -	ret = of_overlay_apply(new_fdt, overlay_root, ovcs_id);
> > -	if (ret < 0) {
> > -		/*
> > -		 * new_fdt and overlay_root now belong to the overlay
> > -		 * changeset.
> > -		 * overlay changeset code is responsible for freeing them.
> > -		 */
> > -		goto out;
> > -	}
> > +	ret = of_overlay_apply(new_fdt, overlay_mem, overlay_root, ovcs_id,
> > +			&kfree_unsafe);
> > +	if (ret < 0)
> > +		goto out_free_overlay_mem;
> >  
> > +	/*
> > +	 * new_fdt and overlay_mem now belong to the overlay changeset.
> > +	 * free_overlay_changeset() is responsible for freeing them.
> > +	 */
> >  	return 0;
> >  
> > +	/*
> > +	 * After overlay_notify(), ovcs->overlay_tree related pointers may have
> > +	 * leaked to drivers, so can not kfree() overlay_mem and new_fdt.  This
> > +	 * will result in a memory leak.
> > +	 */
> > +out_free_overlay_mem:
> > +	if (!kfree_unsafe)
> > +		kfree(overlay_mem);
> >  
> >  out_free_new_fdt:
> > -	kfree(new_fdt);
> > +	if (!kfree_unsafe)
> > +		kfree(new_fdt);
> >  
> > -out:
> >  	return ret;
> >  }
> >  EXPORT_SYMBOL_GPL(of_overlay_fdt_apply);
> > @@ -1237,6 +1249,11 @@ int of_overlay_remove(int *ovcs_id)
> >  
> >  	*ovcs_id = 0;
> >  
> > +	/*
> > +	 * Note that the overlay memory will be kfree()ed by
> > +	 * free_overlay_changeset() even if the notifier for
> > +	 * OF_OVERLAY_POST_REMOVE returns an error.
> > +	 */
> >  	ret_tmp = overlay_notify(ovcs, OF_OVERLAY_POST_REMOVE);
> >  	if (ret_tmp) {
> >  		pr_err("overlay changeset post-remove notify error %d\n",
> 

-- 
Slawomir Stepien

Re: [PATCH v2 1/2] of: overlay: rename variables to be consistent

[Slawomir Stepien]

On kwi 10, 2022 23:10, Frank Rowand wrote:
> adding cc: Jan Kiszka <jan.kiszka@siemens.com>
> 
> On 4/10/22 16:08, frowand.list@gmail.com wrote:
> > From: Frank Rowand <frank.rowand@sony.com>
> > 
> > Variables change name across function calls when there is not a good
> > reason to do so.  Fix by changing "fdt" to "new_fdt" and "tree" to
> > "overlay_tree".
> > 
> > The name disparity was confusing when creating the following commit.
> > The name changes are in this separate commit to make review of the
> > following commmit less complex.
> > 
> > Signed-off-by: Frank Rowand <frank.rowand@sony.com>

Hi Frank

It looks good to me.

Reviewed-by: Slawomir Stepien <slawomir.stepien@nokia.com>

> > ---
> > Changes since v1:
> >   - This patch is added to the series.
> > 
> >  drivers/of/overlay.c | 94 ++++++++++++++++++++++----------------------
> >  1 file changed, 47 insertions(+), 47 deletions(-)
> > 
> > diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
> > index d80160cf34bb..f74aa9ff67aa 100644
> > --- a/drivers/of/overlay.c
> > +++ b/drivers/of/overlay.c
> > @@ -57,7 +57,7 @@ struct fragment {
> >   * struct overlay_changeset
> >   * @id:			changeset identifier
> >   * @ovcs_list:		list on which we are located
> > - * @fdt:		base of memory allocated to hold aligned FDT that was unflattened to create @overlay_tree
> > + * @new_fdt:		Memory allocated to hold unflattened aligned FDT
> >   * @overlay_tree:	expanded device tree that contains the fragment nodes
> >   * @count:		count of fragment structures
> >   * @fragments:		fragment nodes in the overlay expanded device tree
> > @@ -67,7 +67,7 @@ struct fragment {
> >  struct overlay_changeset {
> >  	int id;
> >  	struct list_head ovcs_list;
> > -	const void *fdt;
> > +	const void *new_fdt;
> >  	struct device_node *overlay_tree;
> >  	int count;
> >  	struct fragment *fragments;
> > @@ -718,19 +718,20 @@ static struct device_node *find_target(struct device_node *info_node)
> >  
> >  /**
> >   * init_overlay_changeset() - initialize overlay changeset from overlay tree
> > - * @ovcs:	Overlay changeset to build
> > - * @fdt:	base of memory allocated to hold aligned FDT that was unflattened to create @tree
> > - * @tree:	Contains the overlay fragments and overlay fixup nodes
> > + * @ovcs:		Overlay changeset to build
> > + * @new_fdt:		Memory allocated to hold unflattened aligned FDT
> > + * @overlay_tree:	Contains the overlay fragments and overlay fixup nodes
> >   *
> >   * Initialize @ovcs.  Populate @ovcs->fragments with node information from
> > - * the top level of @tree.  The relevant top level nodes are the fragment
> > - * nodes and the __symbols__ node.  Any other top level node will be ignored.
> > + * the top level of @overlay_tree.  The relevant top level nodes are the
> > + * fragment nodes and the __symbols__ node.  Any other top level node will
> > + * be ignored.
> >   *
> >   * Return: 0 on success, -ENOMEM if memory allocation failure, -EINVAL if error
> > - * detected in @tree, or -ENOSPC if idr_alloc() error.
> > + * detected in @overlay_tree, or -ENOSPC if idr_alloc() error.
> >   */
> >  static int init_overlay_changeset(struct overlay_changeset *ovcs,
> > -		const void *fdt, struct device_node *tree)
> > +		const void *new_fdt, struct device_node *overlay_tree)
> >  {
> >  	struct device_node *node, *overlay_node;
> >  	struct fragment *fragment;
> > @@ -741,17 +742,17 @@ static int init_overlay_changeset(struct overlay_changeset *ovcs,
> >  	 * Warn for some issues.  Can not return -EINVAL for these until
> >  	 * of_unittest_apply_overlay() is fixed to pass these checks.
> >  	 */
> > -	if (!of_node_check_flag(tree, OF_DYNAMIC))
> > -		pr_debug("%s() tree is not dynamic\n", __func__);
> > +	if (!of_node_check_flag(overlay_tree, OF_DYNAMIC))
> > +		pr_debug("%s() overlay_tree is not dynamic\n", __func__);
> >  
> > -	if (!of_node_check_flag(tree, OF_DETACHED))
> > -		pr_debug("%s() tree is not detached\n", __func__);
> > +	if (!of_node_check_flag(overlay_tree, OF_DETACHED))
> > +		pr_debug("%s() overlay_tree is not detached\n", __func__);
> >  
> > -	if (!of_node_is_root(tree))
> > -		pr_debug("%s() tree is not root\n", __func__);
> > +	if (!of_node_is_root(overlay_tree))
> > +		pr_debug("%s() overlay_tree is not root\n", __func__);
> >  
> > -	ovcs->overlay_tree = tree;
> > -	ovcs->fdt = fdt;
> > +	ovcs->overlay_tree = overlay_tree;
> > +	ovcs->new_fdt = new_fdt;
> >  
> >  	INIT_LIST_HEAD(&ovcs->ovcs_list);
> >  
> > @@ -764,7 +765,7 @@ static int init_overlay_changeset(struct overlay_changeset *ovcs,
> >  	cnt = 0;
> >  
> >  	/* fragment nodes */
> > -	for_each_child_of_node(tree, node) {
> > +	for_each_child_of_node(overlay_tree, node) {
> >  		overlay_node = of_get_child_by_name(node, "__overlay__");
> >  		if (overlay_node) {
> >  			cnt++;
> > @@ -772,7 +773,7 @@ static int init_overlay_changeset(struct overlay_changeset *ovcs,
> >  		}
> >  	}
> >  
> > -	node = of_get_child_by_name(tree, "__symbols__");
> > +	node = of_get_child_by_name(overlay_tree, "__symbols__");
> >  	if (node) {
> >  		cnt++;
> >  		of_node_put(node);
> > @@ -785,7 +786,7 @@ static int init_overlay_changeset(struct overlay_changeset *ovcs,
> >  	}
> >  
> >  	cnt = 0;
> > -	for_each_child_of_node(tree, node) {
> > +	for_each_child_of_node(overlay_tree, node) {
> >  		overlay_node = of_get_child_by_name(node, "__overlay__");
> >  		if (!overlay_node)
> >  			continue;
> > @@ -807,7 +808,7 @@ static int init_overlay_changeset(struct overlay_changeset *ovcs,
> >  	 * if there is a symbols fragment in ovcs->fragments[i] it is
> >  	 * the final element in the array
> >  	 */
> > -	node = of_get_child_by_name(tree, "__symbols__");
> > +	node = of_get_child_by_name(overlay_tree, "__symbols__");
> >  	if (node) {
> >  		ovcs->symbols_fragment = 1;
> >  		fragment = &fragments[cnt];
> > @@ -862,11 +863,11 @@ static void free_overlay_changeset(struct overlay_changeset *ovcs)
> >  	kfree(ovcs->fragments);
> >  	/*
> >  	 * There should be no live pointers into ovcs->overlay_tree and
> > -	 * ovcs->fdt due to the policy that overlay notifiers are not allowed
> > -	 * to retain pointers into the overlay devicetree.
> > +	 * ovcs->new_fdt due to the policy that overlay notifiers are not
> > +	 * allowed to retain pointers into the overlay devicetree.
> >  	 */
> >  	kfree(ovcs->overlay_tree);
> > -	kfree(ovcs->fdt);
> > +	kfree(ovcs->new_fdt);
> >  	kfree(ovcs);
> >  }
> >  
> > @@ -874,16 +875,15 @@ static void free_overlay_changeset(struct overlay_changeset *ovcs)
> >   * internal documentation
> >   *
> >   * of_overlay_apply() - Create and apply an overlay changeset
> > - * @fdt:	base of memory allocated to hold the aligned FDT
> > - * @tree:	Expanded overlay device tree
> > - * @ovcs_id:	Pointer to overlay changeset id
> > + * @new_fdt:		Memory allocated to hold the aligned FDT
> > + * @overlay_tree:	Expanded overlay device tree
> > + * @ovcs_id:		Pointer to overlay changeset id
> >   *
> >   * Creates and applies an overlay changeset.
> >   *
> >   * If an error occurs in a pre-apply notifier, then no changes are made
> >   * to the device tree.
> >   *
> > -
> >   * A non-zero return value will not have created the changeset if error is from:
> >   *   - parameter checks
> >   *   - building the changeset
> > @@ -913,29 +913,29 @@ static void free_overlay_changeset(struct overlay_changeset *ovcs)
> >   * id is returned to *ovcs_id.
> >   */
> >  
> > -static int of_overlay_apply(const void *fdt, struct device_node *tree,
> > -		int *ovcs_id)
> > +static int of_overlay_apply(const void *new_fdt,
> > +		struct device_node *overlay_tree, int *ovcs_id)
> >  {
> >  	struct overlay_changeset *ovcs;
> >  	int ret = 0, ret_revert, ret_tmp;
> >  
> >  	/*
> > -	 * As of this point, fdt and tree belong to the overlay changeset.
> > -	 * overlay changeset code is responsible for freeing them.
> > +	 * As of this point, new_fdt and overlay_tree belong to the overlay
> > +	 * changeset.  overlay changeset code is responsible for freeing them.
> >  	 */
> >  
> >  	if (devicetree_corrupt()) {
> >  		pr_err("devicetree state suspect, refuse to apply overlay\n");
> > -		kfree(fdt);
> > -		kfree(tree);
> > +		kfree(new_fdt);
> > +		kfree(overlay_tree);
> >  		ret = -EBUSY;
> >  		goto out;
> >  	}
> >  
> >  	ovcs = kzalloc(sizeof(*ovcs), GFP_KERNEL);
> >  	if (!ovcs) {
> > -		kfree(fdt);
> > -		kfree(tree);
> > +		kfree(new_fdt);
> > +		kfree(overlay_tree);
> >  		ret = -ENOMEM;
> >  		goto out;
> >  	}
> > @@ -943,20 +943,20 @@ static int of_overlay_apply(const void *fdt, struct device_node *tree,
> >  	of_overlay_mutex_lock();
> >  	mutex_lock(&of_mutex);
> >  
> > -	ret = of_resolve_phandles(tree);
> > +	ret = of_resolve_phandles(overlay_tree);
> >  	if (ret)
> > -		goto err_free_tree;
> > +		goto err_free_overlay_tree;
> >  
> > -	ret = init_overlay_changeset(ovcs, fdt, tree);
> > +	ret = init_overlay_changeset(ovcs, new_fdt, overlay_tree);
> >  	if (ret)
> > -		goto err_free_tree;
> > +		goto err_free_overlay_tree;
> >  
> >  	/*
> >  	 * after overlay_notify(), ovcs->overlay_tree related pointers may have
> > -	 * leaked to drivers, so can not kfree() tree, aka ovcs->overlay_tree;
> > -	 * and can not free memory containing aligned fdt.  The aligned fdt
> > -	 * is contained within the memory at ovcs->fdt, possibly at an offset
> > -	 * from ovcs->fdt.
> > +	 * leaked to drivers, so can not kfree() overlay_tree,
> > +	 * aka ovcs->overlay_tree; and can not free memory containing aligned
> > +	 * fdt.  The aligned fdt is contained within the memory at
> > +	 * ovcs->new_fdt, possibly at an offset from ovcs->new_fdt.
> >  	 */
> >  	ret = overlay_notify(ovcs, OF_OVERLAY_PRE_APPLY);
> >  	if (ret) {
> > @@ -997,9 +997,9 @@ static int of_overlay_apply(const void *fdt, struct device_node *tree,
> >  
> >  	goto out_unlock;
> >  
> > -err_free_tree:
> > -	kfree(fdt);
> > -	kfree(tree);
> > +err_free_overlay_tree:
> > +	kfree(new_fdt);
> > +	kfree(overlay_tree);
> >  
> >  err_free_overlay_changeset:
> >  	free_overlay_changeset(ovcs);
> 

-- 
Slawomir Stepien

Re: [PATCH v2 2/2] of: overlay: rework overlay apply and remove kfree()s

[Rob Herring]

  On Sun, Apr 10, 2022 at 4:08 PM <frowand.list@gmail.com> wrote:
>
> From: Frank Rowand <frank.rowand@sony.com>
>
> Fix various kfree() issues related to of_overlay_apply().
>   - Double kfree() of fdt and tree when init_overlay_changeset()
>     returns an error.
>   - free_overlay_changeset() free the root of the unflattened
>     overlay (variable tree) instead of the memory that contains
>     the unflattened overlay.
>   - For the case of a failure during applying an overlay, move kfree()
>     of new_fdt and overlay_mem into the function that allocated them.
>     For the case of removing an overlay, the kfree() remains in
>     free_overlay_changeset().
>   - Check return value of of_fdt_unflatten_tree() for error instead
>     of checking the returnded value of overlay_root.
>
> More clearly document policy related to lifetime of pointers into
> overlay memory.
>
> Double kfree()
> Reported-by: Slawomir Stepien <slawomir.stepien@nokia.com>
>
> Signed-off-by: Frank Rowand <frank.rowand@sony.com>
> ---
>
> Changes since v1:
>   - Move kfree()s from init_overlay_changeset() to of_overlay_fdt_apply()
>   - Better document lifetime of pointers into overlay, both in overlay.c
>     and Documentation/devicetree/overlay-notes.rst
>
>  Documentation/devicetree/overlay-notes.rst |  23 +++-
>  drivers/of/overlay.c                       | 127 ++++++++++++---------
>  2 files changed, 91 insertions(+), 59 deletions(-)
>
> diff --git a/Documentation/devicetree/overlay-notes.rst b/Documentation/devicetree/overlay-notes.rst
> index b2b8db765b8c..7a6e85f75567 100644
> --- a/Documentation/devicetree/overlay-notes.rst
> +++ b/Documentation/devicetree/overlay-notes.rst
> @@ -119,10 +119,25 @@ Finally, if you need to remove all overlays in one-go, just call
>  of_overlay_remove_all() which will remove every single one in the correct
>  order.
>
> -In addition, there is the option to register notifiers that get called on
> +There is the option to register notifiers that get called on
>  overlay operations. See of_overlay_notifier_register/unregister and
>  enum of_overlay_notify_action for details.
>
> -Note that a notifier callback is not supposed to store pointers to a device
> -tree node or its content beyond OF_OVERLAY_POST_REMOVE corresponding to the
> -respective node it received.
> +A notifier callback for OF_OVERLAY_PRE_APPLY, OF_OVERLAY_POST_APPLY, or
> +OF_OVERLAY_PRE_REMOVE may store pointers to a device tree node in the overlay
> +or its content but these pointers must not persist past the notifier callback
> +for OF_OVERLAY_POST_REMOVE.  The memory containing the overlay will be
> +kfree()ed after OF_OVERLAY_POST_REMOVE notifiers are called.  Note that the
> +memory will be kfree()ed even if the notifier for OF_OVERLAY_POST_REMOVE
> +returns an error.
> +
> +The changeset notifiers in drivers/of/dynamic.c are a second type of notifier
> +that could be triggered by applying or removing an overlay.  These notifiers
> +are not allowed to store pointers to a device tree node in the overlay
> +or its content.  The overlay code does not protect against such pointers
> +remaining active when the memory containing the overlay is freed as a result
> +of removing the overlay.
> +
> +Any other code that retains a pointer to the overlay nodes or data is
> +considered to be a bug because after removing the overlay the pointer
> +will refer to freed memory.
> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
> index f74aa9ff67aa..c8e999518f2f 100644
> --- a/drivers/of/overlay.c
> +++ b/drivers/of/overlay.c
> @@ -58,6 +58,7 @@ struct fragment {
>   * @id:                        changeset identifier
>   * @ovcs_list:         list on which we are located
>   * @new_fdt:           Memory allocated to hold unflattened aligned FDT
> + * @overlay_mem:       the memory chunk that contains @overlay_tree
>   * @overlay_tree:      expanded device tree that contains the fragment nodes
>   * @count:             count of fragment structures
>   * @fragments:         fragment nodes in the overlay expanded device tree
> @@ -68,6 +69,7 @@ struct overlay_changeset {
>         int id;
>         struct list_head ovcs_list;
>         const void *new_fdt;
> +       const void *overlay_mem;
>         struct device_node *overlay_tree;
>         int count;
>         struct fragment *fragments;
> @@ -720,18 +722,20 @@ static struct device_node *find_target(struct device_node *info_node)
>   * init_overlay_changeset() - initialize overlay changeset from overlay tree
>   * @ovcs:              Overlay changeset to build
>   * @new_fdt:           Memory allocated to hold unflattened aligned FDT
> + * @tree_mem:          Memory that contains @overlay_tree
>   * @overlay_tree:      Contains the overlay fragments and overlay fixup nodes
>   *
>   * Initialize @ovcs.  Populate @ovcs->fragments with node information from
>   * the top level of @overlay_tree.  The relevant top level nodes are the
>   * fragment nodes and the __symbols__ node.  Any other top level node will
> - * be ignored.
> + * be ignored.  Populate other @ovcs fields.
>   *
>   * Return: 0 on success, -ENOMEM if memory allocation failure, -EINVAL if error
>   * detected in @overlay_tree, or -ENOSPC if idr_alloc() error.
>   */
>  static int init_overlay_changeset(struct overlay_changeset *ovcs,
> -               const void *new_fdt, struct device_node *overlay_tree)
> +               const void *new_fdt, const void *tree_mem,
> +               struct device_node *overlay_tree)
>  {
>         struct device_node *node, *overlay_node;
>         struct fragment *fragment;
> @@ -751,9 +755,6 @@ static int init_overlay_changeset(struct overlay_changeset *ovcs,
>         if (!of_node_is_root(overlay_tree))
>                 pr_debug("%s() overlay_tree is not root\n", __func__);
>
> -       ovcs->overlay_tree = overlay_tree;
> -       ovcs->new_fdt = new_fdt;
> -
>         INIT_LIST_HEAD(&ovcs->ovcs_list);
>
>         of_changeset_init(&ovcs->cset);
> @@ -832,6 +833,9 @@ static int init_overlay_changeset(struct overlay_changeset *ovcs,
>
>         ovcs->id = id;
>         ovcs->count = cnt;
> +       ovcs->new_fdt = new_fdt;
> +       ovcs->overlay_mem = tree_mem;
> +       ovcs->overlay_tree = overlay_tree;
>         ovcs->fragments = fragments;
>
>         return 0;
> @@ -846,7 +850,7 @@ static int init_overlay_changeset(struct overlay_changeset *ovcs,
>         return ret;
>  }
>
> -static void free_overlay_changeset(struct overlay_changeset *ovcs)
> +static void free_overlay_changeset_contents(struct overlay_changeset *ovcs)
>  {
>         int i;
>
> @@ -861,12 +865,20 @@ static void free_overlay_changeset(struct overlay_changeset *ovcs)
>                 of_node_put(ovcs->fragments[i].overlay);
>         }
>         kfree(ovcs->fragments);
> +}
> +static void free_overlay_changeset(struct overlay_changeset *ovcs)
> +{
> +
> +       free_overlay_changeset_contents(ovcs);
> +
>         /*
> -        * There should be no live pointers into ovcs->overlay_tree and
> +        * There should be no live pointers into ovcs->overlay_mem and
>          * ovcs->new_fdt due to the policy that overlay notifiers are not
> -        * allowed to retain pointers into the overlay devicetree.
> +        * allowed to retain pointers into the overlay devicetree other
> +        * than the window between OF_OVERLAY_PRE_APPLY overlay notifiers
> +        * and the OF_OVERLAY_POST_REMOVE overlay notifiers.
>          */
> -       kfree(ovcs->overlay_tree);
> +       kfree(ovcs->overlay_mem);
>         kfree(ovcs->new_fdt);
>         kfree(ovcs);
>  }
> @@ -876,8 +888,10 @@ static void free_overlay_changeset(struct overlay_changeset *ovcs)
>   *
>   * of_overlay_apply() - Create and apply an overlay changeset
>   * @new_fdt:           Memory allocated to hold the aligned FDT
> + * @tree_mem:          Memory that contains @overlay_tree
>   * @overlay_tree:      Expanded overlay device tree
>   * @ovcs_id:           Pointer to overlay changeset id
> + * @kfree_unsafe:      Pointer to flag to not kfree() @new_fdt and @overlay_tree

This screams hack.

It would be somewhat less hacky if we stored some state information in
ovcs struct that conveys the apply state of the overlay and then used
that to determine if we should do kfree or not.

Perhaps a better fix would be refcounting the overlay as a whole and
freeing everything when the refcount goes to 0.

>   *
>   * Creates and applies an overlay changeset.
>   *
> @@ -910,34 +924,25 @@ static void free_overlay_changeset(struct overlay_changeset *ovcs)
>   * refused.
>   *
>   * Returns 0 on success, or a negative error number.  Overlay changeset
> - * id is returned to *ovcs_id.
> + * id is returned to *ovcs_id.  When references to @new_fdt and @overlay_tree
> + * may exist, *kfree_unsafe is set to true.
>   */
>
> -static int of_overlay_apply(const void *new_fdt,
> -               struct device_node *overlay_tree, int *ovcs_id)
> +static int of_overlay_apply(const void *new_fdt, void *tree_mem,
> +               struct device_node *overlay_tree, int *ovcs_id,
> +               bool *kfree_unsafe)
>  {
>         struct overlay_changeset *ovcs;
>         int ret = 0, ret_revert, ret_tmp;
>
> -       /*
> -        * As of this point, new_fdt and overlay_tree belong to the overlay
> -        * changeset.  overlay changeset code is responsible for freeing them.
> -        */
> -
>         if (devicetree_corrupt()) {
>                 pr_err("devicetree state suspect, refuse to apply overlay\n");
> -               kfree(new_fdt);
> -               kfree(overlay_tree);
> -               ret = -EBUSY;
> -               goto out;
> +               return -EBUSY;
>         }
>
>         ovcs = kzalloc(sizeof(*ovcs), GFP_KERNEL);
>         if (!ovcs) {
> -               kfree(new_fdt);
> -               kfree(overlay_tree);
> -               ret = -ENOMEM;
> -               goto out;
> +               return -ENOMEM;
>         }
>
>         of_overlay_mutex_lock();
> @@ -945,28 +950,27 @@ static int of_overlay_apply(const void *new_fdt,
>
>         ret = of_resolve_phandles(overlay_tree);
>         if (ret)
> -               goto err_free_overlay_tree;
> +               goto err_free_ovcs;
>
> -       ret = init_overlay_changeset(ovcs, new_fdt, overlay_tree);
> +       ret = init_overlay_changeset(ovcs, new_fdt, tree_mem, overlay_tree);
>         if (ret)
> -               goto err_free_overlay_tree;
> +               goto err_free_ovcs_contents;
>
>         /*
> -        * after overlay_notify(), ovcs->overlay_tree related pointers may have
> -        * leaked to drivers, so can not kfree() overlay_tree,
> -        * aka ovcs->overlay_tree; and can not free memory containing aligned
> -        * fdt.  The aligned fdt is contained within the memory at
> -        * ovcs->new_fdt, possibly at an offset from ovcs->new_fdt.
> +        * After overlay_notify(), ovcs->overlay_tree related pointers may have
> +        * leaked to drivers, so can not kfree() ovcs->overlay_mem and
> +        * ovcs->new_fdt until after OF_OVERLAY_POST_REMOVE notifiers.
>          */
> +       *kfree_unsafe = true;
>         ret = overlay_notify(ovcs, OF_OVERLAY_PRE_APPLY);
>         if (ret) {

If OF_OVERLAY_PRE_APPLY failed, I would think kfree would still be okay.

>                 pr_err("overlay changeset pre-apply notify error %d\n", ret);
> -               goto err_free_overlay_changeset;
> +               goto err_free_ovcs_contents;
>         }

Re: [PATCH v2 2/2] of: overlay: rework overlay apply and remove kfree()s

[Frank Rowand]

On 4/12/22 09:00, Rob Herring wrote:
>   On Sun, Apr 10, 2022 at 4:08 PM <frowand.list@gmail.com> wrote:
>>
>> From: Frank Rowand <frank.rowand@sony.com>
>>
>> Fix various kfree() issues related to of_overlay_apply().
>>   - Double kfree() of fdt and tree when init_overlay_changeset()
>>     returns an error.
>>   - free_overlay_changeset() free the root of the unflattened
>>     overlay (variable tree) instead of the memory that contains
>>     the unflattened overlay.
>>   - For the case of a failure during applying an overlay, move kfree()
>>     of new_fdt and overlay_mem into the function that allocated them.
>>     For the case of removing an overlay, the kfree() remains in
>>     free_overlay_changeset().
>>   - Check return value of of_fdt_unflatten_tree() for error instead
>>     of checking the returnded value of overlay_root.
>>
>> More clearly document policy related to lifetime of pointers into
>> overlay memory.
>>
>> Double kfree()
>> Reported-by: Slawomir Stepien <slawomir.stepien@nokia.com>
>>
>> Signed-off-by: Frank Rowand <frank.rowand@sony.com>
>> ---
>>
>> Changes since v1:
>>   - Move kfree()s from init_overlay_changeset() to of_overlay_fdt_apply()
>>   - Better document lifetime of pointers into overlay, both in overlay.c
>>     and Documentation/devicetree/overlay-notes.rst
>>
>>  Documentation/devicetree/overlay-notes.rst |  23 +++-
>>  drivers/of/overlay.c                       | 127 ++++++++++++---------
>>  2 files changed, 91 insertions(+), 59 deletions(-)
>>
>> diff --git a/Documentation/devicetree/overlay-notes.rst b/Documentation/devicetree/overlay-notes.rst
>> index b2b8db765b8c..7a6e85f75567 100644
>> --- a/Documentation/devicetree/overlay-notes.rst
>> +++ b/Documentation/devicetree/overlay-notes.rst
>> @@ -119,10 +119,25 @@ Finally, if you need to remove all overlays in one-go, just call
>>  of_overlay_remove_all() which will remove every single one in the correct
>>  order.
>>
>> -In addition, there is the option to register notifiers that get called on
>> +There is the option to register notifiers that get called on
>>  overlay operations. See of_overlay_notifier_register/unregister and
>>  enum of_overlay_notify_action for details.
>>
>> -Note that a notifier callback is not supposed to store pointers to a device
>> -tree node or its content beyond OF_OVERLAY_POST_REMOVE corresponding to the
>> -respective node it received.
>> +A notifier callback for OF_OVERLAY_PRE_APPLY, OF_OVERLAY_POST_APPLY, or
>> +OF_OVERLAY_PRE_REMOVE may store pointers to a device tree node in the overlay
>> +or its content but these pointers must not persist past the notifier callback
>> +for OF_OVERLAY_POST_REMOVE.  The memory containing the overlay will be
>> +kfree()ed after OF_OVERLAY_POST_REMOVE notifiers are called.  Note that the
>> +memory will be kfree()ed even if the notifier for OF_OVERLAY_POST_REMOVE
>> +returns an error.
>> +
>> +The changeset notifiers in drivers/of/dynamic.c are a second type of notifier
>> +that could be triggered by applying or removing an overlay.  These notifiers
>> +are not allowed to store pointers to a device tree node in the overlay
>> +or its content.  The overlay code does not protect against such pointers
>> +remaining active when the memory containing the overlay is freed as a result
>> +of removing the overlay.
>> +
>> +Any other code that retains a pointer to the overlay nodes or data is
>> +considered to be a bug because after removing the overlay the pointer
>> +will refer to freed memory.
>> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
>> index f74aa9ff67aa..c8e999518f2f 100644
>> --- a/drivers/of/overlay.c
>> +++ b/drivers/of/overlay.c
>> @@ -58,6 +58,7 @@ struct fragment {
>>   * @id:                        changeset identifier
>>   * @ovcs_list:         list on which we are located
>>   * @new_fdt:           Memory allocated to hold unflattened aligned FDT
>> + * @overlay_mem:       the memory chunk that contains @overlay_tree
>>   * @overlay_tree:      expanded device tree that contains the fragment nodes
>>   * @count:             count of fragment structures
>>   * @fragments:         fragment nodes in the overlay expanded device tree
>> @@ -68,6 +69,7 @@ struct overlay_changeset {
>>         int id;
>>         struct list_head ovcs_list;
>>         const void *new_fdt;
>> +       const void *overlay_mem;
>>         struct device_node *overlay_tree;
>>         int count;
>>         struct fragment *fragments;
>> @@ -720,18 +722,20 @@ static struct device_node *find_target(struct device_node *info_node)
>>   * init_overlay_changeset() - initialize overlay changeset from overlay tree
>>   * @ovcs:              Overlay changeset to build
>>   * @new_fdt:           Memory allocated to hold unflattened aligned FDT
>> + * @tree_mem:          Memory that contains @overlay_tree
>>   * @overlay_tree:      Contains the overlay fragments and overlay fixup nodes
>>   *
>>   * Initialize @ovcs.  Populate @ovcs->fragments with node information from
>>   * the top level of @overlay_tree.  The relevant top level nodes are the
>>   * fragment nodes and the __symbols__ node.  Any other top level node will
>> - * be ignored.
>> + * be ignored.  Populate other @ovcs fields.
>>   *
>>   * Return: 0 on success, -ENOMEM if memory allocation failure, -EINVAL if error
>>   * detected in @overlay_tree, or -ENOSPC if idr_alloc() error.
>>   */
>>  static int init_overlay_changeset(struct overlay_changeset *ovcs,
>> -               const void *new_fdt, struct device_node *overlay_tree)
>> +               const void *new_fdt, const void *tree_mem,
>> +               struct device_node *overlay_tree)
>>  {
>>         struct device_node *node, *overlay_node;
>>         struct fragment *fragment;
>> @@ -751,9 +755,6 @@ static int init_overlay_changeset(struct overlay_changeset *ovcs,
>>         if (!of_node_is_root(overlay_tree))
>>                 pr_debug("%s() overlay_tree is not root\n", __func__);
>>
>> -       ovcs->overlay_tree = overlay_tree;
>> -       ovcs->new_fdt = new_fdt;
>> -
>>         INIT_LIST_HEAD(&ovcs->ovcs_list);
>>
>>         of_changeset_init(&ovcs->cset);
>> @@ -832,6 +833,9 @@ static int init_overlay_changeset(struct overlay_changeset *ovcs,
>>
>>         ovcs->id = id;
>>         ovcs->count = cnt;
>> +       ovcs->new_fdt = new_fdt;
>> +       ovcs->overlay_mem = tree_mem;
>> +       ovcs->overlay_tree = overlay_tree;
>>         ovcs->fragments = fragments;
>>
>>         return 0;
>> @@ -846,7 +850,7 @@ static int init_overlay_changeset(struct overlay_changeset *ovcs,
>>         return ret;
>>  }
>>
>> -static void free_overlay_changeset(struct overlay_changeset *ovcs)
>> +static void free_overlay_changeset_contents(struct overlay_changeset *ovcs)
>>  {
>>         int i;
>>
>> @@ -861,12 +865,20 @@ static void free_overlay_changeset(struct overlay_changeset *ovcs)
>>                 of_node_put(ovcs->fragments[i].overlay);
>>         }
>>         kfree(ovcs->fragments);
>> +}
>> +static void free_overlay_changeset(struct overlay_changeset *ovcs)
>> +{
>> +
>> +       free_overlay_changeset_contents(ovcs);
>> +
>>         /*
>> -        * There should be no live pointers into ovcs->overlay_tree and
>> +        * There should be no live pointers into ovcs->overlay_mem and
>>          * ovcs->new_fdt due to the policy that overlay notifiers are not
>> -        * allowed to retain pointers into the overlay devicetree.
>> +        * allowed to retain pointers into the overlay devicetree other
>> +        * than the window between OF_OVERLAY_PRE_APPLY overlay notifiers
>> +        * and the OF_OVERLAY_POST_REMOVE overlay notifiers.
>>          */
>> -       kfree(ovcs->overlay_tree);
>> +       kfree(ovcs->overlay_mem);
>>         kfree(ovcs->new_fdt);
>>         kfree(ovcs);
>>  }
>> @@ -876,8 +888,10 @@ static void free_overlay_changeset(struct overlay_changeset *ovcs)
>>   *
>>   * of_overlay_apply() - Create and apply an overlay changeset
>>   * @new_fdt:           Memory allocated to hold the aligned FDT
>> + * @tree_mem:          Memory that contains @overlay_tree
>>   * @overlay_tree:      Expanded overlay device tree
>>   * @ovcs_id:           Pointer to overlay changeset id
>> + * @kfree_unsafe:      Pointer to flag to not kfree() @new_fdt and @overlay_tree
> 
> This screams hack.

I agree that it screams hack.

> 
> It would be somewhat less hacky if we stored some state information in
> ovcs struct that conveys the apply state of the overlay and then used
> that to determine if we should do kfree or not.

I tried this approach first, and thought the result was rather ugly.  But
it is possible if you prefer.  If the state was stored in ovcs, then
the kfree() code at the end of of_overlay_fdt_apply() would change from:


out_free_overlay_mem:
        if (!kfree_unsafe)
                kfree(overlay_mem);

out_free_new_fdt:
        if (!kfree_unsafe)
                kfree(new_fdt);

        return ret;


to something like (untested, thrown together):


out_free_overlay_mem:
        mutex_lock(&of_mutex);

        ovcs = idr_find(&ovcs_idr, *ovcs_id);
        if (!ovcs) {
                ret = -ENODEV;
                pr_err("remove: Could not find overlay #%d for kfree(overlay_mem)\n", *ovcs_id);
                goto out_unlock;
        }

        if (!ovcs->kfree_unsafe)
                kfree(overlay_mem);

        mutex_unlock(&of_mutex);

out_free_new_fdt:
        mutex_lock(&of_mutex); 

        ovcs = idr_find(&ovcs_idr, *ovcs_id);
        if (!ovcs) {
                ret = -ENODEV;
                pr_err("remove: Could not find overlay #%d for kfree(new_fdt)\n", *ovcs_id);
                goto out_unlock;
        }

        if (!ovcs->kfree_unsafe)
                kfree(new_fdt);

out_unlock:
        mutex_unlock(&of_mutex);

        return ret;


The upside is that new_fdt and overlay_mem would no longer be arguments to
of_overlay_apply() and its children.

> 
> Perhaps a better fix would be refcounting the overlay as a whole and
> freeing everything when the refcount goes to 0.

We already know when the overlay as a whole is no longer referenced -- it
is when of_overlay_remove() is successful.  The refcount would instead be
on new_fdt and overlay_mem.  But the location of incrementing the refcount
is in of_overlay_apply() and its children, so the ugliness of passing
new_fdt and overlay_mem into those functions remains.

I am fine with the "hacky" approach or the saving more state in ovcs.  I
would prefer not to use the refcount approach.

If the "hacky" approach is used, I would do a v3 that adds an additional
comment in of_overlay_remove() that describes the window in that function
where a memory leak can still occur.

> 
>>   *
>>   * Creates and applies an overlay changeset.
>>   *
>> @@ -910,34 +924,25 @@ static void free_overlay_changeset(struct overlay_changeset *ovcs)
>>   * refused.
>>   *
>>   * Returns 0 on success, or a negative error number.  Overlay changeset
>> - * id is returned to *ovcs_id.
>> + * id is returned to *ovcs_id.  When references to @new_fdt and @overlay_tree
>> + * may exist, *kfree_unsafe is set to true.
>>   */
>>
>> -static int of_overlay_apply(const void *new_fdt,
>> -               struct device_node *overlay_tree, int *ovcs_id)
>> +static int of_overlay_apply(const void *new_fdt, void *tree_mem,
>> +               struct device_node *overlay_tree, int *ovcs_id,
>> +               bool *kfree_unsafe)
>>  {
>>         struct overlay_changeset *ovcs;
>>         int ret = 0, ret_revert, ret_tmp;
>>
>> -       /*
>> -        * As of this point, new_fdt and overlay_tree belong to the overlay
>> -        * changeset.  overlay changeset code is responsible for freeing them.
>> -        */
>> -
>>         if (devicetree_corrupt()) {
>>                 pr_err("devicetree state suspect, refuse to apply overlay\n");
>> -               kfree(new_fdt);
>> -               kfree(overlay_tree);
>> -               ret = -EBUSY;
>> -               goto out;
>> +               return -EBUSY;
>>         }
>>
>>         ovcs = kzalloc(sizeof(*ovcs), GFP_KERNEL);
>>         if (!ovcs) {
>> -               kfree(new_fdt);
>> -               kfree(overlay_tree);
>> -               ret = -ENOMEM;
>> -               goto out;
>> +               return -ENOMEM;
>>         }
>>
>>         of_overlay_mutex_lock();
>> @@ -945,28 +950,27 @@ static int of_overlay_apply(const void *new_fdt,
>>
>>         ret = of_resolve_phandles(overlay_tree);
>>         if (ret)
>> -               goto err_free_overlay_tree;
>> +               goto err_free_ovcs;
>>
>> -       ret = init_overlay_changeset(ovcs, new_fdt, overlay_tree);
>> +       ret = init_overlay_changeset(ovcs, new_fdt, tree_mem, overlay_tree);
>>         if (ret)
>> -               goto err_free_overlay_tree;
>> +               goto err_free_ovcs_contents;
>>
>>         /*
>> -        * after overlay_notify(), ovcs->overlay_tree related pointers may have
>> -        * leaked to drivers, so can not kfree() overlay_tree,
>> -        * aka ovcs->overlay_tree; and can not free memory containing aligned
>> -        * fdt.  The aligned fdt is contained within the memory at
>> -        * ovcs->new_fdt, possibly at an offset from ovcs->new_fdt.
>> +        * After overlay_notify(), ovcs->overlay_tree related pointers may have
>> +        * leaked to drivers, so can not kfree() ovcs->overlay_mem and
>> +        * ovcs->new_fdt until after OF_OVERLAY_POST_REMOVE notifiers.
>>          */
>> +       *kfree_unsafe = true;
>>         ret = overlay_notify(ovcs, OF_OVERLAY_PRE_APPLY);
>>         if (ret) {
> 
> If OF_OVERLAY_PRE_APPLY failed, I would think kfree would still be okay.

Not with the existing policy.

If we tightened up the policy so that the OF_OVERLAY_PRE_APPLY handler is required
to release all references to the overlay memory even with an error return then the
kfree would be ok if OF_OVERLAY_PRE_APPLY failed.

> 
>>                 pr_err("overlay changeset pre-apply notify error %d\n", ret);
>> -               goto err_free_overlay_changeset;
>> +               goto err_free_ovcs_contents;
>>         }

Re: [PATCH v2 2/2] of: overlay: rework overlay apply and remove kfree()s

[Frank Rowand]

Hi Rob,

On 4/12/22 12:20, Frank Rowand wrote:
> On 4/12/22 09:00, Rob Herring wrote:
>>   On Sun, Apr 10, 2022 at 4:08 PM <frowand.list@gmail.com> wrote:
>>>
>>> From: Frank Rowand <frank.rowand@sony.com>
>>>
>>> Fix various kfree() issues related to of_overlay_apply().
>>>   - Double kfree() of fdt and tree when init_overlay_changeset()
>>>     returns an error.
>>>   - free_overlay_changeset() free the root of the unflattened
>>>     overlay (variable tree) instead of the memory that contains
>>>     the unflattened overlay.
>>>   - For the case of a failure during applying an overlay, move kfree()
>>>     of new_fdt and overlay_mem into the function that allocated them.
>>>     For the case of removing an overlay, the kfree() remains in
>>>     free_overlay_changeset().
>>>   - Check return value of of_fdt_unflatten_tree() for error instead
>>>     of checking the returnded value of overlay_root.
>>>
>>> More clearly document policy related to lifetime of pointers into
>>> overlay memory.
>>>
>>> Double kfree()
>>> Reported-by: Slawomir Stepien <slawomir.stepien@nokia.com>
>>>
>>> Signed-off-by: Frank Rowand <frank.rowand@sony.com>
>>> ---
>>>
>>> Changes since v1:
>>>   - Move kfree()s from init_overlay_changeset() to of_overlay_fdt_apply()
>>>   - Better document lifetime of pointers into overlay, both in overlay.c
>>>     and Documentation/devicetree/overlay-notes.rst
>>>
>>>  Documentation/devicetree/overlay-notes.rst |  23 +++-
>>>  drivers/of/overlay.c                       | 127 ++++++++++++---------
>>>  2 files changed, 91 insertions(+), 59 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/overlay-notes.rst b/Documentation/devicetree/overlay-notes.rst
>>> index b2b8db765b8c..7a6e85f75567 100644
>>> --- a/Documentation/devicetree/overlay-notes.rst
>>> +++ b/Documentation/devicetree/overlay-notes.rst
>>> @@ -119,10 +119,25 @@ Finally, if you need to remove all overlays in one-go, just call
>>>  of_overlay_remove_all() which will remove every single one in the correct
>>>  order.
>>>
>>> -In addition, there is the option to register notifiers that get called on
>>> +There is the option to register notifiers that get called on
>>>  overlay operations. See of_overlay_notifier_register/unregister and
>>>  enum of_overlay_notify_action for details.
>>>
>>> -Note that a notifier callback is not supposed to store pointers to a device
>>> -tree node or its content beyond OF_OVERLAY_POST_REMOVE corresponding to the
>>> -respective node it received.
>>> +A notifier callback for OF_OVERLAY_PRE_APPLY, OF_OVERLAY_POST_APPLY, or
>>> +OF_OVERLAY_PRE_REMOVE may store pointers to a device tree node in the overlay
>>> +or its content but these pointers must not persist past the notifier callback
>>> +for OF_OVERLAY_POST_REMOVE.  The memory containing the overlay will be
>>> +kfree()ed after OF_OVERLAY_POST_REMOVE notifiers are called.  Note that the
>>> +memory will be kfree()ed even if the notifier for OF_OVERLAY_POST_REMOVE
>>> +returns an error.
>>> +
>>> +The changeset notifiers in drivers/of/dynamic.c are a second type of notifier
>>> +that could be triggered by applying or removing an overlay.  These notifiers
>>> +are not allowed to store pointers to a device tree node in the overlay
>>> +or its content.  The overlay code does not protect against such pointers
>>> +remaining active when the memory containing the overlay is freed as a result
>>> +of removing the overlay.
>>> +
>>> +Any other code that retains a pointer to the overlay nodes or data is
>>> +considered to be a bug because after removing the overlay the pointer
>>> +will refer to freed memory.
>>> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
>>> index f74aa9ff67aa..c8e999518f2f 100644
>>> --- a/drivers/of/overlay.c
>>> +++ b/drivers/of/overlay.c
>>> @@ -58,6 +58,7 @@ struct fragment {
>>>   * @id:                        changeset identifier
>>>   * @ovcs_list:         list on which we are located
>>>   * @new_fdt:           Memory allocated to hold unflattened aligned FDT
>>> + * @overlay_mem:       the memory chunk that contains @overlay_tree
>>>   * @overlay_tree:      expanded device tree that contains the fragment nodes
>>>   * @count:             count of fragment structures
>>>   * @fragments:         fragment nodes in the overlay expanded device tree
>>> @@ -68,6 +69,7 @@ struct overlay_changeset {
>>>         int id;
>>>         struct list_head ovcs_list;
>>>         const void *new_fdt;
>>> +       const void *overlay_mem;
>>>         struct device_node *overlay_tree;
>>>         int count;
>>>         struct fragment *fragments;
>>> @@ -720,18 +722,20 @@ static struct device_node *find_target(struct device_node *info_node)
>>>   * init_overlay_changeset() - initialize overlay changeset from overlay tree
>>>   * @ovcs:              Overlay changeset to build
>>>   * @new_fdt:           Memory allocated to hold unflattened aligned FDT
>>> + * @tree_mem:          Memory that contains @overlay_tree
>>>   * @overlay_tree:      Contains the overlay fragments and overlay fixup nodes
>>>   *
>>>   * Initialize @ovcs.  Populate @ovcs->fragments with node information from
>>>   * the top level of @overlay_tree.  The relevant top level nodes are the
>>>   * fragment nodes and the __symbols__ node.  Any other top level node will
>>> - * be ignored.
>>> + * be ignored.  Populate other @ovcs fields.
>>>   *
>>>   * Return: 0 on success, -ENOMEM if memory allocation failure, -EINVAL if error
>>>   * detected in @overlay_tree, or -ENOSPC if idr_alloc() error.
>>>   */
>>>  static int init_overlay_changeset(struct overlay_changeset *ovcs,
>>> -               const void *new_fdt, struct device_node *overlay_tree)
>>> +               const void *new_fdt, const void *tree_mem,
>>> +               struct device_node *overlay_tree)
>>>  {
>>>         struct device_node *node, *overlay_node;
>>>         struct fragment *fragment;
>>> @@ -751,9 +755,6 @@ static int init_overlay_changeset(struct overlay_changeset *ovcs,
>>>         if (!of_node_is_root(overlay_tree))
>>>                 pr_debug("%s() overlay_tree is not root\n", __func__);
>>>
>>> -       ovcs->overlay_tree = overlay_tree;
>>> -       ovcs->new_fdt = new_fdt;
>>> -
>>>         INIT_LIST_HEAD(&ovcs->ovcs_list);
>>>
>>>         of_changeset_init(&ovcs->cset);
>>> @@ -832,6 +833,9 @@ static int init_overlay_changeset(struct overlay_changeset *ovcs,
>>>
>>>         ovcs->id = id;
>>>         ovcs->count = cnt;
>>> +       ovcs->new_fdt = new_fdt;
>>> +       ovcs->overlay_mem = tree_mem;
>>> +       ovcs->overlay_tree = overlay_tree;
>>>         ovcs->fragments = fragments;
>>>
>>>         return 0;
>>> @@ -846,7 +850,7 @@ static int init_overlay_changeset(struct overlay_changeset *ovcs,
>>>         return ret;
>>>  }
>>>
>>> -static void free_overlay_changeset(struct overlay_changeset *ovcs)
>>> +static void free_overlay_changeset_contents(struct overlay_changeset *ovcs)
>>>  {
>>>         int i;
>>>
>>> @@ -861,12 +865,20 @@ static void free_overlay_changeset(struct overlay_changeset *ovcs)
>>>                 of_node_put(ovcs->fragments[i].overlay);
>>>         }
>>>         kfree(ovcs->fragments);
>>> +}
>>> +static void free_overlay_changeset(struct overlay_changeset *ovcs)
>>> +{
>>> +
>>> +       free_overlay_changeset_contents(ovcs);
>>> +
>>>         /*
>>> -        * There should be no live pointers into ovcs->overlay_tree and
>>> +        * There should be no live pointers into ovcs->overlay_mem and
>>>          * ovcs->new_fdt due to the policy that overlay notifiers are not
>>> -        * allowed to retain pointers into the overlay devicetree.
>>> +        * allowed to retain pointers into the overlay devicetree other
>>> +        * than the window between OF_OVERLAY_PRE_APPLY overlay notifiers
>>> +        * and the OF_OVERLAY_POST_REMOVE overlay notifiers.
>>>          */
>>> -       kfree(ovcs->overlay_tree);
>>> +       kfree(ovcs->overlay_mem);
>>>         kfree(ovcs->new_fdt);
>>>         kfree(ovcs);
>>>  }
>>> @@ -876,8 +888,10 @@ static void free_overlay_changeset(struct overlay_changeset *ovcs)
>>>   *
>>>   * of_overlay_apply() - Create and apply an overlay changeset
>>>   * @new_fdt:           Memory allocated to hold the aligned FDT
>>> + * @tree_mem:          Memory that contains @overlay_tree
>>>   * @overlay_tree:      Expanded overlay device tree
>>>   * @ovcs_id:           Pointer to overlay changeset id
>>> + * @kfree_unsafe:      Pointer to flag to not kfree() @new_fdt and @overlay_tree
>>
>> This screams hack.
> 
> I agree that it screams hack.
> 
>>
>> It would be somewhat less hacky if we stored some state information in
>> ovcs struct that conveys the apply state of the overlay and then used
>> that to determine if we should do kfree or not.
> 
> I tried this approach first, and thought the result was rather ugly.  But
> it is possible if you prefer.  If the state was stored in ovcs, then
> the kfree() code at the end of of_overlay_fdt_apply() would change from:
> 
> 
> out_free_overlay_mem:
>         if (!kfree_unsafe)
>                 kfree(overlay_mem);
> 
> out_free_new_fdt:
>         if (!kfree_unsafe)
>                 kfree(new_fdt);
> 
>         return ret;
> 
> 
> to something like (untested, thrown together):
> 
> 
> out_free_overlay_mem:
>         mutex_lock(&of_mutex);
> 
>         ovcs = idr_find(&ovcs_idr, *ovcs_id);
>         if (!ovcs) {
>                 ret = -ENODEV;
>                 pr_err("remove: Could not find overlay #%d for kfree(overlay_mem)\n", *ovcs_id);
>                 goto out_unlock;
>         }
> 
>         if (!ovcs->kfree_unsafe)
>                 kfree(overlay_mem);
> 
>         mutex_unlock(&of_mutex);
> 
> out_free_new_fdt:
>         mutex_lock(&of_mutex); 
> 
>         ovcs = idr_find(&ovcs_idr, *ovcs_id);
>         if (!ovcs) {
>                 ret = -ENODEV;
>                 pr_err("remove: Could not find overlay #%d for kfree(new_fdt)\n", *ovcs_id);
>                 goto out_unlock;
>         }
> 
>         if (!ovcs->kfree_unsafe)
>                 kfree(new_fdt);
> 
> out_unlock:
>         mutex_unlock(&of_mutex);
> 
>         return ret;
> 
> 
> The upside is that new_fdt and overlay_mem would no longer be arguments to
> of_overlay_apply() and its children.

So that chunk above was obviously not the nicest way to implement, but it
gave me the push to accomplish the same result but cleanly.

Patch v3 is coming, much improved thanks to your comments.

-Frank

> 
>>
>> Perhaps a better fix would be refcounting the overlay as a whole and
>> freeing everything when the refcount goes to 0.
> 
> We already know when the overlay as a whole is no longer referenced -- it
> is when of_overlay_remove() is successful.  The refcount would instead be
> on new_fdt and overlay_mem.  But the location of incrementing the refcount
> is in of_overlay_apply() and its children, so the ugliness of passing
> new_fdt and overlay_mem into those functions remains.
> 
> I am fine with the "hacky" approach or the saving more state in ovcs.  I
> would prefer not to use the refcount approach.
> 
> If the "hacky" approach is used, I would do a v3 that adds an additional
> comment in of_overlay_remove() that describes the window in that function
> where a memory leak can still occur.
> 
>>
>>>   *
>>>   * Creates and applies an overlay changeset.
>>>   *
>>> @@ -910,34 +924,25 @@ static void free_overlay_changeset(struct overlay_changeset *ovcs)
>>>   * refused.
>>>   *
>>>   * Returns 0 on success, or a negative error number.  Overlay changeset
>>> - * id is returned to *ovcs_id.
>>> + * id is returned to *ovcs_id.  When references to @new_fdt and @overlay_tree
>>> + * may exist, *kfree_unsafe is set to true.
>>>   */
>>>
>>> -static int of_overlay_apply(const void *new_fdt,
>>> -               struct device_node *overlay_tree, int *ovcs_id)
>>> +static int of_overlay_apply(const void *new_fdt, void *tree_mem,
>>> +               struct device_node *overlay_tree, int *ovcs_id,
>>> +               bool *kfree_unsafe)
>>>  {
>>>         struct overlay_changeset *ovcs;
>>>         int ret = 0, ret_revert, ret_tmp;
>>>
>>> -       /*
>>> -        * As of this point, new_fdt and overlay_tree belong to the overlay
>>> -        * changeset.  overlay changeset code is responsible for freeing them.
>>> -        */
>>> -
>>>         if (devicetree_corrupt()) {
>>>                 pr_err("devicetree state suspect, refuse to apply overlay\n");
>>> -               kfree(new_fdt);
>>> -               kfree(overlay_tree);
>>> -               ret = -EBUSY;
>>> -               goto out;
>>> +               return -EBUSY;
>>>         }
>>>
>>>         ovcs = kzalloc(sizeof(*ovcs), GFP_KERNEL);
>>>         if (!ovcs) {
>>> -               kfree(new_fdt);
>>> -               kfree(overlay_tree);
>>> -               ret = -ENOMEM;
>>> -               goto out;
>>> +               return -ENOMEM;
>>>         }
>>>
>>>         of_overlay_mutex_lock();
>>> @@ -945,28 +950,27 @@ static int of_overlay_apply(const void *new_fdt,
>>>
>>>         ret = of_resolve_phandles(overlay_tree);
>>>         if (ret)
>>> -               goto err_free_overlay_tree;
>>> +               goto err_free_ovcs;
>>>
>>> -       ret = init_overlay_changeset(ovcs, new_fdt, overlay_tree);
>>> +       ret = init_overlay_changeset(ovcs, new_fdt, tree_mem, overlay_tree);
>>>         if (ret)
>>> -               goto err_free_overlay_tree;
>>> +               goto err_free_ovcs_contents;
>>>
>>>         /*
>>> -        * after overlay_notify(), ovcs->overlay_tree related pointers may have
>>> -        * leaked to drivers, so can not kfree() overlay_tree,
>>> -        * aka ovcs->overlay_tree; and can not free memory containing aligned
>>> -        * fdt.  The aligned fdt is contained within the memory at
>>> -        * ovcs->new_fdt, possibly at an offset from ovcs->new_fdt.
>>> +        * After overlay_notify(), ovcs->overlay_tree related pointers may have
>>> +        * leaked to drivers, so can not kfree() ovcs->overlay_mem and
>>> +        * ovcs->new_fdt until after OF_OVERLAY_POST_REMOVE notifiers.
>>>          */
>>> +       *kfree_unsafe = true;
>>>         ret = overlay_notify(ovcs, OF_OVERLAY_PRE_APPLY);
>>>         if (ret) {
>>
>> If OF_OVERLAY_PRE_APPLY failed, I would think kfree would still be okay.
> 
> Not with the existing policy.
> 
> If we tightened up the policy so that the OF_OVERLAY_PRE_APPLY handler is required
> to release all references to the overlay memory even with an error return then the
> kfree would be ok if OF_OVERLAY_PRE_APPLY failed.
> 
>>
>>>                 pr_err("overlay changeset pre-apply notify error %d\n", ret);
>>> -               goto err_free_overlay_changeset;
>>> +               goto err_free_ovcs_contents;
>>>         }
>