* [PATCH] drm/drm_lease: Do not call drm_master_put() twice in case drm_lease_create() fails
@ 2017-12-12 12:04 Marius Vlad
2017-12-12 15:30 ` Daniel Vetter
2017-12-13 18:10 ` [PATCH v2] drm/drm_lease: Prevent deadlock " Marius Vlad
0 siblings, 2 replies; 9+ messages in thread
From: Marius Vlad @ 2017-12-12 12:04 UTC (permalink / raw)
To: dri-devel; +Cc: daniel.vetter, keithp
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/drm_lease: Do not call drm_master_put() twice in case drm_lease_create() fails
2017-12-12 12:04 [PATCH] drm/drm_lease: Do not call drm_master_put() twice in case drm_lease_create() fails Marius Vlad
@ 2017-12-12 15:30 ` Daniel Vetter
2017-12-12 15:44 ` Marius-cristian Vlad
2017-12-13 18:10 ` [PATCH v2] drm/drm_lease: Prevent deadlock " Marius Vlad
1 sibling, 1 reply; 9+ messages in thread
From: Daniel Vetter @ 2017-12-12 15:30 UTC (permalink / raw)
To: Marius Vlad; +Cc: daniel.vetter, keithp, dri-devel
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/drm_lease: Do not call drm_master_put() twice in case drm_lease_create() fails
2017-12-12 15:30 ` Daniel Vetter
@ 2017-12-12 15:44 ` Marius-cristian Vlad
2017-12-13 8:23 ` Daniel Vetter
0 siblings, 1 reply; 9+ messages in thread
From: Marius-cristian Vlad @ 2017-12-12 15:44 UTC (permalink / raw)
To: daniel; +Cc: daniel.vetter, keithp, dri-devel
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/drm_lease: Do not call drm_master_put() twice in case drm_lease_create() fails
2017-12-12 15:44 ` Marius-cristian Vlad
@ 2017-12-13 8:23 ` Daniel Vetter
2017-12-13 9:18 ` Marius-cristian Vlad
0 siblings, 1 reply; 9+ messages in thread
From: Daniel Vetter @ 2017-12-13 8:23 UTC (permalink / raw)
To: Marius-cristian Vlad; +Cc: daniel.vetter, keithp, dri-devel
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH] drm/drm_lease: Do not call drm_master_put() twice in case drm_lease_create() fails
2017-12-13 8:23 ` Daniel Vetter
@ 2017-12-13 9:18 ` Marius-cristian Vlad
2017-12-13 10:44 ` Daniel Vetter
0 siblings, 1 reply; 9+ messages in thread
From: Marius-cristian Vlad @ 2017-12-13 9:18 UTC (permalink / raw)
To: Daniel Vetter; +Cc: daniel.vetter, keithp, dri-devel
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/drm_lease: Do not call drm_master_put() twice in case drm_lease_create() fails
2017-12-13 9:18 ` Marius-cristian Vlad
@ 2017-12-13 10:44 ` Daniel Vetter
0 siblings, 0 replies; 9+ messages in thread
From: Daniel Vetter @ 2017-12-13 10:44 UTC (permalink / raw)
To: Marius-cristian Vlad; +Cc: daniel.vetter, keithp, dri-devel
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2] drm/drm_lease: Prevent deadlock in case drm_lease_create() fails
2017-12-12 12:04 [PATCH] drm/drm_lease: Do not call drm_master_put() twice in case drm_lease_create() fails Marius Vlad
2017-12-12 15:30 ` Daniel Vetter
@ 2017-12-13 18:10 ` Marius Vlad
2017-12-14 7:25 ` Daniel Vetter
2017-12-14 7:29 ` Daniel Vetter
1 sibling, 2 replies; 9+ messages in thread
From: Marius Vlad @ 2017-12-13 18:10 UTC (permalink / raw)
To: dri-devel; +Cc: daniel.vetter, keithp
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2] drm/drm_lease: Prevent deadlock in case drm_lease_create() fails
2017-12-13 18:10 ` [PATCH v2] drm/drm_lease: Prevent deadlock " Marius Vlad
@ 2017-12-14 7:25 ` Daniel Vetter
2017-12-14 7:29 ` Daniel Vetter
1 sibling, 0 replies; 9+ messages in thread
From: Daniel Vetter @ 2017-12-14 7:25 UTC (permalink / raw)
To: Marius Vlad; +Cc: daniel.vetter, keithp, dri-devel
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] drm/drm_lease: Prevent deadlock in case drm_lease_create() fails
2017-12-13 18:10 ` [PATCH v2] drm/drm_lease: Prevent deadlock " Marius Vlad
2017-12-14 7:25 ` Daniel Vetter
@ 2017-12-14 7:29 ` Daniel Vetter
1 sibling, 0 replies; 9+ messages in thread
From: Daniel Vetter @ 2017-12-14 7:29 UTC (permalink / raw)
To: Marius Vlad, Dave Airlie; +Cc: daniel.vetter, keithp, dri-devel
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
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2017-12-14 7:29 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-12-12 12:04 [PATCH] drm/drm_lease: Do not call drm_master_put() twice in case drm_lease_create() fails Marius Vlad
2017-12-12 15:30 ` Daniel Vetter
2017-12-12 15:44 ` Marius-cristian Vlad
2017-12-13 8:23 ` Daniel Vetter
2017-12-13 9:18 ` Marius-cristian Vlad
2017-12-13 10:44 ` Daniel Vetter
2017-12-13 18:10 ` [PATCH v2] drm/drm_lease: Prevent deadlock " Marius Vlad
2017-12-14 7:25 ` Daniel Vetter
2017-12-14 7:29 ` Daniel Vetter
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.