[Intel-gfx] [PATCH 38/46] drm/i915: Allow a context to define its set of engines

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-02-25 10:41:28)
> 
> On 06/02/2019 13:03, Chris Wilson wrote:
> > +static int
> > +get_engines(struct i915_gem_context *ctx,
> > +         struct drm_i915_gem_context_param *args)
> > +{
> > +     struct i915_context_param_engines *local;
> > +     unsigned int n, count, size;
> > +     int err;
> > +
> > +restart:
> > +     count = READ_ONCE(ctx->nengine);
> > +     if (count > (INT_MAX - sizeof(*local)) / sizeof(*local->class_instance))
> > +             return -ENOMEM; /* unrepresentable! */
> > +
> > +     size = sizeof(*local) + count * sizeof(*local->class_instance);
> > +     if (!args->size) {
> > +             args->size = size;
> > +             return 0;
> > +     }
> > +     if (args->size < size)
> > +             return -EINVAL;
> > +
> > +     local = kmalloc(size, GFP_KERNEL);
> > +     if (!local)
> > +             return -ENOMEM;
> > +
> > +     if (mutex_lock_interruptible(&ctx->i915->drm.struct_mutex)) {
> > +             err = -EINTR;
> > +             goto err;
> > +     }
> > +
> > +     if (READ_ONCE(ctx->nengine) != count) {
> > +             mutex_unlock(&ctx->i915->drm.struct_mutex);
> > +             kfree(local);
> > +             goto restart;
> > +     }
> > +
> > +     local->extensions = 0;
> > +     for (n = 0; n < count; n++) {
> > +             if (ctx->engines[n]) {
> > +                     local->class_instance[n].engine_class =
> > +                             ctx->engines[n]->uabi_class;
> > +                     local->class_instance[n].engine_instance =
> > +                             ctx->engines[n]->instance;
> > +             } else {
> > +                     local->class_instance[n].engine_class =
> > +                             I915_ENGINE_CLASS_INVALID;
> > +                     local->class_instance[n].engine_instance =
> > +                             I915_ENGINE_CLASS_INVALID_NONE;
> > +             }
> > +     }
> > +
> > +     mutex_unlock(&ctx->i915->drm.struct_mutex);
> > +
> > +     if (copy_to_user(u64_to_user_ptr(args->value), local, size)) {
> > +             err = -EFAULT;
> > +             goto err;
> > +     }
> > +
> 
> Sripada, Radhakrishna <radhakrishna.sripada at intel.com> reports leakage 
> of local on the success path here.
> 
> Alternatively, perhaps you could simplify by using stack space, with 
> some reasonable max size for future proofing, and a GEM_WARN_ON error 
> return or something.

I don't expect this getter to be called frequently, so resisting the urge
to optimise. And similarly not impose restrictions we may need to lift
later on. For once, kiss.
-Chris