Re: [Intel-gfx] [PATCH] drm/i915: do not dereference dangling engine pointer on fence release

[Nirmoy Das <nirmoy.das@intel.com>]

Hi Andrzej,

On 6/5/2023 6:48 PM, Andrzej Hajda wrote:
> rq->engine can be a dangling pointer if rq->execution_mask has more than
> one bit set, ie it could be already freed virtual engine. Changing check
> order prevents dereferncing it in intel_engine_is_virtual(rq->engine).
> Full description of possible scenarios at the inline comment before
> the change.


I came to the same conclusion but Chris mentioned "you create a guc 
virtual engine with just one bit in execution_mask" :)

Suggestion from Chris was to have "guc virtual bit" in there or 
"eliminate the single engine guc virtuals".


Regards,

Nirmoy

> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/7926
> Signed-off-by: Andrzej Hajda <andrzej.hajda@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_request.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
> index 630a732aaecca8..8775952f5c1bbd 100644
> --- a/drivers/gpu/drm/i915/i915_request.c
> +++ b/drivers/gpu/drm/i915/i915_request.c
> @@ -166,8 +166,8 @@ static void i915_fence_release(struct dma_fence *fence)
>   	 * know that if the rq->execution_mask is a single bit, rq->engine
>   	 * can be a physical engine with the exact corresponding mask.
>   	 */
> -	if (!intel_engine_is_virtual(rq->engine) &&
> -	    is_power_of_2(rq->execution_mask) &&
> +	if (is_power_of_2(rq->execution_mask) &&
> +	    !intel_engine_is_virtual(rq->engine) &&
>   	    !cmpxchg(&rq->engine->request_pool, NULL, rq))
>   		return;
>