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

[PATCH v2 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 cleanup order in of_overlay_apply()

 drivers/of/overlay.c | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 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 v2 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,
  - Ignore uninitialized IDs (zero).

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>
---
 drivers/of/overlay.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index 3b7a3980ff50d6bf..312cd658bec0083b 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -630,11 +630,10 @@ 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)
+	if (ovcs->id > 0)
 		idr_remove(&ovcs_idr, ovcs->id);
 
 	for (i = 0; i < ovcs->count; i++) {
-- 
2.7.4

[PATCH v2 2/2] of: overlay: Fix cleanup order in of_overlay_apply()

[Geert Uytterhoeven]

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

Move "mutex_lock(&of_mutex)" up, as suggested by Frank, as
free_overlay_changeset() should be called with that mutex held if any
non-trivial cleanup is to be done.

Merge the two tail statements of the success and error paths, now they
became identical.

Fixes: f948d6d8b792bb90 ("of: overlay: avoid race condition between applying multiple overlays")
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
This is v2 of commit bd80e2555c5c9d45 ("of: overlay: Fix cleanup order
in of_overlay_apply()"), which was dropped by Rob.
---
 drivers/of/overlay.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index 312cd658bec0083b..0ddb7748ac85498f 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -705,12 +705,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)
@@ -754,17 +753,13 @@ int of_overlay_apply(struct device_node *tree, int *ovcs_id)
 			ret = ret_tmp;
 	}
 
-	mutex_unlock(&of_mutex);
-	of_overlay_mutex_unlock();
-
-	goto out;
+	goto out_unlock;
 
 err_free_overlay_changeset:
 	free_overlay_changeset(ovcs);
 
+out_unlock:
 	mutex_unlock(&of_mutex);
-
-err_overlay_unlock:
 	of_overlay_mutex_unlock();
 
 out:
-- 
2.7.4

Re: [PATCH v2 2/2] of: overlay: Fix cleanup order in of_overlay_apply()

[Rob Herring]

On Mon, Dec 4, 2017 at 9:47 AM, Geert Uytterhoeven
<geert+renesas@glider.be> wrote:
> The special overlay mutex is taken first, hence it should be released
> last in the error path.
>
> Move "mutex_lock(&of_mutex)" up, as suggested by Frank, as
> free_overlay_changeset() should be called with that mutex held if any
> non-trivial cleanup is to be done.

Not holding the of_mutex for of_resolve_phandles is just wrong.
Without it, a node and new phandle could be added via of_attach_node
making the max phandle wrong.

Now, with the 2 mutexes adjacent, what is the point of even having the
of_overlay_mutex? Seems like we should just drop it.

I also don't think we really need to hold the mutex during post-apply
notifiers. It also seems like some steps could be moved outside the
mutex(es) like init_overlay_changeset().

Rob

Re: [PATCH v2 2/2] of: overlay: Fix cleanup order in of_overlay_apply()

[Geert Uytterhoeven]

Hi Rob,

On Mon, Dec 4, 2017 at 8:35 PM, Rob Herring <robh+dt@kernel.org> wrote:
> On Mon, Dec 4, 2017 at 9:47 AM, Geert Uytterhoeven
> <geert+renesas@glider.be> wrote:
>> The special overlay mutex is taken first, hence it should be released
>> last in the error path.
>>
>> Move "mutex_lock(&of_mutex)" up, as suggested by Frank, as
>> free_overlay_changeset() should be called with that mutex held if any
>> non-trivial cleanup is to be done.
>
> Not holding the of_mutex for of_resolve_phandles is just wrong.
> Without it, a node and new phandle could be added via of_attach_node
> making the max phandle wrong.

After my patch it's held, so what's the problem?

> Now, with the 2 mutexes adjacent, what is the point of even having the
> of_overlay_mutex? Seems like we should just drop it.

Frank?

> I also don't think we really need to hold the mutex during post-apply
> notifiers. It also seems like some steps could be moved outside the
> mutex(es) like init_overlay_changeset().

Perhaps.

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

Re: [PATCH v2 2/2] of: overlay: Fix cleanup order in of_overlay_apply()

[Frank Rowand]

