Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753256AbdLEKth (ORCPT ); Tue, 5 Dec 2017 05:49:37 -0500 Received: from mail-qt0-f194.google.com ([209.85.216.194]:42363 "EHLO mail-qt0-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753086AbdLEKte (ORCPT ); Tue, 5 Dec 2017 05:49:34 -0500 X-Google-Smtp-Source: AGs4zMbpODcdt5xCxpEJWYGgUK43pUvzDeiBoqn29F1CEoqPx1NLgKOK44uAY64MPG6Ns/QUuAMG0UV143SuB+NEuLk= 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 11:49:32 +0100 X-Google-Sender-Auth: 3J_-zZQvjgo1onjm78E8fwzfwh4 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 Content-Length: 2935 Lines: 80 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. 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