Re: [PATCH 1/5] drm/armada: fix leak of crtc structure

[Russell King - ARM Linux <linux@armlinux.org.uk>]

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 <rmk+kernel@armlinux.org.uk>
> > ---
> >  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;
 }
 


-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel