| Message ID | 20230605164840.1234582-1-andrzej.hajda@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: do not dereference dangling engine pointer on fence release | expand |
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; >
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;
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. 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(-)