From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752591AbdLEWrw (ORCPT ); Tue, 5 Dec 2017 17:47:52 -0500 Received: from mail-pg0-f47.google.com ([74.125.83.47]:45988 "EHLO mail-pg0-f47.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751847AbdLEWru (ORCPT ); Tue, 5 Dec 2017 17:47:50 -0500 X-Google-Smtp-Source: AGs4zMYJGfp7sXUBH1rwAufXPwfMH+rhKMGzR8pzT3DDzwTvC3NTVVygLyslYQPU6W935kex88060g== 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> <87ecaacc-1969-c4ef-9c1d-f4279144e93b@gmail.com> From: Frank Rowand Message-ID: Date: Tue, 5 Dec 2017 17:47:35 -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 08:58, Geert Uytterhoeven wrote: > Hi Frank, > > On Tue, Dec 5, 2017 at 2:45 PM, Frank Rowand wrote: >> On 12/05/17 03:01, Geert Uytterhoeven wrote: >>> On Tue, Dec 5, 2017 at 3:07 AM, Frank Rowand 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 >