* [PATCH] drm/i915: Fix bug in user proto-context creation that leaked contexts
@ 2021-09-22 19:43 Matthew Brost
2021-10-01 8:40 ` [Intel-gfx] " Tvrtko Ursulin
0 siblings, 1 reply; 5+ messages in thread
From: Matthew Brost @ 2021-09-22 19:43 UTC (permalink / raw)
To: dri-devel, intel-gfx; +Cc: jason
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915: Fix bug in user proto-context creation that leaked contexts
2021-09-22 19:43 [PATCH] drm/i915: Fix bug in user proto-context creation that leaked contexts Matthew Brost
@ 2021-10-01 8:40 ` Tvrtko Ursulin
2021-10-01 15:48 ` Matthew Brost
0 siblings, 1 reply; 5+ messages in thread
From: Tvrtko Ursulin @ 2021-10-01 8:40 UTC (permalink / raw)
To: Matthew Brost, dri-devel, intel-gfx; +Cc: jason, Daniel Vetter
+ 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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915: Fix bug in user proto-context creation that leaked contexts
2021-10-01 8:40 ` [Intel-gfx] " Tvrtko Ursulin
@ 2021-10-01 15:48 ` Matthew Brost
2021-10-05 9:25 ` Tvrtko Ursulin
0 siblings, 1 reply; 5+ messages in thread
From: Matthew Brost @ 2021-10-01 15:48 UTC (permalink / raw)
To: Tvrtko Ursulin; +Cc: dri-devel, intel-gfx, jason, Daniel Vetter
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915: Fix bug in user proto-context creation that leaked contexts
2021-10-01 15:48 ` Matthew Brost
@ 2021-10-05 9:25 ` Tvrtko Ursulin
0 siblings, 0 replies; 5+ messages in thread
From: Tvrtko Ursulin @ 2021-10-05 9:25 UTC (permalink / raw)
To: Matthew Brost; +Cc: dri-devel, intel-gfx, jason, Daniel Vetter
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] drm/i915: Fix bug in user proto-context creation that leaked contexts
@ 2021-10-01 15:58 Matthew Brost
0 siblings, 0 replies; 5+ messages in thread
From: Matthew Brost @ 2021-10-01 15:58 UTC (permalink / raw)
To: intel-gfx, dri-devel
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
v3:
(Checkpatch / Tvrtko)
- Remove braces around single line if statement
Cc: Jason Ekstrand <jason@jlekstrand.net>
Fixes: d4433c7600f7 ("drm/i915/gem: Use the proto-context to handle create parameters (v5)")
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Signed-off-by: Matthew Brost <matthew.brost@intel.com>
Cc: <stable@vger.kernel.org>
---
drivers/gpu/drm/i915/gem/i915_gem_context.c | 5 ++++-
1 file changed, 4 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 8208fd5b72c3..8c7ea6e56262 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
@@ -898,6 +898,10 @@ 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 +935,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
^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-10-05 9:26 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-22 19:43 [PATCH] drm/i915: Fix bug in user proto-context creation that leaked contexts Matthew Brost
2021-10-01 8:40 ` [Intel-gfx] " Tvrtko Ursulin
2021-10-01 15:48 ` Matthew Brost
2021-10-05 9:25 ` Tvrtko Ursulin
2021-10-01 15:58 Matthew Brost
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).