[Intel-gfx] [PATCH 10/22] drm/i915: Separate GEM context construction and registration to userspace

[Chris Wilson]

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