On 12/04/17 14:45, Geert Uytterhoeven wrote:
> Hi Rob,
> 
> On Mon, Dec 4, 2017 at 8:35 PM, Rob Herring <robh+dt@kernel.org> wrote:
>> On Mon, Dec 4, 2017 at 9:47 AM, Geert Uytterhoeven
>> <geert+renesas@glider.be> wrote:
>>> The special overlay mutex is taken first, hence it should be released
>>> last in the error path.
>>>
>>> Move "mutex_lock(&of_mutex)" up, as suggested by Frank, as
>>> free_overlay_changeset() should be called with that mutex held if any
>>> non-trivial cleanup is to be done.
>>
>> Not holding the of_mutex for of_resolve_phandles is just wrong.
>> Without it, a node and new phandle could be added via of_attach_node
>> making the max phandle wrong.
> 
> After my patch it's held, so what's the problem?
> 
>> Now, with the 2 mutexes adjacent, what is the point of even having the
>> of_overlay_mutex? Seems like we should just drop it.
> 
> Frank?

__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()

I have never been comfortable with the unlock/lock there, but don't have
an alternative yet.


>> I also don't think we really need to hold the mutex during post-apply
>> notifiers. It also seems like some steps could be moved outside the
>> mutex(es) like init_overlay_changeset().
> 
> Perhaps.
> 
> 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
> 

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

[Frank Rowand]

Hi Geert,

Thanks for finding the issues and for the fixes.

Comments in line.


On 12/04/17 10:47, Geert Uytterhoeven wrote:
> 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,
>   - Ignore uninitialized IDs (zero).
> 
> 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>
> ---
>  drivers/of/overlay.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
> index 3b7a3980ff50d6bf..312cd658bec0083b 100644
> --- a/drivers/of/overlay.c
> +++ b/drivers/of/overlay.c
> @@ -630,11 +630,10 @@ 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);
>  

OK

> -	if (ovcs->id)
> +	if (ovcs->id > 0)

Instead of this change, could you please make a change in init_overlay_changeset()?

Current init_overlay_changeset():

        ovcs->id = idr_alloc(&ovcs_idr, ovcs, 1, 0, GFP_KERNEL);
        if (ovcs->id <= 0)
                return ovcs->id;

My proposed version:

        ret = idr_alloc(&ovcs_idr, ovcs, 1, 0, GFP_KERNEL);
        if (ret <= 0)
                return ret;
        ovcs->id = ret;


>  		idr_remove(&ovcs_idr, ovcs->id);
>  
>  	for (i = 0; i < ovcs->count; i++) {
> 

Also, the previous version of the patch, and the discussion around the resulting
bug make me think that I should not have moved 'kfree(ovcs)' into
free_overlay_changeset(), because that kfree is then not very visible in the
error path of of_overlay_apply().  Could you remove 'kfree(ovcs)' from
free_overlay_changeset(), and instead call it immediately after each call
to free_overlay_changeset()?

-Frank

Re: [PATCH v2 2/2] of: overlay: Fix cleanup order in of_overlay_apply()

[Rob Herring]

On Mon, Dec 4, 2017 at 1:45 PM, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> Hi Rob,
>
> On Mon, Dec 4, 2017 at 8:35 PM, Rob Herring <robh+dt@kernel.org> wrote:
>> On Mon, Dec 4, 2017 at 9:47 AM, Geert Uytterhoeven
>> <geert+renesas@glider.be> wrote:
>>> The special overlay mutex is taken first, hence it should be released
>>> last in the error path.
>>>
>>> Move "mutex_lock(&of_mutex)" up, as suggested by Frank, as
>>> free_overlay_changeset() should be called with that mutex held if any
>>> non-trivial cleanup is to be done.
>>
>> Not holding the of_mutex for of_resolve_phandles is just wrong.
>> Without it, a node and new phandle could be added via of_attach_node
>> making the max phandle wrong.
>
> After my patch it's held, so what's the problem?

There's no problem. Just highlighting the issue with the prior
location is more than it seems from your explanation.

Rob

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

[Geert Uytterhoeven]

Hi Frank,

On Tue, Dec 5, 2017 at 3:07 AM, Frank Rowand <frowand.list@gmail.com> wrote:
> On 12/04/17 10:47, Geert Uytterhoeven wrote:
>> 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,
>>   - Ignore uninitialized IDs (zero).
>>
>> 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>
>> ---
>>  drivers/of/overlay.c | 7 +++----
>>  1 file changed, 3 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
>> index 3b7a3980ff50d6bf..312cd658bec0083b 100644
>> --- a/drivers/of/overlay.c
>> +++ b/drivers/of/overlay.c
>> @@ -630,11 +630,10 @@ 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);
>>
>
> OK
>
>> -     if (ovcs->id)
>> +     if (ovcs->id > 0)
>
> Instead of this change, could you please make a change in init_overlay_changeset()?
>
> Current init_overlay_changeset():
>
>         ovcs->id = idr_alloc(&ovcs_idr, ovcs, 1, 0, GFP_KERNEL);
>         if (ovcs->id <= 0)
>                 return ovcs->id;
>
> My proposed version:
>
>         ret = idr_alloc(&ovcs_idr, ovcs, 1, 0, GFP_KERNEL);
>         if (ret <= 0)
>                 return ret;
>         ovcs->id = ret;

