[Intel-gfx] [PATCH] drm/i915: Fix a use after free, and unbalanced refcounting

[Daniel, Thomas]

> -----Original Message-----
> From: Intel-gfx [mailto:intel-gfx-bounces at lists.freedesktop.org] On Behalf Of
> Daniel Vetter
> Sent: Friday, February 13, 2015 1:50 PM
> To: Hoath, Nicholas
> Cc: intel-gfx at lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH] drm/i915: Fix a use after free, and unbalanced
> refcounting
> 
> On Fri, Feb 13, 2015 at 01:30:35PM +0000, Nick Hoath wrote:
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=88652
> >
> > When converting from implicitly tracked execlist queue items to ref counted
> > requests, not all free's of requests were replaced with unrefs, and extraneous
> > refs/unrefs of contexts were added.
> > Correct the unbalanced refcount & replace the free's.
> >
> > Problem introduced in:
> > commit 6d3d8274bc45de4babb62d64562d92af984dd238
> > Author:     Nick Hoath <nicholas.hoath at intel.com>
> > AuthorDate: Thu Jan 15 13:10:39 2015 +0000
> >
> >     drm/i915: Subsume intel_ctx_submit_request in to drm_i915_gem_request
> 
> Imo the commit message should be ammended with a short paragraph explainig
> the various pointers and implied and explicit references we now have
> around requests and contexts. That way review of this will get a bit
> easier and we'll avoid another misunderstanding.
> 
> I even think we should add a comment in the header to request.ctx to
> explain the rules since apparently they've not been fully clear.
Agree that more documentation around these ctx refs would be good to have to clear up confusion.
For example, I initially thought that this patch introduced a new use-after-free because of the removal of the ctx ref in execlists_context_queue().

> 
> > Signed-off-by: Nick Hoath <nicholas.hoath at intel.com>
> 
> But yeah this makes a lot more sense imo. Please feed this to QA for
> stress-testing in all the relevant bugs. Today I have my head full with
> kms code so not a good time for a full in-depth review. But I think it'd
> be good if other people take a look anyway, so please throw this at a few
> ppl from the vpg core team too.
I guess that would be me...
The code changes look OK, would like to see the updated comments and QA results.

Cheers,
Thomas.

> Thanks, Daniel
> 
> > ---
> >  drivers/gpu/drm/i915/i915_gem.c  | 3 +--
> >  drivers/gpu/drm/i915/intel_lrc.c | 3 +--
> >  2 files changed, 2 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c
> b/drivers/gpu/drm/i915/i915_gem.c
> > index 1765989..79e48b2 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -2660,8 +2660,7 @@ static void i915_gem_reset_ring_cleanup(struct
> drm_i915_private *dev_priv,
> >  		if (submit_req->ctx != ring->default_context)
> >  			intel_lr_context_unpin(ring, submit_req->ctx);
> >
> > -		i915_gem_context_unreference(submit_req->ctx);
> > -		kfree(submit_req);
> > +		i915_gem_request_unreference(submit_req);
> >  	}
> >
> >  	/*
> > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> > index aafcef3..a18925d 100644
> > --- a/drivers/gpu/drm/i915/intel_lrc.c
> > +++ b/drivers/gpu/drm/i915/intel_lrc.c
> > @@ -518,12 +518,12 @@ static int execlists_context_queue(struct
> intel_engine_cs *ring,
> >  			return -ENOMEM;
> >  		request->ring = ring;
> >  		request->ctx = to;
> > +		i915_gem_context_reference(request->ctx);
> >  	} else {
> >  		WARN_ON(to != request->ctx);
> >  	}
> >  	request->tail = tail;
> >  	i915_gem_request_reference(request);
> > -	i915_gem_context_reference(request->ctx);
> >
> >  	intel_runtime_pm_get(dev_priv);
> >
> > @@ -740,7 +740,6 @@ void intel_execlists_retire_requests(struct
> intel_engine_cs *ring)
> >  		if (ctx_obj && (ctx != ring->default_context))
> >  			intel_lr_context_unpin(ring, ctx);
> >  		intel_runtime_pm_put(dev_priv);
> > -		i915_gem_context_unreference(ctx);
> >  		list_del(&req->execlist_link);
> >  		i915_gem_request_unreference(req);
> >  	}
> > --
> > 2.1.1
> >
> 
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx