Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> writes:
> If we make sure we grab a strong reference to each object as we dump it,
> we can reduce the locks outside of our iterators to an rcu_read_lock.
>
> This should prevent errors like:
> [ 2138.371911] BUG: KASAN: use-after-free in per_file_stats+0x43/0x380 [i915]
> [ 2138.371924] Read of size 8 at addr ffff888223651000 by task cat/8293
>
> [ 2138.371947] CPU: 0 PID: 8293 Comm: cat Not tainted 5.3.0-rc6-CI-Custom_4352+ #1
> [ 2138.371953] Hardware name: To Be Filled By O.E.M. To Be Filled By O.E.M./J4205-ITX, BIOS P1.40 07/14/2017
> [ 2138.371959] Call Trace:
> [ 2138.371974] dump_stack+0x7c/0xbb
> [ 2138.372099] ? per_file_stats+0x43/0x380 [i915]
> [ 2138.372108] print_address_description+0x73/0x3a0
> [ 2138.372231] ? per_file_stats+0x43/0x380 [i915]
> [ 2138.372352] ? per_file_stats+0x43/0x380 [i915]
> [ 2138.372362] __kasan_report+0x14e/0x192
> [ 2138.372489] ? per_file_stats+0x43/0x380 [i915]
> [ 2138.372502] kasan_report+0xe/0x20
> [ 2138.372625] per_file_stats+0x43/0x380 [i915]
> [ 2138.372751] ? i915_panel_show+0x110/0x110 [i915]
> [ 2138.372761] idr_for_each+0xa7/0x160
> [ 2138.372773] ? idr_get_next_ul+0x110/0x110
> [ 2138.372782] ? do_raw_spin_lock+0x10a/0x1d0
> [ 2138.372923] print_context_stats+0x264/0x510 [i915]
>
> Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> ---
> drivers/gpu/drm/i915/i915_debugfs.c | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 9798f27a697a..708855e051b5 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -237,6 +237,9 @@ static int per_file_stats(int id, void *ptr, void *data)
> struct file_stats *stats = data;
> struct i915_vma *vma;
>
> + if (!kref_get_unless_zero(&obj->base.refcount))
> + return 0;
> +
Not a single callsite using this. Eyesore is the
getting through this and putting thru nice object wrapper.
But for the content, seems to do what it claims.
Reviewed-by: Mika Kuoppala <mika.kuoppala@xxxxxxxxxxxxxxx>
> stats->count++;
> stats->total += obj->base.size;
> if (!atomic_read(&obj->bind_count))
> @@ -284,6 +287,7 @@ static int per_file_stats(int id, void *ptr, void *data)
> }
> spin_unlock(&obj->vma.lock);
>
> + i915_gem_object_put(obj);
> return 0;
> }
>
> @@ -313,10 +317,12 @@ static void print_context_stats(struct seq_file *m,
> i915_gem_context_lock_engines(ctx), it) {
> intel_context_lock_pinned(ce);
> if (intel_context_is_pinned(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_unlock_pinned(ce);
> }
> @@ -328,9 +334,9 @@ static void print_context_stats(struct seq_file *m,
> struct task_struct *task;
> char name[80];
>
> - spin_lock(&file->table_lock);
> + rcu_read_lock();
> idr_for_each(&file->object_idr, per_file_stats, &stats);
> - spin_unlock(&file->table_lock);
> + rcu_read_unlock();
>
> rcu_read_lock();
> task = pid_task(ctx->pid ?: file->pid, PIDTYPE_PID);
> --
> 2.23.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