Sure.

>>               idr_remove(&ovcs_idr, ovcs->id);
>>
>>       for (i = 0; i < ovcs->count; i++) {
>>
>
> Also, the previous version of the patch, and the discussion around the resulting
> bug make me think that I should not have moved 'kfree(ovcs)' into
> free_overlay_changeset(), because that kfree is then not very visible in the
> error path of of_overlay_apply().  Could you remove 'kfree(ovcs)' from
> free_overlay_changeset(), and instead call it immediately after each call
> to free_overlay_changeset()?

Actually I like that free_overlay_changeset() takes care of the deallocation,
especially in light of the kojectification op top from bbb-overlays, which
means you cannot just call kfree(ovcs) anymore (I know this won't go upstream
anytime soon, but I need overlay configfs for my development and testing).

Perhaps the allocation of ovcs should be moved into free_overlay_changeset(),
and the latter being renamed to alloc_overlay_changeset()?
That way allocation and freeing become symmetrical.
It would move the allocation under the mutexes, though.

What do you think?

Thanks!

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

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

[Geert Uytterhoeven]

Hi Frank,

On Tue, Dec 5, 2017 at 9:01 AM, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Tue, Dec 5, 2017 at 3:07 AM, Frank Rowand <frowand.list@gmail.com> wrote:
>> On 12/04/17 10:47, Geert Uytterhoeven wrote:
>>> 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,
>>>   - Ignore uninitialized IDs (zero).
>>>
>>> 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>
>>> ---
>>>  drivers/of/overlay.c | 7 +++----
>>>  1 file changed, 3 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
>>> index 3b7a3980ff50d6bf..312cd658bec0083b 100644
>>> --- a/drivers/of/overlay.c
>>> +++ b/drivers/of/overlay.c
>>> @@ -630,11 +630,10 @@ 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);
>>>
>>
>> OK
>>
>>> -     if (ovcs->id)
>>> +     if (ovcs->id > 0)
>>
>> Instead of this change, could you please make a change in init_overlay_changeset()?
>>
>> Current init_overlay_changeset():
>>
>>         ovcs->id = idr_alloc(&ovcs_idr, ovcs, 1, 0, GFP_KERNEL);
>>         if (ovcs->id <= 0)
>>                 return ovcs->id;
>>
>> My proposed version:
>>
>>         ret = idr_alloc(&ovcs_idr, ovcs, 1, 0, GFP_KERNEL);
>>         if (ret <= 0)
>>                 return ret;
>>         ovcs->id = ret;
>
> Sure.

Actually we should use a temporary variable id here, just like for cnt
and fragments, and store into ovcs->id if everything succeeds.

Else both init_overlay_changeset() and free_overlay_changeset() will
free the ID if something goes wrong. It seems IDR can handle that, but
better safe than sorry.

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

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

[Frank Rowand]

