| Message ID | 20210820224446.30620-20-matthew.brost@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Parallel submission aka multi-bb execbuf | expand |
On 8/20/2021 15:44, Matthew Brost wrote: > Set number of engines before attempting to create contexts so the > function free_engines can clean up properly. > > 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 | 2 +- > 1 file changed, 1 insertion(+), 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 dbaeb924a437..bcaaf514876b 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c > @@ -944,6 +944,7 @@ static struct i915_gem_engines *user_engines(struct i915_gem_context *ctx, > unsigned int n; > > e = alloc_engines(num_engines); This can return null when out of memory. There needs to be an early exit check before dereferencing a null pointer. Not sure if that is a worse bug or not than leaking memory! Either way, it would be good to fix that too. John. > + e->num_engines = num_engines; > for (n = 0; n < num_engines; n++) { > struct intel_context *ce; > int ret; > @@ -977,7 +978,6 @@ static struct i915_gem_engines *user_engines(struct i915_gem_context *ctx, > goto free_engines; > } > } > - e->num_engines = num_engines; > > return e; >
On 20/09/2021 23:57, John Harrison wrote: > On 8/20/2021 15:44, Matthew Brost wrote: >> Set number of engines before attempting to create contexts so the >> function free_engines can clean up properly. >> >> 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 | 2 +- >> 1 file changed, 1 insertion(+), 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 dbaeb924a437..bcaaf514876b 100644 >> --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c >> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c >> @@ -944,6 +944,7 @@ static struct i915_gem_engines >> *user_engines(struct i915_gem_context *ctx, >> unsigned int n; >> e = alloc_engines(num_engines); > This can return null when out of memory. There needs to be an early exit > check before dereferencing a null pointer. Not sure if that is a worse > bug or not than leaking memory! Either way, it would be good to fix that > too. Pull out from the series and send a fix standalone ASAP? Also suggest adding author and reviewer to cc for typically quicker turnaround time. Regards, Tvrtko > John. > >> + e->num_engines = num_engines; >> for (n = 0; n < num_engines; n++) { >> struct intel_context *ce; >> int ret; >> @@ -977,7 +978,6 @@ static struct i915_gem_engines >> *user_engines(struct i915_gem_context *ctx, >> goto free_engines; >> } >> } >> - e->num_engines = num_engines; >> return e; >
On Mon, Sep 20, 2021 at 03:57:06PM -0700, John Harrison wrote: > On 8/20/2021 15:44, Matthew Brost wrote: > > Set number of engines before attempting to create contexts so the > > function free_engines can clean up properly. > > > > 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 | 2 +- > > 1 file changed, 1 insertion(+), 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 dbaeb924a437..bcaaf514876b 100644 > > --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c > > @@ -944,6 +944,7 @@ static struct i915_gem_engines *user_engines(struct i915_gem_context *ctx, > > unsigned int n; > > e = alloc_engines(num_engines); > This can return null when out of memory. There needs to be an early exit > check before dereferencing a null pointer. Not sure if that is a worse bug > or not than leaking memory! Either way, it would be good to fix that too. > Indeed there is another bug. Will fix that one too. Matt > John. > > > + e->num_engines = num_engines; > > for (n = 0; n < num_engines; n++) { > > struct intel_context *ce; > > int ret; > > @@ -977,7 +978,6 @@ static struct i915_gem_engines *user_engines(struct i915_gem_context *ctx, > > goto free_engines; > > } > > } > > - e->num_engines = num_engines; > > return e; >
On Tue, Sep 21, 2021 at 03:49:53PM +0100, Tvrtko Ursulin wrote: > > On 20/09/2021 23:57, John Harrison wrote: > > On 8/20/2021 15:44, Matthew Brost wrote: > > > Set number of engines before attempting to create contexts so the > > > function free_engines can clean up properly. > > > > > > 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 | 2 +- > > > 1 file changed, 1 insertion(+), 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 dbaeb924a437..bcaaf514876b 100644 > > > --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c > > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c > > > @@ -944,6 +944,7 @@ static struct i915_gem_engines > > > *user_engines(struct i915_gem_context *ctx, > > > unsigned int n; > > > e = alloc_engines(num_engines); > > This can return null when out of memory. There needs to be an early exit > > check before dereferencing a null pointer. Not sure if that is a worse > > bug or not than leaking memory! Either way, it would be good to fix that > > too. > > Pull out from the series and send a fix standalone ASAP? Also suggest adding Sure, will do. Matt > author and reviewer to cc for typically quicker turnaround time. > > Regards, > > Tvrtko > > > > John. > > > > > + e->num_engines = num_engines; > > > for (n = 0; n < num_engines; n++) { > > > struct intel_context *ce; > > > int ret; > > > @@ -977,7 +978,6 @@ static struct i915_gem_engines > > > *user_engines(struct i915_gem_context *ctx, > > > goto free_engines; > > > } > > > } > > > - e->num_engines = num_engines; > > > return e; > >
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c index dbaeb924a437..bcaaf514876b 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c @@ -944,6 +944,7 @@ static struct i915_gem_engines *user_engines(struct i915_gem_context *ctx, unsigned int n; e = alloc_engines(num_engines); + e->num_engines = num_engines; for (n = 0; n < num_engines; n++) { struct intel_context *ce; int ret; @@ -977,7 +978,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. 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 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)