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

[PATCH v3 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.

version 2:
  - All patches updated, changelogs in each patch

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

 Documentation/devicetree/overlay-notes.rst |  30 ++-
 drivers/of/overlay.c                       | 280 ++++++++++-----------
 2 files changed, 160 insertions(+), 150 deletions(-)

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

[PATCH v3 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_root".

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 v2:
  - minor additions

 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..0a98e3254a1a 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -57,8 +57,8 @@ 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
- * @overlay_tree:	expanded device tree that contains the fragment nodes
+ * @new_fdt:		Memory allocated to hold unflattened aligned FDT
+ * @overlay_root:	expanded device tree that contains the fragment nodes
  * @count:		count of fragment structures
  * @fragments:		fragment nodes in the overlay expanded device tree
  * @symbols_fragment:	last element of @fragments[] is the  __symbols__ node
@@ -67,8 +67,8 @@ struct fragment {
 struct overlay_changeset {
 	int id;
 	struct list_head ovcs_list;
-	const void *fdt;
-	struct device_node *overlay_tree;
+	const void *new_fdt;
+	struct device_node *overlay_root;
 	int count;
 	struct fragment *fragments;
 	bool symbols_fragment;
@@ -185,7 +185,7 @@ static int overlay_notify(struct overlay_changeset *ovcs,
 
 /*
  * The values of properties in the "/__symbols__" node are paths in
- * the ovcs->overlay_tree.  When duplicating the properties, the paths
+ * the ovcs->overlay_root.  When duplicating the properties, the paths
  * need to be adjusted to be the correct path for the live device tree.
  *
  * The paths refer to a node in the subtree of a fragment node's "__overlay__"
@@ -221,7 +221,7 @@ static struct property *dup_and_fixup_symbol_prop(
 
 	if (path_len < 1)
 		return NULL;
-	fragment_node = __of_find_node_by_path(ovcs->overlay_tree, path + 1);
+	fragment_node = __of_find_node_by_path(ovcs->overlay_root, path + 1);
 	overlay_node = __of_find_node_by_path(fragment_node, "__overlay__/");
 	of_node_put(fragment_node);
 	of_node_put(overlay_node);
@@ -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_root:	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_root.  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_root, 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_root)
 {
 	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_root, OF_DYNAMIC))
+		pr_debug("%s() overlay_root 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_root, OF_DETACHED))
+		pr_debug("%s() overlay_root 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_root))
+		pr_debug("%s() overlay_root is not root\n", __func__);
 
-	ovcs->overlay_tree = tree;
-	ovcs->fdt = fdt;
+	ovcs->overlay_root = overlay_root;
+	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_root, 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_root, "__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_root, 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_root, "__symbols__");
 	if (node) {
 		ovcs->symbols_fragment = 1;
 		fragment = &fragments[cnt];
@@ -861,12 +862,12 @@ 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.
+	 * There should be no live pointers into ovcs->overlay_root and
+	 * 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->overlay_root);
+	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_root:	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,8 +913,8 @@ 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_root, int *ovcs_id)
 {
 	struct overlay_changeset *ovcs;
 	int ret = 0, ret_revert, ret_tmp;
@@ -926,16 +926,16 @@ static int of_overlay_apply(const void *fdt, struct device_node *tree,
 
 	if (devicetree_corrupt()) {
 		pr_err("devicetree state suspect, refuse to apply overlay\n");
-		kfree(fdt);
-		kfree(tree);
+		kfree(new_fdt);
+		kfree(overlay_root);
 		ret = -EBUSY;
 		goto out;
 	}
 
 	ovcs = kzalloc(sizeof(*ovcs), GFP_KERNEL);
 	if (!ovcs) {
-		kfree(fdt);
-		kfree(tree);
+		kfree(new_fdt);
+		kfree(overlay_root);
 		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_root);
 	if (ret)
 		goto err_free_tree;
 
-	ret = init_overlay_changeset(ovcs, fdt, tree);
+	ret = init_overlay_changeset(ovcs, new_fdt, overlay_root);
 	if (ret)
 		goto err_free_tree;
 
 	/*
-	 * after overlay_notify(), ovcs->overlay_tree related pointers may have
+	 * After overlay_notify(), ovcs->overlay_root 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.
+	 * 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) {
@@ -998,8 +998,8 @@ static int of_overlay_apply(const void *fdt, struct device_node *tree,
 	goto out_unlock;
 
 err_free_tree:
-	kfree(fdt);
-	kfree(tree);
+	kfree(new_fdt);
+	kfree(overlay_root);
 
 err_free_overlay_changeset:
 	free_overlay_changeset(ovcs);
-- 
Frank Rowand <frank.rowand@sony.com>

[PATCH v3 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 free_overlay_changeset(), which
    is called by the function that allocated them.
  - For the case of removing an overlay, the kfree() of new_fdt and
    overlay_mem remains in free_overlay_changeset().
  - Check return value of of_fdt_unflatten_tree() for error instead
    of checking the returned value of overlay_root.
  - When storing pointers to allocated objects in ovcs, do so as
    near to the allocation as possible instead of in deeply layered
    function.

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 v2:
  - A version 2 review comment correctly said "This screams hack".
    Restructure as listed below in response to the comment.
  - Quit passing kfree_unsafe in function parameters, move it to
    be a field of ovcs
  - Quit passing a bunch of objects as function parameters, which
    were used only for saving in ovcs
  - Save pointers to allocated objects as near to the allocation as
    possible instead of in a different function.
  - Move object allocation as early in the calling stack (starting
    at of_overlay_fdt_apply()) and move object freeing more fully
    into free_overlay_changeset(), which is called in two places:
    (1) on error path of applying overlay and (2) removal of an
    overlay by of_overlay_remove().
  - Additional notes in the overlay documentation regarding pointers
    into overlay nodes and data.

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 |  30 ++-
 drivers/of/overlay.c                       | 258 ++++++++++-----------
 2 files changed, 149 insertions(+), 139 deletions(-)

diff --git a/Documentation/devicetree/overlay-notes.rst b/Documentation/devicetree/overlay-notes.rst
index b2b8db765b8c..e139f22b363e 100644
--- a/Documentation/devicetree/overlay-notes.rst
+++ b/Documentation/devicetree/overlay-notes.rst
@@ -119,10 +119,32 @@ 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.
+
+Users of overlays must be especially aware of the overall operations that
+occur on the system to ensure that other kernel code does not retain any
+pointers to the overlay nodes or data.  Any example of an inadvertent use
+of such pointers is if a driver or subsystem module is loaded after an
+overlay has been applied, and the driver or subsystem scans the entire
+devicetree or a large portion of it, including the overlay nodes.
diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index 0a98e3254a1a..3072dfeca8e8 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -58,20 +58,24 @@ 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_root
  * @overlay_root:	expanded device tree that contains the fragment nodes
  * @count:		count of fragment structures
  * @fragments:		fragment nodes in the overlay expanded device tree
  * @symbols_fragment:	last element of @fragments[] is the  __symbols__ node
+ * @kfree_unsafe:	pointers into the @new_fdt or @overlay_mem may exist
  * @cset:		changeset to apply fragments to live device tree
  */
 struct overlay_changeset {
 	int id;
 	struct list_head ovcs_list;
 	const void *new_fdt;
+	const void *overlay_mem;
 	struct device_node *overlay_root;
 	int count;
 	struct fragment *fragments;
 	bool symbols_fragment;
+	bool kfree_unsafe;
 	struct of_changeset cset;
 };
 
@@ -115,7 +119,6 @@ void of_overlay_mutex_unlock(void)
 	mutex_unlock(&of_overlay_phandle_mutex);
 }
 
-
 static LIST_HEAD(ovcs_list);
 static DEFINE_IDR(ovcs_idr);
 
@@ -719,53 +722,49 @@ 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
- * @overlay_root:	Contains the overlay fragments and overlay fixup nodes
  *
  * Initialize @ovcs.  Populate @ovcs->fragments with node information from
  * the top level of @overlay_root.  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_root, or -ENOSPC if idr_alloc() error.
+ * detected in @overlay_root.  On error return, the caller of
+ * init_overlay_changeset() must call free_overlay_changeset().
  */
-static int init_overlay_changeset(struct overlay_changeset *ovcs,
-		const void *new_fdt, struct device_node *overlay_root)
+static int init_overlay_changeset(struct overlay_changeset *ovcs)
 {
 	struct device_node *node, *overlay_node;
 	struct fragment *fragment;
 	struct fragment *fragments;
-	int cnt, id, ret;
+	int cnt, ret;
+
+	/*
+	 * None of the resources allocated by this function will be freed in
+	 * the error paths.  Instead the caller of this function is required
+	 * to call free_overlay_changeset() (which will free the resources)
+	 * if error return.
+	 */
 
 	/*
 	 * 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(overlay_root, OF_DYNAMIC))
-		pr_debug("%s() overlay_root is not dynamic\n", __func__);
+	if (!of_node_check_flag(ovcs->overlay_root, OF_DYNAMIC))
+		pr_debug("%s() ovcs->overlay_root is not dynamic\n", __func__);
 
-	if (!of_node_check_flag(overlay_root, OF_DETACHED))
-		pr_debug("%s() overlay_root is not detached\n", __func__);
+	if (!of_node_check_flag(ovcs->overlay_root, OF_DETACHED))
+		pr_debug("%s() ovcs->overlay_root is not detached\n", __func__);
 
-	if (!of_node_is_root(overlay_root))
-		pr_debug("%s() overlay_root is not root\n", __func__);
-
-	ovcs->overlay_root = overlay_root;
-	ovcs->new_fdt = new_fdt;
-
-	INIT_LIST_HEAD(&ovcs->ovcs_list);
+	if (!of_node_is_root(ovcs->overlay_root))
+		pr_debug("%s() ovcs->overlay_root is not root\n", __func__);
 
 	of_changeset_init(&ovcs->cset);
 
-	id = idr_alloc(&ovcs_idr, ovcs, 1, 0, GFP_KERNEL);
-	if (id <= 0)
-		return id;
-
 	cnt = 0;
 
 	/* fragment nodes */
-	for_each_child_of_node(overlay_root, node) {
+	for_each_child_of_node(ovcs->overlay_root, node) {
 		overlay_node = of_get_child_by_name(node, "__overlay__");
 		if (overlay_node) {
 			cnt++;
@@ -773,7 +772,7 @@ static int init_overlay_changeset(struct overlay_changeset *ovcs,
 		}
 	}
 
-	node = of_get_child_by_name(overlay_root, "__symbols__");
+	node = of_get_child_by_name(ovcs->overlay_root, "__symbols__");
 	if (node) {
 		cnt++;
 		of_node_put(node);
@@ -782,11 +781,12 @@ static int init_overlay_changeset(struct overlay_changeset *ovcs,
 	fragments = kcalloc(cnt, sizeof(*fragments), GFP_KERNEL);
 	if (!fragments) {
 		ret = -ENOMEM;
-		goto err_free_idr;
+		goto err_out;
 	}
+	ovcs->fragments = fragments;
 
 	cnt = 0;
-	for_each_child_of_node(overlay_root, node) {
+	for_each_child_of_node(ovcs->overlay_root, node) {
 		overlay_node = of_get_child_by_name(node, "__overlay__");
 		if (!overlay_node)
 			continue;
@@ -798,7 +798,7 @@ static int init_overlay_changeset(struct overlay_changeset *ovcs,
 			of_node_put(fragment->overlay);
 			ret = -EINVAL;
 			of_node_put(node);
-			goto err_free_fragments;
+			goto err_out;
 		}
 
 		cnt++;
@@ -808,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(overlay_root, "__symbols__");
+	node = of_get_child_by_name(ovcs->overlay_root, "__symbols__");
 	if (node) {
 		ovcs->symbols_fragment = 1;
 		fragment = &fragments[cnt];
@@ -818,7 +818,7 @@ static int init_overlay_changeset(struct overlay_changeset *ovcs,
 		if (!fragment->target) {
 			pr_err("symbols in overlay, but not in live tree\n");
 			ret = -EINVAL;
-			goto err_free_fragments;
+			goto err_out;
 		}
 
 		cnt++;
@@ -827,20 +827,14 @@ static int init_overlay_changeset(struct overlay_changeset *ovcs,
 	if (!cnt) {
 		pr_err("no fragments or symbols in overlay\n");
 		ret = -EINVAL;
-		goto err_free_fragments;
+		goto err_out;
 	}
 
-	ovcs->id = id;
 	ovcs->count = cnt;
-	ovcs->fragments = fragments;
 
 	return 0;
 
-err_free_fragments:
-	kfree(fragments);
-err_free_idr:
-	idr_remove(&ovcs_idr, id);
-
+err_out:
 	pr_err("%s() failed, ret = %d\n", __func__, ret);
 
 	return ret;
@@ -853,21 +847,34 @@ static void free_overlay_changeset(struct overlay_changeset *ovcs)
 	if (ovcs->cset.entries.next)
 		of_changeset_destroy(&ovcs->cset);
 
-	if (ovcs->id)
+	if (ovcs->id) {
 		idr_remove(&ovcs_idr, ovcs->id);
+		list_del(&ovcs->ovcs_list);
+		ovcs->id = 0;
+	}
+
 
 	for (i = 0; i < ovcs->count; i++) {
 		of_node_put(ovcs->fragments[i].target);
 		of_node_put(ovcs->fragments[i].overlay);
 	}
 	kfree(ovcs->fragments);
+
 	/*
-	 * There should be no live pointers into ovcs->overlay_root 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 during the window between OF_OVERLAY_PRE_APPLY overlay
+	 * notifiers and the OF_OVERLAY_POST_REMOVE overlay notifiers.
+	 * During the window, ovcs->kfree_unsafe will be true.
+	 *
+	 * A memory leak will occur here if ovcs->kfree_unsafe is true.
 	 */
-	kfree(ovcs->overlay_root);
-	kfree(ovcs->new_fdt);
+
+	if (!ovcs->kfree_unsafe)
+		kfree(ovcs->overlay_mem);
+	if (!ovcs->kfree_unsafe)
+		kfree(ovcs->new_fdt);
 	kfree(ovcs);
 }
 
@@ -875,27 +882,13 @@ static void free_overlay_changeset(struct overlay_changeset *ovcs)
  * internal documentation
  *
  * of_overlay_apply() - Create and apply an overlay changeset
- * @new_fdt:		Memory allocated to hold the aligned FDT
- * @overlay_root:	Expanded overlay device tree
- * @ovcs_id:		Pointer to overlay changeset id
+ * @ovcs:	overlay changeset
  *
  * 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
- *   - overlay changeset pre-apply notifier
- *
  * If an error is returned by an overlay changeset pre-apply notifier
  * then no further overlay changeset pre-apply notifier will be called.
  *
- * A non-zero return value will have created the changeset if error is from:
- *   - overlay changeset entry notifier
- *   - overlay changeset post-apply notifier
- *
  * If an error is returned by an overlay changeset post-apply notifier
  * then no further overlay changeset post-apply notifier will be called.
  *
@@ -909,64 +902,45 @@ static void free_overlay_changeset(struct overlay_changeset *ovcs)
  * following attempt to apply or remove an overlay changeset will be
  * refused.
  *
- * Returns 0 on success, or a negative error number.  Overlay changeset
- * id is returned to *ovcs_id.
+ * Returns 0 on success, or a negative error number.  When references to
+ * ovcs->new_fdt and ovcs->overlay_root may exist, ovcs->kfree_unsafe is
+ * set to true.  On error return, the caller of of_overlay_apply() must
+ * call free_overlay_changeset().
  */
 
-static int of_overlay_apply(const void *new_fdt,
-		struct device_node *overlay_root, int *ovcs_id)
+static int of_overlay_apply(struct overlay_changeset *ovcs)
 {
-	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.
-	 */
-
 	if (devicetree_corrupt()) {
 		pr_err("devicetree state suspect, refuse to apply overlay\n");
-		kfree(new_fdt);
-		kfree(overlay_root);
 		ret = -EBUSY;
 		goto out;
 	}
 
-	ovcs = kzalloc(sizeof(*ovcs), GFP_KERNEL);
-	if (!ovcs) {
-		kfree(new_fdt);
-		kfree(overlay_root);
-		ret = -ENOMEM;
-		goto out;
-	}
-
-	of_overlay_mutex_lock();
-	mutex_lock(&of_mutex);
-
-	ret = of_resolve_phandles(overlay_root);
+	ret = of_resolve_phandles(ovcs->overlay_root);
 	if (ret)
-		goto err_free_tree;
+		goto out;
 
-	ret = init_overlay_changeset(ovcs, new_fdt, overlay_root);
+	ret = init_overlay_changeset(ovcs);
 	if (ret)
-		goto err_free_tree;
+		goto out;
 
 	/*
 	 * After overlay_notify(), ovcs->overlay_root 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->new_fdt, possibly at an
-	 * offset from ovcs->new_fdt.
+	 * leaked to drivers, so can not kfree() ovcs->overlay_mem and
+	 * ovcs->new_fdt until after OF_OVERLAY_POST_REMOVE notifiers.
 	 */
+	ovcs->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 out;
 	}
 
 	ret = build_changeset(ovcs);
 	if (ret)
-		goto err_free_overlay_changeset;
+		goto out;
 
 	ret_revert = 0;
 	ret = __of_changeset_apply_entries(&ovcs->cset, &ret_revert);
@@ -976,7 +950,7 @@ static int of_overlay_apply(const void *new_fdt,
 				 ret_revert);
 			devicetree_state_flags |= DTSF_APPLY_FAIL;
 		}
-		goto err_free_overlay_changeset;
+		goto out;
 	}
 
 	ret = __of_changeset_apply_notify(&ovcs->cset);
@@ -984,9 +958,6 @@ static int of_overlay_apply(const void *new_fdt,
 		pr_err("overlay apply changeset entry notify error %d\n", ret);
 	/* notify failure is not fatal, continue */
 
-	list_add_tail(&ovcs->ovcs_list, &ovcs_list);
-	*ovcs_id = ovcs->id;
-
 	ret_tmp = overlay_notify(ovcs, OF_OVERLAY_POST_APPLY);
 	if (ret_tmp) {
 		pr_err("overlay changeset post-apply notify error %d\n",
@@ -995,19 +966,6 @@ static int of_overlay_apply(const void *new_fdt,
 			ret = ret_tmp;
 	}
 
-	goto out_unlock;
-
-err_free_tree:
-	kfree(new_fdt);
-	kfree(overlay_root);
-
-err_free_overlay_changeset:
-	free_overlay_changeset(ovcs);
-
-out_unlock:
-	mutex_unlock(&of_mutex);
-	of_overlay_mutex_unlock();
-
 out:
 	pr_debug("%s() err=%d\n", __func__, ret);
 
@@ -1015,15 +973,16 @@ static int of_overlay_apply(const void *new_fdt,
 }
 
 int of_overlay_fdt_apply(const void *overlay_fdt, u32 overlay_fdt_size,
-			 int *ovcs_id)
+			 int *ret_ovcs_id)
 {
 	void *new_fdt;
 	void *new_fdt_align;
+	void *overlay_mem;
 	int ret;
 	u32 size;
-	struct device_node *overlay_root = NULL;
+	struct overlay_changeset *ovcs;
 
-	*ovcs_id = 0;
+	*ret_ovcs_id = 0;
 
 	if (overlay_fdt_size < sizeof(struct fdt_header) ||
 	    fdt_check_header(overlay_fdt)) {
@@ -1035,41 +994,62 @@ int of_overlay_fdt_apply(const void *overlay_fdt, u32 overlay_fdt_size,
 	if (overlay_fdt_size < size)
 		return -EINVAL;
 
+	ovcs = kzalloc(sizeof(*ovcs), GFP_KERNEL);
+	if (!ovcs)
+		return -ENOMEM;
+
+	of_overlay_mutex_lock();
+	mutex_lock(&of_mutex);
+
+	ovcs->id = idr_alloc(&ovcs_idr, ovcs, 1, 0, GFP_KERNEL);
+	if (ovcs->id <= 0) {
+		ret = ovcs->id;
+		goto err_free_ovcs;
+	}
+
+	INIT_LIST_HEAD(&ovcs->ovcs_list);
+	list_add_tail(&ovcs->ovcs_list, &ovcs_list);
+
 	/*
 	 * Must create permanent copy of FDT because of_fdt_unflatten_tree()
 	 * will create pointers to the passed in FDT in the unflattened tree.
 	 */
 	new_fdt = kmalloc(size + FDT_ALIGN_SIZE, GFP_KERNEL);
-	if (!new_fdt)
-		return -ENOMEM;
+	if (!new_fdt) {
+		ret = -ENOMEM;
+		goto err_free_ovcs;
+	}
+	ovcs->new_fdt = new_fdt;
 
 	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,
+					    &ovcs->overlay_root);
+	if (!overlay_mem) {
 		pr_err("unable to unflatten overlay_fdt\n");
 		ret = -EINVAL;
-		goto out_free_new_fdt;
+		goto err_free_ovcs;
 	}
+	ovcs->overlay_mem = overlay_mem;
 
-	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(ovcs);
+	if (ret < 0)
+		goto err_free_ovcs;
+
+	mutex_unlock(&of_mutex);
+	of_overlay_mutex_unlock();
+
+	*ret_ovcs_id = ovcs->id;
 
 	return 0;
 
+err_free_ovcs:
+	free_overlay_changeset(ovcs);
 
-out_free_new_fdt:
-	kfree(new_fdt);
+	mutex_unlock(&of_mutex);
+	of_overlay_mutex_unlock();
 
-out:
 	return ret;
 }
 EXPORT_SYMBOL_GPL(of_overlay_fdt_apply);
@@ -1206,28 +1186,26 @@ int of_overlay_remove(int *ovcs_id)
 	if (!ovcs) {
 		ret = -ENODEV;
 		pr_err("remove: Could not find overlay #%d\n", *ovcs_id);
-		goto out_unlock;
+		goto err_unlock;
 	}
 
 	if (!overlay_removal_is_ok(ovcs)) {
 		ret = -EBUSY;
-		goto out_unlock;
+		goto err_unlock;
 	}
 
 	ret = overlay_notify(ovcs, OF_OVERLAY_PRE_REMOVE);
 	if (ret) {
 		pr_err("overlay changeset pre-remove notify error %d\n", ret);
-		goto out_unlock;
+		goto err_unlock;
 	}
 
-	list_del(&ovcs->ovcs_list);
-
 	ret_apply = 0;
 	ret = __of_changeset_revert_entries(&ovcs->cset, &ret_apply);
 	if (ret) {
 		if (ret_apply)
 			devicetree_state_flags |= DTSF_REVERT_FAIL;
-		goto out_unlock;
+		goto err_unlock;
 	}
 
 	ret = __of_changeset_revert_notify(&ovcs->cset);
@@ -1237,6 +1215,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",
@@ -1247,7 +1230,12 @@ int of_overlay_remove(int *ovcs_id)
 
 	free_overlay_changeset(ovcs);
 
-out_unlock:
+err_unlock:
+	/*
+	 * If jumped over free_overlay_changeset(), then did not kfree()
+	 * overlay related memory.  This is a memory leak unless a subsequent
+	 * of_overlay_remove() of this overlay is successful.
+	 */
 	mutex_unlock(&of_mutex);
 
 out:
-- 
Frank Rowand <frank.rowand@sony.com>

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

[Rob Herring]

On Mon, Apr 18, 2022 at 07:52:41PM -0500, 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 free_overlay_changeset(), which
>     is called by the function that allocated them.
>   - For the case of removing an overlay, the kfree() of new_fdt and
>     overlay_mem remains in free_overlay_changeset().

You never set kfree_unsafe back to false anywhere, so after removing you 
still leak memory.

>   - Check return value of of_fdt_unflatten_tree() for error instead
>     of checking the returned value of overlay_root.
>   - When storing pointers to allocated objects in ovcs, do so as
>     near to the allocation as possible instead of in deeply layered
>     function.
> 
> 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 v2:
>   - A version 2 review comment correctly said "This screams hack".
>     Restructure as listed below in response to the comment.
>   - Quit passing kfree_unsafe in function parameters, move it to
>     be a field of ovcs

What I meant was store the notifier state and from that imply when kfree 
is unsafe. Something like this patch on top of yours (untested and still 
some kfree_unsafe comments need to be updated):

diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index 3072dfeca8e8..53c616f576d2 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -63,7 +63,7 @@ struct fragment {
  * @count:		count of fragment structures
  * @fragments:		fragment nodes in the overlay expanded device tree
  * @symbols_fragment:	last element of @fragments[] is the  __symbols__ node
- * @kfree_unsafe:	pointers into the @new_fdt or @overlay_mem may exist
+ * @notify_state:	the last successful notifier called
  * @cset:		changeset to apply fragments to live device tree
  */
 struct overlay_changeset {
@@ -75,7 +75,7 @@ struct overlay_changeset {
 	int count;
 	struct fragment *fragments;
 	bool symbols_fragment;
-	bool kfree_unsafe;
+	enum of_overlay_notify_action notify_state;
 	struct of_changeset cset;
 };
 
@@ -183,6 +183,8 @@ static int overlay_notify(struct overlay_changeset *ovcs,
 		}
 	}
 
+	ovcs->notify_state = action;
+
 	return 0;
 }
 
@@ -831,6 +833,7 @@ static int init_overlay_changeset(struct overlay_changeset *ovcs)
 	}
 
 	ovcs->count = cnt;
+	ovcs->notify_state = OF_OVERLAY_INIT;
 
 	return 0;
 
@@ -866,15 +869,14 @@ static void free_overlay_changeset(struct overlay_changeset *ovcs)
 	 * allowed to retain pointers into the overlay devicetree other
 	 * than during the window between OF_OVERLAY_PRE_APPLY overlay
 	 * notifiers and the OF_OVERLAY_POST_REMOVE overlay notifiers.
-	 * During the window, ovcs->kfree_unsafe will be true.
 	 *
 	 * A memory leak will occur here if ovcs->kfree_unsafe is true.
 	 */
 
-	if (!ovcs->kfree_unsafe)
+	if (ovcs->notify_state == OF_OVERLAY_INIT || ovcs->notify_state == OF_OVERLAY_POST_REMOVE) {
 		kfree(ovcs->overlay_mem);
-	if (!ovcs->kfree_unsafe)
 		kfree(ovcs->new_fdt);
+	}
 	kfree(ovcs);
 }
 
@@ -926,12 +928,6 @@ static int of_overlay_apply(struct overlay_changeset *ovcs)
 	if (ret)
 		goto out;
 
-	/*
-	 * After overlay_notify(), ovcs->overlay_root 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.
-	 */
-	ovcs->kfree_unsafe = true;
 	ret = overlay_notify(ovcs, OF_OVERLAY_PRE_APPLY);
 	if (ret) {
 		pr_err("overlay changeset pre-apply notify error %d\n", ret);
diff --git a/include/linux/of.h b/include/linux/of.h
index 04971e85fbc9..b7b095593eec 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -1543,6 +1543,7 @@ static inline bool of_device_is_system_power_controller(const struct device_node
  */
 
 enum of_overlay_notify_action {
+	OF_OVERLAY_INIT = -1,
 	OF_OVERLAY_PRE_APPLY = 0,
 	OF_OVERLAY_POST_APPLY,
 	OF_OVERLAY_PRE_REMOVE,

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

[Frank Rowand]

On 4/19/22 13:56, Rob Herring wrote:
> On Mon, Apr 18, 2022 at 07:52:41PM -0500, 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 free_overlay_changeset(), which
>>     is called by the function that allocated them.
>>   - For the case of removing an overlay, the kfree() of new_fdt and
>>     overlay_mem remains in free_overlay_changeset().
> 
> You never set kfree_unsafe back to false anywhere, so after removing you 
> still leak memory.

Embarrassing and ironic that I would leave that line out.  There should
be an ovcs->kfree_unsafe = false immediately before
"overlay_notify(ovcs, OF_OVERLAY_POST_REMOVE)" in of_overlay_remove().

> 
>>   - Check return value of of_fdt_unflatten_tree() for error instead
>>     of checking the returned value of overlay_root.
>>   - When storing pointers to allocated objects in ovcs, do so as
>>     near to the allocation as possible instead of in deeply layered
>>     function.
>>
>> 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 v2:
>>   - A version 2 review comment correctly said "This screams hack".
>>     Restructure as listed below in response to the comment.
>>   - Quit passing kfree_unsafe in function parameters, move it to
>>     be a field of ovcs
> 
> What I meant was store the notifier state and from that imply when kfree 
> is unsafe. Something like this patch on top of yours (untested and still 
> some kfree_unsafe comments need to be updated):

Got it.  I like the approach you show below.  Patch v4 should be appearing
Tuesday.

-Frank

> 
> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
> index 3072dfeca8e8..53c616f576d2 100644
> --- a/drivers/of/overlay.c
> +++ b/drivers/of/overlay.c
> @@ -63,7 +63,7 @@ struct fragment {
>   * @count:		count of fragment structures
>   * @fragments:		fragment nodes in the overlay expanded device tree
>   * @symbols_fragment:	last element of @fragments[] is the  __symbols__ node
> - * @kfree_unsafe:	pointers into the @new_fdt or @overlay_mem may exist
> + * @notify_state:	the last successful notifier called
>   * @cset:		changeset to apply fragments to live device tree
>   */
>  struct overlay_changeset {
> @@ -75,7 +75,7 @@ struct overlay_changeset {
>  	int count;
>  	struct fragment *fragments;
>  	bool symbols_fragment;
> -	bool kfree_unsafe;
> +	enum of_overlay_notify_action notify_state;
>  	struct of_changeset cset;
>  };
>  
> @@ -183,6 +183,8 @@ static int overlay_notify(struct overlay_changeset *ovcs,
>  		}
>  	}
>  
> +	ovcs->notify_state = action;
> +
>  	return 0;
>  }
>  
> @@ -831,6 +833,7 @@ static int init_overlay_changeset(struct overlay_changeset *ovcs)
>  	}
>  
>  	ovcs->count = cnt;
> +	ovcs->notify_state = OF_OVERLAY_INIT;
>  
>  	return 0;
>  
> @@ -866,15 +869,14 @@ static void free_overlay_changeset(struct overlay_changeset *ovcs)
>  	 * allowed to retain pointers into the overlay devicetree other
>  	 * than during the window between OF_OVERLAY_PRE_APPLY overlay
>  	 * notifiers and the OF_OVERLAY_POST_REMOVE overlay notifiers.
> -	 * During the window, ovcs->kfree_unsafe will be true.
>  	 *
>  	 * A memory leak will occur here if ovcs->kfree_unsafe is true.
>  	 */
>  
> -	if (!ovcs->kfree_unsafe)
> +	if (ovcs->notify_state == OF_OVERLAY_INIT || ovcs->notify_state == OF_OVERLAY_POST_REMOVE) {
>  		kfree(ovcs->overlay_mem);
> -	if (!ovcs->kfree_unsafe)
>  		kfree(ovcs->new_fdt);
> +	}
>  	kfree(ovcs);
>  }
>  
> @@ -926,12 +928,6 @@ static int of_overlay_apply(struct overlay_changeset *ovcs)
>  	if (ret)
>  		goto out;
>  
> -	/*
> -	 * After overlay_notify(), ovcs->overlay_root 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.
> -	 */
> -	ovcs->kfree_unsafe = true;
>  	ret = overlay_notify(ovcs, OF_OVERLAY_PRE_APPLY);
>  	if (ret) {
>  		pr_err("overlay changeset pre-apply notify error %d\n", ret);
> diff --git a/include/linux/of.h b/include/linux/of.h
> index 04971e85fbc9..b7b095593eec 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -1543,6 +1543,7 @@ static inline bool of_device_is_system_power_controller(const struct device_node
>   */
>  
>  enum of_overlay_notify_action {
> +	OF_OVERLAY_INIT = -1,
>  	OF_OVERLAY_PRE_APPLY = 0,
>  	OF_OVERLAY_POST_APPLY,
>  	OF_OVERLAY_PRE_REMOVE,