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

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

[Matthew Brost]

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);
+	}
+	e->num_engines = num_engines;
+
 	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;
 
-- 
2.32.0

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

[Tvrtko Ursulin]

+ Daniel as reviewer and maybe merge, avoid falling through cracks at least.

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.

> +	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.

> +
>   	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:

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko

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

[Matthew Brost]

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.

> 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

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

[Tvrtko Ursulin]

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