[Intel-gfx] [PATCH 10/22] drm/i915: Separate GEM context construction and registration to userspace
Tvrtko Ursulin
tvrtko.ursulin at linux.intel.com
Mon Mar 18 16:22:12 UTC 2019
On 18/03/2019 09:51, Chris Wilson wrote:
> In later patches, it became apparent that userspace can see a partially
> constructed GEM context and begin using it before it was ready, to much
> hilarity. Close this window of opportunity by lifting the registration of
> the context with userspace (the insertion of the context into the filp's
> idr) to the very end of the CONTEXT_CREATE ioctl.
>
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> ---
> drivers/gpu/drm/i915/i915_gem_context.c | 143 +++++++++++-------
> drivers/gpu/drm/i915/i915_gem_gtt.c | 7 +-
> drivers/gpu/drm/i915/i915_gem_gtt.h | 8 +-
> drivers/gpu/drm/i915/selftests/huge_pages.c | 2 +-
> .../gpu/drm/i915/selftests/i915_gem_context.c | 12 +-
> drivers/gpu/drm/i915/selftests/i915_gem_gtt.c | 2 +-
> drivers/gpu/drm/i915/selftests/mock_context.c | 17 ++-
> 7 files changed, 116 insertions(+), 75 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index d776d43707e0..5df3d423ec6c 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -337,15 +337,13 @@ static u32 default_desc_template(const struct drm_i915_private *i915,
> }
>
> static struct i915_gem_context *
> -__create_hw_context(struct drm_i915_private *dev_priv,
> - struct drm_i915_file_private *file_priv)
> +__create_context(struct drm_i915_private *dev_priv)
> {
> struct i915_gem_context *ctx;
> - int ret;
> int i;
>
> ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
> - if (ctx == NULL)
> + if (!ctx)
> return ERR_PTR(-ENOMEM);
>
> kref_init(&ctx->ref);
> @@ -362,29 +360,6 @@ __create_hw_context(struct drm_i915_private *dev_priv,
> INIT_LIST_HEAD(&ctx->handles_list);
> INIT_LIST_HEAD(&ctx->hw_id_link);
>
> - /* Default context will never have a file_priv */
> - ret = DEFAULT_CONTEXT_HANDLE;
> - if (file_priv) {
> - ret = idr_alloc(&file_priv->context_idr, ctx,
> - DEFAULT_CONTEXT_HANDLE, 0, GFP_KERNEL);
> - if (ret < 0)
> - goto err_lut;
> - }
> - ctx->user_handle = ret;
> -
> - ctx->file_priv = file_priv;
> - if (file_priv) {
> - ctx->pid = get_task_pid(current, PIDTYPE_PID);
> - ctx->name = kasprintf(GFP_KERNEL, "%s[%d]/%x",
> - current->comm,
> - pid_nr(ctx->pid),
> - ctx->user_handle);
> - if (!ctx->name) {
> - ret = -ENOMEM;
> - goto err_pid;
> - }
> - }
> -
> /* NB: Mark all slices as needing a remap so that when the context first
> * loads it will restore whatever remap state already exists. If there
> * is no remap info, it will be a NOP. */
> @@ -401,25 +376,10 @@ __create_hw_context(struct drm_i915_private *dev_priv,
> ctx->hang_timestamp[i] = jiffies - CONTEXT_FAST_HANG_JIFFIES;
>
> return ctx;
> -
> -err_pid:
> - put_pid(ctx->pid);
> - idr_remove(&file_priv->context_idr, ctx->user_handle);
> -err_lut:
> - context_close(ctx);
> - return ERR_PTR(ret);
> -}
> -
> -static void __destroy_hw_context(struct i915_gem_context *ctx,
> - struct drm_i915_file_private *file_priv)
> -{
> - idr_remove(&file_priv->context_idr, ctx->user_handle);
> - context_close(ctx);
> }
>
> static struct i915_gem_context *
> -i915_gem_create_context(struct drm_i915_private *dev_priv,
> - struct drm_i915_file_private *file_priv)
> +i915_gem_create_context(struct drm_i915_private *dev_priv)
> {
> struct i915_gem_context *ctx;
>
> @@ -428,18 +388,18 @@ i915_gem_create_context(struct drm_i915_private *dev_priv,
> /* Reap the most stale context */
> contexts_free_first(dev_priv);
>
> - ctx = __create_hw_context(dev_priv, file_priv);
> + ctx = __create_context(dev_priv);
> if (IS_ERR(ctx))
> return ctx;
>
> if (HAS_FULL_PPGTT(dev_priv)) {
> struct i915_hw_ppgtt *ppgtt;
>
> - ppgtt = i915_ppgtt_create(dev_priv, file_priv);
> + ppgtt = i915_ppgtt_create(dev_priv);
> if (IS_ERR(ppgtt)) {
> DRM_DEBUG_DRIVER("PPGTT setup failed (%ld)\n",
> PTR_ERR(ppgtt));
> - __destroy_hw_context(ctx, file_priv);
> + context_close(ctx);
> return ERR_CAST(ppgtt);
> }
>
> @@ -475,7 +435,7 @@ i915_gem_context_create_gvt(struct drm_device *dev)
> if (ret)
> return ERR_PTR(ret);
>
> - ctx = i915_gem_create_context(to_i915(dev), NULL);
> + ctx = i915_gem_create_context(to_i915(dev));
> if (IS_ERR(ctx))
> goto out;
>
> @@ -511,7 +471,7 @@ i915_gem_context_create_kernel(struct drm_i915_private *i915, int prio)
> struct i915_gem_context *ctx;
> int err;
>
> - ctx = i915_gem_create_context(i915, NULL);
> + ctx = i915_gem_create_context(i915);
> if (IS_ERR(ctx))
> return ctx;
>
> @@ -625,25 +585,79 @@ static int context_idr_cleanup(int id, void *p, void *data)
> return 0;
> }
>
> +static int gem_context_register(struct i915_gem_context *ctx,
> + struct drm_i915_file_private *fpriv)
> +{
> + int ret;
> +
Assert struct mutex for now? Without it userspace can still see not
fully initialized ctx. It is kind of two arguments but good for
documentation nevertheless I think.
> + ctx->pid = get_task_pid(current, PIDTYPE_PID);
> +
> + if (ctx->ppgtt)
> + ctx->ppgtt->vm.file = fpriv;
Thinking about i915_vm_set_file(vm, fpriv), not sure.
> +
> + /* And (nearly) finally expose ourselves to userspace via the idr */
> + ret = idr_alloc(&fpriv->context_idr, ctx,
> + DEFAULT_CONTEXT_HANDLE, 0, GFP_KERNEL);
> + if (ret < 0)
> + goto err_pid;
> +
> + ctx->file_priv = fpriv;
> + ctx->user_handle = ret;
> +
> + ctx->name = kasprintf(GFP_KERNEL, "%s[%d]/%x",
> + current->comm,
> + pid_nr(ctx->pid),
> + ctx->user_handle);
> + if (!ctx->name) {
> + ret = -ENOMEM;
> + goto err_idr;
> + }
> +
> + return 0;
> +
> +err_idr:
> + idr_remove(&fpriv->context_idr, ctx->user_handle);
> + ctx->file_priv = NULL;
> +err_pid:
> + put_pid(ctx->pid);
> + ctx->pid = NULL;
To avoid this, and in the spirit of the patch, I think it would be
better to just store everything in locals and assign to ctx members once
passed the point of failure.
> + return ret;
> +}
> +
> int i915_gem_context_open(struct drm_i915_private *i915,
> struct drm_file *file)
> {
> struct drm_i915_file_private *file_priv = file->driver_priv;
> struct i915_gem_context *ctx;
> + int err;
>
> idr_init(&file_priv->context_idr);
>
> mutex_lock(&i915->drm.struct_mutex);
> - ctx = i915_gem_create_context(i915, file_priv);
> - mutex_unlock(&i915->drm.struct_mutex);
> +
> + ctx = i915_gem_create_context(i915);
> if (IS_ERR(ctx)) {
> - idr_destroy(&file_priv->context_idr);
> - return PTR_ERR(ctx);
> + err = PTR_ERR(ctx);
> + goto err;
> }
>
> + err = gem_context_register(ctx, file_priv);
> + if (err)
> + goto err_ctx;
> +
> + GEM_BUG_ON(ctx->user_handle != DEFAULT_CONTEXT_HANDLE);
> GEM_BUG_ON(i915_gem_context_is_kernel(ctx));
>
> + mutex_unlock(&i915->drm.struct_mutex);
> +
> return 0;
> +
> +err_ctx:
> + context_close(ctx);
> +err:
> + mutex_unlock(&i915->drm.struct_mutex);
> + idr_destroy(&file_priv->context_idr);
> + return PTR_ERR(ctx);
> }
>
> void i915_gem_context_close(struct drm_file *file)
> @@ -835,17 +849,28 @@ int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
> if (ret)
> return ret;
>
> - ctx = i915_gem_create_context(i915, file_priv);
> - mutex_unlock(&dev->struct_mutex);
> - if (IS_ERR(ctx))
> - return PTR_ERR(ctx);
> + ctx = i915_gem_create_context(i915);
> + if (IS_ERR(ctx)) {
> + ret = PTR_ERR(ctx);
> + goto err_unlock;
> + }
>
> - GEM_BUG_ON(i915_gem_context_is_kernel(ctx));
> + ret = gem_context_register(ctx, file_priv);
> + if (ret)
> + goto err_ctx;
> +
> + mutex_unlock(&dev->struct_mutex);
>
> args->ctx_id = ctx->user_handle;
> DRM_DEBUG("HW context %d created\n", args->ctx_id);
>
> return 0;
> +
> +err_ctx:
> + context_close(ctx);
> +err_unlock:
> + mutex_unlock(&dev->struct_mutex);
> + return ret;
> }
>
> int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data,
> @@ -870,7 +895,9 @@ int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data,
> if (ret)
> goto out;
>
> - __destroy_hw_context(ctx, file_priv);
> + idr_remove(&file_priv->context_idr, ctx->user_handle);
> + context_close(ctx);
> +
> mutex_unlock(&dev->struct_mutex);
>
> out:
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index b8055c8d4e71..b9e0e3a00223 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -2069,8 +2069,7 @@ __hw_ppgtt_create(struct drm_i915_private *i915)
> }
>
> struct i915_hw_ppgtt *
> -i915_ppgtt_create(struct drm_i915_private *i915,
> - struct drm_i915_file_private *fpriv)
> +i915_ppgtt_create(struct drm_i915_private *i915)
> {
> struct i915_hw_ppgtt *ppgtt;
>
> @@ -2078,8 +2077,6 @@ i915_ppgtt_create(struct drm_i915_private *i915,
> if (IS_ERR(ppgtt))
> return ppgtt;
>
> - ppgtt->vm.file = fpriv;
> -
> trace_i915_ppgtt_create(&ppgtt->vm);
>
> return ppgtt;
> @@ -2657,7 +2654,7 @@ int i915_gem_init_aliasing_ppgtt(struct drm_i915_private *i915)
> struct i915_hw_ppgtt *ppgtt;
> int err;
>
> - ppgtt = i915_ppgtt_create(i915, ERR_PTR(-EPERM));
> + ppgtt = i915_ppgtt_create(i915);
vm.file changes to NULL now, but after some grepping I did not find that
it matters.
> if (IS_ERR(ppgtt))
> return PTR_ERR(ppgtt);
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
> index 35f21a2ae36c..b76ab4c2a0e6 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.h
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
> @@ -603,15 +603,17 @@ int i915_gem_init_ggtt(struct drm_i915_private *dev_priv);
> void i915_ggtt_cleanup_hw(struct drm_i915_private *dev_priv);
>
> int i915_ppgtt_init_hw(struct drm_i915_private *dev_priv);
> -void i915_ppgtt_release(struct kref *kref);
> -struct i915_hw_ppgtt *i915_ppgtt_create(struct drm_i915_private *dev_priv,
> - struct drm_i915_file_private *fpriv);
> +
> +struct i915_hw_ppgtt *i915_ppgtt_create(struct drm_i915_private *dev_priv);
> void i915_ppgtt_close(struct i915_address_space *vm);
> +void i915_ppgtt_release(struct kref *kref);
> +
> static inline void i915_ppgtt_get(struct i915_hw_ppgtt *ppgtt)
> {
> if (ppgtt)
> kref_get(&ppgtt->ref);
> }
> +
> static inline void i915_ppgtt_put(struct i915_hw_ppgtt *ppgtt)
> {
> if (ppgtt)
> diff --git a/drivers/gpu/drm/i915/selftests/huge_pages.c b/drivers/gpu/drm/i915/selftests/huge_pages.c
> index 218cfc361de3..c5c8ba6c059f 100644
> --- a/drivers/gpu/drm/i915/selftests/huge_pages.c
> +++ b/drivers/gpu/drm/i915/selftests/huge_pages.c
> @@ -1710,7 +1710,7 @@ int i915_gem_huge_page_mock_selftests(void)
> mkwrite_device_info(dev_priv)->ppgtt_size = 48;
>
> mutex_lock(&dev_priv->drm.struct_mutex);
> - ppgtt = i915_ppgtt_create(dev_priv, ERR_PTR(-ENODEV));
> + ppgtt = i915_ppgtt_create(dev_priv);
> if (IS_ERR(ppgtt)) {
> err = PTR_ERR(ppgtt);
> goto out_unlock;
> diff --git a/drivers/gpu/drm/i915/selftests/i915_gem_context.c b/drivers/gpu/drm/i915/selftests/i915_gem_context.c
> index f18c78ebff07..4dc96e28d89f 100644
> --- a/drivers/gpu/drm/i915/selftests/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/selftests/i915_gem_context.c
> @@ -76,7 +76,7 @@ static int live_nop_switch(void *arg)
> }
>
> for (n = 0; n < nctx; n++) {
> - ctx[n] = i915_gem_create_context(i915, file->driver_priv);
> + ctx[n] = live_context(i915, file);
> if (IS_ERR(ctx[n])) {
> err = PTR_ERR(ctx[n]);
> goto out_unlock;
> @@ -514,7 +514,7 @@ static int igt_ctx_exec(void *arg)
> struct i915_gem_context *ctx;
> unsigned int id;
>
> - ctx = i915_gem_create_context(i915, file->driver_priv);
> + ctx = live_context(i915, file);
> if (IS_ERR(ctx)) {
> err = PTR_ERR(ctx);
> goto out_unlock;
> @@ -960,7 +960,7 @@ __igt_ctx_sseu(struct drm_i915_private *i915,
>
> mutex_lock(&i915->drm.struct_mutex);
>
> - ctx = i915_gem_create_context(i915, file->driver_priv);
> + ctx = live_context(i915, file);
> if (IS_ERR(ctx)) {
> ret = PTR_ERR(ctx);
> goto out_unlock;
> @@ -1070,7 +1070,7 @@ static int igt_ctx_readonly(void *arg)
> if (err)
> goto out_unlock;
>
> - ctx = i915_gem_create_context(i915, file->driver_priv);
> + ctx = live_context(i915, file);
> if (IS_ERR(ctx)) {
> err = PTR_ERR(ctx);
> goto out_unlock;
> @@ -1390,13 +1390,13 @@ static int igt_vm_isolation(void *arg)
> if (err)
> goto out_unlock;
>
> - ctx_a = i915_gem_create_context(i915, file->driver_priv);
> + ctx_a = live_context(i915, file);
> if (IS_ERR(ctx_a)) {
> err = PTR_ERR(ctx_a);
> goto out_unlock;
> }
>
> - ctx_b = i915_gem_create_context(i915, file->driver_priv);
> + ctx_b = live_context(i915, file);
> if (IS_ERR(ctx_b)) {
> err = PTR_ERR(ctx_b);
> goto out_unlock;
> diff --git a/drivers/gpu/drm/i915/selftests/i915_gem_gtt.c b/drivers/gpu/drm/i915/selftests/i915_gem_gtt.c
> index 826fd51c331e..01084f6b4fb7 100644
> --- a/drivers/gpu/drm/i915/selftests/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/selftests/i915_gem_gtt.c
> @@ -1010,7 +1010,7 @@ static int exercise_ppgtt(struct drm_i915_private *dev_priv,
> return PTR_ERR(file);
>
> mutex_lock(&dev_priv->drm.struct_mutex);
> - ppgtt = i915_ppgtt_create(dev_priv, file->driver_priv);
> + ppgtt = i915_ppgtt_create(dev_priv);
> if (IS_ERR(ppgtt)) {
> err = PTR_ERR(ppgtt);
> goto out_unlock;
> diff --git a/drivers/gpu/drm/i915/selftests/mock_context.c b/drivers/gpu/drm/i915/selftests/mock_context.c
> index 8efa6892c6cd..1cc8be732435 100644
> --- a/drivers/gpu/drm/i915/selftests/mock_context.c
> +++ b/drivers/gpu/drm/i915/selftests/mock_context.c
> @@ -88,9 +88,24 @@ void mock_init_contexts(struct drm_i915_private *i915)
> struct i915_gem_context *
> live_context(struct drm_i915_private *i915, struct drm_file *file)
Live context, mock context identity crisis. :) Nothing to act upon, just
amusing myself..
> {
> + struct i915_gem_context *ctx;
> + int err;
> +
> lockdep_assert_held(&i915->drm.struct_mutex);
>
> - return i915_gem_create_context(i915, file->driver_priv);
> + ctx = i915_gem_create_context(i915);
> + if (IS_ERR(ctx))
> + return ctx;
> +
> + err = gem_context_register(ctx, file->driver_priv);
> + if (err)
> + goto err_ctx;
> +
> + return ctx;
> +
> +err_ctx:
> + i915_gem_context_put(ctx);
> + return ERR_PTR(err);
Never too early for onion unwind, yes? :)
> }
>
> struct i915_gem_context *
>
Looks good. Cleanup of gem_context_register to use local would make me
completely happy.
Regards,
Tvrtko
More information about the Intel-gfx
mailing list