Hi Andrzej, On Thu, Jul 06, 2023 at 05:16:11PM +0200, Andrzej Hajda wrote: > GuC virtual engines can be removed before request removal. On the other > side driver expects rq->engine to be a valid pointer for a whole life of > request. Setting rq->engine to an always valid engine should solve > the issue. > > The patch fixes bug detected by KASAN with following signature: > i915 0000:00:02.0: [drm:i915_drop_caches_set [i915]] Dropping caches: 0x0000005c [0x0000005c] > BUG: KASAN: slab-use-after-free in i915_fence_release+0x2a2/0x2f0 [i915] > Read of size 4 at addr ffff88813ffda6e8 by task kworker/u32:10/157 > ... > Allocated by task 1184: > ... > guc_create_virtual+0x4d/0x1160 [i915] > i915_gem_create_context+0x11ee/0x18c0 [i915] > ... > Freed by task 151: > ... > intel_guc_deregister_done_process_msg+0x324/0x6d0 [i915] > ... so the only difference between v1 and v2 is this part of the log? > Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/7926 > Signed-off-by: Andrzej Hajda <andrzej.hajda@xxxxxxxxx> > --- > drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > index a0e3ef1c65d246..2c877ea5eda6f0 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > @@ -3461,6 +3461,8 @@ static void guc_prio_fini(struct i915_request *rq, struct intel_context *ce) > static void remove_from_context(struct i915_request *rq) > { > struct intel_context *ce = request_to_scheduling_context(rq); > + struct intel_engine_cs *engine; > + intel_engine_mask_t tmp; > > GEM_BUG_ON(intel_context_is_child(ce)); > > @@ -3478,6 +3480,15 @@ static void remove_from_context(struct i915_request *rq) > > atomic_dec(&ce->guc_id.ref); > i915_request_notify_execute_cb_imm(rq); > + > + /* > + * GuC virtual engine can disappear after this call, so let's assign > + * something valid, as driver expects this to be always valid pointer. > + */ > + for_each_engine_masked(engine, rq->engine->gt, rq->execution_mask, tmp) { > + rq->engine = engine; yes... here the context might lose the virtual engine... I wonder whether this is the rigth solution, though. Maybe we should set rq->engine = NULL; and check for NULL? Don't know. Andi > + break; > + } > } > > static const struct intel_context_ops guc_context_ops = { > -- > 2.34.1
