[Intel-gfx] [PATCH] drm/atomic: Fix double free in drm_atomic_state_default_clear

[Gustavo Padovan]

Hi Maarten,

2017-01-31 Maarten Lankhorst <maarten.lankhorst at 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 at vger.kernel.org> # v4.8+
> Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> Reviewed-by: Daniel Vetter <daniel.vetter at 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 at collabora.com>

Gustavo