* [PATCH] of: overlay: fix memory leak of ovcs on error exit path
@ 2017-11-29 19:17 Colin King
2017-11-30 12:14 ` Frank Rowand
0 siblings, 1 reply; 10+ messages in thread
From: Colin King @ 2017-11-29 19:17 UTC (permalink / raw)
To: Pantelis Antoniou, Rob Herring, Frank Rowand,
devicetree-u79uwXL29TY76Z2rM5mHXA
Cc: kernel-janitors-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
From: Colin Ian King <colin.king-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
Currently if the call to of_resolve_phandles fails then then ovcs
is not kfree'd on the error exit path. Rather than try and make
the clean up exit path more convoluted, fix this by just kfree'ing
ovcs at the point of error detection and exit via the same exit
path.
Detected by CoverityScan, CID#1462296 ("Resource Leak")
Fixes: f948d6d8b792 ("of: overlay: avoid race condition between applying multiple overlays")
Signed-off-by: Colin Ian King <colin.king-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
---
drivers/of/overlay.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index 53bc9e3f0b98..6c8efe7d8cbb 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -708,8 +708,10 @@ int of_overlay_apply(struct device_node *tree, int *ovcs_id)
of_overlay_mutex_lock();
ret = of_resolve_phandles(tree);
- if (ret)
+ if (ret) {
+ kfree(ovcs);
goto err_overlay_unlock;
+ }
mutex_lock(&of_mutex);
--
2.14.1
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] of: overlay: fix memory leak of ovcs on error exit path
2017-11-29 19:17 [PATCH] of: overlay: fix memory leak of ovcs on error exit path Colin King
@ 2017-11-30 12:14 ` Frank Rowand
[not found] ` <dcb4e2a4-707b-0ccc-e12b-c6fa4ef251b7-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
0 siblings, 1 reply; 10+ messages in thread
From: Frank Rowand @ 2017-11-30 12:14 UTC (permalink / raw)
To: Colin King, Pantelis Antoniou, Rob Herring, devicetree
Cc: kernel-janitors, linux-kernel
On 11/29/17 14:17, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> Currently if the call to of_resolve_phandles fails then then ovcs
> is not kfree'd on the error exit path. Rather than try and make
> the clean up exit path more convoluted, fix this by just kfree'ing
> ovcs at the point of error detection and exit via the same exit
> path.
>
> Detected by CoverityScan, CID#1462296 ("Resource Leak")
>
> Fixes: f948d6d8b792 ("of: overlay: avoid race condition between applying multiple overlays")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
> drivers/of/overlay.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
> index 53bc9e3f0b98..6c8efe7d8cbb 100644
> --- a/drivers/of/overlay.c
> +++ b/drivers/of/overlay.c
> @@ -708,8 +708,10 @@ int of_overlay_apply(struct device_node *tree, int *ovcs_id)
> of_overlay_mutex_lock();
>
> ret = of_resolve_phandles(tree);
> - if (ret)
> + if (ret) {
> + kfree(ovcs);
> goto err_overlay_unlock;
> + }
>
> mutex_lock(&of_mutex);
>
>
False coverity warning. ovcs is freed in free_overlay_changeset().
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] of: overlay: fix memory leak of ovcs on error exit path
[not found] ` <dcb4e2a4-707b-0ccc-e12b-c6fa4ef251b7-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
@ 2017-11-30 12:18 ` Colin Ian King
[not found] ` <806a0467-87c8-4100-c7f2-54cfa8732465-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
2017-11-30 12:50 ` Dan Carpenter
1 sibling, 1 reply; 10+ messages in thread
From: Colin Ian King @ 2017-11-30 12:18 UTC (permalink / raw)
To: Frank Rowand, Pantelis Antoniou, Rob Herring,
devicetree-u79uwXL29TY76Z2rM5mHXA
Cc: kernel-janitors-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
On 30/11/17 12:14, Frank Rowand wrote:
> On 11/29/17 14:17, Colin King wrote:
>> From: Colin Ian King <colin.king-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
>>
>> Currently if the call to of_resolve_phandles fails then then ovcs
>> is not kfree'd on the error exit path. Rather than try and make
>> the clean up exit path more convoluted, fix this by just kfree'ing
>> ovcs at the point of error detection and exit via the same exit
>> path.
>>
>> Detected by CoverityScan, CID#1462296 ("Resource Leak")
>>
>> Fixes: f948d6d8b792 ("of: overlay: avoid race condition between applying multiple overlays")
>> Signed-off-by: Colin Ian King <colin.king-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
>> ---
>> drivers/of/overlay.c | 4 +++-
>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
>> index 53bc9e3f0b98..6c8efe7d8cbb 100644
>> --- a/drivers/of/overlay.c
>> +++ b/drivers/of/overlay.c
>> @@ -708,8 +708,10 @@ int of_overlay_apply(struct device_node *tree, int *ovcs_id)
>> of_overlay_mutex_lock();
>>
>> ret = of_resolve_phandles(tree);
>> - if (ret)
>> + if (ret) {
>> + kfree(ovcs);
>> goto err_overlay_unlock;
>> + }
>>
>> mutex_lock(&of_mutex);
>>
>>
>
> False coverity warning. ovcs is freed in free_overlay_changeset().
>
The error exit path is via err_overlay_unlock:
err_overlay_unlock:
of_overlay_mutex_unlock();
out:
pr_debug("%s() err=%d\n", __func__, ret);
return ret;
..so there is no call to free_overlay_changeset there.
Colin
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] of: overlay: fix memory leak of ovcs on error exit path
[not found] ` <dcb4e2a4-707b-0ccc-e12b-c6fa4ef251b7-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2017-11-30 12:18 ` Colin Ian King
@ 2017-11-30 12:50 ` Dan Carpenter
2017-11-30 12:54 ` Colin Ian King
` (2 more replies)
1 sibling, 3 replies; 10+ messages in thread
From: Dan Carpenter @ 2017-11-30 12:50 UTC (permalink / raw)
To: Frank Rowand, Geert Uytterhoeven
Cc: Colin King, Pantelis Antoniou, Rob Herring,
devicetree-u79uwXL29TY76Z2rM5mHXA,
kernel-janitors-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
On Thu, Nov 30, 2017 at 07:14:45AM -0500, Frank Rowand wrote:
> On 11/29/17 14:17, Colin King wrote:
> > From: Colin Ian King <colin.king-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
> >
> > Currently if the call to of_resolve_phandles fails then then ovcs
> > is not kfree'd on the error exit path. Rather than try and make
> > the clean up exit path more convoluted, fix this by just kfree'ing
> > ovcs at the point of error detection and exit via the same exit
> > path.
> >
> > Detected by CoverityScan, CID#1462296 ("Resource Leak")
> >
> > Fixes: f948d6d8b792 ("of: overlay: avoid race condition between applying multiple overlays")
The Fixes tag is wrong. It should be:
Fixes: bd80e2555c5c ("of: overlay: Fix cleanup order in of_overlay_apply()")
> > Signed-off-by: Colin Ian King <colin.king-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
> > ---
> > drivers/of/overlay.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
> > index 53bc9e3f0b98..6c8efe7d8cbb 100644
> > --- a/drivers/of/overlay.c
> > +++ b/drivers/of/overlay.c
> > @@ -708,8 +708,10 @@ int of_overlay_apply(struct device_node *tree, int *ovcs_id)
> > of_overlay_mutex_lock();
> >
> > ret = of_resolve_phandles(tree);
> > - if (ret)
> > + if (ret) {
> > + kfree(ovcs);
> > goto err_overlay_unlock;
> > + }
> >
> > mutex_lock(&of_mutex);
> >
> >
>
> False coverity warning. ovcs is freed in free_overlay_changeset().
You're looking at an older version of the code and Colin is looking at
linux-next.
regards,
dan carpenter
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] of: overlay: fix memory leak of ovcs on error exit path
2017-11-30 12:50 ` Dan Carpenter
@ 2017-11-30 12:54 ` Colin Ian King
2017-11-30 13:51 ` Geert Uytterhoeven
2017-12-04 15:15 ` Geert Uytterhoeven
2 siblings, 0 replies; 10+ messages in thread
From: Colin Ian King @ 2017-11-30 12:54 UTC (permalink / raw)
To: Dan Carpenter, Frank Rowand, Geert Uytterhoeven
Cc: Pantelis Antoniou, Rob Herring, devicetree, kernel-janitors,
linux-kernel
On 30/11/17 12:50, Dan Carpenter wrote:
> On Thu, Nov 30, 2017 at 07:14:45AM -0500, Frank Rowand wrote:
>> On 11/29/17 14:17, Colin King wrote:
>>> From: Colin Ian King <colin.king@canonical.com>
>>>
>>> Currently if the call to of_resolve_phandles fails then then ovcs
>>> is not kfree'd on the error exit path. Rather than try and make
>>> the clean up exit path more convoluted, fix this by just kfree'ing
>>> ovcs at the point of error detection and exit via the same exit
>>> path.
>>>
>>> Detected by CoverityScan, CID#1462296 ("Resource Leak")
>>>
>>> Fixes: f948d6d8b792 ("of: overlay: avoid race condition between applying multiple overlays")
>
> The Fixes tag is wrong. It should be:
>
> Fixes: bd80e2555c5c ("of: overlay: Fix cleanup order in of_overlay_apply()")
Good catch. I'll send a V2.
Colin
>
>>> Signed-off-by: Colin Ian King <colin.king@canonical.com>
>>> ---
>>> drivers/of/overlay.c | 4 +++-
>>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
>>> index 53bc9e3f0b98..6c8efe7d8cbb 100644
>>> --- a/drivers/of/overlay.c
>>> +++ b/drivers/of/overlay.c
>>> @@ -708,8 +708,10 @@ int of_overlay_apply(struct device_node *tree, int *ovcs_id)
>>> of_overlay_mutex_lock();
>>>
>>> ret = of_resolve_phandles(tree);
>>> - if (ret)
>>> + if (ret) {
>>> + kfree(ovcs);
>>> goto err_overlay_unlock;
>>> + }
>>>
>>> mutex_lock(&of_mutex);
>>>
>>>
>>
>> False coverity warning. ovcs is freed in free_overlay_changeset().
>
> You're looking at an older version of the code and Colin is looking at
> linux-next.
>
> regards,
> dan carpenter
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] of: overlay: fix memory leak of ovcs on error exit path
[not found] ` <806a0467-87c8-4100-c7f2-54cfa8732465-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
@ 2017-11-30 13:37 ` Frank Rowand
2017-11-30 15:01 ` Frank Rowand
0 siblings, 1 reply; 10+ messages in thread
From: Frank Rowand @ 2017-11-30 13:37 UTC (permalink / raw)
To: Colin Ian King, Pantelis Antoniou, Rob Herring,
devicetree-u79uwXL29TY76Z2rM5mHXA
Cc: kernel-janitors-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
Hi Colin, Rob,
On 11/30/17 07:18, Colin Ian King wrote:
> On 30/11/17 12:14, Frank Rowand wrote:
>> On 11/29/17 14:17, Colin King wrote:
>>> From: Colin Ian King <colin.king-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
>>>
>>> Currently if the call to of_resolve_phandles fails then then ovcs
>>> is not kfree'd on the error exit path. Rather than try and make
>>> the clean up exit path more convoluted, fix this by just kfree'ing
>>> ovcs at the point of error detection and exit via the same exit
>>> path.
>>>
>>> Detected by CoverityScan, CID#1462296 ("Resource Leak")
>>>
>>> Fixes: f948d6d8b792 ("of: overlay: avoid race condition between applying multiple overlays")
>>> Signed-off-by: Colin Ian King <colin.king-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
>>> ---
>>> drivers/of/overlay.c | 4 +++-
>>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
>>> index 53bc9e3f0b98..6c8efe7d8cbb 100644
>>> --- a/drivers/of/overlay.c
>>> +++ b/drivers/of/overlay.c
>>> @@ -708,8 +708,10 @@ int of_overlay_apply(struct device_node *tree, int *ovcs_id)
>>> of_overlay_mutex_lock();
>>>
>>> ret = of_resolve_phandles(tree);
>>> - if (ret)
>>> + if (ret) {
>>> + kfree(ovcs);
>>> goto err_overlay_unlock;
>>> + }
>>>
>>> mutex_lock(&of_mutex);
>>>
>>>
>>
>> False coverity warning. ovcs is freed in free_overlay_changeset().
>>
>
> The error exit path is via err_overlay_unlock:
>
> err_overlay_unlock:
> of_overlay_mutex_unlock();
>
> out:
> pr_debug("%s() err=%d\n", __func__, ret);
>
> return ret;
>
> ..so there is no call to free_overlay_changeset there.
>
> Colin
>
OK, I was looking at 4.15-rc1. You must be looking at a later version where
"[PATCH 1/2] of: overlay: Fix cleanup order in of_overlay_apply()" has been
applied. Thanks for providing the extra details about the exit path so I
could see that.
Rob, I think that the fix for cleanup order was not the best way to fix that
problem. A better method would have been to move "mutex_lock(&of_mutex);"
up 5 lines, to just before calling of_reserve_phandles(). The problem
found by coverity was caused by the "Fix cleanup order" patch.
I can create that alternate fix if you would like, but I am traveling
right now and don't want to submit a patch without boot testing, so
there will be a slight delay.
-Frank
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] of: overlay: fix memory leak of ovcs on error exit path
2017-11-30 12:50 ` Dan Carpenter
2017-11-30 12:54 ` Colin Ian King
@ 2017-11-30 13:51 ` Geert Uytterhoeven
2017-12-04 15:15 ` Geert Uytterhoeven
2 siblings, 0 replies; 10+ messages in thread
From: Geert Uytterhoeven @ 2017-11-30 13:51 UTC (permalink / raw)
To: Dan Carpenter
Cc: Frank Rowand, Geert Uytterhoeven, Colin King, Pantelis Antoniou,
Rob Herring, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
kernel-janitors-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Hi Dan,
On Thu, Nov 30, 2017 at 1:50 PM, Dan Carpenter <dan.carpenter-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org> wrote:
> On Thu, Nov 30, 2017 at 07:14:45AM -0500, Frank Rowand wrote:
>> On 11/29/17 14:17, Colin King wrote:
>> > From: Colin Ian King <colin.king-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
>> >
>> > Currently if the call to of_resolve_phandles fails then then ovcs
>> > is not kfree'd on the error exit path. Rather than try and make
>> > the clean up exit path more convoluted, fix this by just kfree'ing
>> > ovcs at the point of error detection and exit via the same exit
>> > path.
>> >
>> > Detected by CoverityScan, CID#1462296 ("Resource Leak")
>> >
>> > Fixes: f948d6d8b792 ("of: overlay: avoid race condition between applying multiple overlays")
>
> The Fixes tag is wrong. It should be:
>
> Fixes: bd80e2555c5c ("of: overlay: Fix cleanup order in of_overlay_apply()")
Oops, sorry for that.
>> > Signed-off-by: Colin Ian King <colin.king-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
>> > ---
>> > drivers/of/overlay.c | 4 +++-
>> > 1 file changed, 3 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
>> > index 53bc9e3f0b98..6c8efe7d8cbb 100644
>> > --- a/drivers/of/overlay.c
>> > +++ b/drivers/of/overlay.c
>> > @@ -708,8 +708,10 @@ int of_overlay_apply(struct device_node *tree, int *ovcs_id)
>> > of_overlay_mutex_lock();
>> >
>> > ret = of_resolve_phandles(tree);
>> > - if (ret)
>> > + if (ret) {
>> > + kfree(ovcs);
>> > goto err_overlay_unlock;
>> > + }
>> >
>> > mutex_lock(&of_mutex);
>>
>> False coverity warning. ovcs is freed in free_overlay_changeset().
>
> You're looking at an older version of the code and Colin is looking at
> linux-next.
BTW, it's a bit confusing that sometimes ovcs must be freed manually, and
sometimes this is taken care of by free_overlay_changeset().
Especially when building on top of that, and adding more error cases in
between err_free_overlay_changeset and err_overlay_unlock.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] of: overlay: fix memory leak of ovcs on error exit path
2017-11-30 13:37 ` Frank Rowand
@ 2017-11-30 15:01 ` Frank Rowand
2017-11-30 15:26 ` Rob Herring
0 siblings, 1 reply; 10+ messages in thread
From: Frank Rowand @ 2017-11-30 15:01 UTC (permalink / raw)
To: Colin Ian King, Pantelis Antoniou, Rob Herring, devicetree
Cc: kernel-janitors, linux-kernel
On 11/30/17 08:37, Frank Rowand wrote:
> Hi Colin, Rob,
>
> On 11/30/17 07:18, Colin Ian King wrote:
>> On 30/11/17 12:14, Frank Rowand wrote:
>>> On 11/29/17 14:17, Colin King wrote:
>>>> From: Colin Ian King <colin.king@canonical.com>
>>>>
>>>> Currently if the call to of_resolve_phandles fails then then ovcs
>>>> is not kfree'd on the error exit path. Rather than try and make
>>>> the clean up exit path more convoluted, fix this by just kfree'ing
>>>> ovcs at the point of error detection and exit via the same exit
>>>> path.
>>>>
>>>> Detected by CoverityScan, CID#1462296 ("Resource Leak")
>>>>
>>>> Fixes: f948d6d8b792 ("of: overlay: avoid race condition between applying multiple overlays")
>>>> Signed-off-by: Colin Ian King <colin.king@canonical.com>
>>>> ---
>>>> drivers/of/overlay.c | 4 +++-
>>>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
>>>> index 53bc9e3f0b98..6c8efe7d8cbb 100644
>>>> --- a/drivers/of/overlay.c
>>>> +++ b/drivers/of/overlay.c
>>>> @@ -708,8 +708,10 @@ int of_overlay_apply(struct device_node *tree, int *ovcs_id)
>>>> of_overlay_mutex_lock();
>>>>
>>>> ret = of_resolve_phandles(tree);
>>>> - if (ret)
>>>> + if (ret) {
>>>> + kfree(ovcs);
>>>> goto err_overlay_unlock;
>>>> + }
>>>>
>>>> mutex_lock(&of_mutex);
>>>>
>>>>
>>>
>>> False coverity warning. ovcs is freed in free_overlay_changeset().
>>>
>>
>> The error exit path is via err_overlay_unlock:
>>
>> err_overlay_unlock:
>> of_overlay_mutex_unlock();
>>
>> out:
>> pr_debug("%s() err=%d\n", __func__, ret);
>>
>> return ret;
>>
>> ..so there is no call to free_overlay_changeset there.
>>
>> Colin
>>
>
> OK, I was looking at 4.15-rc1. You must be looking at a later version where
> "[PATCH 1/2] of: overlay: Fix cleanup order in of_overlay_apply()" has been
> applied. Thanks for providing the extra details about the exit path so I
> could see that.
>
> Rob, I think that the fix for cleanup order was not the best way to fix that
> problem. A better method would have been to move "mutex_lock(&of_mutex);"
> up 5 lines, to just before calling of_reserve_phandles().
It is getting late (midnight my time), so I really should revisit this all
tomorrow. My last comment ("move ... up 5 lines") is probably wrong.
I'll look at this after some sleep.
> The problem
> found by coverity was caused by the "Fix cleanup order" patch.
>
> I can create that alternate fix if you would like, but I am traveling
> right now and don't want to submit a patch without boot testing, so
> there will be a slight delay.
>
> -Frank
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] of: overlay: fix memory leak of ovcs on error exit path
2017-11-30 15:01 ` Frank Rowand
@ 2017-11-30 15:26 ` Rob Herring
0 siblings, 0 replies; 10+ messages in thread
From: Rob Herring @ 2017-11-30 15:26 UTC (permalink / raw)
To: Frank Rowand
Cc: Colin Ian King, Pantelis Antoniou, devicetree@vger.kernel.org,
kernel-janitors, linux-kernel@vger.kernel.org
On Thu, Nov 30, 2017 at 9:01 AM, Frank Rowand <frowand.list@gmail.com> wrote:
> On 11/30/17 08:37, Frank Rowand wrote:
>> Hi Colin, Rob,
>>
>> On 11/30/17 07:18, Colin Ian King wrote:
>>> On 30/11/17 12:14, Frank Rowand wrote:
>>>> On 11/29/17 14:17, Colin King wrote:
>>>>> From: Colin Ian King <colin.king@canonical.com>
>>>>>
>>>>> Currently if the call to of_resolve_phandles fails then then ovcs
>>>>> is not kfree'd on the error exit path. Rather than try and make
>>>>> the clean up exit path more convoluted, fix this by just kfree'ing
>>>>> ovcs at the point of error detection and exit via the same exit
>>>>> path.
>>>>>
>>>>> Detected by CoverityScan, CID#1462296 ("Resource Leak")
>>>>>
>>>>> Fixes: f948d6d8b792 ("of: overlay: avoid race condition between applying multiple overlays")
>>>>> Signed-off-by: Colin Ian King <colin.king@canonical.com>
>>>>> ---
>>>>> drivers/of/overlay.c | 4 +++-
>>>>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
>>>>> index 53bc9e3f0b98..6c8efe7d8cbb 100644
>>>>> --- a/drivers/of/overlay.c
>>>>> +++ b/drivers/of/overlay.c
>>>>> @@ -708,8 +708,10 @@ int of_overlay_apply(struct device_node *tree, int *ovcs_id)
>>>>> of_overlay_mutex_lock();
>>>>>
>>>>> ret = of_resolve_phandles(tree);
>>>>> - if (ret)
>>>>> + if (ret) {
>>>>> + kfree(ovcs);
>>>>> goto err_overlay_unlock;
>>>>> + }
>>>>>
>>>>> mutex_lock(&of_mutex);
>>>>>
>>>>>
>>>>
>>>> False coverity warning. ovcs is freed in free_overlay_changeset().
>>>>
>>>
>>> The error exit path is via err_overlay_unlock:
>>>
>>> err_overlay_unlock:
>>> of_overlay_mutex_unlock();
>>>
>>> out:
>>> pr_debug("%s() err=%d\n", __func__, ret);
>>>
>>> return ret;
>>>
>>> ..so there is no call to free_overlay_changeset there.
>>>
>>> Colin
>>>
>>
>> OK, I was looking at 4.15-rc1. You must be looking at a later version where
>> "[PATCH 1/2] of: overlay: Fix cleanup order in of_overlay_apply()" has been
>> applied. Thanks for providing the extra details about the exit path so I
>> could see that.
>>
>> Rob, I think that the fix for cleanup order was not the best way to fix that
>> problem. A better method would have been to move "mutex_lock(&of_mutex);"
>> up 5 lines, to just before calling of_reserve_phandles().
>
> It is getting late (midnight my time), so I really should revisit this all
> tomorrow. My last comment ("move ... up 5 lines") is probably wrong.
>
> I'll look at this after some sleep.
I'm dropping "of: overlay: Fix cleanup order in of_overlay_apply()",
so someone please fix this in the original patch.
Rob
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] of: overlay: fix memory leak of ovcs on error exit path
2017-11-30 12:50 ` Dan Carpenter
2017-11-30 12:54 ` Colin Ian King
2017-11-30 13:51 ` Geert Uytterhoeven
@ 2017-12-04 15:15 ` Geert Uytterhoeven
2 siblings, 0 replies; 10+ messages in thread
From: Geert Uytterhoeven @ 2017-12-04 15:15 UTC (permalink / raw)
To: Dan Carpenter
Cc: Frank Rowand, Geert Uytterhoeven, Colin King, Pantelis Antoniou,
Rob Herring, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
kernel-janitors-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Hi Dan,
On Thu, Nov 30, 2017 at 1:50 PM, Dan Carpenter <dan.carpenter-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org> wrote:
> On Thu, Nov 30, 2017 at 07:14:45AM -0500, Frank Rowand wrote:
>> On 11/29/17 14:17, Colin King wrote:
>> > From: Colin Ian King <colin.king-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
>> >
>> > Currently if the call to of_resolve_phandles fails then then ovcs
>> > is not kfree'd on the error exit path. Rather than try and make
>> > the clean up exit path more convoluted, fix this by just kfree'ing
>> > ovcs at the point of error detection and exit via the same exit
>> > path.
>> >
>> > Detected by CoverityScan, CID#1462296 ("Resource Leak")
>> >
>> > Fixes: f948d6d8b792 ("of: overlay: avoid race condition between applying multiple overlays")
>
> The Fixes tag is wrong. It should be:
>
> Fixes: bd80e2555c5c ("of: overlay: Fix cleanup order in of_overlay_apply()")
>
>> > Signed-off-by: Colin Ian King <colin.king-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
>> > ---
>> > drivers/of/overlay.c | 4 +++-
>> > 1 file changed, 3 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
>> > index 53bc9e3f0b98..6c8efe7d8cbb 100644
>> > --- a/drivers/of/overlay.c
>> > +++ b/drivers/of/overlay.c
>> > @@ -708,8 +708,10 @@ int of_overlay_apply(struct device_node *tree, int *ovcs_id)
>> > of_overlay_mutex_lock();
>> >
>> > ret = of_resolve_phandles(tree);
>> > - if (ret)
>> > + if (ret) {
>> > + kfree(ovcs);
>> > goto err_overlay_unlock;
>> > + }
>> >
>> > mutex_lock(&of_mutex);
>> >
>> >
>>
>> False coverity warning. ovcs is freed in free_overlay_changeset().
>
> You're looking at an older version of the code and Colin is looking at
> linux-next.
Actually Colin was right: the bug was introduced in commit f948d6d8b792:
While free_overlay_changeset() is called in the error path, that function
returns early because of:
if (!ovcs->cset.entries.next)
return;
So it never gets to freeing the passed pointer.
I'm fixing it for good (let's hope so ;-)
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2017-12-04 15:15 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-11-29 19:17 [PATCH] of: overlay: fix memory leak of ovcs on error exit path Colin King
2017-11-30 12:14 ` Frank Rowand
[not found] ` <dcb4e2a4-707b-0ccc-e12b-c6fa4ef251b7-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2017-11-30 12:18 ` Colin Ian King
[not found] ` <806a0467-87c8-4100-c7f2-54cfa8732465-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
2017-11-30 13:37 ` Frank Rowand
2017-11-30 15:01 ` Frank Rowand
2017-11-30 15:26 ` Rob Herring
2017-11-30 12:50 ` Dan Carpenter
2017-11-30 12:54 ` Colin Ian King
2017-11-30 13:51 ` Geert Uytterhoeven
2017-12-04 15:15 ` Geert Uytterhoeven
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).