Re: [Intel-gfx] [PATCH 1/3] drm/i915/gt: Do not allow setting ring size for legacy ring submission

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

On 21/06/2021 14:12, Tvrtko Ursulin wrote:
> 
> On 21/06/2021 14:07, Maarten Lankhorst wrote:
>> Op 21-06-2021 om 14:52 schreef Tvrtko Ursulin:
>>>
>>> On 21/06/2021 13:08, Tvrtko Ursulin wrote:
>>>>
>>>> I had some questions on the trybot mailing list, let me copy&paste..
>>>>
>>>> On 21/06/2021 12:41, Maarten Lankhorst wrote:
>>>>> It doesn't work for legacy ring submission, and is in the best case
>>>>> ignored.
>>>>
>>>> Looks rejected instead of ignored:
>>>>
>>>> static int set_ringsize(struct i915_gem_context *ctx,
>>>>               struct drm_i915_gem_context_param *args)
>>>> {
>>>>       if (!HAS_LOGICAL_RING_CONTEXTS(ctx->i915))
>>>>           return -ENODEV;
>>>>>
>>>>> In the worst case we end up freeing engine->legacy.ring for all other
>>>>> active engines, resulting in a use-after-free.
>>>>
>>>> Worst case is cloning because ring_context_alloc is not taking a 
>>>> reference to engine->legacy.ring, or something else?
>>>
>>> No can't be that, it was my incomplete analysis last week. Since 
>>> ring_context_destroy does not actually free the legacy ring I don't 
>>> see any use after free paths.
>>>
>>> Regards,
>>
>> Hmm, it gets stuck inside intel_context_set_ring_size when cloning 
>> engines..
>>
>> I guess it can't happen in practice, just the code introduces the race 
>> by preallocating
>> inside intel_context_lock_pinned()..
> 
> "The code" being the rest of your series? Haven't looked in there, but 
> can't find a problem in upstream. Since as you say, copy_ring_size will 
> run but intel_context_set_ring_size will not free-and-allocate old/new 
> ring since cloned context does not have a state allocated yet.

P.S. Putting a HAS_LOGICAL_RING_CONTEXTS check in copy_ring_size would 
be a bit unfortunate because layering is a bit broken at the moment and 
that wouldn't make it better.

To clarify my thinking: At the moment allocating the ring is 
responsibility of a backend specific hook. Apart from the generic 
intel_context_set_ring_size which breaks that by allocating in the layer 
above the backend. So proper fix could be to introduce backend specific 
hooks for ring allocation/freeing.

*If* you need to allocate the state so early.. not sure about that. I'd 
first need to understand why. If you say it is a race then it was all 
accidental?

Regards,

Tvrtko

> Regards,
> 
> Tvrtko
> 
>> copy_ring_size() should only be called for HAS_LOGICAL_RING_CONTEXTS().
>> I guess that makes this patch obsolete. It can safely be dropped from 
>> the series,
>> I think I should probably introduce a check to only set the size when 
>> HAS_LOGICAL_RING_CONTEXTS
>> evaluates to true, but that wouldn't block the rest of this series.
>>
>> ~Maarten
>>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx