Re: [Intel-gfx] [bug report] drm/i915: Move submission tasklet to i915_sched_engine

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 06/10/2023 19:50, John Harrison wrote:
> Tvrtko, would you have any thoughts on this one?

I wasn't really involved in that work so without digging deep can only 
say that smatch seems to be noticing a genuine inconsistency. Whether or 
not it is possible at runtime Matt should know better.

3e28d37146db ("drm/i915: Move priolist to new i915_sched_engine object") 
is what added the if (ve->base.sched_engine) guard - maybe that isn't 
needed, I don't know.

Regards,

Tvrtko


> On 10/4/2023 02:57, Dan Carpenter wrote:
>> Hello Matthew Brost,
>>
>> This is a semi-automatic email about new static checker warnings.
>>
>> The patch 22916bad07a5: "drm/i915: Move submission tasklet to
>> i915_sched_engine" from Jun 17, 2021, leads to the following Smatch
>> complaint:
>>
>>      drivers/gpu/drm/i915/gt/intel_execlists_submission.c:3659 
>> rcu_virtual_context_destroy()
>>      warn: variable dereferenced before check 've->base.sched_engine' 
>> (see line 3633)
>>
>> drivers/gpu/drm/i915/gt/intel_execlists_submission.c
>>    3632         */
>>    3633        tasklet_kill(&ve->base.sched_engine->tasklet);
>>                               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>> The patch introduced a new dereference here
>>
>>    3634
>>    3635        /* Decouple ourselves from the siblings, no more access 
>> allowed. */
>>    3636        for (n = 0; n < ve->num_siblings; n++) {
>>    3637            struct intel_engine_cs *sibling = ve->siblings[n];
>>    3638            struct rb_node *node = &ve->nodes[sibling->id].rb;
>>    3639
>>    3640            if (RB_EMPTY_NODE(node))
>>    3641                continue;
>>    3642
>>    3643            spin_lock_irq(&sibling->sched_engine->lock);
>>    3644
>>    3645            /* Detachment is lazily performed in the 
>> sched_engine->tasklet */
>>    3646            if (!RB_EMPTY_NODE(node))
>>    3647                rb_erase_cached(node, 
>> &sibling->execlists.virtual);
>>    3648
>>    3649            spin_unlock_irq(&sibling->sched_engine->lock);
>>    3650        }
>>    3651        
>> GEM_BUG_ON(__tasklet_is_scheduled(&ve->base.sched_engine->tasklet));
>>    3652        GEM_BUG_ON(!list_empty(virtual_queue(ve)));
>>    3653
>>    3654        lrc_fini(&ve->context);
>>    3655        intel_context_fini(&ve->context);
>>    3656
>>    3657        if (ve->base.breadcrumbs)
>>    3658            intel_breadcrumbs_put(ve->base.breadcrumbs);
>>    3659        if (ve->base.sched_engine)
>>                      ^^^^^^^^^^^^^^^^^^^^^
>> But previous code had assumed the sched_engine could be NULL.
>>
>>    3660            i915_sched_engine_put(ve->base.sched_engine);
>>    3661        intel_engine_free_request_pool(&ve->base);
>>
>> regards,
>> dan carpenter
>