Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Date:   Tue, 19 Apr 2022 13:56:57 -0500
From:   Rob Herring <robh@kernel.org>
To:     frowand.list@gmail.com
Cc:     pantelis.antoniou@konsulko.com,
        Slawomir Stepien <slawomir.stepien@nokia.com>,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        Slawomir Stepien <sst@poczta.fm>,
        Jan Kiszka <jan.kiszka@siemens.com>,
        Geert Uytterhoeven <geert+renesas@glider.be>,
        Alan Tull <atull@kernel.org>
Subject: Re: [PATCH v3 2/2] of: overlay: rework overlay apply and remove
 kfree()s
Message-ID: <Yl8F+cthdYbDBdWX@robh.at.kernel.org>
References: <20220419005241.831411-1-frowand.list@gmail.com>
 <20220419005241.831411-3-frowand.list@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220419005241.831411-3-frowand.list@gmail.com>
Precedence: bulk

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,