| Message ID | 20210527162650.1182544-27-jason@jlekstrand.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/gem: ioctl clean-ups (v6) | expand |
On Thu, May 27, 2021 at 11:26:47AM -0500, Jason Ekstrand wrote: > When the APIs were added to manage the engine set on a GEM context > directly from userspace, the questionable choice was made to allow > changing the engine set on a context at any time. This is horribly racy > and there's absolutely no reason why any userspace would want to do this > outside of trying to exercise interesting race conditions. By removing > support for CONTEXT_PARAM_ENGINES from ctx_setparam, we make it > impossible to change the engine set after the context has been fully > created. > > This doesn't yet let us delete all the deferred engine clean-up code as > that's still used for handling the case where the client dies or calls > GEM_CONTEXT_DESTROY while work is in flight. However, moving to an API > where the engine set is effectively immutable gives us more options to > potentially clean that code up a bit going forward. It also removes a > whole class of ways in which a client can hurt itself or try to get > around kernel context banning. On this I do kinda wonder why we don't refcount drm_file and then gc all these tricks. Iirc drm_file disappearing at a bad time is a large reasons for all these tricks we need for context/request/engine cleanup. But yeah definitely needs a pile more analysis. > > v2 (Jason Ekstrand): > - Expand the commit mesage I already gave you an r-b assuming you type some commit message ... Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > Signed-off-by: Jason Ekstrand <jason@jlekstrand.net> > --- > drivers/gpu/drm/i915/gem/i915_gem_context.c | 303 -------------------- > 1 file changed, 303 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c > index a528c8f3354a0..e6a6ead477ff4 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c > @@ -1819,305 +1819,6 @@ static int set_sseu(struct i915_gem_context *ctx, > return ret; > } > > -struct set_engines { > - struct i915_gem_context *ctx; > - struct i915_gem_engines *engines; > -}; > - > -static int > -set_engines__load_balance(struct i915_user_extension __user *base, void *data) > -{ > - struct i915_context_engines_load_balance __user *ext = > - container_of_user(base, typeof(*ext), base); > - const struct set_engines *set = data; > - struct drm_i915_private *i915 = set->ctx->i915; > - struct intel_engine_cs *stack[16]; > - struct intel_engine_cs **siblings; > - struct intel_context *ce; > - struct intel_sseu null_sseu = {}; > - u16 num_siblings, idx; > - unsigned int n; > - int err; > - > - if (!HAS_EXECLISTS(i915)) > - return -ENODEV; > - > - if (intel_uc_uses_guc_submission(&i915->gt.uc)) > - return -ENODEV; /* not implement yet */ > - > - if (get_user(idx, &ext->engine_index)) > - return -EFAULT; > - > - if (idx >= set->engines->num_engines) { > - drm_dbg(&i915->drm, "Invalid placement value, %d >= %d\n", > - idx, set->engines->num_engines); > - return -EINVAL; > - } > - > - idx = array_index_nospec(idx, set->engines->num_engines); > - if (set->engines->engines[idx]) { > - drm_dbg(&i915->drm, > - "Invalid placement[%d], already occupied\n", idx); > - return -EEXIST; > - } > - > - if (get_user(num_siblings, &ext->num_siblings)) > - return -EFAULT; > - > - err = check_user_mbz(&ext->flags); > - if (err) > - return err; > - > - err = check_user_mbz(&ext->mbz64); > - if (err) > - return err; > - > - siblings = stack; > - if (num_siblings > ARRAY_SIZE(stack)) { > - siblings = kmalloc_array(num_siblings, > - sizeof(*siblings), > - GFP_KERNEL); > - if (!siblings) > - return -ENOMEM; > - } > - > - for (n = 0; n < num_siblings; n++) { > - struct i915_engine_class_instance ci; > - > - if (copy_from_user(&ci, &ext->engines[n], sizeof(ci))) { > - err = -EFAULT; > - goto out_siblings; > - } > - > - siblings[n] = intel_engine_lookup_user(i915, > - ci.engine_class, > - ci.engine_instance); > - if (!siblings[n]) { > - drm_dbg(&i915->drm, > - "Invalid sibling[%d]: { class:%d, inst:%d }\n", > - n, ci.engine_class, ci.engine_instance); > - err = -EINVAL; > - goto out_siblings; > - } > - } > - > - ce = intel_execlists_create_virtual(siblings, n); > - if (IS_ERR(ce)) { > - err = PTR_ERR(ce); > - goto out_siblings; > - } > - > - intel_context_set_gem(ce, set->ctx, null_sseu); > - > - if (cmpxchg(&set->engines->engines[idx], NULL, ce)) { > - intel_context_put(ce); > - err = -EEXIST; > - goto out_siblings; > - } > - > -out_siblings: > - if (siblings != stack) > - kfree(siblings); > - > - return err; > -} > - > -static int > -set_engines__bond(struct i915_user_extension __user *base, void *data) > -{ > - struct i915_context_engines_bond __user *ext = > - container_of_user(base, typeof(*ext), base); > - const struct set_engines *set = data; > - struct drm_i915_private *i915 = set->ctx->i915; > - struct i915_engine_class_instance ci; > - struct intel_engine_cs *virtual; > - struct intel_engine_cs *master; > - u16 idx, num_bonds; > - int err, n; > - > - if (get_user(idx, &ext->virtual_index)) > - return -EFAULT; > - > - if (idx >= set->engines->num_engines) { > - drm_dbg(&i915->drm, > - "Invalid index for virtual engine: %d >= %d\n", > - idx, set->engines->num_engines); > - return -EINVAL; > - } > - > - idx = array_index_nospec(idx, set->engines->num_engines); > - if (!set->engines->engines[idx]) { > - drm_dbg(&i915->drm, "Invalid engine at %d\n", idx); > - return -EINVAL; > - } > - virtual = set->engines->engines[idx]->engine; > - > - if (intel_engine_is_virtual(virtual)) { > - drm_dbg(&i915->drm, > - "Bonding with virtual engines not allowed\n"); > - return -EINVAL; > - } > - > - err = check_user_mbz(&ext->flags); > - if (err) > - return err; > - > - for (n = 0; n < ARRAY_SIZE(ext->mbz64); n++) { > - err = check_user_mbz(&ext->mbz64[n]); > - if (err) > - return err; > - } > - > - if (copy_from_user(&ci, &ext->master, sizeof(ci))) > - return -EFAULT; > - > - master = intel_engine_lookup_user(i915, > - ci.engine_class, ci.engine_instance); > - if (!master) { > - drm_dbg(&i915->drm, > - "Unrecognised master engine: { class:%u, instance:%u }\n", > - ci.engine_class, ci.engine_instance); > - return -EINVAL; > - } > - > - if (get_user(num_bonds, &ext->num_bonds)) > - return -EFAULT; > - > - for (n = 0; n < num_bonds; n++) { > - struct intel_engine_cs *bond; > - > - if (copy_from_user(&ci, &ext->engines[n], sizeof(ci))) > - return -EFAULT; > - > - bond = intel_engine_lookup_user(i915, > - ci.engine_class, > - ci.engine_instance); > - if (!bond) { > - drm_dbg(&i915->drm, > - "Unrecognised engine[%d] for bonding: { class:%d, instance: %d }\n", > - n, ci.engine_class, ci.engine_instance); > - return -EINVAL; > - } > - } > - > - return 0; > -} > - > -static const i915_user_extension_fn set_engines__extensions[] = { > - [I915_CONTEXT_ENGINES_EXT_LOAD_BALANCE] = set_engines__load_balance, > - [I915_CONTEXT_ENGINES_EXT_BOND] = set_engines__bond, > -}; > - > -static int > -set_engines(struct i915_gem_context *ctx, > - const struct drm_i915_gem_context_param *args) > -{ > - struct drm_i915_private *i915 = ctx->i915; > - struct i915_context_param_engines __user *user = > - u64_to_user_ptr(args->value); > - struct intel_sseu null_sseu = {}; > - struct set_engines set = { .ctx = ctx }; > - unsigned int num_engines, n; > - u64 extensions; > - int err; > - > - if (!args->size) { /* switch back to legacy user_ring_map */ > - if (!i915_gem_context_user_engines(ctx)) > - return 0; > - > - set.engines = default_engines(ctx, null_sseu); > - if (IS_ERR(set.engines)) > - return PTR_ERR(set.engines); > - > - goto replace; > - } > - > - if (args->size < sizeof(*user) || > - !IS_ALIGNED(args->size - sizeof(*user), sizeof(*user->engines))) { > - drm_dbg(&i915->drm, "Invalid size for engine array: %d\n", > - args->size); > - return -EINVAL; > - } > - > - num_engines = (args->size - sizeof(*user)) / sizeof(*user->engines); > - /* RING_MASK has no shift so we can use it directly here */ > - if (num_engines > I915_EXEC_RING_MASK + 1) > - return -EINVAL; > - > - set.engines = alloc_engines(num_engines); > - if (!set.engines) > - return -ENOMEM; > - > - for (n = 0; n < num_engines; n++) { > - struct i915_engine_class_instance ci; > - struct intel_engine_cs *engine; > - struct intel_context *ce; > - > - if (copy_from_user(&ci, &user->engines[n], sizeof(ci))) { > - __free_engines(set.engines, n); > - return -EFAULT; > - } > - > - if (ci.engine_class == (u16)I915_ENGINE_CLASS_INVALID && > - ci.engine_instance == (u16)I915_ENGINE_CLASS_INVALID_NONE) { > - set.engines->engines[n] = NULL; > - continue; > - } > - > - engine = intel_engine_lookup_user(ctx->i915, > - ci.engine_class, > - ci.engine_instance); > - if (!engine) { > - drm_dbg(&i915->drm, > - "Invalid engine[%d]: { class:%d, instance:%d }\n", > - n, ci.engine_class, ci.engine_instance); > - __free_engines(set.engines, n); > - return -ENOENT; > - } > - > - ce = intel_context_create(engine); > - if (IS_ERR(ce)) { > - __free_engines(set.engines, n); > - return PTR_ERR(ce); > - } > - > - intel_context_set_gem(ce, ctx, null_sseu); > - > - set.engines->engines[n] = ce; > - } > - set.engines->num_engines = num_engines; > - > - err = -EFAULT; > - if (!get_user(extensions, &user->extensions)) > - err = i915_user_extensions(u64_to_user_ptr(extensions), > - set_engines__extensions, > - ARRAY_SIZE(set_engines__extensions), > - &set); > - if (err) { > - free_engines(set.engines); > - return err; > - } > - > -replace: > - mutex_lock(&ctx->engines_mutex); > - if (i915_gem_context_is_closed(ctx)) { > - mutex_unlock(&ctx->engines_mutex); > - free_engines(set.engines); > - return -ENOENT; > - } > - if (args->size) > - i915_gem_context_set_user_engines(ctx); > - else > - i915_gem_context_clear_user_engines(ctx); > - set.engines = rcu_replace_pointer(ctx->engines, set.engines, 1); > - mutex_unlock(&ctx->engines_mutex); > - > - /* Keep track of old engine sets for kill_context() */ > - engines_idle_release(ctx, set.engines); > - > - return 0; > -} > - > static int > set_persistence(struct i915_gem_context *ctx, > const struct drm_i915_gem_context_param *args) > @@ -2200,10 +1901,6 @@ static int ctx_setparam(struct drm_i915_file_private *fpriv, > ret = set_sseu(ctx, args); > break; > > - case I915_CONTEXT_PARAM_ENGINES: > - ret = set_engines(ctx, args); > - break; > - > case I915_CONTEXT_PARAM_PERSISTENCE: > ret = set_persistence(ctx, args); > break; > -- > 2.31.1 > > _______________________________________________ > 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 a528c8f3354a0..e6a6ead477ff4 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c @@ -1819,305 +1819,6 @@ static int set_sseu(struct i915_gem_context *ctx, return ret; } -struct set_engines { - struct i915_gem_context *ctx; - struct i915_gem_engines *engines; -}; - -static int -set_engines__load_balance(struct i915_user_extension __user *base, void *data) -{ - struct i915_context_engines_load_balance __user *ext = - container_of_user(base, typeof(*ext), base); - const struct set_engines *set = data; - struct drm_i915_private *i915 = set->ctx->i915; - struct intel_engine_cs *stack[16]; - struct intel_engine_cs **siblings; - struct intel_context *ce; - struct intel_sseu null_sseu = {}; - u16 num_siblings, idx; - unsigned int n; - int err; - - if (!HAS_EXECLISTS(i915)) - return -ENODEV; - - if (intel_uc_uses_guc_submission(&i915->gt.uc)) - return -ENODEV; /* not implement yet */ - - if (get_user(idx, &ext->engine_index)) - return -EFAULT; - - if (idx >= set->engines->num_engines) { - drm_dbg(&i915->drm, "Invalid placement value, %d >= %d\n", - idx, set->engines->num_engines); - return -EINVAL; - } - - idx = array_index_nospec(idx, set->engines->num_engines); - if (set->engines->engines[idx]) { - drm_dbg(&i915->drm, - "Invalid placement[%d], already occupied\n", idx); - return -EEXIST; - } - - if (get_user(num_siblings, &ext->num_siblings)) - return -EFAULT; - - err = check_user_mbz(&ext->flags); - if (err) - return err; - - err = check_user_mbz(&ext->mbz64); - if (err) - return err; - - siblings = stack; - if (num_siblings > ARRAY_SIZE(stack)) { - siblings = kmalloc_array(num_siblings, - sizeof(*siblings), - GFP_KERNEL); - if (!siblings) - return -ENOMEM; - } - - for (n = 0; n < num_siblings; n++) { - struct i915_engine_class_instance ci; - - if (copy_from_user(&ci, &ext->engines[n], sizeof(ci))) { - err = -EFAULT; - goto out_siblings; - } - - siblings[n] = intel_engine_lookup_user(i915, - ci.engine_class, - ci.engine_instance); - if (!siblings[n]) { - drm_dbg(&i915->drm, - "Invalid sibling[%d]: { class:%d, inst:%d }\n", - n, ci.engine_class, ci.engine_instance); - err = -EINVAL; - goto out_siblings; - } - } - - ce = intel_execlists_create_virtual(siblings, n); - if (IS_ERR(ce)) { - err = PTR_ERR(ce); - goto out_siblings; - } - - intel_context_set_gem(ce, set->ctx, null_sseu); - - if (cmpxchg(&set->engines->engines[idx], NULL, ce)) { - intel_context_put(ce); - err = -EEXIST; - goto out_siblings; - } - -out_siblings: - if (siblings != stack) - kfree(siblings); - - return err; -} - -static int -set_engines__bond(struct i915_user_extension __user *base, void *data) -{ - struct i915_context_engines_bond __user *ext = - container_of_user(base, typeof(*ext), base); - const struct set_engines *set = data; - struct drm_i915_private *i915 = set->ctx->i915; - struct i915_engine_class_instance ci; - struct intel_engine_cs *virtual; - struct intel_engine_cs *master; - u16 idx, num_bonds; - int err, n; - - if (get_user(idx, &ext->virtual_index)) - return -EFAULT; - - if (idx >= set->engines->num_engines) { - drm_dbg(&i915->drm, - "Invalid index for virtual engine: %d >= %d\n", - idx, set->engines->num_engines); - return -EINVAL; - } - - idx = array_index_nospec(idx, set->engines->num_engines); - if (!set->engines->engines[idx]) { - drm_dbg(&i915->drm, "Invalid engine at %d\n", idx); - return -EINVAL; - } - virtual = set->engines->engines[idx]->engine; - - if (intel_engine_is_virtual(virtual)) { - drm_dbg(&i915->drm, - "Bonding with virtual engines not allowed\n"); - return -EINVAL; - } - - err = check_user_mbz(&ext->flags); - if (err) - return err; - - for (n = 0; n < ARRAY_SIZE(ext->mbz64); n++) { - err = check_user_mbz(&ext->mbz64[n]); - if (err) - return err; - } - - if (copy_from_user(&ci, &ext->master, sizeof(ci))) - return -EFAULT; - - master = intel_engine_lookup_user(i915, - ci.engine_class, ci.engine_instance); - if (!master) { - drm_dbg(&i915->drm, - "Unrecognised master engine: { class:%u, instance:%u }\n", - ci.engine_class, ci.engine_instance); - return -EINVAL; - } - - if (get_user(num_bonds, &ext->num_bonds)) - return -EFAULT; - - for (n = 0; n < num_bonds; n++) { - struct intel_engine_cs *bond; - - if (copy_from_user(&ci, &ext->engines[n], sizeof(ci))) - return -EFAULT; - - bond = intel_engine_lookup_user(i915, - ci.engine_class, - ci.engine_instance); - if (!bond) { - drm_dbg(&i915->drm, - "Unrecognised engine[%d] for bonding: { class:%d, instance: %d }\n", - n, ci.engine_class, ci.engine_instance); - return -EINVAL; - } - } - - return 0; -} - -static const i915_user_extension_fn set_engines__extensions[] = { - [I915_CONTEXT_ENGINES_EXT_LOAD_BALANCE] = set_engines__load_balance, - [I915_CONTEXT_ENGINES_EXT_BOND] = set_engines__bond, -}; - -static int -set_engines(struct i915_gem_context *ctx, - const struct drm_i915_gem_context_param *args) -{ - struct drm_i915_private *i915 = ctx->i915; - struct i915_context_param_engines __user *user = - u64_to_user_ptr(args->value); - struct intel_sseu null_sseu = {}; - struct set_engines set = { .ctx = ctx }; - unsigned int num_engines, n; - u64 extensions; - int err; - - if (!args->size) { /* switch back to legacy user_ring_map */ - if (!i915_gem_context_user_engines(ctx)) - return 0; - - set.engines = default_engines(ctx, null_sseu); - if (IS_ERR(set.engines)) - return PTR_ERR(set.engines); - - goto replace; - } - - if (args->size < sizeof(*user) || - !IS_ALIGNED(args->size - sizeof(*user), sizeof(*user->engines))) { - drm_dbg(&i915->drm, "Invalid size for engine array: %d\n", - args->size); - return -EINVAL; - } - - num_engines = (args->size - sizeof(*user)) / sizeof(*user->engines); - /* RING_MASK has no shift so we can use it directly here */ - if (num_engines > I915_EXEC_RING_MASK + 1) - return -EINVAL; - - set.engines = alloc_engines(num_engines); - if (!set.engines) - return -ENOMEM; - - for (n = 0; n < num_engines; n++) { - struct i915_engine_class_instance ci; - struct intel_engine_cs *engine; - struct intel_context *ce; - - if (copy_from_user(&ci, &user->engines[n], sizeof(ci))) { - __free_engines(set.engines, n); - return -EFAULT; - } - - if (ci.engine_class == (u16)I915_ENGINE_CLASS_INVALID && - ci.engine_instance == (u16)I915_ENGINE_CLASS_INVALID_NONE) { - set.engines->engines[n] = NULL; - continue; - } - - engine = intel_engine_lookup_user(ctx->i915, - ci.engine_class, - ci.engine_instance); - if (!engine) { - drm_dbg(&i915->drm, - "Invalid engine[%d]: { class:%d, instance:%d }\n", - n, ci.engine_class, ci.engine_instance); - __free_engines(set.engines, n); - return -ENOENT; - } - - ce = intel_context_create(engine); - if (IS_ERR(ce)) { - __free_engines(set.engines, n); - return PTR_ERR(ce); - } - - intel_context_set_gem(ce, ctx, null_sseu); - - set.engines->engines[n] = ce; - } - set.engines->num_engines = num_engines; - - err = -EFAULT; - if (!get_user(extensions, &user->extensions)) - err = i915_user_extensions(u64_to_user_ptr(extensions), - set_engines__extensions, - ARRAY_SIZE(set_engines__extensions), - &set); - if (err) { - free_engines(set.engines); - return err; - } - -replace: - mutex_lock(&ctx->engines_mutex); - if (i915_gem_context_is_closed(ctx)) { - mutex_unlock(&ctx->engines_mutex); - free_engines(set.engines); - return -ENOENT; - } - if (args->size) - i915_gem_context_set_user_engines(ctx); - else - i915_gem_context_clear_user_engines(ctx); - set.engines = rcu_replace_pointer(ctx->engines, set.engines, 1); - mutex_unlock(&ctx->engines_mutex); - - /* Keep track of old engine sets for kill_context() */ - engines_idle_release(ctx, set.engines); - - return 0; -} - static int set_persistence(struct i915_gem_context *ctx, const struct drm_i915_gem_context_param *args) @@ -2200,10 +1901,6 @@ static int ctx_setparam(struct drm_i915_file_private *fpriv, ret = set_sseu(ctx, args); break; - case I915_CONTEXT_PARAM_ENGINES: - ret = set_engines(ctx, args); - break; - case I915_CONTEXT_PARAM_PERSISTENCE: ret = set_persistence(ctx, args); break;
When the APIs were added to manage the engine set on a GEM context directly from userspace, the questionable choice was made to allow changing the engine set on a context at any time. This is horribly racy and there's absolutely no reason why any userspace would want to do this outside of trying to exercise interesting race conditions. By removing support for CONTEXT_PARAM_ENGINES from ctx_setparam, we make it impossible to change the engine set after the context has been fully created. This doesn't yet let us delete all the deferred engine clean-up code as that's still used for handling the case where the client dies or calls GEM_CONTEXT_DESTROY while work is in flight. However, moving to an API where the engine set is effectively immutable gives us more options to potentially clean that code up a bit going forward. It also removes a whole class of ways in which a client can hurt itself or try to get around kernel context banning. v2 (Jason Ekstrand): - Expand the commit mesage Signed-off-by: Jason Ekstrand <jason@jlekstrand.net> --- drivers/gpu/drm/i915/gem/i915_gem_context.c | 303 -------------------- 1 file changed, 303 deletions(-)