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

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

[Marius Vlad]

This case can been seen when creating the lease with same objects passed.

Signed-off-by: Marius Vlad <marius-cristian.vlad@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);
-
 	mutex_unlock(&dev->mode_config.idr_mutex);
 
 	return ERR_PTR(error);
-- 
2.9.3

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Daniel Vetter]

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@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
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Marius-cristian Vlad]

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:
        drm_master_put(&lessee); <- but we already done this in
drm_lease_create().


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@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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Daniel Vetter]

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@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
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Marius-cristian Vlad]

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. 

-----Original Message-----
From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter
Sent: Wednesday, December 13, 2017 10:23 AM
To: Marius-cristian Vlad <marius-cristian.vlad@nxp.com>
Cc: daniel@ffwll.ch; dri-devel@lists.freedesktop.org; keithp@keithp.com; daniel.vetter@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@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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [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@ffwll.ch] On Behalf Of Daniel Vetter
> Sent: Wednesday, December 13, 2017 10:23 AM
> To: Marius-cristian Vlad <marius-cristian.vlad@nxp.com>
> Cc: daniel@ffwll.ch; dri-devel@lists.freedesktop.org; keithp@keithp.com; daniel.vetter@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@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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2] drm/drm_lease: Prevent deadlock in case drm_lease_create() fails

[Marius Vlad]

This case can been seen when creating the lease with the same objects passed.

[  605.515097] 2 locks held by testapp/3337:
[  605.519027]  #0:  (&dev->mode_config.idr_mutex){......}, at: [<ffff0000085f1664>] drm_mode_create_lease_ioctl+0x384/0x858
[  605.530045]  #1:  (&dev->mode_config.idr_mutex){......}, at: [<ffff0000085f11bc>] drm_lease_destroy+0x2c/0x110

Which was causing the process to hang:

[  605.398827] [<ffff0000080856cc>] __switch_to+0x94/0xa8
[  605.404030] [<ffff000008c05d00>] __schedule+0x1b0/0x698
[  605.409322] [<ffff000008c06224>] schedule+0x3c/0xa8
[  605.414260] [<ffff000008c06628>] schedule_preempt_disabled+0x20/0x38
[  605.420677] [<ffff000008c07370>] mutex_lock_nested+0x158/0x340
[  605.426572] [<ffff0000085f11bc>] drm_lease_destroy+0x2c/0x110
[  605.432389] [<ffff0000085cecf0>] drm_master_put+0xc0/0xc8
[  605.437845] [<ffff0000085f175c>] drm_mode_create_lease_ioctl+0x47c/0x858
[  605.444612] [<ffff0000085d4460>] drm_ioctl+0x198/0x448
[  605.449811] [<ffff000008201134>] do_vfs_ioctl+0xa4/0x748
[  605.455192] [<ffff000008201864>] SyS_ioctl+0x8c/0xa0
[  605.460216] [<ffff000008082f4c>] __sys_trace_return+0x0/0x4

drm_mode_create_lease_ioctl() calls drm_lease_create() which acquires a lock
on dev->mode_config.idr_mutex. In case of failure, drm_lease_create() calls 
drm_master_put() which in turn tries to acquire the same lock when calling
drm_lease_destroy(). 

v2: - Reverse the order at exit in case of fail, so that unlocking takes place
before dropping the reference.
    - Include detail information about deadlock (Daniel Vetter)


Signed-off-by: Marius Vlad <marius-cristian.vlad@nxp.com>
---
 drivers/gpu/drm/drm_lease.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_lease.c b/drivers/gpu/drm/drm_lease.c
index d1eb56a..59849f0 100644
--- a/drivers/gpu/drm/drm_lease.c
+++ b/drivers/gpu/drm/drm_lease.c
@@ -254,10 +254,10 @@ static struct drm_master *drm_lease_create(struct drm_master *lessor, struct idr
 	return lessee;
 
 out_lessee:
-	drm_master_put(&lessee);
-
 	mutex_unlock(&dev->mode_config.idr_mutex);
 
+	drm_master_put(&lessee);
+
 	return ERR_PTR(error);
 }
 
-- 
2.9.3

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] drm/drm_lease: Prevent deadlock in case drm_lease_create() fails

[Daniel Vetter]

