* [PATCH v2 0/2] of: overlay: Fix of_overlay_apply() error path @ 2017-12-04 15:47 Geert Uytterhoeven 2017-12-04 15:47 ` [PATCH v2 1/2] of: overlay: Fix memory leak in " Geert Uytterhoeven 2017-12-04 15:47 ` [PATCH v2 2/2] of: overlay: Fix cleanup order in of_overlay_apply() Geert Uytterhoeven 0 siblings, 2 replies; 14+ messages in thread From: Geert Uytterhoeven @ 2017-12-04 15:47 UTC (permalink / raw) To: Pantelis Antoniou, Rob Herring, Frank Rowand Cc: Colin King, Dan Carpenter, devicetree, linux-kernel, 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2 1/2] of: overlay: Fix memory leak in of_overlay_apply() error path 2017-12-04 15:47 [PATCH v2 0/2] of: overlay: Fix of_overlay_apply() error path Geert Uytterhoeven @ 2017-12-04 15:47 ` Geert Uytterhoeven 2017-12-05 2:07 ` Frank Rowand 2017-12-04 15:47 ` [PATCH v2 2/2] of: overlay: Fix cleanup order in of_overlay_apply() Geert Uytterhoeven 1 sibling, 1 reply; 14+ messages in thread From: Geert Uytterhoeven @ 2017-12-04 15:47 UTC (permalink / raw) To: Pantelis Antoniou, Rob Herring, Frank Rowand Cc: Colin King, Dan Carpenter, devicetree, linux-kernel, 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v2 1/2] of: overlay: Fix memory leak in of_overlay_apply() error path 2017-12-04 15:47 ` [PATCH v2 1/2] of: overlay: Fix memory leak in " Geert Uytterhoeven @ 2017-12-05 2:07 ` Frank Rowand 2017-12-05 8:01 ` Geert Uytterhoeven 0 siblings, 1 reply; 14+ messages in thread From: Frank Rowand @ 2017-12-05 2:07 UTC (permalink / raw) To: Geert Uytterhoeven, Pantelis Antoniou, Rob Herring Cc: Colin King, Dan Carpenter, devicetree, linux-kernel 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 1/2] of: overlay: Fix memory leak in of_overlay_apply() error path 2017-12-05 2:07 ` Frank Rowand @ 2017-12-05 8:01 ` Geert Uytterhoeven 2017-12-05 10:49 ` Geert Uytterhoeven 2017-12-05 13:45 ` Frank Rowand 0 siblings, 2 replies; 14+ messages in thread From: Geert Uytterhoeven @ 2017-12-05 8:01 UTC (permalink / raw) To: Frank Rowand Cc: Geert Uytterhoeven, Pantelis Antoniou, Rob Herring, Colin King, Dan Carpenter, devicetree, linux-kernel 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 1/2] of: overlay: Fix memory leak in of_overlay_apply() error path 2017-12-05 8:01 ` Geert Uytterhoeven @ 2017-12-05 10:49 ` Geert Uytterhoeven 2017-12-05 13:46 ` Frank Rowand 2017-12-05 13:45 ` Frank Rowand 1 sibling, 1 reply; 14+ messages in thread From: Geert Uytterhoeven @ 2017-12-05 10:49 UTC (permalink / raw) To: Frank Rowand Cc: Geert Uytterhoeven, Pantelis Antoniou, Rob Herring, Colin King, Dan Carpenter, devicetree, linux-kernel 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 1/2] of: overlay: Fix memory leak in of_overlay_apply() error path 2017-12-05 10:49 ` Geert Uytterhoeven @ 2017-12-05 13:46 ` Frank Rowand 0 siblings, 0 replies; 14+ messages in thread From: Frank Rowand @ 2017-12-05 13:46 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Geert Uytterhoeven, Pantelis Antoniou, Rob Herring, Colin King, Dan Carpenter, devicetree, linux-kernel 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 > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 1/2] of: overlay: Fix memory leak in of_overlay_apply() error path 2017-12-05 8:01 ` Geert Uytterhoeven 2017-12-05 10:49 ` Geert Uytterhoeven @ 2017-12-05 13:45 ` Frank Rowand 2017-12-05 13:58 ` Geert Uytterhoeven 1 sibling, 1 reply; 14+ messages in thread From: Frank Rowand @ 2017-12-05 13:45 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Geert Uytterhoeven, Pantelis Antoniou, Rob Herring, Colin King, Dan Carpenter, devicetree, linux-kernel 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 > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 1/2] of: overlay: Fix memory leak in of_overlay_apply() error path 2017-12-05 13:45 ` Frank Rowand @ 2017-12-05 13:58 ` Geert Uytterhoeven 2017-12-05 22:47 ` Frank Rowand 0 siblings, 1 reply; 14+ messages in thread From: Geert Uytterhoeven @ 2017-12-05 13:58 UTC (permalink / raw) To: Frank Rowand Cc: Geert Uytterhoeven, Pantelis Antoniou, Rob Herring, Colin King, Dan Carpenter, devicetree, linux-kernel 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 1/2] of: overlay: Fix memory leak in of_overlay_apply() error path 2017-12-05 13:58 ` Geert Uytterhoeven @ 2017-12-05 22:47 ` Frank Rowand 0 siblings, 0 replies; 14+ messages in thread From: Frank Rowand @ 2017-12-05 22:47 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Geert Uytterhoeven, Pantelis Antoniou, Rob Herring, Colin King, Dan Carpenter, devicetree, linux-kernel 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 > ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2 2/2] of: overlay: Fix cleanup order in of_overlay_apply() 2017-12-04 15:47 [PATCH v2 0/2] of: overlay: Fix of_overlay_apply() error path Geert Uytterhoeven 2017-12-04 15:47 ` [PATCH v2 1/2] of: overlay: Fix memory leak in " Geert Uytterhoeven @ 2017-12-04 15:47 ` Geert Uytterhoeven 2017-12-04 19:35 ` Rob Herring 1 sibling, 1 reply; 14+ messages in thread From: Geert Uytterhoeven @ 2017-12-04 15:47 UTC (permalink / raw) To: Pantelis Antoniou, Rob Herring, Frank Rowand Cc: Colin King, Dan Carpenter, devicetree, linux-kernel, 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v2 2/2] of: overlay: Fix cleanup order in of_overlay_apply() 2017-12-04 15:47 ` [PATCH v2 2/2] of: overlay: Fix cleanup order in of_overlay_apply() Geert Uytterhoeven @ 2017-12-04 19:35 ` Rob Herring 2017-12-04 19:45 ` Geert Uytterhoeven 0 siblings, 1 reply; 14+ messages in thread From: Rob Herring @ 2017-12-04 19:35 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Pantelis Antoniou, Frank Rowand, Colin King, Dan Carpenter, devicetree, linux-kernel 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 2/2] of: overlay: Fix cleanup order in of_overlay_apply() 2017-12-04 19:35 ` Rob Herring @ 2017-12-04 19:45 ` Geert Uytterhoeven 2017-12-05 1:16 ` Frank Rowand 2017-12-05 2:25 ` Rob Herring 0 siblings, 2 replies; 14+ messages in thread From: Geert Uytterhoeven @ 2017-12-04 19:45 UTC (permalink / raw) To: Rob Herring Cc: Geert Uytterhoeven, Pantelis Antoniou, Frank Rowand, Colin King, Dan Carpenter, devicetree, linux-kernel 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 2/2] of: overlay: Fix cleanup order in of_overlay_apply() 2017-12-04 19:45 ` Geert Uytterhoeven @ 2017-12-05 1:16 ` Frank Rowand 2017-12-05 2:25 ` Rob Herring 1 sibling, 0 replies; 14+ messages in thread From: Frank Rowand @ 2017-12-05 1:16 UTC (permalink / raw) To: Geert Uytterhoeven, Rob Herring Cc: Geert Uytterhoeven, Pantelis Antoniou, Colin King, Dan Carpenter, devicetree, linux-kernel 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 > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 2/2] of: overlay: Fix cleanup order in of_overlay_apply() 2017-12-04 19:45 ` Geert Uytterhoeven 2017-12-05 1:16 ` Frank Rowand @ 2017-12-05 2:25 ` Rob Herring 1 sibling, 0 replies; 14+ messages in thread From: Rob Herring @ 2017-12-05 2:25 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Geert Uytterhoeven, Pantelis Antoniou, Frank Rowand, Colin King, Dan Carpenter, devicetree, linux-kernel 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2017-12-05 22:47 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-12-04 15:47 [PATCH v2 0/2] of: overlay: Fix of_overlay_apply() error path Geert Uytterhoeven 2017-12-04 15:47 ` [PATCH v2 1/2] of: overlay: Fix memory leak in " Geert Uytterhoeven 2017-12-05 2:07 ` Frank Rowand 2017-12-05 8:01 ` Geert Uytterhoeven 2017-12-05 10:49 ` Geert Uytterhoeven 2017-12-05 13:46 ` Frank Rowand 2017-12-05 13:45 ` Frank Rowand 2017-12-05 13:58 ` Geert Uytterhoeven 2017-12-05 22:47 ` Frank Rowand 2017-12-04 15:47 ` [PATCH v2 2/2] of: overlay: Fix cleanup order in of_overlay_apply() Geert Uytterhoeven 2017-12-04 19:35 ` Rob Herring 2017-12-04 19:45 ` Geert Uytterhoeven 2017-12-05 1:16 ` Frank Rowand 2017-12-05 2:25 ` Rob Herring
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).