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

[Tvrtko Ursulin]

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.

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
>