diff mbox series

[2/2] drm/i915: Handle legacy cursor update as normal update

Message ID 20230814065006.47160-2-dev@lankhorst.se (mailing list archive)
State New, archived
Headers show
Series [1/2] drm/i915: Swap ggtt_vma during legacy cursor update | expand

Commit Message

Maarten Lankhorst Aug. 14, 2023, 6:50 a.m. UTC 
From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Abuse the vblank worker to make the changes as small as possible. We
need a way to sync flip_done, but if we wait on flip_done, all async
tests start failing.

Changes since v1:
- Prevent null deref when crtc is inactive.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_crtc.c    | 28 +++++++++++++-------
 drivers/gpu/drm/i915/display/intel_crtc.h    |  6 +++--
 drivers/gpu/drm/i915/display/intel_display.c | 10 ++++---
 3 files changed, 30 insertions(+), 14 deletions(-)

Comments

kernel test robot Aug. 18, 2023, 8:37 a.m. UTC | #1 
Hello,

kernel test robot noticed "BUG:KASAN:slab-use-after-free_in_intel_wait_for_vblank_workers" on:

commit: cfd54d37e5cd9511b5a4a98bba6d4b2f596149cf ("[Intel-gfx] [PATCH 2/2] drm/i915: Handle legacy cursor update as normal update")
url: https://github.com/intel-lab-lkp/linux/commits/Maarten-Lankhorst/drm-i915-Handle-legacy-cursor-update-as-normal-update/20230814-145051
base: git://anongit.freedesktop.org/drm/drm-tip drm-tip
patch link: https://lore.kernel.org/all/20230814065006.47160-2-dev@lankhorst.se/
patch subject: [Intel-gfx] [PATCH 2/2] drm/i915: Handle legacy cursor update as normal update

in testcase: igt
version: igt-x86_64-0f075441-1_20230520
with following parameters:

	group: group-23



compiler: gcc-12
test machine: 20 threads 1 sockets (Commet Lake) with 16G memory

(please refer to attached dmesg/kmsg for entire log/backtrace)



If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <oliver.sang@intel.com>
| Closes: https://lore.kernel.org/oe-lkp/202308181627.2fec1157-oliver.sang@intel.com


kern :err : [  162.196982] BUG: KASAN: slab-use-after-free in intel_wait_for_vblank_workers (drivers/gpu/drm/i915/display/intel_crtc.c:395 drivers/gpu/drm/i915/display/intel_crtc.c:447) i915
kern  :err   : [  162.206530] Read of size 1 at addr ffff88811d8dc150 by task kworker/0:0H/8