On 12/05/17 03:01, Geert Uytterhoeven wrote:
> Hi Frank,
> 
> On Tue, Dec 5, 2017 at 3:07 AM, Frank Rowand <frowand.list@gmail.com> wrote:
>> On 12/04/17 10:47, Geert Uytterhoeven wrote:
>>> 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,
>>>   - Ignore uninitialized IDs (zero).
>>>
>>> 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>
>>> ---
>>>  drivers/of/overlay.c | 7 +++----
>>>  1 file changed, 3 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
>>> index 3b7a3980ff50d6bf..312cd658bec0083b 100644
>>> --- a/drivers/of/overlay.c
>>> +++ b/drivers/of/overlay.c
>>> @@ -630,11 +630,10 @@ 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);
>>>
>>
>> OK
>>
>>> -     if (ovcs->id)
>>> +     if (ovcs->id > 0)
>>
>> Instead of this change, could you please make a change in init_overlay_changeset()?
>>
>> Current init_overlay_changeset():
>>
>>         ovcs->id = idr_alloc(&ovcs_idr, ovcs, 1, 0, GFP_KERNEL);
>>         if (ovcs->id <= 0)
>>                 return ovcs->id;
>>
>> My proposed version:
>>
>>         ret = idr_alloc(&ovcs_idr, ovcs, 1, 0, GFP_KERNEL);
>>         if (ret <= 0)
>>                 return ret;
>>         ovcs->id = ret;
> 
> Sure.
> 
>>>               idr_remove(&ovcs_idr, ovcs->id);
>>>
>>>       for (i = 0; i < ovcs->count; i++) {
>>>
>>
>> Also, the previous version of the patch, and the discussion around the resulting
>> bug make me think that I should not have moved 'kfree(ovcs)' into
>> free_overlay_changeset(), because that kfree is then not very visible in the
>> error path of of_overlay_apply().  Could you remove 'kfree(ovcs)' from
>> free_overlay_changeset(), and instead call it immediately after each call
>> to free_overlay_changeset()?
> 
> Actually I like that free_overlay_changeset() takes care of the deallocation,
> especially in light of the kojectification op top from bbb-overlays, which
> means you cannot just call kfree(ovcs) anymore (I know this won't go upstream
> anytime soon, but I need overlay configfs for my development and testing).

OK, knowing that kobjectification is being considered I am willing to leave the
kfree(ovcs) where it is for now.


> Perhaps the allocation of ovcs should be moved into free_overlay_changeset(),

                                          I think this ^^^^^^^^^^^^^^^^^^^^^^^
is a typo, and you meant init_overlay_changeset().

> and the latter being renamed to alloc_overlay_changeset()?
> That way allocation and freeing become symmetrical.
> It would move the allocation under the mutexes, though.

I considered moving the kzalloc() into init_overlay_changeset() when I
created it, but decided not to because the type of the first argument of
init_overlay_changeset() would change from
   struct overlay_changeset *
to
   struct overlay_changeset **,
and usage of ovcs would become _slightly_ more ugly and complex in
init_overlay_changeset().


> 
> What do you think?
> 
> Thanks!
> 
> 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
> 

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

[Frank Rowand]

On 12/05/17 05:49, Geert Uytterhoeven wrote:
> Hi Frank,
> 
> On Tue, Dec 5, 2017 at 9:01 AM, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>> On Tue, Dec 5, 2017 at 3:07 AM, Frank Rowand <frowand.list@gmail.com> wrote:
>>> On 12/04/17 10:47, Geert Uytterhoeven wrote:
>>>> 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,
>>>>   - Ignore uninitialized IDs (zero).
>>>>
>>>> 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>
>>>> ---
>>>>  drivers/of/overlay.c | 7 +++----
>>>>  1 file changed, 3 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
>>>> index 3b7a3980ff50d6bf..312cd658bec0083b 100644
>>>> --- a/drivers/of/overlay.c
>>>> +++ b/drivers/of/overlay.c
>>>> @@ -630,11 +630,10 @@ 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);
>>>>
>>>
>>> OK
>>>
>>>> -     if (ovcs->id)
>>>> +     if (ovcs->id > 0)
>>>
>>> Instead of this change, could you please make a change in init_overlay_changeset()?
>>>
>>> Current init_overlay_changeset():
>>>
>>>         ovcs->id = idr_alloc(&ovcs_idr, ovcs, 1, 0, GFP_KERNEL);
>>>         if (ovcs->id <= 0)
>>>                 return ovcs->id;
>>>
>>> My proposed version:
>>>
>>>         ret = idr_alloc(&ovcs_idr, ovcs, 1, 0, GFP_KERNEL);
>>>         if (ret <= 0)
>>>                 return ret;
>>>         ovcs->id = ret;
>>
>> Sure.
> 
> Actually we should use a temporary variable id here, just like for cnt
> and fragments, and store into ovcs->id if everything succeeds.

OK.  That would make the flow in init_overlay_changeset() more consistent.
And of course the idr_remove() after err_free_idr: would use that temporary
variable id.


> Else both init_overlay_changeset() and free_overlay_changeset() will
> free the ID if something goes wrong. It seems IDR can handle that, but
> better safe than sorry.> 
> 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
> 

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

[Geert Uytterhoeven]

Hi Frank,

