| Message ID | 20190318095204.9913-10-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/22] drm/i915: Flush pages on acquisition | expand |
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@chris-wilson.co.uk> > Cc: Tvrtko Ursulin <tvrtko.ursulin@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
Quoting Tvrtko Ursulin (2019-03-18 16:22:12) > > On 18/03/2019 09:51, Chris Wilson wrote: > > +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. The goal is that we need to fix that now. And we can't hold struct_mutex across the extensions, as we want to wrap ctx_setparam which expects to be able to take struct_mutex. So it has to be registered outside of struct_mutex in this or the next path. > > + 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. Nah, longer term needs a better fix since ppgtt is shared. (I'm just ignoring this for now, since they all must have the same fpriv.) > > + > > + /* 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. Ah. Yes. That should solve the problem of that idr_alloc() not being last as it needs to be. -Chris
Quoting Chris Wilson (2019-03-18 16:30:58) > Quoting Tvrtko Ursulin (2019-03-18 16:22:12) > > > > On 18/03/2019 09:51, Chris Wilson wrote: > > > +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. > > The goal is that we need to fix that now. And we can't hold struct_mutex > across the extensions, as we want to wrap ctx_setparam which expects to > be able to take struct_mutex. So it has to be registered outside of > struct_mutex in this or the next path. Waitasec... It is the very next patch that drops the struct_mutex. (I was thinking it was removed in this patch.) -Chris
On 18/03/2019 16:32, Chris Wilson wrote: > Quoting Chris Wilson (2019-03-18 16:30:58) >> Quoting Tvrtko Ursulin (2019-03-18 16:22:12) >>> >>> On 18/03/2019 09:51, Chris Wilson wrote: >>>> +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. >> >> The goal is that we need to fix that now. And we can't hold struct_mutex >> across the extensions, as we want to wrap ctx_setparam which expects to >> be able to take struct_mutex. So it has to be registered outside of >> struct_mutex in this or the next path. > > Waitasec... It is the very next patch that drops the struct_mutex. (I > was thinking it was removed in this patch.) So in the next patch you update the assert to ctx_idr_lock. Too evil? :)) Regards, Tvrtko
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; + + ctx->pid = get_task_pid(current, PIDTYPE_PID); + + if (ctx->ppgtt) + ctx->ppgtt->vm.file = fpriv; + + /* 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; + 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); 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) { + 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); } struct i915_gem_context *
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@chris-wilson.co.uk> Cc: Tvrtko Ursulin <tvrtko.ursulin@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(-)