kern  :err   : [  162.216391] CPU: 0 PID: 8 Comm: kworker/0:0H Not tainted 6.5.0-rc6-00947-gcfd54d37e5cd #1
kern  :err   : [  162.225319] Workqueue: events_highpri intel_atomic_cleanup_work [i915]
kern  :err   : [  162.232683] Call Trace:
kern  :err   : [  162.235861]  <TASK>
kern :err : [  162.238688] dump_stack_lvl (lib/dump_stack.c:107 (discriminator 1)) 
kern :err : [  162.243085] print_address_description+0x2c/0x3a0 
kern :err : [  162.249618] ? intel_wait_for_vblank_workers (drivers/gpu/drm/i915/display/intel_crtc.c:395 drivers/gpu/drm/i915/display/intel_crtc.c:447) i915
kern :err : [  162.256370] print_report (mm/kasan/report.c:476) 
kern :err : [  162.260681] ? kasan_addr_to_slab (mm/kasan/common.c:35) 
kern :err : [  162.265515] ? intel_wait_for_vblank_workers (drivers/gpu/drm/i915/display/intel_crtc.c:395 drivers/gpu/drm/i915/display/intel_crtc.c:447) i915
kern :err : [  162.272267] kasan_report (mm/kasan/report.c:590) 
kern :err : [  162.276584] ? intel_wait_for_vblank_workers (drivers/gpu/drm/i915/display/intel_crtc.c:395 drivers/gpu/drm/i915/display/intel_crtc.c:447) i915
kern :err : [  162.283336] intel_wait_for_vblank_workers (drivers/gpu/drm/i915/display/intel_crtc.c:395 drivers/gpu/drm/i915/display/intel_crtc.c:447) i915
kern :err : [  162.289911] intel_atomic_cleanup_work (drivers/gpu/drm/i915/display/intel_display.c:6901) i915
kern :err : [  162.296191] ? drm_dev_put (drivers/gpu/drm/drm_drv.c:827) drm
kern :err : [  162.301672] process_one_work (kernel/workqueue.c:2605) 
kern :err : [  162.306507] worker_thread (include/linux/list.h:292 kernel/workqueue.c:2752) 
kern :err : [  162.311080] ? rescuer_thread (kernel/workqueue.c:2694) 
kern :err : [  162.315828] kthread (kernel/kthread.c:389) 
kern :err : [  162.319791] ? kthread_complete_and_exit (kernel/kthread.c:342) 
kern :err : [  162.325323] ret_from_fork (arch/x86/kernel/process.c:151) 
kern :err : [  162.329630] ? kthread_complete_and_exit (kernel/kthread.c:342) 
kern :err : [  162.335181] ret_from_fork_asm (arch/x86/entry/entry_64.S:312) 
kern  :err   : [  162.339840]  </TASK>

kern  :err   : [  162.344980] Allocated by task 4201:
kern :warn : [  162.349214] kasan_save_stack (mm/kasan/common.c:46) 
kern :warn : [  162.353787] kasan_set_track (mm/kasan/common.c:52) 
kern :warn : [  162.358270] __kasan_kmalloc (mm/kasan/common.c:374 mm/kasan/common.c:383) 
kern :warn : [  162.362757] __kmalloc_node_track_caller (include/linux/kasan.h:196 mm/slab_common.c:985 mm/slab_common.c:1005) 
kern :warn : [  162.368283] kmemdup (mm/util.c:131) 
kern :warn : [  162.372075] intel_crtc_duplicate_state (include/linux/fortify-string.h:765 drivers/gpu/drm/i915/display/intel_atomic.c:242) i915
kern :warn : [  162.378364] drm_atomic_get_crtc_state (drivers/gpu/drm/drm_atomic.c:363) drm
kern :warn : [  162.384453] drm_atomic_get_plane_state (drivers/gpu/drm/drm_atomic.c:567) drm
kern :warn : [  162.390622] drm_atomic_helper_update_plane (drivers/gpu/drm/drm_atomic_helper.c:3127) drm_kms_helper
kern :warn : [  162.397997] drm_mode_cursor_universal (drivers/gpu/drm/drm_plane.c:1086) drm
kern :warn : [  162.404086] drm_mode_cursor_common (drivers/gpu/drm/drm_plane.c:1172) drm
kern :warn : [  162.409973] drm_mode_cursor_ioctl (drivers/gpu/drm/drm_plane.c:1188) drm
kern :warn : [  162.415628] drm_ioctl_kernel (drivers/gpu/drm/drm_ioctl.c:795) drm
kern :warn : [  162.420933] drm_ioctl (drivers/gpu/drm/drm_ioctl.c:893) drm
kern :warn : [  162.425627] __x64_sys_ioctl (fs/ioctl.c:52 fs/ioctl.c:870 fs/ioctl.c:856 fs/ioctl.c:856) 
kern :warn : [  162.430284] do_syscall_64 (arch/x86/entry/common.c:50 arch/x86/entry/common.c:80) 
kern :warn : [  162.434590] entry_SYSCALL_64_after_hwframe (arch/x86/entry/entry_64.S:120) 

kern  :err   : [  162.442600] Freed by task 266:
kern :warn : [  162.446384] kasan_save_stack (mm/kasan/common.c:46) 
kern :warn : [  162.450957] kasan_set_track (mm/kasan/common.c:52) 
kern :warn : [  162.455435] kasan_save_free_info (mm/kasan/generic.c:524) 
kern :warn : [  162.460355] __kasan_slab_free (mm/kasan/common.c:238 mm/kasan/common.c:200 mm/kasan/common.c:244) 
kern :warn : [  162.465185] __kmem_cache_free (mm/slub.c:1818 mm/slub.c:3801 mm/slub.c:3814) 
kern :warn : [  162.470016] drm_atomic_state_default_clear (drivers/gpu/drm/drm_atomic.c:228) drm
kern :warn : [  162.476541] intel_atomic_state_clear (drivers/gpu/drm/i915/display/intel_atomic.c:343) i915
kern :warn : [  162.482512] __drm_atomic_state_free (drivers/gpu/drm/drm_atomic.c:313) drm
kern :warn : [  162.488342] intel_atomic_helper_free_state (drivers/gpu/drm/i915/display/intel_display.c:6850) i915
kern :warn : [  162.494833] process_one_work (kernel/workqueue.c:2605) 
kern :warn : [  162.499668] worker_thread (include/linux/list.h:292 kernel/workqueue.c:2752) 
kern :warn : [  162.504244] kthread (kernel/kthread.c:389) 
kern :warn : [  162.508205] ret_from_fork (arch/x86/kernel/process.c:151) 
kern :warn : [  162.512521] ret_from_fork_asm (arch/x86/entry/entry_64.S:312) 

kern  :err   : [  162.519398] The buggy address belongs to the object at ffff88811d8dc000
which belongs to the cache kmalloc-8k of size 8192
kern  :err   : [  162.533366] The buggy address is located 336 bytes inside of
freed 8192-byte region [ffff88811d8dc000, ffff88811d8de000)

kern  :err   : [  162.549380] The buggy address belongs to the physical page:
kern  :warn  : [  162.555691] page:00000000f71065d9 refcount:1 mapcount:0 mapping:0000000000000000 index:0x0 pfn:0x11d8d8
kern  :warn  : [  162.565835] head:00000000f71065d9 order:3 entire_mapcount:0 nr_pages_mapped:0 pincount:0
kern  :warn  : [  162.574675] flags: 0x17ffffc0010200(slab|head|node=0|zone=2|lastcpupid=0x1fffff)
kern  :warn  : [  162.582819] page_type: 0xffffffff()
kern  :warn  : [  162.587049] raw: 0017ffffc0010200 ffff88810c843180 dead000000000122 0000000000000000
kern  :warn  : [  162.595537] raw: 0000000000000000 0000000080020002 00000001ffffffff 0000000000000000
kern  :warn  : [  162.604024] page dumped because: kasan: bad access detected

kern  :err   : [  162.612551] Memory state around the buggy address:
kern  :err   : [  162.618082]  ffff88811d8dc000: fa fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
kern  :err   : [  162.626051]  ffff88811d8dc080: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
kern  :err   : [  162.634018] >ffff88811d8dc100: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
kern  :err   : [  162.641981]                                                  ^
kern  :err   : [  162.648551]  ffff88811d8dc180: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
kern  :err   : [  162.656516]  ffff88811d8dc200: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
kern  :err   : [  162.664482] ==================================================================
kern  :warn  : [  162.672474] Disabling lock debugging due to kernel taint
user  :info  : [  183.845750] [IGT] kms_cursor_legacy: starting dynamic subtest pipe-B
user  :notice: [  183.847199] Total updates 140241 (median of 20 processes is 6978.00)

user  :notice: [  183.862752] Dynamic subtest pipe-A: SUCCESS (21.683s)

user  :notice: [  183.871250] Starting dynamic subtest: pipe-B

user  :info  : [  205.481554] [IGT] kms_cursor_legacy: starting dynamic subtest pipe-C
user  :notice: [  205.483064] Total updates 146561 (median of 20 processes is 7323.50)

user  :notice: [  205.498578] Dynamic subtest pipe-B: SUCCESS (21.629s)

