Re: [PATCH] drm/i915: Don't adjust priority on an already signaled fence

[Mika Kuoppala <mika.kuoppala@linux.intel.com>]

Chris Wilson <chris@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@gmail.com>
> Reported-by: Mike Keehan <mike@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@chris-wilson.co.uk>
> Cc: Alexandru Chirvasitu <achirvasub@gmail.com>
> Cc: Michał Winiarski <michal.winiarski@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@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?

-Mika


>  	if (prio <= READ_ONCE(request->priotree.priority))
>  		return;
>  
> -- 
> 2.15.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx