* [PATCH] drm/i915: Skip stale object handle for debugfs per-file-stats
@ 2020-06-30 15:27 Chris Wilson
2020-06-30 16:16 ` Mika Kuoppala
0 siblings, 1 reply; 3+ messages in thread
From: Chris Wilson @ 2020-06-30 15:27 UTC (permalink / raw)
To: intel-gfx; +Cc: Chris Wilson, Guenter Roeck, Mika Kuoppala, stable
As we close a handle GEM object, we update the drm_file's idr with an
error pointer to indicate the in-progress closure, and finally set it to
NULL. If we read the idr directly, we may then see an invalid object
pointer, and in our debugfs per_file_stats() we therefore need to
protect against the entry being invalid.
[ 1016.651637] RIP: 0010:per_file_stats+0xe/0x16e
[ 1016.651646] Code: d2 41 0f b6 8e 69 8c 00 00 48 89 df 48 c7 c6 7b 74 8c be 31 c0 e8 0c 89 cf ff eb d2 0f 1f 44 00 00 55 48 89 e5 41
57 41 56 53 <8b> 06 85 c0 0f 84 4d 01 00 00 49 89 d6 48 89 f3 3d ff ff ff 7f 73
[ 1016.651651] RSP: 0018:ffffad3a01337ba0 EFLAGS: 00010293
[ 1016.651656] RAX: 0000000000000018 RBX: ffff96fe040d65e0 RCX: 0000000000000002
[ 1016.651660] RDX: ffffad3a01337c50 RSI: 0000000000000000 RDI: 00000000000001e8
[ 1016.651663] RBP: ffffad3a01337bb8 R08: 0000000000000000 R09: 00000000000001c0
[ 1016.651667] R10: 0000000000000000 R11: ffffffffbdbe5fce R12: 0000000000000000
[ 1016.651671] R13: ffffffffbdbe5fce R14: ffffad3a01337c50 R15: 0000000000000001
[ 1016.651676] FS: 00007a597e2d7480(0000) GS:ffff96ff3bb00000(0000) knlGS:0000000000000000
[ 1016.651680] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 1016.651683] CR2: 0000000000000000 CR3: 0000000171fc2001 CR4: 00000000003606e0
[ 1016.651687] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 1016.651690] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[ 1016.651693] Call Trace:
[ 1016.651693] Call Trace:
[ 1016.651703] idr_for_each+0x8a/0xe8
[ 1016.651711] i915_gem_object_info+0x2a3/0x3eb
[ 1016.651720] seq_read+0x162/0x3ca
[ 1016.651727] full_proxy_read+0x5b/0x8d
[ 1016.651733] __vfs_read+0x45/0x1bb
[ 1016.651741] vfs_read+0xc9/0x15e
[ 1016.651746] ksys_read+0x7e/0xde
[ 1016.651752] do_syscall_64+0x54/0x68
[ 1016.651758] entry_SYSCALL_64_after_hwframe+0x44/0xa9
Reported-by: Guenter Roeck <linux@roeck-us.net>
Fixes: a8c15954d64a ("drm/i915: Protect debugfs per_file_stats with RCU lock")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: stable@vger.kernel.org
---
drivers/gpu/drm/i915/i915_debugfs.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 8594a8ef08ce..9ca94a435b75 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -230,7 +230,7 @@ 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))
+ if (IS_ERR_OR_NULL(obj) || !kref_get_unless_zero(&obj->base.refcount))
return 0;
stats->count++;
--
2.20.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] drm/i915: Skip stale object handle for debugfs per-file-stats
2020-06-30 15:27 [PATCH] drm/i915: Skip stale object handle for debugfs per-file-stats Chris Wilson
@ 2020-06-30 16:16 ` Mika Kuoppala
2020-06-30 17:29 ` Chris Wilson
0 siblings, 1 reply; 3+ messages in thread
From: Mika Kuoppala @ 2020-06-30 16:16 UTC (permalink / raw)
To: Chris Wilson, intel-gfx; +Cc: Chris Wilson, Guenter Roeck, stable
Chris Wilson <chris@chris-wilson.co.uk> writes:
> As we close a handle GEM object, we update the drm_file's idr with an
> error pointer to indicate the in-progress closure, and finally set it to
The error pointer part stage seems to be missing.
But the finding is valid.
Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> NULL. If we read the idr directly, we may then see an invalid object
> pointer, and in our debugfs per_file_stats() we therefore need to
> protect against the entry being invalid.
>
> [ 1016.651637] RIP: 0010:per_file_stats+0xe/0x16e
> [ 1016.651646] Code: d2 41 0f b6 8e 69 8c 00 00 48 89 df 48 c7 c6 7b 74 8c be 31 c0 e8 0c 89 cf ff eb d2 0f 1f 44 00 00 55 48 89 e5 41
> 57 41 56 53 <8b> 06 85 c0 0f 84 4d 01 00 00 49 89 d6 48 89 f3 3d ff ff ff 7f 73
> [ 1016.651651] RSP: 0018:ffffad3a01337ba0 EFLAGS: 00010293
> [ 1016.651656] RAX: 0000000000000018 RBX: ffff96fe040d65e0 RCX: 0000000000000002
> [ 1016.651660] RDX: ffffad3a01337c50 RSI: 0000000000000000 RDI: 00000000000001e8
> [ 1016.651663] RBP: ffffad3a01337bb8 R08: 0000000000000000 R09: 00000000000001c0
> [ 1016.651667] R10: 0000000000000000 R11: ffffffffbdbe5fce R12: 0000000000000000
> [ 1016.651671] R13: ffffffffbdbe5fce R14: ffffad3a01337c50 R15: 0000000000000001
> [ 1016.651676] FS: 00007a597e2d7480(0000) GS:ffff96ff3bb00000(0000) knlGS:0000000000000000
> [ 1016.651680] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 1016.651683] CR2: 0000000000000000 CR3: 0000000171fc2001 CR4: 00000000003606e0
> [ 1016.651687] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> [ 1016.651690] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> [ 1016.651693] Call Trace:
> [ 1016.651693] Call Trace:
> [ 1016.651703] idr_for_each+0x8a/0xe8
> [ 1016.651711] i915_gem_object_info+0x2a3/0x3eb
> [ 1016.651720] seq_read+0x162/0x3ca
> [ 1016.651727] full_proxy_read+0x5b/0x8d
> [ 1016.651733] __vfs_read+0x45/0x1bb
> [ 1016.651741] vfs_read+0xc9/0x15e
> [ 1016.651746] ksys_read+0x7e/0xde
> [ 1016.651752] do_syscall_64+0x54/0x68
> [ 1016.651758] entry_SYSCALL_64_after_hwframe+0x44/0xa9
>
> Reported-by: Guenter Roeck <linux@roeck-us.net>
> Fixes: a8c15954d64a ("drm/i915: Protect debugfs per_file_stats with RCU lock")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: stable@vger.kernel.org
> ---
> drivers/gpu/drm/i915/i915_debugfs.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 8594a8ef08ce..9ca94a435b75 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -230,7 +230,7 @@ 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))
> + if (IS_ERR_OR_NULL(obj) || !kref_get_unless_zero(&obj->base.refcount))
> return 0;
>
> stats->count++;
> --
> 2.20.1
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] drm/i915: Skip stale object handle for debugfs per-file-stats
2020-06-30 16:16 ` Mika Kuoppala
@ 2020-06-30 17:29 ` Chris Wilson
0 siblings, 0 replies; 3+ messages in thread
From: Chris Wilson @ 2020-06-30 17:29 UTC (permalink / raw)
To: Mika Kuoppala, intel-gfx; +Cc: Guenter Roeck, stable
Quoting Mika Kuoppala (2020-06-30 17:16:43)
> Chris Wilson <chris@chris-wilson.co.uk> writes:
>
> > As we close a handle GEM object, we update the drm_file's idr with an
> > error pointer to indicate the in-progress closure, and finally set it to
>
> The error pointer part stage seems to be missing.
Yeah, the ERR_PTR stage seems to be my faulty memory, we just set it to
NULL to indicate in-progress. Ok, I'm not going totally mad:
commit f6cd7daecff558fab2c45d15283d3e52f688342d
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date: Fri Apr 15 12:55:08 2016 +0100
drm: Release driver references to handle before making it available again
...
v2: Use NULL rather than an ERR_PTR to avoid having to adjust callers.
idr_alloc() tracks existing handles using an internal bitmap, so we are
free to use the NULL object as our stale identifier.
-Chris
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-06-30 17:30 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-30 15:27 [PATCH] drm/i915: Skip stale object handle for debugfs per-file-stats Chris Wilson
2020-06-30 16:16 ` Mika Kuoppala
2020-06-30 17:29 ` Chris Wilson
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).