user  :notice: [  205.507030] Starting dynamic subtest: pipe-C

user  :info  : [  227.139947] [IGT] kms_cursor_legacy: starting dynamic subtest all-pipes


The kernel config and materials to reproduce are available at:
https://download.01.org/0day-ci/archive/20230818/202308181627.2fec1157-oliver.sang@intel.com
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_crtc.c b/drivers/gpu/drm/i915/display/intel_crtc.c
index f06b987f5558..be6959c6eb0d 100644
--- a/drivers/gpu/drm/i915/display/intel_crtc.c
+++ b/drivers/gpu/drm/i915/display/intel_crtc.c
@@ -389,11 +389,17 @@  int intel_crtc_init(struct drm_i915_private *dev_priv, enum pipe pipe)
 	return ret;
 }
 
-static bool intel_crtc_needs_vblank_work(const struct intel_crtc_state *crtc_state)
+static bool intel_crtc_needs_vblank_work(const struct intel_atomic_state *state,
+					 const struct intel_crtc_state *crtc_state)
 {
-	return crtc_state->hw.active &&
-		!intel_crtc_needs_modeset(crtc_state) &&
-		!crtc_state->preload_luts &&
+	if (!crtc_state->hw.active || intel_crtc_needs_modeset(crtc_state))
+		return false;
+
+	/* Init for legacy cursor update, so we can sync on teardown */
+	if (state->base.legacy_cursor_update)
+		return true;
+
+	return !crtc_state->preload_luts &&
 		intel_crtc_needs_color_update(crtc_state);
 }
 
