[Intel-gfx] [PATCH] drm/i915: Don't adjust priority on an already signaled fence
Chris Wilson
chris at chris-wilson.co.uk
Mon Jan 8 09:44:07 UTC 2018
Quoting Mika Kuoppala (2018-01-08 09:27:38)
> Chris Wilson <chris at chris-wilson.co.uk> writes:
>
> > When we retire a signaled fence, we free the dependency tree. However,
> > we skip clearing the list so that if we then try to adjust the priority
> > of the signaled fence, we may walk the list of freed dependencies.
> >
> > [ 3083.156757] ==================================================================
> > [ 3083.156806] BUG: KASAN: use-after-free in execlists_schedule+0x199/0x660 [i915]
> > [ 3083.156810] Read of size 8 at addr ffff8806bf20f400 by task Xorg/831
> >
> > [ 3083.156815] CPU: 0 PID: 831 Comm: Xorg Not tainted 4.15.0-rc6-no-psn+ #1
> > [ 3083.156817] Hardware name: Notebook N24_25BU/N24_25BU, BIOS 5.12 02/17/2017
> > [ 3083.156818] Call Trace:
> > [ 3083.156823] dump_stack+0x5c/0x7a
> > [ 3083.156827] print_address_description+0x6b/0x290
> > [ 3083.156830] kasan_report+0x28f/0x380
> > [ 3083.156872] ? execlists_schedule+0x199/0x660 [i915]
> > [ 3083.156914] execlists_schedule+0x199/0x660 [i915]
> > [ 3083.156956] ? intel_crtc_atomic_check+0x146/0x4e0 [i915]
> > [ 3083.156997] ? execlists_submit_request+0xe0/0xe0 [i915]
> > [ 3083.157038] ? i915_vma_misplaced.part.4+0x25/0xb0 [i915]
> > [ 3083.157079] ? __i915_vma_do_pin+0x7c8/0xc80 [i915]
> > [ 3083.157121] ? intel_atomic_state_alloc+0x44/0x60 [i915]
> > [ 3083.157130] ? drm_atomic_helper_page_flip+0x3e/0xb0 [drm_kms_helper]
> > [ 3083.157145] ? drm_mode_page_flip_ioctl+0x7d2/0x850 [drm]
> > [ 3083.157159] ? drm_ioctl_kernel+0xa7/0xf0 [drm]
> > [ 3083.157172] ? drm_ioctl+0x45b/0x560 [drm]
> > [ 3083.157211] i915_gem_object_wait_priority+0x14c/0x2c0 [i915]
> > [ 3083.157251] ? i915_gem_get_aperture_ioctl+0x150/0x150 [i915]
> > [ 3083.157290] ? i915_vma_pin_fence+0x1d8/0x320 [i915]
> > [ 3083.157331] ? intel_pin_and_fence_fb_obj+0x175/0x250 [i915]
> > [ 3083.157372] ? intel_rotation_info_size+0x60/0x60 [i915]
> > [ 3083.157413] ? intel_link_compute_m_n+0x80/0x80 [i915]
> > [ 3083.157428] ? drm_dev_printk+0x1b0/0x1b0 [drm]
> > [ 3083.157443] ? drm_dev_printk+0x1b0/0x1b0 [drm]
> > [ 3083.157485] intel_prepare_plane_fb+0x2f8/0x5a0 [i915]
> > [ 3083.157527] ? intel_crtc_get_vblank_counter+0x80/0x80 [i915]
> > [ 3083.157536] drm_atomic_helper_prepare_planes+0xa0/0x1c0 [drm_kms_helper]
> > [ 3083.157587] intel_atomic_commit+0x12e/0x4e0 [i915]
> > [ 3083.157605] drm_atomic_helper_page_flip+0xa2/0xb0 [drm_kms_helper]
> > [ 3083.157621] drm_mode_page_flip_ioctl+0x7d2/0x850 [drm]
> > [ 3083.157638] ? drm_mode_cursor2_ioctl+0x10/0x10 [drm]
> > [ 3083.157652] ? drm_lease_owner+0x1a/0x30 [drm]
> > [ 3083.157668] ? drm_mode_cursor2_ioctl+0x10/0x10 [drm]
> > [ 3083.157681] drm_ioctl_kernel+0xa7/0xf0 [drm]
> > [ 3083.157696] drm_ioctl+0x45b/0x560 [drm]
> > [ 3083.157711] ? drm_mode_cursor2_ioctl+0x10/0x10 [drm]
> > [ 3083.157725] ? drm_getstats+0x20/0x20 [drm]
> > [ 3083.157729] ? timerqueue_del+0x49/0x80
> > [ 3083.157732] ? __remove_hrtimer+0x62/0xb0
> > [ 3083.157735] ? hrtimer_try_to_cancel+0x173/0x210
> > [ 3083.157738] do_vfs_ioctl+0x13b/0x880
> > [ 3083.157741] ? ioctl_preallocate+0x140/0x140
> > [ 3083.157744] ? _raw_spin_unlock_irq+0xe/0x30
> > [ 3083.157746] ? do_setitimer+0x234/0x370
> > [ 3083.157750] ? SyS_setitimer+0x19e/0x1b0
> > [ 3083.157752] ? SyS_alarm+0x140/0x140
> > [ 3083.157755] ? __rcu_read_unlock+0x66/0x80
> > [ 3083.157757] ? __fget+0xc4/0x100
> > [ 3083.157760] SyS_ioctl+0x74/0x80
> > [ 3083.157763] entry_SYSCALL_64_fastpath+0x1a/0x7d
> > [ 3083.157765] RIP: 0033:0x7f6135d0c6a7
> > [ 3083.157767] RSP: 002b:00007fff01451888 EFLAGS: 00003246 ORIG_RAX: 0000000000000010
> > [ 3083.157769] RAX: ffffffffffffffda RBX: 0000000000000004 RCX: 00007f6135d0c6a7
> > [ 3083.157771] RDX: 00007fff01451950 RSI: 00000000c01864b0 RDI: 000000000000000c
> > [ 3083.157772] RBP: 00007f613076f600 R08: 0000000000000001 R09: 0000000000000000
> > [ 3083.157773] R10: 0000000000000060 R11: 0000000000003246 R12: 0000000000000000
> > [ 3083.157774] R13: 0000000000000060 R14: 000000000000001b R15: 0000000000000060
> >
> > [ 3083.157779] Allocated by task 831:
> > [ 3083.157783] kmem_cache_alloc+0xc0/0x200
> > [ 3083.157822] i915_gem_request_await_dma_fence+0x2c4/0x5d0 [i915]
> > [ 3083.157861] i915_gem_request_await_object+0x321/0x370 [i915]
> > [ 3083.157900] i915_gem_do_execbuffer+0x1165/0x19c0 [i915]
> > [ 3083.157937] i915_gem_execbuffer2+0x1ad/0x550 [i915]
> > [ 3083.157950] drm_ioctl_kernel+0xa7/0xf0 [drm]
> > [ 3083.157962] drm_ioctl+0x45b/0x560 [drm]
> > [ 3083.157964] do_vfs_ioctl+0x13b/0x880
> > [ 3083.157966] SyS_ioctl+0x74/0x80
> > [ 3083.157968] entry_SYSCALL_64_fastpath+0x1a/0x7d
> >
> > [ 3083.157971] Freed by task 831:
> > [ 3083.157973] kmem_cache_free+0x77/0x220
> > [ 3083.158012] i915_gem_request_retire+0x72c/0xa70 [i915]
> > [ 3083.158051] i915_gem_request_alloc+0x1e9/0x8b0 [i915]
> > [ 3083.158089] i915_gem_do_execbuffer+0xa96/0x19c0 [i915]
> > [ 3083.158127] i915_gem_execbuffer2+0x1ad/0x550 [i915]
> > [ 3083.158140] drm_ioctl_kernel+0xa7/0xf0 [drm]
> > [ 3083.158153] drm_ioctl+0x45b/0x560 [drm]
> > [ 3083.158155] do_vfs_ioctl+0x13b/0x880
> > [ 3083.158156] SyS_ioctl+0x74/0x80
> > [ 3083.158158] entry_SYSCALL_64_fastpath+0x1a/0x7d
> >
> > [ 3083.158162] The buggy address belongs to the object at ffff8806bf20f400
> > which belongs to the cache i915_dependency of size 64
> > [ 3083.158166] The buggy address is located 0 bytes inside of
> > 64-byte region [ffff8806bf20f400, ffff8806bf20f440)
> > [ 3083.158168] The buggy address belongs to the page:
> > [ 3083.158171] page:00000000d43decc4 count:1 mapcount:0 mapping: (null) index:0x0
> > [ 3083.158174] flags: 0x17ffe0000000100(slab)
> > [ 3083.158179] raw: 017ffe0000000100 0000000000000000 0000000000000000 0000000180200020
> > [ 3083.158182] raw: ffffea001afc16c0 0000000500000005 ffff880731b881c0 0000000000000000
> > [ 3083.158184] page dumped because: kasan: bad access detected
> >
> > [ 3083.158187] Memory state around the buggy address:
> > [ 3083.158190] ffff8806bf20f300: fb fb fb fb fb fb fb fb fc fc fc fc fc fc fc fc
> > [ 3083.158192] ffff8806bf20f380: fb fb fb fb fb fb fb fb fc fc fc fc fc fc fc fc
> > [ 3083.158195] >ffff8806bf20f400: fb fb fb fb fb fb fb fb fc fc fc fc fc fc fc fc
> > [ 3083.158196] ^
> > [ 3083.158199] ffff8806bf20f480: fb fb fb fb fb fb fb fb fc fc fc fc fc fc fc fc
> > [ 3083.158201] ffff8806bf20f500: fb fb fb fb fb fb fb fb fc fc fc fc fc fc fc fc
> > [ 3083.158203] ==================================================================
> >
> > Reported-by: Alexandru Chirvasitu <achirvasub at gmail.com>
> > Reported-by: Mike Keehan <mike at keehan.net>
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104436
> > Fixes: 1f181225f8ec ("drm/i915/execlists: Keep request->priority for its lifetime")
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > Cc: Alexandru Chirvasitu <achirvasub at gmail.com>
> > Cc: Michał Winiarski <michal.winiarski at intel.com>
> > Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> > ---
> > drivers/gpu/drm/i915/i915_gem.c | 2 +-
> > drivers/gpu/drm/i915/intel_lrc.c | 3 +++
> > 2 files changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index ba9f67c256f4..8bc3283484be 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -467,7 +467,7 @@ static void __fence_set_priority(struct dma_fence *fence, int prio)
> > struct drm_i915_gem_request *rq;
> > struct intel_engine_cs *engine;
> >
> > - if (!dma_fence_is_i915(fence))
> > + if (dma_fence_is_signaled(fence) || !dma_fence_is_i915(fence))
>
> Generic status first and then if it is ours. Should be ok.
>
> > return;
> >
> > rq = to_request(fence);
> > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> > index 4e150b095a11..ff25f209d0a5 100644
> > --- a/drivers/gpu/drm/i915/intel_lrc.c
> > +++ b/drivers/gpu/drm/i915/intel_lrc.c
> > @@ -1002,6 +1002,9 @@ static void execlists_schedule(struct drm_i915_gem_request *request, int prio)
> >
> > GEM_BUG_ON(prio == I915_PRIORITY_INVALID);
> >
> > + if (i915_gem_request_completed(request))
> > + return;
> > +
>
> Not that I would mind but I don't see the connection to
> the above. Is there one?
i915_gem_request_completed() is rejecting all signaled fences, it's the
function that dma_fence_is_signaled() calls for i915 requests.
Adding dma_fence_is_signaled() I consider to be a micro-optimisation,
the true safe guard here is checking in execlists_schedule() where the
rules on when the priotree can be accessed are kept.
Thanks for the review, pushed the urgent bug fix. If this is the only
fallout from enabling preemption, we are doing well!
-Chris
More information about the Intel-gfx
mailing list