* [PATCH] drm/i915: return DRIVER_NAME for the fence driver name
@ 2021-06-16 12:28 Matthew Auld
2021-06-16 13:32 ` Daniel Vetter
` (2 more replies)
0 siblings, 3 replies; 4+ messages in thread
From: Matthew Auld @ 2021-06-16 12:28 UTC (permalink / raw)
To: intel-gfx; +Cc: Michael Mason, dri-devel
The first tracepoint for a request is trace_dma_fence_init which is
called in the ctor before we have properly setup the request->engine. So
if it's a non-recycled request the rq->engine might be NULL, or some
garbage value, which leads to a crash.
Since we are not permitted to use kmem_cache_zalloc() here with
SLAB_TYPESAFE_BY_RCU, one approach is simply to return DRIVER_NAME. We
can then revisit this later if we decide to get rid of
SLAB_TYPESAFE_BY_RCU.
Fixes: 855e39e65cfc ("drm/i915: Initialise basic fence before acquiring seqno")
Signed-off-by: Matthew Auld <matthew.auld@intel.com>
Cc: Michael Mason <michael.w.mason@intel.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
---
drivers/gpu/drm/i915/i915_request.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
index 1014c71cf7f5..55fa94bde22e 100644
--- a/drivers/gpu/drm/i915/i915_request.c
+++ b/drivers/gpu/drm/i915/i915_request.c
@@ -61,7 +61,7 @@ static struct i915_global_request {
static const char *i915_fence_get_driver_name(struct dma_fence *fence)
{
- return dev_name(to_request(fence)->engine->i915->drm.dev);
+ return DRIVER_NAME;
}
static const char *i915_fence_get_timeline_name(struct dma_fence *fence)
--
2.26.3
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] drm/i915: return DRIVER_NAME for the fence driver name
2021-06-16 12:28 [PATCH] drm/i915: return DRIVER_NAME for the fence driver name Matthew Auld
@ 2021-06-16 13:32 ` Daniel Vetter
2021-06-22 21:32 ` Mason, Michael W
2021-06-23 10:12 ` Thomas Zimmermann
2 siblings, 0 replies; 4+ messages in thread
From: Daniel Vetter @ 2021-06-16 13:32 UTC (permalink / raw)
To: Matthew Auld; +Cc: intel-gfx, Michael Mason, dri-devel
On Wed, Jun 16, 2021 at 01:28:33PM +0100, Matthew Auld wrote:
> The first tracepoint for a request is trace_dma_fence_init which is
> called in the ctor before we have properly setup the request->engine. So
> if it's a non-recycled request the rq->engine might be NULL, or some
> garbage value, which leads to a crash.
I'd hammer it more in here that we cannot move the dma_fence_init out of
the slab constructor (aka ctor), because that breaks the magic request
recycling through SLAB_TYPESAFE_BY_RCU. You do explain it all, but in a
rather dense fashion. More verbosity here would be good.
> Since we are not permitted to use kmem_cache_zalloc() here with
This part I'm not really clear on ... what would zalloc break? Ideally
references to the commits/bugs/functions in the code that would blow up
would be good here.
> SLAB_TYPESAFE_BY_RCU, one approach is simply to return DRIVER_NAME. We
> can then revisit this later if we decide to get rid of
> SLAB_TYPESAFE_BY_RCU.
>
> Fixes: 855e39e65cfc ("drm/i915: Initialise basic fence before acquiring seqno")
> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> Cc: Michael Mason <michael.w.mason@intel.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
As a stop-gab the code change looks reasonable, but I think the commit
message needs work and much more of the history/background we're hitting
here.
Thanks, Daniel
> ---
> drivers/gpu/drm/i915/i915_request.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
> index 1014c71cf7f5..55fa94bde22e 100644
> --- a/drivers/gpu/drm/i915/i915_request.c
> +++ b/drivers/gpu/drm/i915/i915_request.c
> @@ -61,7 +61,7 @@ static struct i915_global_request {
>
> static const char *i915_fence_get_driver_name(struct dma_fence *fence)
> {
> - return dev_name(to_request(fence)->engine->i915->drm.dev);
> + return DRIVER_NAME;
> }
>
> static const char *i915_fence_get_timeline_name(struct dma_fence *fence)
> --
> 2.26.3
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 4+ messages in thread
* RE: [PATCH] drm/i915: return DRIVER_NAME for the fence driver name
2021-06-16 12:28 [PATCH] drm/i915: return DRIVER_NAME for the fence driver name Matthew Auld
2021-06-16 13:32 ` Daniel Vetter
@ 2021-06-22 21:32 ` Mason, Michael W
2021-06-23 10:12 ` Thomas Zimmermann
2 siblings, 0 replies; 4+ messages in thread
From: Mason, Michael W @ 2021-06-22 21:32 UTC (permalink / raw)
To: Auld, Matthew, intel-gfx; +Cc: dri-devel
This fixes the NULL pointer dereference in i915_fence_get_driver_name(),
but now it crashes in i915_fence_get_timeline with another NULL pointer
dereference. It attempts to use i915_gem_context.name, which apparently
also hasn't been initialized
static const char *i915_fence_get_timeline_name(struct dma_fence *fence)
{
const struct i915_gem_context *ctx;
/*
* The timeline struct (as part of the ppgtt underneath a context)
* may be freed when the request is no longer in use by the GPU.
* We could extend the life of a context to beyond that of all
* fences, possibly keeping the hw resource around indefinitely,
* or we just give them a false name. Since
* dma_fence_ops.get_timeline_name is a debug feature, the occasional
* lie seems justifiable.
*/
if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
return "signaled";
ctx = i915_request_gem_context(to_request(fence));
if (!ctx)
return "[" DRIVER_NAME "]";
return ctx->name;
}
<1>[ 414.327761] BUG: kernel NULL pointer dereference, address: 0000000000000020
<1>[ 414.327766] #PF: supervisor read access in kernel mode
<1>[ 414.327768] #PF: error_code(0x0000) - not-present page
<6>[ 414.327769] PGD 0 P4D 0
<4>[ 414.327772] Oops: 0000 [#1] PREEMPT SMP NOPTI
<4>[ 414.327774] CPU: 3 PID: 1866 Comm: chrome Tainted: G U 5.4.125 #2
<4>[ 414.327776] Hardware name: Google Voxel/Voxel, BIOS Google_Voxel.13913.0.0 04/12/2021
<4>[ 414.327781] RIP: 0010:i915_fence_get_timeline_name+0x1d/0x37
<4>[ 414.327783] Code: 55 48 89 e5 48 c7 c0 ee d1 72 bd 5d c3 0f 1f 44 00 00 55 48 89 e5 48 8b 4f 30 48 c7 c0 b4 49 72 bd f6 c1 01 75 1c 48 8b 47 60 <48> 8b 40 20 48 85 c0 74 08 48 05 30 01 00 00 eb 07 48 c7 c0 79 7b
<4>[ 414.327785] RSP: 0018:ffffa4a300d177b8 EFLAGS: 00010246
<4>[ 414.327787] RAX: 0000000000000000 RBX: ffffffffbd7a8680 RCX: 0000000000000000
<4>[ 414.327788] RDX: ffff9c27b3b0f4c0 RSI: ffff9c27b3b0f480 RDI: ffff9c27b3b0f480
<4>[ 414.327789] RBP: ffffa4a300d177b8 R08: 0000000000000000 R09: 0000000000000004
<4>[ 414.327791] R10: 0000000000000001 R11: ffffffffbca45536 R12: 0000000000000005
<4>[ 414.327792] R13: 0000000000000004 R14: ffff9c27b6c79bb0 R15: ffff9c27b3b0f480
<4>[ 414.327794] FS: 00007898a2965e00(0000) GS:ffff9c27b7f80000(0000) knlGS:0000000000000000
<4>[ 414.327795] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
<4>[ 414.327797] CR2: 0000000000000020 CR3: 000000022ded2006 CR4: 0000000000762ee0
<4>[ 414.327798] PKRU: 55555554
<4>[ 414.327799] Call Trace:
<4>[ 414.327804] trace_event_raw_event_dma_fence+0xc9/0x1e5
<4>[ 414.327807] dma_fence_init+0xa6/0xca
<4>[ 414.327809] __i915_request_ctor+0x7f/0xa9
<4>[ 414.327812] setup_object+0x88/0x8a
<4>[ 414.327815] new_slab+0x22e/0x429
<4>[ 414.327817] ___slab_alloc+0x2c8/0x42e
<4>[ 414.327820] ? __i915_request_create+0x68/0x238
<4>[ 414.327822] ? __i915_request_create+0x68/0x238
<4>[ 414.327824] __slab_alloc+0x3c/0x5f
<4>[ 414.327826] kmem_cache_alloc+0x19b/0x201
<4>[ 414.327828] __i915_request_create+0x68/0x238
<4>[ 414.327830] i915_request_create+0x8a/0xca
<4>[ 414.327833] i915_gem_do_execbuffer+0x12f9/0x1830
<4>[ 414.327837] i915_gem_execbuffer2_ioctl+0x157/0x398
<4>[ 414.327839] ? i915_gem_do_execbuffer+0x1830/0x1830
<4>[ 414.327840] drm_ioctl_kernel+0x94/0xf6
<4>[ 414.327842] drm_ioctl+0x276/0x39b
<4>[ 414.327845] ? i915_gem_do_execbuffer+0x1830/0x1830
<4>[ 414.327847] do_vfs_ioctl+0x4f4/0x771
<4>[ 414.327849] ksys_ioctl+0x58/0x83
<4>[ 414.327851] __x64_sys_ioctl+0x1a/0x1e
<4>[ 414.327853] do_syscall_64+0x54/0x7e
<4>[ 414.327856] entry_SYSCALL_64_after_hwframe+0x44/0xa9
<4>[ 414.327859] RIP: 0033:0x7898a2f88f07
> -----Original Message-----
> From: Auld, Matthew <matthew.auld@intel.com>
> Sent: Wednesday, June 16, 2021 5:29 AM
> To: intel-gfx@lists.freedesktop.org
> Cc: dri-devel@lists.freedesktop.org; Mason, Michael W
> <michael.w.mason@intel.com>; Daniel Vetter <daniel@ffwll.ch>
> Subject: [PATCH] drm/i915: return DRIVER_NAME for the fence driver name
>
> The first tracepoint for a request is trace_dma_fence_init which is called in
> the ctor before we have properly setup the request->engine. So if it's a non-
> recycled request the rq->engine might be NULL, or some garbage value,
> which leads to a crash.
>
> Since we are not permitted to use kmem_cache_zalloc() here with
> SLAB_TYPESAFE_BY_RCU, one approach is simply to return DRIVER_NAME.
> We can then revisit this later if we decide to get rid of
> SLAB_TYPESAFE_BY_RCU.
>
> Fixes: 855e39e65cfc ("drm/i915: Initialise basic fence before acquiring
> seqno")
> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> Cc: Michael Mason <michael.w.mason@intel.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> ---
> drivers/gpu/drm/i915/i915_request.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_request.c
> b/drivers/gpu/drm/i915/i915_request.c
> index 1014c71cf7f5..55fa94bde22e 100644
> --- a/drivers/gpu/drm/i915/i915_request.c
> +++ b/drivers/gpu/drm/i915/i915_request.c
> @@ -61,7 +61,7 @@ static struct i915_global_request {
>
> static const char *i915_fence_get_driver_name(struct dma_fence *fence) {
> - return dev_name(to_request(fence)->engine->i915->drm.dev);
> + return DRIVER_NAME;
> }
>
> static const char *i915_fence_get_timeline_name(struct dma_fence
> *fence)
> --
> 2.26.3
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] drm/i915: return DRIVER_NAME for the fence driver name
2021-06-16 12:28 [PATCH] drm/i915: return DRIVER_NAME for the fence driver name Matthew Auld
2021-06-16 13:32 ` Daniel Vetter
2021-06-22 21:32 ` Mason, Michael W
@ 2021-06-23 10:12 ` Thomas Zimmermann
2 siblings, 0 replies; 4+ messages in thread
From: Thomas Zimmermann @ 2021-06-23 10:12 UTC (permalink / raw)
To: Matthew Auld, intel-gfx; +Cc: Michael Mason, dri-devel
[-- Attachment #1.1: Type: text/plain, Size: 1811 bytes --]
Hi
Am 16.06.21 um 14:28 schrieb Matthew Auld:
> The first tracepoint for a request is trace_dma_fence_init which is
> called in the ctor before we have properly setup the request->engine. So
> if it's a non-recycled request the rq->engine might be NULL, or some
> garbage value, which leads to a crash.
>
> Since we are not permitted to use kmem_cache_zalloc() here with
> SLAB_TYPESAFE_BY_RCU, one approach is simply to return DRIVER_NAME. We
> can then revisit this later if we decide to get rid of
> SLAB_TYPESAFE_BY_RCU.
>
> Fixes: 855e39e65cfc ("drm/i915: Initialise basic fence before acquiring seqno")
> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> Cc: Michael Mason <michael.w.mason@intel.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> ---
> drivers/gpu/drm/i915/i915_request.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
> index 1014c71cf7f5..55fa94bde22e 100644
> --- a/drivers/gpu/drm/i915/i915_request.c
> +++ b/drivers/gpu/drm/i915/i915_request.c
> @@ -61,7 +61,7 @@ static struct i915_global_request {
>
> static const char *i915_fence_get_driver_name(struct dma_fence *fence)
> {
> - return dev_name(to_request(fence)->engine->i915->drm.dev);
> + return DRIVER_NAME;
There was recently a discussion about using struct drm_driver.name
consistently throughout the source code. I'd like to suggest to do this
here.
Best regards
Thomas
> }
>
> static const char *i915_fence_get_timeline_name(struct dma_fence *fence)
>
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-06-23 10:12 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-16 12:28 [PATCH] drm/i915: return DRIVER_NAME for the fence driver name Matthew Auld
2021-06-16 13:32 ` Daniel Vetter
2021-06-22 21:32 ` Mason, Michael W
2021-06-23 10:12 ` Thomas Zimmermann
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).