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

[Daniel Vetter <daniel@ffwll.ch>]

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@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.

> Signed-off-by: Nick Hoath <nicholas.hoath@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.

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@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx