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 -- [email protected]
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
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 <[email protected]>
Fixes: f948d6d8b792bb90 ("of: overlay: avoid race condition between applying multiple overlays")
Signed-off-by: Geert Uytterhoeven <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
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 <[email protected]>
Applied.
Rob