[Intel-gfx] [PATCH] drm/i915: Fix null pointer dereference in ring cleanup code

[Chris Wilson]

On Mon, Nov 03, 2014 at 01:54:00PM +0100, Daniel Vetter wrote:
> On Fri, Oct 31, 2014 at 04:07:35PM +0000, Chris Wilson wrote:
> > On Fri, Oct 31, 2014 at 02:52:40PM +0000, Damien Lespiau wrote:
> > > On Fri, Oct 31, 2014 at 12:00:26PM +0000, John.C.Harrison at Intel.com wrote:
> > > > From: John Harrison <John.C.Harrison at Intel.com>
> > > > 
> > > > If a ring failed to initialise for any reason then the error path would try to
> > > > clean up all rings including those that had not yet been allocated. The ring
> > > > clean up code did a check that the ring was valid before starting its work.
> > > > Unfortunately, that was after it had already dereferenced the ring to obtain a
> > > > dev_private pointer.
> > > > 
> > > > Signed-off-by: John Harrison <John.C.Harrison at Intel.com>
> > > 
> > > This looks good to me.
> > 
> > Really? These functions(!!!) are only called under controlled conditions...
> > I would have been happy to see this follow my suggestion I made to fix
> > this bug months ago.
> 
> Hm, do you mean to shuffle the ring_initialized checks into callers? Or
> something else?


i915_gem_cleanup_engines()
{
	/* Not the regular for_each_engine so we can cleanup a failed setup */
	for (int i =0 ; i < I915_NUM_ENGINES; i++)
		intel_engine_cleanup(&to_i915(dev)->engine[i]);
}

And that is callable then from an incomplete setup as well as normal
teardown.

And intel_engine_cleanup() need just be:
void intel_engine_cleanup(struct intel_engine_cs *engine)
{
	WARN_ON(engine->last_request);

	if (engine->cleanup)
		engine->cleanup(engine);
}

Remove bugs by removing code, win.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre