Re: [Intel-gfx] [PATCH v2] drm/i915/gt: update request engine before removing virtual GuC engine

[Andrzej Hajda <andrzej.hajda@intel.com>]

[-- Attachment #1: Type: text/plain, Size: 2967 bytes --]



On 11.07.2023 12:18, Andi Shyti wrote:
> 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?

yes, I forgot to add changelog.

>
>> Closes:https://gitlab.freedesktop.org/drm/intel/-/issues/7926
>> Signed-off-by: Andrzej Hajda<andrzej.hajda@intel.com>
>> ---
>>   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.

Setting NULL causes occasional null page de-reference in

i915_request_wait_timeout:

mutex_release(&rq->engine->gt->reset.mutex.dep_map, _THIS_IP_)

rq->engine after removing rq from context is (IMHO) used as a set of 
aliases for gt and i915 (despite rq itself contains the alias to i915).

Regards

Andrzej

>
> Andi
>
>> +		break;
>> +	}
>>   }
>>   
>>   static const struct intel_context_ops guc_context_ops = {
>> -- 
>> 2.34.1

[-- Attachment #2: Type: text/html, Size: 4507 bytes --]