| Message ID | 20191110185806.17413-2-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/25] drm/i915: Protect context while grabbing its name for the request | expand |
Chris Wilson <chris@chris-wilson.co.uk> writes: > Use a small char buffer inside the i915_gem_context to store the user > friendly name so that ctx->name has the same lifetime as the RCU > protected GEM context. That is, e.g. when using print_request() that > prints the timeline name (ctx->name), the name will not be prematurely > freed upon the context being closed and the last reference dropped. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Cc: Matthew Auld <matthew.auld@intel.com> > --- > drivers/gpu/drm/i915/gem/i915_gem_context.c | 12 ++---------- > .../gpu/drm/i915/gem/i915_gem_context_types.h | 18 +++++++++--------- > .../gpu/drm/i915/gem/selftests/mock_context.c | 4 +--- > 3 files changed, 12 insertions(+), 22 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c > index 94b51b13ef17..982770e8163d 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c > @@ -277,9 +277,7 @@ static void i915_gem_context_free(struct i915_gem_context *ctx) > if (ctx->timeline) > intel_timeline_put(ctx->timeline); > > - kfree(ctx->name); > put_pid(ctx->pid); > - > mutex_destroy(&ctx->mutex); > > kfree_rcu(ctx, rcu); > @@ -789,12 +787,8 @@ static int gem_context_register(struct i915_gem_context *ctx, > mutex_unlock(&ctx->mutex); > > ctx->pid = get_task_pid(current, PIDTYPE_PID); > - ctx->name = kasprintf(GFP_KERNEL, "%s[%d]", > - current->comm, pid_nr(ctx->pid)); > - if (!ctx->name) { > - ret = -ENOMEM; > - goto err_pid; > - } > + snprintf(ctx->name, sizeof(ctx->name), "%s[%d]", > + current->comm, pid_nr(ctx->pid)); > > /* And finally expose ourselves to userspace via the idr */ > mutex_lock(&fpriv->context_idr_lock); > @@ -803,8 +797,6 @@ static int gem_context_register(struct i915_gem_context *ctx, > if (ret >= 0) > goto out; > > - kfree(fetch_and_zero(&ctx->name)); > -err_pid: > put_pid(fetch_and_zero(&ctx->pid)); > out: > return ret; > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context_types.h b/drivers/gpu/drm/i915/gem/i915_gem_context_types.h > index 861d7d92fe9f..69df5459c350 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_context_types.h > +++ b/drivers/gpu/drm/i915/gem/i915_gem_context_types.h > @@ -100,15 +100,6 @@ struct i915_gem_context { > */ > struct pid *pid; > > - /** > - * @name: arbitrary name > - * > - * A name is constructed for the context from the creator's process > - * name, pid and user handle in order to uniquely identify the > - * context in messages. > - */ > - const char *name; > - > /** link: place with &drm_i915_private.context_list */ > struct list_head link; > struct llist_node free_link; > @@ -176,6 +167,15 @@ struct i915_gem_context { > * per vm, which may be one per context or shared with the global GTT) > */ > struct radix_tree_root handles_vma; > + > + /** > + * @name: arbitrary name, used for user debug > + * > + * A name is constructed for the context from the creator's process > + * name, pid and user handle in order to uniquely identify the > + * context in messages. > + */ > + char name[TASK_COMM_LEN + 8]; I consider 24 bytes as quite cheap for robustness and simplicity. In a playground where we throw pages around like sand. Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com> > }; > > #endif /* __I915_GEM_CONTEXT_TYPES_H__ */ > diff --git a/drivers/gpu/drm/i915/gem/selftests/mock_context.c b/drivers/gpu/drm/i915/gem/selftests/mock_context.c > index cdcb006321a7..53e89efb09c0 100644 > --- a/drivers/gpu/drm/i915/gem/selftests/mock_context.c > +++ b/drivers/gpu/drm/i915/gem/selftests/mock_context.c > @@ -37,9 +37,7 @@ mock_context(struct drm_i915_private *i915, > if (name) { > struct i915_ppgtt *ppgtt; > > - ctx->name = kstrdup(name, GFP_KERNEL); > - if (!ctx->name) > - goto err_put; > + strncpy(ctx->name, name, sizeof(ctx->name)); > > ppgtt = mock_ppgtt(i915, name); > if (!ppgtt) > -- > 2.24.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c index 94b51b13ef17..982770e8163d 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c @@ -277,9 +277,7 @@ static void i915_gem_context_free(struct i915_gem_context *ctx) if (ctx->timeline) intel_timeline_put(ctx->timeline); - kfree(ctx->name); put_pid(ctx->pid); - mutex_destroy(&ctx->mutex); kfree_rcu(ctx, rcu); @@ -789,12 +787,8 @@ static int gem_context_register(struct i915_gem_context *ctx, mutex_unlock(&ctx->mutex); ctx->pid = get_task_pid(current, PIDTYPE_PID); - ctx->name = kasprintf(GFP_KERNEL, "%s[%d]", - current->comm, pid_nr(ctx->pid)); - if (!ctx->name) { - ret = -ENOMEM; - goto err_pid; - } + snprintf(ctx->name, sizeof(ctx->name), "%s[%d]", + current->comm, pid_nr(ctx->pid)); /* And finally expose ourselves to userspace via the idr */ mutex_lock(&fpriv->context_idr_lock); @@ -803,8 +797,6 @@ static int gem_context_register(struct i915_gem_context *ctx, if (ret >= 0) goto out; - kfree(fetch_and_zero(&ctx->name)); -err_pid: put_pid(fetch_and_zero(&ctx->pid)); out: return ret; diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context_types.h b/drivers/gpu/drm/i915/gem/i915_gem_context_types.h index 861d7d92fe9f..69df5459c350 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_context_types.h +++ b/drivers/gpu/drm/i915/gem/i915_gem_context_types.h @@ -100,15 +100,6 @@ struct i915_gem_context { */ struct pid *pid; - /** - * @name: arbitrary name - * - * A name is constructed for the context from the creator's process - * name, pid and user handle in order to uniquely identify the - * context in messages. - */ - const char *name; - /** link: place with &drm_i915_private.context_list */ struct list_head link; struct llist_node free_link; @@ -176,6 +167,15 @@ struct i915_gem_context { * per vm, which may be one per context or shared with the global GTT) */ struct radix_tree_root handles_vma; + + /** + * @name: arbitrary name, used for user debug + * + * A name is constructed for the context from the creator's process + * name, pid and user handle in order to uniquely identify the + * context in messages. + */ + char name[TASK_COMM_LEN + 8]; }; #endif /* __I915_GEM_CONTEXT_TYPES_H__ */ diff --git a/drivers/gpu/drm/i915/gem/selftests/mock_context.c b/drivers/gpu/drm/i915/gem/selftests/mock_context.c index cdcb006321a7..53e89efb09c0 100644 --- a/drivers/gpu/drm/i915/gem/selftests/mock_context.c +++ b/drivers/gpu/drm/i915/gem/selftests/mock_context.c @@ -37,9 +37,7 @@ mock_context(struct drm_i915_private *i915, if (name) { struct i915_ppgtt *ppgtt; - ctx->name = kstrdup(name, GFP_KERNEL); - if (!ctx->name) - goto err_put; + strncpy(ctx->name, name, sizeof(ctx->name)); ppgtt = mock_ppgtt(i915, name); if (!ppgtt)
Use a small char buffer inside the i915_gem_context to store the user friendly name so that ctx->name has the same lifetime as the RCU protected GEM context. That is, e.g. when using print_request() that prints the timeline name (ctx->name), the name will not be prematurely freed upon the context being closed and the last reference dropped. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Cc: Matthew Auld <matthew.auld@intel.com> --- drivers/gpu/drm/i915/gem/i915_gem_context.c | 12 ++---------- .../gpu/drm/i915/gem/i915_gem_context_types.h | 18 +++++++++--------- .../gpu/drm/i915/gem/selftests/mock_context.c | 4 +--- 3 files changed, 12 insertions(+), 22 deletions(-)