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

Tvrtko Ursulin
2021-10-01T08:40:28Z

+ 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