[PATCH 03/56] drm/i915/gem: Use GEM context tracking for i915_gem_objects
Chris Wilson
chris at chris-wilson.co.uk
Sun Jan 17 15:30:43 UTC 2021
Rather than take an indirect jump to the drm midlayer (that introduces a
use-after-free in reading the ctx->file backpointer) to find all the vma
on objects associated with the ctx->file, walk the LUT table stored in
the context that tracks all the objects in use with the context.
The improper serialisation with device closure resulting in a
use-after-free is a much older issue, we have also introduced a new
incorrect list iteration due to commit a4e7ccdac38e ("drm/i915: Move
context management under GEM") as the link is no longer guarded by the
context's reference context.
Fixes: a4e7ccdac38e ("drm/i915: Move context management under GEM")
Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
Cc: CQ Tang <cq.tang at intel.com>
Cc: Lucas De Marchi <lucas.demarchi at intel.com>
---
drivers/gpu/drm/i915/gem/i915_gem_context.c | 4 +-
drivers/gpu/drm/i915/i915_debugfs.c | 181 +++++++-------------
drivers/gpu/drm/i915/i915_vma.c | 4 +-
3 files changed, 69 insertions(+), 120 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c
index 4d2f40cf237b..7879dd01e517 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
@@ -606,7 +606,7 @@ static void context_close(struct i915_gem_context *ctx)
lut_close(ctx);
spin_lock(&ctx->i915->gem.contexts.lock);
- list_del(&ctx->link);
+ list_del_rcu(&ctx->link);
spin_unlock(&ctx->i915->gem.contexts.lock);
mutex_unlock(&ctx->mutex);
@@ -908,7 +908,7 @@ static int gem_context_register(struct i915_gem_context *ctx,
goto err_pid;
spin_lock(&i915->gem.contexts.lock);
- list_add_tail(&ctx->link, &i915->gem.contexts.list);
+ list_add_tail_rcu(&ctx->link, &i915->gem.contexts.list);
spin_unlock(&i915->gem.contexts.lock);
return 0;
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index de8e0e44cfb6..3f5172d495fe 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -220,143 +220,90 @@ i915_debugfs_describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj)
seq_printf(m, " (%s)", engine->name);
}
-struct file_stats {
- struct i915_address_space *vm;
+struct ctx_stats {
unsigned long count;
u64 total;
- u64 active, inactive;
- u64 closed;
+ u64 active;
+ u64 inactive;
};
-static int per_file_stats(int id, void *ptr, void *data)
-{
- struct drm_i915_gem_object *obj = ptr;
- struct file_stats *stats = data;
- struct i915_vma *vma;
-
- if (IS_ERR_OR_NULL(obj) || !kref_get_unless_zero(&obj->base.refcount))
- return 0;
-
- stats->count++;
- stats->total += obj->base.size;
-
- spin_lock(&obj->vma.lock);
- if (!stats->vm) {
- for_each_ggtt_vma(vma, obj) {
- if (!drm_mm_node_allocated(&vma->node))
- continue;
-
- if (i915_vma_is_active(vma))
- stats->active += vma->node.size;
- else
- stats->inactive += vma->node.size;
-
- if (i915_vma_is_closed(vma))
- stats->closed += vma->node.size;
- }
- } else {
- struct rb_node *p = obj->vma.tree.rb_node;
-
- while (p) {
- long cmp;
-
- vma = rb_entry(p, typeof(*vma), obj_node);
- cmp = i915_vma_compare(vma, stats->vm, NULL);
- if (cmp == 0) {
- if (drm_mm_node_allocated(&vma->node)) {
- if (i915_vma_is_active(vma))
- stats->active += vma->node.size;
- else
- stats->inactive += vma->node.size;
-
- if (i915_vma_is_closed(vma))
- stats->closed += vma->node.size;
- }
- break;
- }
- if (cmp < 0)
- p = p->rb_right;
- else
- p = p->rb_left;
- }
- }
- spin_unlock(&obj->vma.lock);
-
- i915_gem_object_put(obj);
- return 0;
-}
-
-#define print_file_stats(m, name, stats) do { \
+#define print_ctx_stats(m, name, stats) do { \
if (stats.count) \
- seq_printf(m, "%s: %lu objects, %llu bytes (%llu active, %llu inactive, %llu closed)\n", \
+ seq_printf(m, "%s: %lu objects, %llu bytes (%llu active, %llu inactive)\n", \
name, \
stats.count, \
stats.total, \
stats.active, \
- stats.inactive, \
- stats.closed); \
+ stats.inactive); \
} while (0)
+static void vma_stats(struct i915_vma *vma, struct ctx_stats *stats)
+{
+ u64 size;
+
+ if (!vma || !drm_mm_node_allocated(&vma->node))
+ return;
+
+ size = vma->size;
+ stats->total += size;
+
+ if (i915_vma_is_active(vma))
+ stats->active += size;
+ else
+ stats->inactive += size;
+
+ stats->count++;
+}
+
+static void context_stats(struct i915_gem_context *ctx, struct ctx_stats *stats)
+{
+ struct radix_tree_iter iter;
+ void __rcu **slot;
+
+ radix_tree_for_each_slot(slot, &ctx->handles_vma, &iter, 0)
+ vma_stats(rcu_dereference_raw(*slot), stats);
+}
+
+static void
+kcontext_stats(struct i915_gem_context *ctx, struct ctx_stats *stats)
+{
+ struct i915_gem_engines_iter it;
+ struct intel_context *ce;
+
+ for_each_gem_engine(ce, rcu_dereference(ctx->engines), it) {
+ if (intel_context_pin_if_active(ce)) {
+ vma_stats(ce->state, stats);
+ vma_stats(ce->ring->vma, stats);
+ intel_context_unpin(ce);
+ }
+ }
+}
+
static void print_context_stats(struct seq_file *m,
struct drm_i915_private *i915)
{
- struct file_stats kstats = {};
- struct i915_gem_context *ctx, *cn;
+ struct ctx_stats kstats = {};
+ struct i915_gem_context *ctx;
- spin_lock(&i915->gem.contexts.lock);
- list_for_each_entry_safe(ctx, cn, &i915->gem.contexts.list, link) {
- struct i915_gem_engines_iter it;
- struct intel_context *ce;
+ rcu_read_lock();
+ list_for_each_entry_rcu(ctx, &i915->gem.contexts.list, link) {
+ struct ctx_stats stats = {};
+ struct task_struct *task;
+ char name[80] = "<unknown>";
- if (!kref_get_unless_zero(&ctx->ref))
- continue;
+ kcontext_stats(ctx, &kstats);
+ context_stats(ctx, &stats);
- spin_unlock(&i915->gem.contexts.lock);
-
- for_each_gem_engine(ce,
- i915_gem_context_lock_engines(ctx), it) {
- if (intel_context_pin_if_active(ce)) {
- rcu_read_lock();
- if (ce->state)
- per_file_stats(0,
- ce->state->obj, &kstats);
- per_file_stats(0, ce->ring->vma->obj, &kstats);
- rcu_read_unlock();
- intel_context_unpin(ce);
- }
+ if (ctx->pid) {
+ task = pid_task(ctx->pid, PIDTYPE_PID);
+ if (task)
+ memcpy(name, task->comm, sizeof(task->comm));
}
- i915_gem_context_unlock_engines(ctx);
-
- mutex_lock(&ctx->mutex);
- if (!IS_ERR_OR_NULL(ctx->file_priv)) {
- struct file_stats stats = {
- .vm = rcu_access_pointer(ctx->vm),
- };
- struct drm_file *file = ctx->file_priv->file;
- struct task_struct *task;
- char name[80];
-
- rcu_read_lock();
- idr_for_each(&file->object_idr, per_file_stats, &stats);
- rcu_read_unlock();
-
- rcu_read_lock();
- task = pid_task(ctx->pid ?: file->pid, PIDTYPE_PID);
- snprintf(name, sizeof(name), "%s",
- task ? task->comm : "<unknown>");
- rcu_read_unlock();
-
- print_file_stats(m, name, stats);
- }
- mutex_unlock(&ctx->mutex);
-
- spin_lock(&i915->gem.contexts.lock);
- list_safe_reset_next(ctx, cn, link);
- i915_gem_context_put(ctx);
+ print_ctx_stats(m, name, stats);
}
- spin_unlock(&i915->gem.contexts.lock);
+ rcu_read_unlock();
- print_file_stats(m, "[k]contexts", kstats);
+ print_ctx_stats(m, "[k]contexts", kstats);
}
static int i915_gem_object_info(struct seq_file *m, void *data)
diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
index caa9b041616b..9b36a6552bf0 100644
--- a/drivers/gpu/drm/i915/i915_vma.c
+++ b/drivers/gpu/drm/i915/i915_vma.c
@@ -1402,7 +1402,9 @@ static struct i915_global_vma global = { {
int __init i915_global_vma_init(void)
{
- global.slab_vmas = KMEM_CACHE(i915_vma, SLAB_HWCACHE_ALIGN);
+ global.slab_vmas = KMEM_CACHE(i915_vma,
+ SLAB_HWCACHE_ALIGN |
+ SLAB_TYPESAFE_BY_RCU);
if (!global.slab_vmas)
return -ENOMEM;
--
2.20.1
More information about the Intel-gfx-trybot
mailing list