Re: [PATCH] drm/atomic: Fix double free in drm_atomic_state_default_clear

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux