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@xxxxxxxxx>
Actually, I'm just a messenger ;)
Reported-by: Krzysztof Olinski <krzysztof.e.olinski@xxxxxxxxx>
Reviewed-by: Michał Winiarski <michal.winiarski@xxxxxxxxx>
-Michał
> Fixes: 80b204bce8f2 ("drm/i915: Enable multiple timelines")
> Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Cc: Michał Winiarski <michal.winiarski@xxxxxxxxx>
> Cc: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
> Cc: <stable@xxxxxxxxxxxxxxx> # 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