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

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

[Chris Wilson]

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@intel.com>
Fixes: 80b204bce8f2 ("drm/i915: Enable multiple timelines")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Michał Winiarski <michal.winiarski@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: <stable@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

Re: [PATCH] drm/i915: Use a dummy timeline name for a signaled fence

[Chris Wilson]

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
s/current/currently/

> 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
s/along/around/

> constant after the fence has been signaled. This avoids the potential
> use-after-free.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: Use a dummy timeline name for a signaled fence

[Joonas Lahtinen]

On to, 2017-03-30 at 12:19 +0100, Chris Wilson wrote:
> 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
> s/current/currently/
> 
> > 
> > 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
> s/along/around/
> 
> > 
> > constant after the fence has been signaled. This avoids the potential
> > use-after-free.

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation

Re: [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@intel.com>

Actually, I'm just a messenger ;)

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

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

-Michał

> Fixes: 80b204bce8f2 ("drm/i915: Enable multiple timelines")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Michał Winiarski <michal.winiarski@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: <stable@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

Re: [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@intel.com>

Actually, I'm just a messenger ;)

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

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

-Michał

> Fixes: 80b204bce8f2 ("drm/i915: Enable multiple timelines")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Michał Winiarski <michal.winiarski@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: <stable@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

✓ Fi.CI.BAT: success for drm/i915: Use a dummy timeline name for a signaled fence

[Patchwork]

== Series Details ==

Series: drm/i915: Use a dummy timeline name for a signaled fence
URL   : https://patchwork.freedesktop.org/series/22193/
State : success

== Summary ==

Series 22193v1 drm/i915: Use a dummy timeline name for a signaled fence
https://patchwork.freedesktop.org/api/1.0/series/22193/revisions/1/mbox/

Test gem_exec_flush:
        Subgroup basic-batch-kernel-default-uc:
                fail       -> PASS       (fi-snb-2600) fdo#100007

fdo#100007 https://bugs.freedesktop.org/show_bug.cgi?id=100007

fi-bdw-5557u     total:278  pass:267  dwarn:0   dfail:0   fail:0   skip:11  time: 429s
fi-bdw-gvtdvm    total:278  pass:256  dwarn:8   dfail:0   fail:0   skip:14  time: 426s
fi-bsw-n3050     total:278  pass:239  dwarn:0   dfail:0   fail:0   skip:39  time: 564s
fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19  time: 509s
fi-bxt-t5700     total:278  pass:258  dwarn:0   dfail:0   fail:0   skip:20  time: 561s
fi-byt-j1900     total:278  pass:251  dwarn:0   dfail:0   fail:0   skip:27  time: 489s
fi-byt-n2820     total:278  pass:247  dwarn:0   dfail:0   fail:0   skip:31  time: 481s
fi-hsw-4770      total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time: 413s
fi-hsw-4770r     total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time: 408s
fi-ilk-650       total:278  pass:228  dwarn:0   dfail:0   fail:0   skip:50  time: 423s
fi-ivb-3520m     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 496s
fi-ivb-3770      total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 463s
fi-kbl-7500u     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 452s
fi-kbl-7560u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time: 568s
fi-skl-6260u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time: 458s
fi-skl-6700hq    total:278  pass:261  dwarn:0   dfail:0   fail:0   skip:17  time: 578s
fi-skl-6700k     total:278  pass:256  dwarn:4   dfail:0   fail:0   skip:18  time: 462s
fi-skl-gvtdvm    total:278  pass:265  dwarn:0   dfail:0   fail:0   skip:13  time: 433s
fi-snb-2520m     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time: 524s
fi-snb-2600      total:278  pass:249  dwarn:0   dfail:0   fail:0   skip:29  time: 399s
fi-skl-6770hq failed to collect. IGT log at Patchwork_4359/fi-skl-6770hq/igt.log

71dab8f683b2d609926524f9a6a60b00e4bfa543 drm-tip: 2017y-03m-30d-11h-44m-46s UTC integration manifest
e627708 drm/i915: Use a dummy timeline name for a signaled fence

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4359/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: ✓ Fi.CI.BAT: success for drm/i915: Use a dummy timeline name for a signaled fence

[Chris Wilson]

On Thu, Mar 30, 2017 at 01:10:15PM -0000, Patchwork wrote:
> == Series Details ==
> 
> Series: drm/i915: Use a dummy timeline name for a signaled fence
> URL   : https://patchwork.freedesktop.org/series/22193/
> State : success
> 
> == Summary ==
> 
> Series 22193v1 drm/i915: Use a dummy timeline name for a signaled fence
> https://patchwork.freedesktop.org/api/1.0/series/22193/revisions/1/mbox/
> 
> Test gem_exec_flush:
>         Subgroup basic-batch-kernel-default-uc:
>                 fail       -> PASS       (fi-snb-2600) fdo#100007
> 
> fdo#100007 https://bugs.freedesktop.org/show_bug.cgi?id=100007

Pushed, and I put in my earplugs so I can't hear the cries of confusion.
Lalalalala,
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx