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;
Message-ID: <0d5e74b1-f8aa-593b-8d8a-ded84f33662c@gmail.com>
Date:   Wed, 20 Apr 2022 17:31:36 -0500
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.5.0
Subject: Re: [PATCH v4 2/2] of: overlay: rework overlay apply and remove
 kfree()s
Content-Language: en-US
From:   Frank Rowand <frowand.list@gmail.com>
To:     Rob Herring <robh+dt@kernel.org>, pantelis.antoniou@konsulko.com,
        Slawomir Stepien <slawomir.stepien@nokia.com>
Cc:     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>
References: <20220420222505.928492-1-frowand.list@gmail.com>
 <20220420222505.928492-3-frowand.list@gmail.com>
In-Reply-To: <20220420222505.928492-3-frowand.list@gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 4/20/22 17:25, frowand.list@gmail.com wrote:
> From: Frank Rowand <frank.rowand@sony.com>
> 

< snip >

The changes between v3 and v4, via interdiff:


diff -u b/drivers/of/overlay.c b/drivers/of/overlay.c
--- b/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -60,10 +60,10 @@
  * @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
+ * @notify_state:	most recent notify action used on overlay
  * @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 {
@@ -72,10 +72,10 @@
 	const void *new_fdt;
 	const void *overlay_mem;
 	struct device_node *overlay_root;
+	enum of_overlay_notify_action notify_state;
 	int count;
 	struct fragment *fragments;
 	bool symbols_fragment;
-	bool kfree_unsafe;
 	struct of_changeset cset;
 };
 
@@ -165,6 +165,8 @@
 	struct of_overlay_notify_data nd;
 	int i, ret;
 
+	ovcs->notify_state = action;
+
 	for (i = 0; i < ovcs->count; i++) {
 		struct fragment *fragment = &ovcs->fragments[i];
 
@@ -864,17 +866,17 @@
 	 * 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 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.
+	 * than during the window from OF_OVERLAY_PRE_APPLY overlay
+	 * notifiers until the OF_OVERLAY_POST_REMOVE overlay notifiers.
 	 *
-	 * A memory leak will occur here if ovcs->kfree_unsafe is true.
+	 * A memory leak will occur here if within the window.
 	 */
 
-	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);
 }
 
@@ -902,10 +904,8 @@
  * following attempt to apply or remove an overlay changeset will be
  * refused.
  *
- * 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().
+ * Returns 0 on success, or a negative error number.  On error return,
+ * the caller of of_overlay_apply() must call free_overlay_changeset().
  */
 
 static int of_overlay_apply(struct overlay_changeset *ovcs)
@@ -926,12 +926,6 @@
 	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);
@@ -1001,6 +995,11 @@
 	of_overlay_mutex_lock();
 	mutex_lock(&of_mutex);
 
+	/*
+	 * ovcs->notify_state must be set to OF_OVERLAY_INIT before allocating
+	 * ovcs resources, implicitly set by kzalloc() of ovcs
+	 */
+
 	ovcs->id = idr_alloc(&ovcs_idr, ovcs, 1, 0, GFP_KERNEL);
 	if (ovcs->id <= 0) {
 		ret = ovcs->id;
only in patch2:
unchanged:
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -1543,7 +1543,8 @@ static inline bool of_device_is_system_power_controller(const struct device_node
  */
 
 enum of_overlay_notify_action {
-	OF_OVERLAY_PRE_APPLY = 0,
+	OF_OVERLAY_INIT = 0,	/* kzalloc() of ovcs sets this value */
+	OF_OVERLAY_PRE_APPLY,
 	OF_OVERLAY_POST_APPLY,
 	OF_OVERLAY_PRE_REMOVE,
 	OF_OVERLAY_POST_REMOVE,