From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753145AbdLENqc (ORCPT ); Tue, 5 Dec 2017 08:46:32 -0500 Received: from mail-pf0-f194.google.com ([209.85.192.194]:35553 "EHLO mail-pf0-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752151AbdLENq2 (ORCPT ); Tue, 5 Dec 2017 08:46:28 -0500 X-Google-Smtp-Source: AGs4zMbQ2TG9VpE+nnCfEizD+Nnnd236pFxrt11wyEMgZnCpkOFtBdh6poyGZp3zAq7UELdPBZqrlg== Subject: Re: [PATCH v2 1/2] of: overlay: Fix memory leak in of_overlay_apply() error path To: Geert Uytterhoeven Cc: Geert Uytterhoeven , Pantelis Antoniou , Rob Herring , Colin King , Dan Carpenter , "devicetree@vger.kernel.org" , "linux-kernel@vger.kernel.org" References: <1512402456-8176-1-git-send-email-geert+renesas@glider.be> <1512402456-8176-2-git-send-email-geert+renesas@glider.be> From: Frank Rowand Message-ID: <64cf7e5d-176e-cae0-fd12-93825f2f14a2@gmail.com> Date: Tue, 5 Dec 2017 08:46:16 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 12/05/17 05:49, Geert Uytterhoeven wrote: > Hi Frank, > > On Tue, Dec 5, 2017 at 9:01 AM, Geert Uytterhoeven wrote: >> On Tue, Dec 5, 2017 at 3:07 AM, Frank Rowand 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 >>>> Fixes: f948d6d8b792bb90 ("of: overlay: avoid race condition between applying multiple overlays") >>>> Signed-off-by: Geert Uytterhoeven >>>> --- >>>> 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 >