[Intel-gfx] [PATCH] drm/i915: Use a dummy timeline name for a signaled fence

[Michał Winiarski]

On Thu, Mar 30, 2017 at 12:16:14PM +0100, Chris Wilson wrote:
> Michał Winiarski pointed out that the debugging infrastructure (such as
> trace_dma_fence_release) likes to pretty print the timeline name, long
> after we have freed the timeline. Our timelines currently live as part of
> the GTT (due to the strict ordering we current use through each) which
> belong to the context. We aim to free the context and release its
> hardware resources as soon as we able to (i.e. when the last
> fence/request using it has been signaled and retired). As the
> .get_timeline_name is purely a debug feature, rather than extending the
> lifetime of the context, or splitting it into many different release
> phases just to keep the name along, replace the timeline name with a
> constant after the fence has been signaled. This avoids the potential
> use-after-free.
> 
> Reported-by: Michał Winiarski <michal.winiarski at intel.com>

Actually, I'm just a messenger ;)

Reported-by: Krzysztof Olinski <krzysztof.e.olinski at intel.com>

Reviewed-by: Michał Winiarski <michal.winiarski at intel.com>

-Michał

> Fixes: 80b204bce8f2 ("drm/i915: Enable multiple timelines")
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Michał Winiarski <michal.winiarski at intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> Cc: <stable at vger.kernel.org> # v4.10+
> ---
>  drivers/gpu/drm/i915/i915_gem_request.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_request.c b/drivers/gpu/drm/i915/i915_gem_request.c
> index cb9209589d2e..2cb2206b2f6b 100644
> --- a/drivers/gpu/drm/i915/i915_gem_request.c
> +++ b/drivers/gpu/drm/i915/i915_gem_request.c
> @@ -37,6 +37,17 @@ static const char *i915_fence_get_driver_name(struct dma_fence *fence)
>  
>  static const char *i915_fence_get_timeline_name(struct dma_fence *fence)
>  {
> +	/* The timeline struct (as part of the ppgtt underneath a context)
> +	 * may be freed when the request is no longer in use by the GPU.
> +	 * We could extend the life of a context to beyond that of all
> +	 * fences, possibly keeping the hw resource around indefinitely,
> +	 * or we just give them a false name. Since
> +	 * dma_fence_ops.get_timeline_name is a debug feature, the occasional
> +	 * lie seems justifiable.
> +	 */
> +	if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
> +		return "signaled";
> +
>  	return to_request(fence)->timeline->common->name;
>  }
>  
> -- 
> 2.11.0