| Message ID | 20210922194333.8956-1-matthew.brost@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Fix bug in user proto-context creation that leaked contexts | expand |
+ 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
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
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
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;
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(-)