@@ -438,7 +444,7 @@  void intel_wait_for_vblank_workers(struct intel_atomic_state *state)
 	int i;
 
 	for_each_new_intel_crtc_in_state(state, crtc, crtc_state, i) {
-		if (!intel_crtc_needs_vblank_work(crtc_state))
+		if (!intel_crtc_needs_vblank_work(state, crtc_state))
 			continue;
 
 		drm_vblank_work_flush(&crtc_state->vblank_work);
@@ -470,6 +476,7 @@  static int intel_mode_vblank_start(const struct drm_display_mode *mode)
 
 /**
  * intel_pipe_update_start() - start update of a set of display registers
+ * @state: the intel atomic state
  * @new_crtc_state: the new crtc state
  *
  * Mark the start of an update to pipe registers that should be updated
@@ -480,7 +487,8 @@  static int intel_mode_vblank_start(const struct drm_display_mode *mode)
  * until a subsequent call to intel_pipe_update_end(). That is done to
  * avoid random delays.
  */
-void intel_pipe_update_start(struct intel_crtc_state *new_crtc_state)
+void intel_pipe_update_start(struct intel_atomic_state *state,
+			     struct intel_crtc_state *new_crtc_state)
 {
 	struct intel_crtc *crtc = to_intel_crtc(new_crtc_state->uapi.crtc);
 	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
@@ -497,7 +505,7 @@  void intel_pipe_update_start(struct intel_crtc_state *new_crtc_state)
 	if (new_crtc_state->do_async_flip)
 		return;
 
-	if (intel_crtc_needs_vblank_work(new_crtc_state))
+	if (intel_crtc_needs_vblank_work(state, new_crtc_state))
 		intel_crtc_vblank_work_init(new_crtc_state);
 
 	if (new_crtc_state->vrr.enable) {
@@ -635,13 +643,15 @@  static void dbg_vblank_evade(struct intel_crtc *crtc, ktime_t end) {}
 
 /**
  * intel_pipe_update_end() - end update of a set of display registers
+ * @state: the intel atomic state
  * @new_crtc_state: the new crtc state
  *
  * Mark the end of an update started with intel_pipe_update_start(). This
  * re-enables interrupts and verifies the update was actually completed
  * before a vblank.
  */
-void intel_pipe_update_end(struct intel_crtc_state *new_crtc_state)
+void intel_pipe_update_end(struct intel_atomic_state *state,
+			   struct intel_crtc_state *new_crtc_state)
 {
 	struct intel_crtc *crtc = to_intel_crtc(new_crtc_state->uapi.crtc);
 	enum pipe pipe = crtc->pipe;
@@ -669,7 +679,7 @@  void intel_pipe_update_end(struct intel_crtc_state *new_crtc_state)
 	 * Would be slightly nice to just grab the vblank count and arm the
 	 * event outside of the critical section - the spinlock might spin for a
 	 * while ... */
-	if (intel_crtc_needs_vblank_work(new_crtc_state)) {
+	if (intel_crtc_needs_vblank_work(state, new_crtc_state)) {
 		drm_vblank_work_schedule(&new_crtc_state->vblank_work,
 					 drm_crtc_accurate_vblank_count(&crtc->base) + 1,
 					 false);
diff --git a/drivers/gpu/drm/i915/display/intel_crtc.h b/drivers/gpu/drm/i915/display/intel_crtc.h
index 51a4c8df9e65..ca7f45a454a0 100644
--- a/drivers/gpu/drm/i915/display/intel_crtc.h
+++ b/drivers/gpu/drm/i915/display/intel_crtc.h
@@ -36,8 +36,10 @@  void intel_crtc_state_reset(struct intel_crtc_state *crtc_state,
 u32 intel_crtc_get_vblank_counter(struct intel_crtc *crtc);
 void intel_crtc_vblank_on(const struct intel_crtc_state *crtc_state);
 void intel_crtc_vblank_off(const struct intel_crtc_state *crtc_state);
-void intel_pipe_update_start(struct intel_crtc_state *new_crtc_state);
-void intel_pipe_update_end(struct intel_crtc_state *new_crtc_state);
+void intel_pipe_update_start(struct intel_atomic_state *state,
+			     struct intel_crtc_state *new_crtc_state);
+void intel_pipe_update_end(struct intel_atomic_state *state,
+			   struct intel_crtc_state *new_crtc_state);
 void intel_wait_for_vblank_workers(struct intel_atomic_state *state);
 struct intel_crtc *intel_first_crtc(struct drm_i915_private *i915);
 struct intel_crtc *intel_crtc_for_pipe(struct drm_i915_private *i915,
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 45a932c9b1b3..f083ef0f53d4 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -6611,7 +6611,7 @@  static void intel_update_crtc(struct intel_atomic_state *state,
 	intel_crtc_planes_update_noarm(state, crtc);
 
 	/* Perform vblank evasion around commit operation */
-	intel_pipe_update_start(new_crtc_state);
+	intel_pipe_update_start(state, new_crtc_state);
 
 	commit_pipe_pre_planes(state, crtc);
 
@@ -6619,7 +6619,7 @@  static void intel_update_crtc(struct intel_atomic_state *state,
 
 	commit_pipe_post_planes(state, crtc);
 
-	intel_pipe_update_end(new_crtc_state);
+	intel_pipe_update_end(state, new_crtc_state);
 
 	/*
 	 * We usually enable FIFO underrun interrupts as part of the
@@ -6921,6 +6921,9 @@  static void intel_atomic_cleanup_work(struct work_struct *work)
 	struct intel_crtc *crtc;
 	int i;
 
+	if (state->base.legacy_cursor_update)
+		intel_wait_for_vblank_workers(state);
+
 	for_each_old_intel_crtc_in_state(state, crtc, old_crtc_state, i)
 		intel_color_cleanup_commit(old_crtc_state);
 
@@ -7116,7 +7119,8 @@  static void intel_atomic_commit_tail(struct intel_atomic_state *state)
 	if (state->modeset)
 		intel_set_cdclk_post_plane_update(state);
 
-	intel_wait_for_vblank_workers(state);
+	if (!state->base.legacy_cursor_update)
+		intel_wait_for_vblank_workers(state);
 
 	/* FIXME: We should call drm_atomic_helper_commit_hw_done() here
 	 * already, but still need the state for the delayed optimization. To