On Tue, Jan 31, 2017 at 10:04:09AM -0200, Gustavo Padovan wrote:
> Hi Maarten,
>
> 2017-01-31 Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>:
>
> > 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@xxxxxxxxxxxxxxx> # v4.8+
> > Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
> > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> > Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxx>
Applied to drm-misc-fixes, thanks for the patch&review.
-Daneil
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
