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>
Gustavo
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
