| Message ID | 20171212120414.8675-1-marius-cristian.vlad@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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
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
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);
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(-)