drm/i915/gem: Delay tracking the GEM context until it is registered
diff mbox series

Message ID 20200723183348.4037-1-chris@chris-wilson.co.uk
State New
Headers show
Series
  • drm/i915/gem: Delay tracking the GEM context until it is registered
Related show

Commit Message

Chris Wilson July 23, 2020, 6:33 p.m. UTC
Avoid exposing a partially constructed context by deferring the
list_add() from the initial construction to the end of registration.
Otherwise, if we peek into the list of contexts from inside debugfs, we
may see the partially constructed context and change down some dangling
incomplete pointers.

Reported-by: CQ Tang <cq.tang@intel.com>
Fixes: b91715417244 ("drm/i915: Extend CONTEXT_CREATE to set parameters upon construction")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: CQ Tang <cq.tang@intel.com>
Cc: <stable@vger.kernel.org> # v5.2+
---
 drivers/gpu/drm/i915/gem/i915_gem_context.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

Comments

Chris Wilson July 23, 2020, 6:35 p.m. UTC | #1
Quoting Chris Wilson (2020-07-23 19:33:48)
> Avoid exposing a partially constructed context by deferring the
> list_add() from the initial construction to the end of registration.
> Otherwise, if we peek into the list of contexts from inside debugfs, we
> may see the partially constructed context and change down some dangling

s/change/chase/

> incomplete pointers.
Mika Kuoppala July 24, 2020, 11:55 a.m. UTC | #2
Chris Wilson <chris@chris-wilson.co.uk> writes:

> Quoting Chris Wilson (2020-07-23 19:33:48)
>> Avoid exposing a partially constructed context by deferring the
>> list_add() from the initial construction to the end of registration.
>> Otherwise, if we peek into the list of contexts from inside debugfs, we
>> may see the partially constructed context and change down some dangling
>
> s/change/chase/

that.

Are you sure about the fixes as it is not the commit that
actually introduces the registration into context.list?

For me it looks like it is a4e7ccdac38e.
-Mika

>
>> incomplete pointers.
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Chris Wilson July 24, 2020, 12:06 p.m. UTC | #3
Quoting Mika Kuoppala (2020-07-24 12:55:39)
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> 
> > Quoting Chris Wilson (2020-07-23 19:33:48)
> >> Avoid exposing a partially constructed context by deferring the
> >> list_add() from the initial construction to the end of registration.
> >> Otherwise, if we peek into the list of contexts from inside debugfs, we
> >> may see the partially constructed context and change down some dangling
> >
> > s/change/chase/
> 
> that.
> 
> Are you sure about the fixes as it is not the commit that
> actually introduces the registration into context.list?
> 
> For me it looks like it is a4e7ccdac38e.

The one I picked was where we started adding more context setup after
the final step of list_add in __create_context(). More apropos would be
3aa9945a528e ("drm/i915: Separate GEM context construction and registration to userspace")
in the same kernel.

In the other direction, we have
f6e8aa387171 ("drm/i915: Report the number of closed vma held by each context in debugfs")
where we start using the contexts.list in debugfs.

In a4e7ccdac38e we are only moving the list_add(&ctx->link) around in
__create_context().
-Chris
Mika Kuoppala July 24, 2020, 12:31 p.m. UTC | #4
Chris Wilson <chris@chris-wilson.co.uk> writes:

> Quoting Mika Kuoppala (2020-07-24 12:55:39)
>> Chris Wilson <chris@chris-wilson.co.uk> writes:
>> 
>> > Quoting Chris Wilson (2020-07-23 19:33:48)
>> >> Avoid exposing a partially constructed context by deferring the
>> >> list_add() from the initial construction to the end of registration.
>> >> Otherwise, if we peek into the list of contexts from inside debugfs, we
>> >> may see the partially constructed context and change down some dangling
>> >
>> > s/change/chase/
>> 
>> that.
>> 
>> Are you sure about the fixes as it is not the commit that
>> actually introduces the registration into context.list?
>> 
>> For me it looks like it is a4e7ccdac38e.
>
> The one I picked was where we started adding more context setup after
> the final step of list_add in __create_context(). More apropos would be
> 3aa9945a528e ("drm/i915: Separate GEM context construction and registration to userspace")
> in the same kernel.

Ok, thanks for clearing that out.

The i915_perf iteration seems to not get hurt either so,

Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>

>
> In the other direction, we have
> f6e8aa387171 ("drm/i915: Report the number of closed vma held by each context in debugfs")
> where we start using the contexts.list in debugfs.
>
> In a4e7ccdac38e we are only moving the list_add(&ctx->link) around in
> __create_context().
> -Chris

Patch
diff mbox series

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c
index d0bdb6d447ed..efc4ba34c06e 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
@@ -713,6 +713,7 @@  __create_context(struct drm_i915_private *i915)
 	ctx->i915 = i915;
 	ctx->sched.priority = I915_USER_PRIORITY(I915_PRIORITY_NORMAL);
 	mutex_init(&ctx->mutex);
+	INIT_LIST_HEAD(&ctx->link);
 
 	spin_lock_init(&ctx->stale.lock);
 	INIT_LIST_HEAD(&ctx->stale.engines);
@@ -740,10 +741,6 @@  __create_context(struct drm_i915_private *i915)
 	for (i = 0; i < ARRAY_SIZE(ctx->hang_timestamp); i++)
 		ctx->hang_timestamp[i] = jiffies - CONTEXT_FAST_HANG_JIFFIES;
 
-	spin_lock(&i915->gem.contexts.lock);
-	list_add_tail(&ctx->link, &i915->gem.contexts.list);
-	spin_unlock(&i915->gem.contexts.lock);
-
 	return ctx;
 
 err_free:
@@ -931,6 +928,7 @@  static int gem_context_register(struct i915_gem_context *ctx,
 				struct drm_i915_file_private *fpriv,
 				u32 *id)
 {
+	struct drm_i915_private *i915 = ctx->i915;
 	struct i915_address_space *vm;
 	int ret;
 
@@ -949,8 +947,16 @@  static int gem_context_register(struct i915_gem_context *ctx,
 	/* And finally expose ourselves to userspace via the idr */
 	ret = xa_alloc(&fpriv->context_xa, id, ctx, xa_limit_32b, GFP_KERNEL);
 	if (ret)
-		put_pid(fetch_and_zero(&ctx->pid));
+		goto err_pid;
+
+	spin_lock(&i915->gem.contexts.lock);
+	list_add_tail(&ctx->link, &i915->gem.contexts.list);
+	spin_unlock(&i915->gem.contexts.lock);
+
+	return 0;
 
+err_pid:
+	put_pid(fetch_and_zero(&ctx->pid));
 	return ret;
 }