Re: [PATCH 1/2] drm/i915: Don't leak connector state on SDVO init failure

From: Daniel Vetter <daniel@ffwll.ch>
To: Matt Roper <matthew.d.roper@intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 1/2] drm/i915: Don't leak connector state on SDVO init failure
Date: Fri, 11 Dec 2015 18:28:43 +0100	[thread overview]
Message-ID: <20151211172843.GO20822@phenom.ffwll.local> (raw)
In-Reply-To: <20151211015757.GA855@intel.com>

On Thu, Dec 10, 2015 at 05:57:57PM -0800, Matt Roper wrote:
> On Thu, Dec 10, 2015 at 03:14:38PM +0100, Daniel Vetter wrote:
> > On Tue, Dec 08, 2015 at 02:48:51PM -0800, Matt Roper wrote:
> > > In all of our various SDVO setup functions, we allocate an SDVO
> > > connector (along with an associated connector->state) object, then
> > > perform initialization.  If that initialization fails, we need to make
> > > sure to free the state object as well as the connector.
> > > 
> > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> > 
> > Where do we alloc connector->state in sdvo_connector_init()? I thought the
> > flow is 1) register all the kms objects with our pile of _init() functions
> > 2) do hw readout, which is the thing which creates all these states.
> 
> It's allocated by intel_sdvo_connector_alloc -> intel_connector_init
> which gets run before intel_sdvo_connector_init() even gets called.
> There are two separate "init" functions that get called for SDVO
> connectors...the first allocates the state object, but the second can
> fail too and trigger cleanup (which was failing to destroy the state).

Ah right, didn't find that. Started a full audit but stuck at
intel_dp_init. That has a kfree(intel_connector), but seems to miss this
too.

On the patch itself: Can't we just call drm_connector_cleanup? And an
intel_connectore_free which wraps that + the kfree would be nice. Well,
that would mean we'd need to move drm_connector_init into
intel_connector_init, but I think that would make a lot of sense either
way.
-Daniel

> 
> > 
> > None of the other connectors seem to have this issue (or at least I don't
> > see a patch for them).
> 
> I don't think any of the other connector types have a second, special
> init function that gets run after intel_connector_init() is finished
> that I can see. 
> 
> 
> Matt
> 
> > -Daniel
> > 
> > > ---
> > >  drivers/gpu/drm/i915/intel_sdvo.c | 8 ++++++++
> > >  1 file changed, 8 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
> > > index 06679f1..ff28867 100644
> > > --- a/drivers/gpu/drm/i915/intel_sdvo.c
> > > +++ b/drivers/gpu/drm/i915/intel_sdvo.c
> > > @@ -2479,6 +2479,8 @@ intel_sdvo_dvi_init(struct intel_sdvo *intel_sdvo, int device)
> > >  	}
> > >  
> > >  	if (intel_sdvo_connector_init(intel_sdvo_connector, intel_sdvo) < 0) {
> > > +		drm_atomic_helper_connector_destroy_state(connector,
> > > +							  connector->state);
> > >  		kfree(intel_sdvo_connector);
> > >  		return false;
> > >  	}
> > > @@ -2514,6 +2516,8 @@ intel_sdvo_tv_init(struct intel_sdvo *intel_sdvo, int type)
> > >  	intel_sdvo->is_tv = true;
> > >  
> > >  	if (intel_sdvo_connector_init(intel_sdvo_connector, intel_sdvo) < 0) {
> > > +		drm_atomic_helper_connector_destroy_state(connector,
> > > +							  connector->state);
> > >  		kfree(intel_sdvo_connector);
> > >  		return false;
> > >  	}
> > > @@ -2561,6 +2565,8 @@ intel_sdvo_analog_init(struct intel_sdvo *intel_sdvo, int device)
> > >  	}
> > >  
> > >  	if (intel_sdvo_connector_init(intel_sdvo_connector, intel_sdvo) < 0) {
> > > +		drm_atomic_helper_connector_destroy_state(connector,
> > > +							  connector->state);
> > >  		kfree(intel_sdvo_connector);
> > >  		return false;
> > >  	}
> > > @@ -2596,6 +2602,8 @@ intel_sdvo_lvds_init(struct intel_sdvo *intel_sdvo, int device)
> > >  	}
> > >  
> > >  	if (intel_sdvo_connector_init(intel_sdvo_connector, intel_sdvo) < 0) {
> > > +		drm_atomic_helper_connector_destroy_state(connector,
> > > +							  connector->state);
> > >  		kfree(intel_sdvo_connector);
> > >  		return false;
> > >  	}
> > > -- 
> > > 2.1.4
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> 
> -- 
> Matt Roper
> Graphics Software Engineer
> IoTG Platform Enabling & Development
> Intel Corporation
> (916) 356-2795

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx