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@xxxxxxxxx> --- 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;