On Wed, Dec 13, 2017 at 08:10:48PM +0200, Marius Vlad wrote:
> This case can been seen when creating the lease with the same objects passed.
> 
> [  605.515097] 2 locks held by testapp/3337:
> [  605.519027]  #0:  (&dev->mode_config.idr_mutex){......}, at: [<ffff0000085f1664>] drm_mode_create_lease_ioctl+0x384/0x858
> [  605.530045]  #1:  (&dev->mode_config.idr_mutex){......}, at: [<ffff0000085f11bc>] drm_lease_destroy+0x2c/0x110
> 
> Which was causing the process to hang:
> 
> [  605.398827] [<ffff0000080856cc>] __switch_to+0x94/0xa8
> [  605.404030] [<ffff000008c05d00>] __schedule+0x1b0/0x698
> [  605.409322] [<ffff000008c06224>] schedule+0x3c/0xa8
> [  605.414260] [<ffff000008c06628>] schedule_preempt_disabled+0x20/0x38
> [  605.420677] [<ffff000008c07370>] mutex_lock_nested+0x158/0x340
> [  605.426572] [<ffff0000085f11bc>] drm_lease_destroy+0x2c/0x110
> [  605.432389] [<ffff0000085cecf0>] drm_master_put+0xc0/0xc8
> [  605.437845] [<ffff0000085f175c>] drm_mode_create_lease_ioctl+0x47c/0x858
> [  605.444612] [<ffff0000085d4460>] drm_ioctl+0x198/0x448
> [  605.449811] [<ffff000008201134>] do_vfs_ioctl+0xa4/0x748
> [  605.455192] [<ffff000008201864>] SyS_ioctl+0x8c/0xa0
> [  605.460216] [<ffff000008082f4c>] __sys_trace_return+0x0/0x4
> 
> drm_mode_create_lease_ioctl() calls drm_lease_create() which acquires a lock
> on dev->mode_config.idr_mutex. In case of failure, drm_lease_create() calls 
> drm_master_put() which in turn tries to acquire the same lock when calling
> drm_lease_destroy(). 
> 
> v2: - Reverse the order at exit in case of fail, so that unlocking takes place
> before dropping the reference.
>     - Include detail information about deadlock (Daniel Vetter)
> 
> 
> Signed-off-by: Marius Vlad <marius-cristian.vlad@nxp.com>

Yeah that makes a lot more sense. Applied to -fixes, thanks.
-Daniel

> ---
>  drivers/gpu/drm/drm_lease.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_lease.c b/drivers/gpu/drm/drm_lease.c
> index d1eb56a..59849f0 100644
> --- a/drivers/gpu/drm/drm_lease.c
> +++ b/drivers/gpu/drm/drm_lease.c
> @@ -254,10 +254,10 @@ static struct drm_master *drm_lease_create(struct drm_master *lessor, struct idr
>  	return lessee;
>  
>  out_lessee:
> -	drm_master_put(&lessee);
> -
>  	mutex_unlock(&dev->mode_config.idr_mutex);
>  
> +	drm_master_put(&lessee);
> +
>  	return ERR_PTR(error);
>  }
>  
> -- 
> 2.9.3
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] drm/drm_lease: Prevent deadlock in case drm_lease_create() fails

[Daniel Vetter]

On Wed, Dec 13, 2017 at 08:10:48PM +0200, Marius Vlad wrote:
> This case can been seen when creating the lease with the same objects passed.
> 
> [  605.515097] 2 locks held by testapp/3337:
> [  605.519027]  #0:  (&dev->mode_config.idr_mutex){......}, at: [<ffff0000085f1664>] drm_mode_create_lease_ioctl+0x384/0x858
> [  605.530045]  #1:  (&dev->mode_config.idr_mutex){......}, at: [<ffff0000085f11bc>] drm_lease_destroy+0x2c/0x110
> 
> Which was causing the process to hang:
> 
> [  605.398827] [<ffff0000080856cc>] __switch_to+0x94/0xa8
> [  605.404030] [<ffff000008c05d00>] __schedule+0x1b0/0x698
> [  605.409322] [<ffff000008c06224>] schedule+0x3c/0xa8
> [  605.414260] [<ffff000008c06628>] schedule_preempt_disabled+0x20/0x38
> [  605.420677] [<ffff000008c07370>] mutex_lock_nested+0x158/0x340
> [  605.426572] [<ffff0000085f11bc>] drm_lease_destroy+0x2c/0x110
> [  605.432389] [<ffff0000085cecf0>] drm_master_put+0xc0/0xc8
> [  605.437845] [<ffff0000085f175c>] drm_mode_create_lease_ioctl+0x47c/0x858
> [  605.444612] [<ffff0000085d4460>] drm_ioctl+0x198/0x448
> [  605.449811] [<ffff000008201134>] do_vfs_ioctl+0xa4/0x748
> [  605.455192] [<ffff000008201864>] SyS_ioctl+0x8c/0xa0
> [  605.460216] [<ffff000008082f4c>] __sys_trace_return+0x0/0x4
> 
> drm_mode_create_lease_ioctl() calls drm_lease_create() which acquires a lock
> on dev->mode_config.idr_mutex. In case of failure, drm_lease_create() calls 
> drm_master_put() which in turn tries to acquire the same lock when calling
> drm_lease_destroy(). 
> 
> v2: - Reverse the order at exit in case of fail, so that unlocking takes place
> before dropping the reference.
>     - Include detail information about deadlock (Daniel Vetter)
> 
> 
> Signed-off-by: Marius Vlad <marius-cristian.vlad@nxp.com>

An igt testcase for this case specifically would also be great, but I just
noticed that Dave Airlie hasn't pushed the basic tests yet. I'll poke him
about that when he's back.
-Daniel

> ---
>  drivers/gpu/drm/drm_lease.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_lease.c b/drivers/gpu/drm/drm_lease.c
> index d1eb56a..59849f0 100644
> --- a/drivers/gpu/drm/drm_lease.c
> +++ b/drivers/gpu/drm/drm_lease.c
> @@ -254,10 +254,10 @@ static struct drm_master *drm_lease_create(struct drm_master *lessor, struct idr
>  	return lessee;
>  
>  out_lessee:
> -	drm_master_put(&lessee);
> -
>  	mutex_unlock(&dev->mode_config.idr_mutex);
>  
> +	drm_master_put(&lessee);
> +
>  	return ERR_PTR(error);
>  }
>  
> -- 
> 2.9.3
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel