Re: [Intel-gfx] [PATCH] drm/i915: Fix bug in user proto-context creation that leaked contexts

Tvrtko Ursulin
2021-10-05T09:26:14Z

On 01/10/2021 16:48, Matthew Brost wrote:
> On Fri, Oct 01, 2021 at 09:40:19AM +0100, Tvrtko Ursulin wrote:
>>
>> + Daniel as reviewer and maybe merge, avoid falling through cracks at least.
>>
>
> Ty, working on push rights myself.

I ended up pushing it myself to avoid having a potential crash in the
driver for too long. Hope people will not mind.

Regards,

Tvrtko

>> On 22/09/2021 20:43, Matthew Brost wrote:
>>> Set number of engines before attempting to create contexts so the
>>> function free_engines can clean up properly. Also check return of
>>> alloc_engines for NULL.
>>>
>>> v2:
>>> (Tvrtko)
>>> - Send as stand alone patch
>>> (John Harrison)
>>> - Check for alloc_engines returning NULL
>>>
>>> Cc: Jason Ekstrand <jason@jlekstrand.net>
>>> Fixes: d4433c7600f7 ("drm/i915/gem: Use the proto-context to handle create parameters (v5)")
>>> Signed-off-by: Matthew Brost <matthew.brost@intel.com>
>>> Cc: <stable@vger.kernel.org>
>>> ---
>>> drivers/gpu/drm/i915/gem/i915_gem_context.c | 6 +++++-
>>> 1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c
>>> index c2ab0e22db0a..9627c7aac6a3 100644
>>> --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
>>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
>>> @@ -898,6 +898,11 @@ static struct i915_gem_engines *user_engines(struct i915_gem_context *ctx,
>>> unsigned int n;
>>> e = alloc_engines(num_engines);
>>> + if (!e) {
>>> + return ERR_PTR(-ENOMEM);
>>> + }
>>
>> Ideally remove the braces and respin.
>>
>
> Yep, checkpatch didn't like this. Will respin.
>
>>> + e->num_engines = num_engines;
>>
>> Theoretically you could have put it next to "e->engines[n] = ce" assignment
>> so the pattern is the same as in default_engines(). Kind of makes more sense
>> that the number is not set before anything is created, but as it doesn't
>> really matter since free_engines handles sparse arrays so there is argument
>> to have a simpler single assignment as well.
>>
>
> I like a single assignment, let's not overthink this.
>
>>> +
>>> for (n = 0; n < num_engines; n++) {
>>> struct intel_context *ce;
>>> int ret;
>>> @@ -931,7 +936,6 @@ static struct i915_gem_engines *user_engines(struct i915_gem_context *ctx,
>>> goto free_engines;
>>> }
>>> }
>>> - e->num_engines = num_engines;
>>> return e;
>>>
>>
>> Fix looks good to me. I did not want to butt in but since more than a week
>> has passed without it getting noticed:
>>
>
> Again, ty.
>
> Matt
>
>> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> Regards,
>>
>> Tvrtko