[PATCH] drm/drm_lease: Do not call drm_master_put() twice in case drm_lease_create() fails

[Daniel Vetter]

On Wed, Dec 13, 2017 at 09:18:55AM +0000, Marius-cristian Vlad wrote:
> Well I don't have an igt test for it, but here's what happens when I try to
> create a new lease which hasn't been revoked (so, it's currently created but not revoked and
> trying to create a new one):
> 
> [  210.347052] [drm:drm_ioctl] pid=3309, dev=0xe200, auth=1, DRM_IOCTL_MODE_CREATE_LEASE
> [  210.347068] [drm:drm_mode_create_lease_ioctl] Adding object 44 to lease
> [  210.347081] [drm:drm_mode_create_lease_ioctl] Adding object 25 to lease
> [  210.347091] [drm:drm_mode_create_lease_ioctl] Adding object 26 to lease
> [  210.347100] [drm:drm_mode_object_unreference] OBJ ID: 44 (5)
> [  210.347111] [drm:drm_mode_create_lease_ioctl] Creating lease
> [  210.347120] [drm:drm_mode_create_lease_ioctl] lessor 0
> [  210.347136] [drm:drm_mode_create_lease_ioctl] object 23 failed -16
> [ nothing printed anymore ] process is stuck
> 
> Doing an echo w > /proc/sysrq-trigger shows the following:
> 
> [  267.732954] sysrq: SysRq : Show Blocked State
> [  267.737359]   task                        PC stack   pid father
> [  267.743543] weston          D    0  3309   3278 0x00000200
> [  267.749249] Call trace:
> [  267.751708] [<ffff000008085604>] __switch_to+0x8c/0xa0
> [  267.756898] [<ffff000008bcfe10>] __schedule+0x178/0x580
> [  267.762161] [<ffff000008bd0254>] schedule+0x3c/0xa8
> [  267.767079] [<ffff000008bd0650>] schedule_preempt_disabled+0x20/0x38
> [  267.773477] [<ffff000008bd1b90>] __mutex_lock_slowpath+0xc0/0x140
> [  267.779605] [<ffff000008bd1c54>] mutex_lock+0x44/0x60
> [  267.784700] [<ffff0000085d4f50>] drm_lease_destroy+0x28/0x108
> [  267.790483] [<ffff0000085b31c0>] drm_master_put+0xc0/0xc8
> [  267.795922] [<ffff0000085d54d8>] drm_mode_create_lease_ioctl+0x468/0x808
> [  267.802664] [<ffff0000085b87e0>] drm_ioctl+0x198/0x448
> [  267.807840] [<ffff0000081f067c>] do_vfs_ioctl+0xa4/0x748
> [  267.813187] [<ffff0000081f0dac>] SyS_ioctl+0x8c/0xa0
> [  267.819522] [<ffff000008082f4c>] __sys_trace_return+0x0/0x4
> 
> I was under the impression that drm_lease_destroy() gets called twice. 

That's a deadlock, not a double free. Please include crucial information
like this in your patch next time around. Enabling lockdep should help you
figure out what's going wrong here.
-Daniel
> 
> -----Original Message-----
> From: Daniel Vetter [mailto:daniel.vetter at ffwll.ch] On Behalf Of Daniel Vetter
> Sent: Wednesday, December 13, 2017 10:23 AM
> To: Marius-cristian Vlad <marius-cristian.vlad at nxp.com>
> Cc: daniel at ffwll.ch; dri-devel at lists.freedesktop.org; keithp at keithp.com; daniel.vetter at ffwll.ch
> Subject: Re: [PATCH] drm/drm_lease: Do not call drm_master_put() twice in case drm_lease_create() fails
> 
> On Tue, Dec 12, 2017 at 03:44:07PM +0000, Marius-cristian Vlad wrote:
> > drm_mode_create_lease_ioctl() -> drm_lease_create()
> > 
> > drm_lease_create() -> fails and drm_master_put() is called
> > twice: once in drm_lease_create() and once in 
> > drm_mode_create_lease_ioctl().
> > 
> > From drm_mode_create_lease_ioctl():
> > 
> > 	lessee = drm_lease_create(lessor, &leases);
> >         if (IS_ERR(lessee)) {
> >                 ret = PTR_ERR(lessee);
> >                 goto out_leases;
> >         }
> > ....
> > out_lessee:
> 
> out_lessee != out_leases
> 
> >         drm_master_put(&lessee); <- but we already done this in 
> > drm_lease_create().
> 
> This is the path I checked, looks all correct to me. Where exactly have you observed the leak? Do we have a testcase (igt very much preferred, sicne then at least the intel team will CI it constantly) that reproduces the leak?
> -Daniel
> 
> > 
> > 
> > On Tue, 2017-12-12 at 16:30 +0100, Daniel Vetter wrote:
> > > On Tue, Dec 12, 2017 at 02:04:14PM +0200, Marius Vlad wrote:
> > > > This case can been seen when creating the lease with same objects 
> > > > passed.
> > > > 
> > > > Signed-off-by: Marius Vlad <marius-cristian.vlad at nxp.com>
> > > > ---
> > > >  drivers/gpu/drm/drm_lease.c | 2 --
> > > >  1 file changed, 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/drm_lease.c 
> > > > b/drivers/gpu/drm/drm_lease.c index d1eb56a..ae57f33 100644
> > > > --- a/drivers/gpu/drm/drm_lease.c
> > > > +++ b/drivers/gpu/drm/drm_lease.c
> > > > @@ -254,8 +254,6 @@ static struct drm_master 
> > > > *drm_lease_create(struct drm_master *lessor, struct idr
> > > >  	return lessee;
> > > >  
> > > >  out_lessee:
> > > > -	drm_master_put(&lessee);
> > > 
> > > I'm not really following here ... the lessee reference we're 
> > > dropping here is created in drm_master_create. We're only calling 
> > > drm_master_put if that succeeded. Removing this line here looks like 
> > > now we're leaking.
> > > 
> > > Where is the double-free? I don't see the 2nd drm_master_put() 
> > > anywhere ... drm_mode_create_lease_ioctl also seems to be doing the 
> > > right thing from just staring at it.
> > > -Daniel
> 
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fblog.ffwll.ch&data=02%7C01%7Cmarius-cristian.vlad%40nxp.com%7C3f53f9f6b4f3453595c808d54202c161%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636487501964257048&sdata=VE9ojrJ0Hja1wVuY%2FmN%2FeDGXT5pljXJK7bCKSCzf87E%3D&reserved=0

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch