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

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

Op 21-06-2021 om 15:20 schreef Tvrtko Ursulin:
>
> 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? 

I noticed it mostly when debugging. I fixed it currenly by not allocating state in set_ring_size unnecessarily, hence this patch is no longer needed. :)

So if that's the only thing, I can just drop this patch entirely.

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx