| Message ID | 20171129191750.25254-1-colin.king@canonical.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | of: overlay: fix memory leak of ovcs on error exit path | expand |
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(). -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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()") > > 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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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(). 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Dan, On Thu, Nov 30, 2017 at 1:50 PM, Dan Carpenter <dan.carpenter@oracle.com> 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()") Oops, sorry for that. >> > 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. 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@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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Dan, On Thu, Nov 30, 2017 at 1:50 PM, Dan Carpenter <dan.carpenter@oracle.com> 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()") > >> > 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. 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@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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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);