[PATCH v3 0/2] of: overlay: Fix of_overlay_apply() error path

[PATCH v3 0/2] of: overlay: Fix of_overlay_apply() error path

[Geert Uytterhoeven]

	Hi Pantelis, Rob, Frank,

Here's a replacement for commit bd80e2555c5c9d45 ("of: overlay: Fix
cleanup order in of_overlay_apply()"), which was applied by Rob, and
dropped later.

The first patch fixes the memory leak reported by Colin.
The second patch is a replacement for the bad dropped commit, and
depends on the first patch for proper cleanup.

All OF unittests pass.

Thanks!

Geert Uytterhoeven (2):
  of: overlay: Fix memory leak in of_overlay_apply() error path
  of: overlay: Fix (un)locking in of_overlay_apply()

 drivers/of/overlay.c | 31 +++++++++++++------------------
 1 file changed, 13 insertions(+), 18 deletions(-)

-- 
2.7.4

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

[PATCH v3 1/2] of: overlay: Fix memory leak in of_overlay_apply() error path

[Geert Uytterhoeven]

If of_resolve_phandles() fails, free_overlay_changeset() is called in
the error path.  However, that function returns early if the list hasn't
been initialized yet, before freeing the object.

Explicitly calling kfree() instead would solve that issue. However, that
complicates matter, by having to consider which of two different methods
to use to dispose of the same object.

Hence make free_overlay_changeset() consider initialization state of the
different parts of the object, making it always safe to call (once!) to
dispose of a (partially) initialized overlay_changeset:
  - Only destroy the changeset if the list was initialized,
  - Make init_overlay_changeset() store the ID in ovcs->id on success,
    to avoid calling idr_remove() with an error value or an already
    released ID.

Reported-by: Colin King <colin.king@canonical.com>
Fixes: f948d6d8b792bb90 ("of: overlay: avoid race condition between applying multiple overlays")
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
v3:
  - Store into ovcs->id on success only, drop id > 0 check before
    release,

v2:
  - New.
---
 drivers/of/overlay.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index c150abb9049d776d..bdb9695ed2d889a7 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -522,7 +522,7 @@ static int init_overlay_changeset(struct overlay_changeset *ovcs,
 	struct device_node *node, *overlay_node;
 	struct fragment *fragment;
 	struct fragment *fragments;
-	int cnt, ret;
+	int cnt, id, ret;
 
 	/*
 	 * Warn for some issues.  Can not return -EINVAL for these until
@@ -543,9 +543,9 @@ static int init_overlay_changeset(struct overlay_changeset *ovcs,
 
 	of_changeset_init(&ovcs->cset);
 
-	ovcs->id = idr_alloc(&ovcs_idr, ovcs, 1, 0, GFP_KERNEL);
-	if (ovcs->id <= 0)
-		return ovcs->id;
+	id = idr_alloc(&ovcs_idr, ovcs, 1, 0, GFP_KERNEL);
+	if (id <= 0)
+		return id;
 
 	cnt = 0;
 
@@ -611,6 +611,7 @@ static int init_overlay_changeset(struct overlay_changeset *ovcs,
 		goto err_free_fragments;
 	}
 
+	ovcs->id = id;
 	ovcs->count = cnt;
 	ovcs->fragments = fragments;
 
@@ -619,7 +620,7 @@ static int init_overlay_changeset(struct overlay_changeset *ovcs,
 err_free_fragments:
 	kfree(fragments);
 err_free_idr:
-	idr_remove(&ovcs_idr, ovcs->id);
+	idr_remove(&ovcs_idr, id);
 
 	pr_err("%s() failed, ret = %d\n", __func__, ret);
 
@@ -630,9 +631,8 @@ static void free_overlay_changeset(struct overlay_changeset *ovcs)
 {
 	int i;
 
-	if (!ovcs->cset.entries.next)
-		return;
-	of_changeset_destroy(&ovcs->cset);
+	if (ovcs->cset.entries.next)
+		of_changeset_destroy(&ovcs->cset);
 
 	if (ovcs->id)
 		idr_remove(&ovcs_idr, ovcs->id);
-- 
2.7.4

[PATCH v3 2/2] of: overlay: Fix (un)locking in of_overlay_apply()

[Geert Uytterhoeven]

The special overlay mutex is taken first, hence it should be released
last in the error path.

of_resolve_phandles() must be called with of_mutex held.  Without it, a
node and new phandle could be added via of_attach_node(), making the max
phandle wrong.

free_overlay_changeset() must be called with of_mutex held, if any
non-trivial cleanup is to be done.

Hence move "mutex_lock(&of_mutex)" up, as suggested by Frank, and merge
the two tail statements of the success and error paths, now they became
identical.

Note that while the two mutexes are adjacent, we still need both:
__of_changeset_apply_notify(), which is called by __of_changeset_apply()
unlocks of_mutex, then does notifications then locks of_mutex.  So the
mutex get released in the middle of of_overlay_apply()

Fixes: f948d6d8b792bb90 ("of: overlay: avoid race condition between applying multiple overlays")
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
v3:
  - Actually base on top of the revert of commit bd80e2555c5c9d45 ("of:
    overlay: Fix cleanup order in of_overlay_apply()"), which was
    dropped by Rob,
  - Improve patch description,

v2:
  - Rework on top of "of: overlay: Fix memory leak in of_overlay_apply()
    error path".
---
 drivers/of/overlay.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index bdb9695ed2d889a7..1ae4ff832b23a36e 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -706,12 +706,11 @@ int of_overlay_apply(struct device_node *tree, int *ovcs_id)
 	}
 
 	of_overlay_mutex_lock();
+	mutex_lock(&of_mutex);
 
 	ret = of_resolve_phandles(tree);
 	if (ret)
-		goto err_overlay_unlock;
-
-	mutex_lock(&of_mutex);
+		goto err_free_overlay_changeset;
 
 	ret = init_overlay_changeset(ovcs, tree);
 	if (ret)
@@ -755,18 +754,14 @@ int of_overlay_apply(struct device_node *tree, int *ovcs_id)
 			ret = ret_tmp;
 	}
 
-	mutex_unlock(&of_mutex);
-	of_overlay_mutex_unlock();
-
-	goto out;
-
-err_overlay_unlock:
-	of_overlay_mutex_unlock();
+	goto out_unlock;
 
 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);
-- 
2.7.4

Re: [PATCH v3 0/2] of: overlay: Fix of_overlay_apply() error path

[Frank Rowand]

On 12/05/17 10:27, Geert Uytterhoeven wrote:
> 	Hi Pantelis, Rob, Frank,
> 
> Here's a replacement for commit bd80e2555c5c9d45 ("of: overlay: Fix
> cleanup order in of_overlay_apply()"), which was applied by Rob, and
> dropped later.
> 
> The first patch fixes the memory leak reported by Colin.
> The second patch is a replacement for the bad dropped commit, and
> depends on the first patch for proper cleanup.
> 
> All OF unittests pass.
> 
> Thanks!
> 
> Geert Uytterhoeven (2):
>   of: overlay: Fix memory leak in of_overlay_apply() error path
>   of: overlay: Fix (un)locking in of_overlay_apply()
> 
>  drivers/of/overlay.c | 31 +++++++++++++------------------
>  1 file changed, 13 insertions(+), 18 deletions(-)
> 

Thank you, the code is much improved by these patches.

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

Re: [PATCH v3 0/2] of: overlay: Fix of_overlay_apply() error path

[Rob Herring]

On Tue, Dec 05, 2017 at 09:40:33PM -0500, Frank Rowand wrote:
> On 12/05/17 10:27, Geert Uytterhoeven wrote:
> > 	Hi Pantelis, Rob, Frank,
> > 
> > Here's a replacement for commit bd80e2555c5c9d45 ("of: overlay: Fix
> > cleanup order in of_overlay_apply()"), which was applied by Rob, and
> > dropped later.
> > 
> > The first patch fixes the memory leak reported by Colin.
> > The second patch is a replacement for the bad dropped commit, and
> > depends on the first patch for proper cleanup.
> > 
> > All OF unittests pass.
> > 
> > Thanks!
> > 
> > Geert Uytterhoeven (2):
> >   of: overlay: Fix memory leak in of_overlay_apply() error path
> >   of: overlay: Fix (un)locking in of_overlay_apply()
> > 
> >  drivers/of/overlay.c | 31 +++++++++++++------------------
> >  1 file changed, 13 insertions(+), 18 deletions(-)
> > 
> 
> Thank you, the code is much improved by these patches.
> 
> Reviewed-by: Frank Rowand <frank.rowand@sony.com>

Applied.

Rob