From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752577AbdLEIBw (ORCPT ); Tue, 5 Dec 2017 03:01:52 -0500 Received: from mail-qt0-f193.google.com ([209.85.216.193]:45166 "EHLO mail-qt0-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752510AbdLEIBt (ORCPT ); Tue, 5 Dec 2017 03:01:49 -0500 X-Google-Smtp-Source: AGs4zMbNVeFAWD7dwWy2JMa3uUaF9GOcxAzG7/vxKy9tkClO2pFQih5S5sfwAA+3cK3qryrdXlboFzmkMIhbuTcKecE= MIME-Version: 1.0 In-Reply-To: References: <1512402456-8176-1-git-send-email-geert+renesas@glider.be> <1512402456-8176-2-git-send-email-geert+renesas@glider.be> From: Geert Uytterhoeven Date: Tue, 5 Dec 2017 09:01:48 +0100 X-Google-Sender-Auth: 1tqLqu_Du48fmpGCQ1PK-Iv5bFU Message-ID: Subject: Re: [PATCH v2 1/2] of: overlay: Fix memory leak in of_overlay_apply() error path To: Frank Rowand Cc: Geert Uytterhoeven , Pantelis Antoniou , Rob Herring , Colin King , Dan Carpenter , "devicetree@vger.kernel.org" , "linux-kernel@vger.kernel.org" Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Frank, 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. >> 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