On Tue, Dec 5, 2017 at 2:45 PM, Frank Rowand <frowand.list@gmail.com> wrote:
> On 12/05/17 03:01, Geert Uytterhoeven wrote:
>> On Tue, Dec 5, 2017 at 3:07 AM, Frank Rowand <frowand.list@gmail.com> wrote:
>>> Also, the previous version of the patch, and the discussion around the resulting
>>> bug make me think that I should not have moved 'kfree(ovcs)' into
>>> free_overlay_changeset(), because that kfree is then not very visible in the
>>> error path of of_overlay_apply().  Could you remove 'kfree(ovcs)' from
>>> free_overlay_changeset(), and instead call it immediately after each call
>>> to free_overlay_changeset()?
>>
>> Actually I like that free_overlay_changeset() takes care of the deallocation,
>> especially in light of the kojectification op top from bbb-overlays, which
>> means you cannot just call kfree(ovcs) anymore (I know this won't go upstream
>> anytime soon, but I need overlay configfs for my development and testing).
>
> OK, knowing that kobjectification is being considered I am willing to leave the
> kfree(ovcs) where it is for now.
>
>> Perhaps the allocation of ovcs should be moved into free_overlay_changeset(),
>
>                                           I think this ^^^^^^^^^^^^^^^^^^^^^^^
> is a typo, and you meant init_overlay_changeset().

Yes it is.

>> and the latter being renamed to alloc_overlay_changeset()?
>> That way allocation and freeing become symmetrical.
>> It would move the allocation under the mutexes, though.
>
> I considered moving the kzalloc() into init_overlay_changeset() when I
> created it, but decided not to because the type of the first argument of
> init_overlay_changeset() would change from
>    struct overlay_changeset *
> to
>    struct overlay_changeset **,
> and usage of ovcs would become _slightly_ more ugly and complex in
> init_overlay_changeset().

I would let alloc_overlay_changeset() return struct overlay_changeset *
instead.

If you care about why it failed, it can return ERR_PTR(error) instead of
NULL ;-)

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

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

[Frank Rowand]

On 12/05/17 08:58, Geert Uytterhoeven wrote:
> Hi Frank,
> 
> On Tue, Dec 5, 2017 at 2:45 PM, Frank Rowand <frowand.list@gmail.com> wrote:
>> On 12/05/17 03:01, Geert Uytterhoeven wrote:
>>> On Tue, Dec 5, 2017 at 3:07 AM, Frank Rowand <frowand.list@gmail.com> wrote:
>>>> Also, the previous version of the patch, and the discussion around the resulting
>>>> bug make me think that I should not have moved 'kfree(ovcs)' into
>>>> free_overlay_changeset(), because that kfree is then not very visible in the
>>>> error path of of_overlay_apply().  Could you remove 'kfree(ovcs)' from
>>>> free_overlay_changeset(), and instead call it immediately after each call
>>>> to free_overlay_changeset()?
>>>
>>> Actually I like that free_overlay_changeset() takes care of the deallocation,
>>> especially in light of the kojectification op top from bbb-overlays, which
>>> means you cannot just call kfree(ovcs) anymore (I know this won't go upstream
>>> anytime soon, but I need overlay configfs for my development and testing).
>>
>> OK, knowing that kobjectification is being considered I am willing to leave the
>> kfree(ovcs) where it is for now.
>>
>>> Perhaps the allocation of ovcs should be moved into free_overlay_changeset(),
>>
>>                                           I think this ^^^^^^^^^^^^^^^^^^^^^^^
>> is a typo, and you meant init_overlay_changeset().
> 
> Yes it is.
> 
>>> and the latter being renamed to alloc_overlay_changeset()?
>>> That way allocation and freeing become symmetrical.
>>> It would move the allocation under the mutexes, though.
>>
>> I considered moving the kzalloc() into init_overlay_changeset() when I
>> created it, but decided not to because the type of the first argument of
>> init_overlay_changeset() would change from
>>    struct overlay_changeset *
>> to
>>    struct overlay_changeset **,
>> and usage of ovcs would become _slightly_ more ugly and complex in
>> init_overlay_changeset().
> 
> I would let alloc_overlay_changeset() return struct overlay_changeset *
> instead.
> 
> If you care about why it failed, it can return ERR_PTR(error) instead of
> NULL ;-)

Yes, it should continue to return the error reason.

Thanks,

Frank


> 
> 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
>