Re: [Intel-gfx] [PATCH 2/2] drm/i915: Avoid using rq->engine after free during i915_fence_release

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 21/05/2020 09:53, Chris Wilson wrote:
> In order to be valid to dereference during the i915_fence_release, after
> retiring the fence and releasing its refererences, we assume that
> rq->engine can only be a real engine (that stay intact until the device
> is shutdown after all fences have been flushed). However, due to a quirk
> of preempt-to-busy, we may retire a request that still belongs to a
> virtual engine and so eventually free it with rq->engine being invalid.
> To avoid dereferencing that invalid engine, we look at the
> execution_mask which if it indicates it may be executed on more than one
> engine, we know it originated on a virtual engine and may still be on
> one.
> 
> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/1906
> Fixes: 43acd6516ca9 ("drm/i915: Keep a per-engine request pool")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_request.c | 25 +++++++++++++++++++++++--
>   1 file changed, 23 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
> index 526c1e9acbd5..6e357183bece 100644
> --- a/drivers/gpu/drm/i915/i915_request.c
> +++ b/drivers/gpu/drm/i915/i915_request.c
> @@ -121,8 +121,29 @@ static void i915_fence_release(struct dma_fence *fence)
>   	i915_sw_fence_fini(&rq->submit);
>   	i915_sw_fence_fini(&rq->semaphore);
>   
> -	/* Keep one request on each engine for reserved use under mempressure */
> -	if (!cmpxchg(&rq->engine->request_pool, NULL, rq))
> +	/*
> +	 * Keep one request on each engine for reserved use under mempressure
> +	 *
> +	 * We do not hold a reference to the engine here and so have to be
> +	 * very careful in what rq->engine we poke. The virtual engine is
> +	 * referenced via the rq->context and we released that ref during
> +	 * i915_request_retire(), ergo we must not dereference a virtual
> +	 * engine here. Not that we would want to, as the only consumer of
> +	 * the reserved engine->request_pool is the powermanagent parking,

power management

> +	 * which must-not-fail, and that is only run on the physical engines.
> +	 *
> +	 * Since the request must have been executed to be have completed,
> +	 * we know that it will have been processed by the HW and will
> +	 * not be unsubmitted again, so rq->engine and rq->execution_mask
> +	 * at this point is stable. rq->execution_mask will be a single
> +	 * bit if the last and only engine it could execution on was a
> +	 * physical engine, if it's multiple bits then it started on and
> +	 * could still be on a virtual engine. Thus if the mask is not a
> +	 * power-of-two we assume that rq->engine may still be a virtual
> +	 * engien and so a dangling invalid pointer that we cannot 

engine

But.. submit fence can mask out execution_mask bits and make it appear 
the request was on a physical engine. What then?

Regards,

Tvrtko

dereference
> +	 */
> +	if (is_power_of_2(rq->execution_mask) &&
> +	    !cmpxchg(&rq->engine->request_pool, NULL, rq))
>   		return;
>   
>   	kmem_cache_free(global.slab_requests, rq);
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx