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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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@xxxxxxxxxxxxxx>
Fixes: d4433c7600f7 ("drm/i915/gem: Use the proto-context to handle create parameters (v5)")
Signed-off-by: Matthew Brost <matthew.brost@xxxxxxxxx>
Cc: <stable@xxxxxxxxxxxxxxx>
---
   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@xxxxxxxxx>

Regards,

Tvrtko



[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux