From patchwork Wed Nov 29 19:16:43 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: "Russell King (Oracle)" X-Patchwork-Id: 10084355 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 6D35160234 for ; Thu, 30 Nov 2017 08:18:20 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 5AD3929EED for ; Thu, 30 Nov 2017 08:18:20 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 4F77929EF1; Thu, 30 Nov 2017 08:18:20 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-4.1 required=2.0 tests=BAYES_00,DKIM_SIGNED, RCVD_IN_DNSWL_MED,T_DKIM_INVALID autolearn=ham version=3.3.1 Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher DHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id 5351D29EED for ; Thu, 30 Nov 2017 08:18:19 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id EA29F6EA2E; Thu, 30 Nov 2017 08:18:15 +0000 (UTC) X-Original-To: dri-devel@lists.freedesktop.org Delivered-To: dri-devel@lists.freedesktop.org Received: from pandora.armlinux.org.uk (pandora.armlinux.org.uk [IPv6:2001:4d48:ad52:3201:214:fdff:fe10:1be6]) by gabe.freedesktop.org (Postfix) with ESMTPS id 846186E051 for ; Wed, 29 Nov 2017 19:16:57 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=armlinux.org.uk; s=pandora-2014; h=Sender:In-Reply-To:Content-Type:MIME-Version:References:Message-ID:Subject:Cc:To:From:Date; bh=Fa+H2uHt/gCknDY7dnpjOkOPxtLJgrbZqYzymN6lLuA=; b=OhGfbzO5lwlZVq1ZpzFs13204oCDHYAdk2sVQjd9eAOWbb+znZuLoTcOa4+oY2IP35GqlmsF2sY2ZRKTEy0Y2etvZSwBKFaX4hTbr6lRqrQ4YFM2P1mIKbOUbM7IkPUidgME5TZVQBojeTSYyxX/9TCZZ9PC+AVwXDkdpALH2ZQ=; Received: from n2100.armlinux.org.uk ([2002:4e20:1eda:1:214:fdff:fe10:4f86]:46994) by pandora.armlinux.org.uk with esmtpsa (TLSv1:DHE-RSA-AES256-SHA:256) (Exim 4.82_1-5b7a7c0-XX) (envelope-from ) id 1eK7qj-00053E-Px; Wed, 29 Nov 2017 19:16:50 +0000 Received: from linux by n2100.armlinux.org.uk with local (Exim 4.76) (envelope-from ) id 1eK7qf-0000GJ-0R; Wed, 29 Nov 2017 19:16:45 +0000 Date: Wed, 29 Nov 2017 19:16:43 +0000 From: Russell King - ARM Linux To: Sean Paul Subject: Re: [PATCH 1/5] drm/armada: fix leak of crtc structure Message-ID: <20171129191643.GF8356@n2100.armlinux.org.uk> References: <20171129114516.GD8356@n2100.armlinux.org.uk> <20171129145342.ripjickq7o75cvfj@art_vandelay> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20171129145342.ripjickq7o75cvfj@art_vandelay> User-Agent: Mutt/1.5.23 (2014-03-12) X-Mailman-Approved-At: Thu, 30 Nov 2017 08:18:15 +0000 Cc: David Airlie , dri-devel@lists.freedesktop.org X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" X-Virus-Scanned: ClamAV using ClamSMTP On Wed, Nov 29, 2017 at 09:53:42AM -0500, Sean Paul wrote: > On Wed, Nov 29, 2017 at 11:45:43AM +0000, Russell King wrote: > > Fix the leak of the CRTC structure in the failure paths of > > armada_drm_crtc_create(). > > > > Signed-off-by: Russell King > > --- > > drivers/gpu/drm/armada/armada_crtc.c | 10 ++++++++-- > > 1 file changed, 8 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/armada/armada_crtc.c b/drivers/gpu/drm/armada/armada_crtc.c > > index 2e065facdce7..844d488b6654 100644 > > --- a/drivers/gpu/drm/armada/armada_crtc.c > > +++ b/drivers/gpu/drm/armada/armada_crtc.c > > @@ -1246,11 +1246,14 @@ static int armada_drm_crtc_create(struct drm_device *drm, struct device *dev, > > dcrtc->crtc.port = port; > > > > primary = kzalloc(sizeof(*primary), GFP_KERNEL); > > - if (!primary) > > + if (!primary) { > > + kfree(dcrtc); > > return -ENOMEM; > > + } > > > > ret = armada_drm_plane_init(primary); > > if (ret) { > > + kfree(dcrtc); > > kfree(primary); > > return ret; > > } > > @@ -1262,14 +1265,17 @@ static int armada_drm_crtc_create(struct drm_device *drm, struct device *dev, > > NULL, > > DRM_PLANE_TYPE_PRIMARY, NULL); > > if (ret) { > > + kfree(dcrtc); > > kfree(primary); > > return ret; > > } > > > > ret = drm_crtc_init_with_planes(drm, &dcrtc->crtc, &primary->base, NULL, > > &armada_crtc_funcs, NULL); > > - if (ret) > > + if (ret) { > > + kfree(dcrtc); > > goto err_crtc_init; > > The mixed use of cleaning up at every exit and the lone label is a bit messy and > prone to bugs such as the one you're fixing. How about you label everything > and then just clean up once? It's not that simple - see alternative patch below. How we clean stuff up depends on how far through the initialisation we are, and that doesn't lend itself to a sequential cleanup methodology. > It also seems like you're missing cleanup if armada_overlay_plane_create() > fails? It's not missing. At that point, the CRTC has been registered, and any failure will be cleaned up by the drm_mode_config_cleanup() in the error paths in armada_drm_bind() in armada_drv.c It's rather non-obvious because it uses the ->destroy callbacks. diff --git a/drivers/gpu/drm/armada/armada_crtc.c b/drivers/gpu/drm/armada/armada_crtc.c index d987198a2ac6..79ce877bf45f 100644 --- a/drivers/gpu/drm/armada/armada_crtc.c +++ b/drivers/gpu/drm/armada/armada_crtc.c @@ -1225,17 +1225,13 @@ static int armada_drm_crtc_create(struct drm_device *drm, struct device *dev, ret = devm_request_irq(dev, irq, armada_drm_irq, 0, "armada_drm_crtc", dcrtc); - if (ret < 0) { - kfree(dcrtc); - return ret; - } + if (ret < 0) + goto err_crtc; if (dcrtc->variant->init) { ret = dcrtc->variant->init(dcrtc, dev); - if (ret) { - kfree(dcrtc); - return ret; - } + if (ret) + goto err_crtc; } /* Ensure AXI pipeline is enabled */ @@ -1247,15 +1243,14 @@ static int armada_drm_crtc_create(struct drm_device *drm, struct device *dev, primary = kzalloc(sizeof(*primary), GFP_KERNEL); if (!primary) { - kfree(dcrtc); - return -ENOMEM; + ret = -ENOMEM; + goto err_crtc; } ret = armada_drm_plane_init(primary); if (ret) { - kfree(dcrtc); kfree(primary); - return ret; + goto err_crtc; } ret = drm_universal_plane_init(drm, &primary->base, 0, @@ -1265,17 +1260,14 @@ static int armada_drm_crtc_create(struct drm_device *drm, struct device *dev, NULL, DRM_PLANE_TYPE_PRIMARY, NULL); if (ret) { - kfree(dcrtc); kfree(primary); - return ret; + goto err_crtc; } ret = drm_crtc_init_with_planes(drm, &dcrtc->crtc, &primary->base, NULL, &armada_crtc_funcs, NULL); - if (ret) { - kfree(dcrtc); + if (ret) goto err_crtc_init; - } drm_crtc_helper_add(&dcrtc->crtc, &armada_crtc_helper_funcs); @@ -1288,6 +1280,9 @@ static int armada_drm_crtc_create(struct drm_device *drm, struct device *dev, err_crtc_init: primary->base.funcs->destroy(&primary->base); +err_crtc: + kfree(dcrtc); + return ret; }