| Message ID | 1485854725-27640-1-git-send-email-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Maarten, 2017-01-31 Maarten Lankhorst <maarten.lankhorst@linux.intel.com>: > drm_atomic_helper_page_flip and drm_atomic_ioctl set their own events > in crtc_state->event. But when it's set the event is freed in 2 places. > > Solve this by only freeing the event in the atomic ioctl when it > allocated its own event. > > This has been broken twice. The first time when the code was introduced, > but only in the corner case when an event is allocated, but more crtc's > were included by atomic check and then failing. This can mostly > happen when you do an atomic modeset in i915 and the display clock is > changed, which forces all crtc's to be included to the state. > > This has been broken worse by adding in-fences support, which caused > the double free to be done unconditionally. > > [IGT] kms_rotation_crc: starting subtest primary-rotation-180 > ============================================================================= > BUG kmalloc-128 (Tainted: G U ): Object already free > ----------------------------------------------------------------------------- > > Disabling lock debugging due to kernel taint > INFO: Allocated in drm_atomic_helper_setup_commit+0x285/0x2f0 [drm_kms_helper] age=0 cpu=3 pid=1529 > ___slab_alloc+0x308/0x3b0 > __slab_alloc+0xd/0x20 > kmem_cache_alloc_trace+0x92/0x1c0 > drm_atomic_helper_setup_commit+0x285/0x2f0 [drm_kms_helper] > intel_atomic_commit+0x35/0x4f0 [i915] > drm_atomic_commit+0x46/0x50 [drm] > drm_mode_atomic_ioctl+0x7d4/0xab0 [drm] > drm_ioctl+0x2b3/0x490 [drm] > do_vfs_ioctl+0x69c/0x700 > SyS_ioctl+0x4e/0x80 > entry_SYSCALL_64_fastpath+0x13/0x94 > INFO: Freed in drm_event_cancel_free+0xa3/0xb0 [drm] age=0 cpu=3 pid=1529 > __slab_free+0x48/0x2e0 > kfree+0x159/0x1a0 > drm_event_cancel_free+0xa3/0xb0 [drm] > drm_mode_atomic_ioctl+0x86d/0xab0 [drm] > drm_ioctl+0x2b3/0x490 [drm] > do_vfs_ioctl+0x69c/0x700 > SyS_ioctl+0x4e/0x80 > entry_SYSCALL_64_fastpath+0x13/0x94 > INFO: Slab 0xffffde1f0997b080 objects=17 used=2 fp=0xffff92fb65ec2578 flags=0x200000000008101 > INFO: Object 0xffff92fb65ec2578 @offset=1400 fp=0xffff92fb65ec2ae8 > > Redzone ffff92fb65ec2570: bb bb bb bb bb bb bb bb ........ > Object ffff92fb65ec2578: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b kkkkkkkkkkkkkkkk > Object ffff92fb65ec2588: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b kkkkkkkkkkkkkkkk > Object ffff92fb65ec2598: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b kkkkkkkkkkkkkkkk > Object ffff92fb65ec25a8: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b kkkkkkkkkkkkkkkk > Object ffff92fb65ec25b8: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b kkkkkkkkkkkkkkkk > Object ffff92fb65ec25c8: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b kkkkkkkkkkkkkkkk > Object ffff92fb65ec25d8: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b kkkkkkkkkkkkkkkk > Object ffff92fb65ec25e8: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b a5 kkkkkkkkkkkkkkk. > Redzone ffff92fb65ec25f8: bb bb bb bb bb bb bb bb ........ > Padding ffff92fb65ec2738: 5a 5a 5a 5a 5a 5a 5a 5a ZZZZZZZZ > CPU: 3 PID: 180 Comm: kworker/3:2 Tainted: G BU 4.10.0-rc6-patser+ #5039 > Hardware name: /NUC5PPYB, BIOS PYBSWCEL.86A.0031.2015.0601.1712 06/01/2015 > Workqueue: events intel_atomic_helper_free_state [i915] > Call Trace: > dump_stack+0x4d/0x6d > print_trailer+0x20c/0x220 > free_debug_processing+0x1c6/0x330 > ? drm_atomic_state_default_clear+0xf7/0x1c0 [drm] > __slab_free+0x48/0x2e0 > ? drm_atomic_state_default_clear+0xf7/0x1c0 [drm] > kfree+0x159/0x1a0 > drm_atomic_state_default_clear+0xf7/0x1c0 [drm] > ? drm_atomic_state_clear+0x30/0x30 [drm] > intel_atomic_state_clear+0xd/0x20 [i915] > drm_atomic_state_clear+0x1a/0x30 [drm] > __drm_atomic_state_free+0x13/0x60 [drm] > intel_atomic_helper_free_state+0x5d/0x70 [i915] > process_one_work+0x260/0x4a0 > worker_thread+0x2d1/0x4f0 > kthread+0x127/0x130 > ? process_one_work+0x4a0/0x4a0 > ? kthread_stop+0x120/0x120 > ret_from_fork+0x29/0x40 > FIX kmalloc-128: Object at 0xffff92fb65ec2578 not freed > > Fixes: 3b24f7d67581 ("drm/atomic: Add struct drm_crtc_commit to track async updates") > Fixes: 9626014258a5 ("drm/fence: add in-fences support") > Cc: <stable@vger.kernel.org> # v4.8+ > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > drivers/gpu/drm/drm_atomic.c | 13 ++++++++----- > 1 file changed, 8 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > index 50f5cf7b69d1..fdfb1ec17e66 100644 > --- a/drivers/gpu/drm/drm_atomic.c > +++ b/drivers/gpu/drm/drm_atomic.c > @@ -2032,13 +2032,16 @@ static void complete_crtc_signaling(struct drm_device *dev, > } > > for_each_crtc_in_state(state, crtc, crtc_state, i) { > + struct drm_pending_vblank_event *event = crtc_state->event; > /* > - * TEST_ONLY and PAGE_FLIP_EVENT are mutually > - * exclusive, if they weren't, this code should be > - * called on success for TEST_ONLY too. > + * Free the allocated event. drm_atomic_helper_setup_commit > + * can allocate an event too, so only free it if it's ours > + * to prevent a double free in drm_atomic_state_clear. > */ > - if (crtc_state->event) > - drm_event_cancel_free(dev, &crtc_state->event->base); > + if (event && (event->base.fence || event->base.file_priv)) { > + drm_event_cancel_free(dev, &event->base); > + crtc_state->event = NULL; > + } This makes sense to me: Reviewed-by: Gustavo Padovan <gustavo.padovan@collabora.com> Gustavo
On Tue, Jan 31, 2017 at 10:04:09AM -0200, Gustavo Padovan wrote: > Hi Maarten, > > 2017-01-31 Maarten Lankhorst <maarten.lankhorst@linux.intel.com>: > > > drm_atomic_helper_page_flip and drm_atomic_ioctl set their own events > > in crtc_state->event. But when it's set the event is freed in 2 places. > > > > Solve this by only freeing the event in the atomic ioctl when it > > allocated its own event. > > > > This has been broken twice. The first time when the code was introduced, > > but only in the corner case when an event is allocated, but more crtc's > > were included by atomic check and then failing. This can mostly > > happen when you do an atomic modeset in i915 and the display clock is > > changed, which forces all crtc's to be included to the state. > > > > This has been broken worse by adding in-fences support, which caused > > the double free to be done unconditionally. > > > > [IGT] kms_rotation_crc: starting subtest primary-rotation-180 > > ============================================================================= > > BUG kmalloc-128 (Tainted: G U ): Object already free > > ----------------------------------------------------------------------------- > > > > Disabling lock debugging due to kernel taint > > INFO: Allocated in drm_atomic_helper_setup_commit+0x285/0x2f0 [drm_kms_helper] age=0 cpu=3 pid=1529 > > ___slab_alloc+0x308/0x3b0 > > __slab_alloc+0xd/0x20 > > kmem_cache_alloc_trace+0x92/0x1c0 > > drm_atomic_helper_setup_commit+0x285/0x2f0 [drm_kms_helper] > > intel_atomic_commit+0x35/0x4f0 [i915] > > drm_atomic_commit+0x46/0x50 [drm] > > drm_mode_atomic_ioctl+0x7d4/0xab0 [drm] > > drm_ioctl+0x2b3/0x490 [drm] > > do_vfs_ioctl+0x69c/0x700 > > SyS_ioctl+0x4e/0x80 > > entry_SYSCALL_64_fastpath+0x13/0x94 > > INFO: Freed in drm_event_cancel_free+0xa3/0xb0 [drm] age=0 cpu=3 pid=1529 > > __slab_free+0x48/0x2e0 > > kfree+0x159/0x1a0 > > drm_event_cancel_free+0xa3/0xb0 [drm] > > drm_mode_atomic_ioctl+0x86d/0xab0 [drm] > > drm_ioctl+0x2b3/0x490 [drm] > > do_vfs_ioctl+0x69c/0x700 > > SyS_ioctl+0x4e/0x80 > > entry_SYSCALL_64_fastpath+0x13/0x94 > > INFO: Slab 0xffffde1f0997b080 objects=17 used=2 fp=0xffff92fb65ec2578 flags=0x200000000008101 > > INFO: Object 0xffff92fb65ec2578 @offset=1400 fp=0xffff92fb65ec2ae8 > > > > Redzone ffff92fb65ec2570: bb bb bb bb bb bb bb bb ........ > > Object ffff92fb65ec2578: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b kkkkkkkkkkkkkkkk > > Object ffff92fb65ec2588: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b kkkkkkkkkkkkkkkk > > Object ffff92fb65ec2598: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b kkkkkkkkkkkkkkkk > > Object ffff92fb65ec25a8: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b kkkkkkkkkkkkkkkk > > Object ffff92fb65ec25b8: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b kkkkkkkkkkkkkkkk > > Object ffff92fb65ec25c8: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b kkkkkkkkkkkkkkkk > > Object ffff92fb65ec25d8: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b kkkkkkkkkkkkkkkk > > Object ffff92fb65ec25e8: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b a5 kkkkkkkkkkkkkkk. > > Redzone ffff92fb65ec25f8: bb bb bb bb bb bb bb bb ........ > > Padding ffff92fb65ec2738: 5a 5a 5a 5a 5a 5a 5a 5a ZZZZZZZZ > > CPU: 3 PID: 180 Comm: kworker/3:2 Tainted: G BU 4.10.0-rc6-patser+ #5039 > > Hardware name: /NUC5PPYB, BIOS PYBSWCEL.86A.0031.2015.0601.1712 06/01/2015 > > Workqueue: events intel_atomic_helper_free_state [i915] > > Call Trace: > > dump_stack+0x4d/0x6d > > print_trailer+0x20c/0x220 > > free_debug_processing+0x1c6/0x330 > > ? drm_atomic_state_default_clear+0xf7/0x1c0 [drm] > > __slab_free+0x48/0x2e0 > > ? drm_atomic_state_default_clear+0xf7/0x1c0 [drm] > > kfree+0x159/0x1a0 > > drm_atomic_state_default_clear+0xf7/0x1c0 [drm] > > ? drm_atomic_state_clear+0x30/0x30 [drm] > > intel_atomic_state_clear+0xd/0x20 [i915] > > drm_atomic_state_clear+0x1a/0x30 [drm] > > __drm_atomic_state_free+0x13/0x60 [drm] > > intel_atomic_helper_free_state+0x5d/0x70 [i915] > > process_one_work+0x260/0x4a0 > > worker_thread+0x2d1/0x4f0 > > kthread+0x127/0x130 > > ? process_one_work+0x4a0/0x4a0 > > ? kthread_stop+0x120/0x120 > > ret_from_fork+0x29/0x40 > > FIX kmalloc-128: Object at 0xffff92fb65ec2578 not freed > > > > Fixes: 3b24f7d67581 ("drm/atomic: Add struct drm_crtc_commit to track async updates") > > Fixes: 9626014258a5 ("drm/fence: add in-fences support") > > Cc: <stable@vger.kernel.org> # v4.8+ > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > --- > > drivers/gpu/drm/drm_atomic.c | 13 ++++++++----- > > 1 file changed, 8 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > > index 50f5cf7b69d1..fdfb1ec17e66 100644 > > --- a/drivers/gpu/drm/drm_atomic.c > > +++ b/drivers/gpu/drm/drm_atomic.c > > @@ -2032,13 +2032,16 @@ static void complete_crtc_signaling(struct drm_device *dev, > > } > > > > for_each_crtc_in_state(state, crtc, crtc_state, i) { > > + struct drm_pending_vblank_event *event = crtc_state->event; > > /* > > - * TEST_ONLY and PAGE_FLIP_EVENT are mutually > > - * exclusive, if they weren't, this code should be > > - * called on success for TEST_ONLY too. > > + * Free the allocated event. drm_atomic_helper_setup_commit > > + * can allocate an event too, so only free it if it's ours > > + * to prevent a double free in drm_atomic_state_clear. > > */ > > - if (crtc_state->event) > > - drm_event_cancel_free(dev, &crtc_state->event->base); > > + if (event && (event->base.fence || event->base.file_priv)) { > > + drm_event_cancel_free(dev, &event->base); > > + crtc_state->event = NULL; > > + } > > This makes sense to me: > > Reviewed-by: Gustavo Padovan <gustavo.padovan@collabora.com> Applied to drm-misc-fixes, thanks for the patch&review. -Daneil
============================================================================= BUG kmalloc-128 (Tainted: G U ): Object already free ----------------------------------------------------------------------------- Disabling lock debugging due to kernel taint INFO: Allocated in drm_atomic_helper_setup_commit+0x285/0x2f0 [drm_kms_helper] age=0 cpu=3 pid=1529 ___slab_alloc+0x308/0x3b0 __slab_alloc+0xd/0x20 kmem_cache_alloc_trace+0x92/0x1c0 drm_atomic_helper_setup_commit+0x285/0x2f0 [drm_kms_helper] intel_atomic_commit+0x35/0x4f0 [i915] drm_atomic_commit+0x46/0x50 [drm] drm_mode_atomic_ioctl+0x7d4/0xab0 [drm] drm_ioctl+0x2b3/0x490 [drm] do_vfs_ioctl+0x69c/0x700 SyS_ioctl+0x4e/0x80 entry_SYSCALL_64_fastpath+0x13/0x94 INFO: Freed in drm_event_cancel_free+0xa3/0xb0 [drm] age=0 cpu=3 pid=1529 __slab_free+0x48/0x2e0 kfree+0x159/0x1a0 drm_event_cancel_free+0xa3/0xb0 [drm] drm_mode_atomic_ioctl+0x86d/0xab0 [drm] drm_ioctl+0x2b3/0x490 [drm] do_vfs_ioctl+0x69c/0x700 SyS_ioctl+0x4e/0x80 entry_SYSCALL_64_fastpath+0x13/0x94 INFO: Slab 0xffffde1f0997b080 objects=17 used=2 fp=0xffff92fb65ec2578 flags=0x200000000008101 INFO: Object 0xffff92fb65ec2578 @offset=1400 fp=0xffff92fb65ec2ae8 Redzone ffff92fb65ec2570: bb bb bb bb bb bb bb bb ........ Object ffff92fb65ec2578: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b kkkkkkkkkkkkkkkk Object ffff92fb65ec2588: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b kkkkkkkkkkkkkkkk Object ffff92fb65ec2598: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b kkkkkkkkkkkkkkkk Object ffff92fb65ec25a8: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b kkkkkkkkkkkkkkkk Object ffff92fb65ec25b8: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b kkkkkkkkkkkkkkkk Object ffff92fb65ec25c8: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b kkkkkkkkkkkkkkkk Object ffff92fb65ec25d8: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b kkkkkkkkkkkkkkkk Object ffff92fb65ec25e8: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b a5 kkkkkkkkkkkkkkk. Redzone ffff92fb65ec25f8: bb bb bb bb bb bb bb bb ........ Padding ffff92fb65ec2738: 5a 5a 5a 5a 5a 5a 5a 5a ZZZZZZZZ CPU: 3 PID: 180 Comm: kworker/3:2 Tainted: G BU 4.10.0-rc6-patser+ #5039 Hardware name: /NUC5PPYB, BIOS PYBSWCEL.86A.0031.2015.0601.1712 06/01/2015 Workqueue: events intel_atomic_helper_free_state [i915] Call Trace: dump_stack+0x4d/0x6d print_trailer+0x20c/0x220 free_debug_processing+0x1c6/0x330 ? drm_atomic_state_default_clear+0xf7/0x1c0 [drm] __slab_free+0x48/0x2e0 ? drm_atomic_state_default_clear+0xf7/0x1c0 [drm] kfree+0x159/0x1a0 drm_atomic_state_default_clear+0xf7/0x1c0 [drm] ? drm_atomic_state_clear+0x30/0x30 [drm] intel_atomic_state_clear+0xd/0x20 [i915] drm_atomic_state_clear+0x1a/0x30 [drm] __drm_atomic_state_free+0x13/0x60 [drm] intel_atomic_helper_free_state+0x5d/0x70 [i915] process_one_work+0x260/0x4a0 worker_thread+0x2d1/0x4f0 kthread+0x127/0x130 ? process_one_work+0x4a0/0x4a0 ? kthread_stop+0x120/0x120 ret_from_fork+0x29/0x40 FIX kmalloc-128: Object at 0xffff92fb65ec2578 not freed Fixes: 3b24f7d67581 ("drm/atomic: Add struct drm_crtc_commit to track async updates") Fixes: 9626014258a5 ("drm/fence: add in-fences support") Cc: <stable@vger.kernel.org> # v4.8+ Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/gpu/drm/drm_atomic.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 50f5cf7b69d1..fdfb1ec17e66 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -2032,13 +2032,16 @@ static void complete_crtc_signaling(struct drm_device *dev, } for_each_crtc_in_state(state, crtc, crtc_state, i) { + struct drm_pending_vblank_event *event = crtc_state->event; /* - * TEST_ONLY and PAGE_FLIP_EVENT are mutually - * exclusive, if they weren't, this code should be - * called on success for TEST_ONLY too. + * Free the allocated event. drm_atomic_helper_setup_commit + * can allocate an event too, so only free it if it's ours + * to prevent a double free in drm_atomic_state_clear. */ - if (crtc_state->event) - drm_event_cancel_free(dev, &crtc_state->event->base); + if (event && (event->base.fence || event->base.file_priv)) { + drm_event_cancel_free(dev, &event->base); + crtc_state->event = NULL; + } } if (!fence_state)