Re: [PATCH v2 1/2] drm/crtc: fix connector reference counting mismatch in drm_crtc_helper_set_config

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

 



On Tue, Jun 14, 2016 at 06:08:35PM +0200, Daniel Vetter wrote:
> On Thu, Jun 02, 2016 at 07:27:51PM +0200, Philipp Zabel wrote:
> > Since commit 0955c1250e96 ("drm/crtc: take references to connectors used
> > in a modeset. (v2)"), the reference counts of all connectors in the
> > drm_mode_set given to drm_crtc_helper_set_config are incremented, and then
> > the reference counts of all connectors are decremented on success, but in a
> > temporary copy of the connector structure. This leads to the following
> > error after the first modeset on imx-drm:
> > 
> >     Unable to handle kernel NULL pointer dereference at virtual address 00000004
> >     pgd = ad8c4000
> >     [00000004] *pgd=3d9c5831, *pte=00000000, *ppte=00000000
> >     Internal error: Oops: 817 [#1] PREEMPT SMP ARM
> >     Modules linked in:
> >     CPU: 1 PID: 190 Comm: kmsfb-manage Not tainted 4.7.0-rc1+ #657
> >     Hardware name: Freescale i.MX6 Quad/DualLit: [<80506098>]    lr : [<80252e94>]    psr: 200c0013
> >     sp : adca7ca8  ip : adca7b90  fp : adca7cd4
> >     r10: 00000000  r9 : 00000100  r8 : 00000200
> >     r7 : af3c9800  r6 : aded7848  r5 : aded7800  r4 : 00000000
> >     r3 : af3ca058  r2 : 00000200  r1 : af3ca058  r0 : 00000000
> >     Flags: nzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
> >     Control: 10c5387d  Table: 3d8c404a  DAC: 00000051
> >     Process kmsfb-manage (pid: 190, stack limit = 0xadca6210)
> >     Stack: (0xadca7ca8 to 0xadca8000)
> >     7ca0:                   805190e0 aded7800 aded7820 80501a88 8155a290 af3c9c6c
> >     7cc0: adca7ddc 0000000f adca7cec adca7cd8 80519104 80506044 805190e0 aded7800
> >     7ce0: adca7d04 adca7cf0 80501ac0 805190ec aded7820 aded7814 adca7d24 adca7d08
> >     7d00: 804fdb80 80501a94 aded7800 af3ca010 aded7afc af3c9c60 adca7d94 adca7d28
> >     7d20: 804e3518 804fdb20 00000000 af3c9b1c adca7d50 81506f44 00000000 8093c500
> >     7d40: af3c9c6c ae4f2ca8 ae4f2c18 00000000 00000000 ae637f00 00000000 aded7800
> >     7d60: 00000001 af3c9800 af23c300 ae77fcc0 ae4f2c18 00000001 af3c9800 8155a290
> >     7d80: af1af700 adca6000 adca7db4 adca7d98 804fea6c 804e2de4 adca7e50 adb3d940
> >     7da0: 00000001 af3c9800 adca7e24 adca7db8 8050440c 804fea0c ae77fcc0 00000003
> >     7dc0: adca7e24 adb3d940 af1af700 ae77fcc0 ae77fccc ae4f2c18 8083d44c ae77fcc0
> >     7de0: ae4002 80d03040 adca7e64 adca7e40 adca7e50 80503f08
> >     7e40: 7ebd5630 adca7e50 00000068 c06864a2 7ebd5be8 00000000 00000001 00000018
> >     7e60: 00000026 00000000 00000000 00000000 00000001 000115bc 05010500 05a0059f
> >     7e80: 03200000 03360321 00000337 0000003c 00000000 00000040 30383231 30303878
> >     7ea0: 00000000 00000000 00000000 00000000 00000000 00000000 80173058 80172e30
> >     7ec0: 80d77d32 00004000 adf7d900 00000003 00000000 7ebd5630 af342bb0 adfe3b80
> >     7ee0: 80272f50 00000003 adca6000 00000000 adca7f7c adca7f00 802725ec 804f52cc
> >     7f00: 802809cc 80178450 00000000 00000000 80280880 80145904 adb3d8c0 adf7d990
> >     7f20: ffffffff 00000003 00004000 01614c10 c06864a2 00000003 adca6000 00000000
> >     7f40: adca7f6c adca7f50 80280b04 8028088c 000115bc adfe3b81 7ebd5630 adfe3b80
> >     7f60: c06864a2 00000003 adca6000 00000000 adca7fa4 adca7f80 80272f50 80272548
> >     7f80: 000115bc 00017050 00000001 01614c10 00000036 801089e4 00000000 adca7fa8
> >     7fa0: 80108840 80272f18 00017050 00000001 00000003 c06864a2 7ebd5630 000115bc
> >     7fc0: 00017050 00000001 01614c10 00000036 00000003 00000000 00000026 00000018
> >     7fe0: 00016f38 7ebd562c 0000b5e9 76ef31e6 400c0030 00000003 ff5f37db bfe7dd4d
> >     Backtrace:
> >     [<80506038>] (drm_connector_cleanup) from [<80519104>] (dw_hdmi_connector_destroy+0x24/0x28)
> >      r10:0000000f r9:adca7ddc r8:af3c9c6c r7:8155a290 r6:80501a88 r5:aded7820
> >      r4:aded7800 r3:805190e0
> >     [<805190e0>] (dw_hdmi_connector_destroy) from [<80501ac0>] (drm_connector_free+0x38/0x3c)
> >      r4:aded7800 nreference) from [<804e3518>] (drm_crtc_helper_set_config+0x740/0xbf4)
> >      r6:af3c9c60 r5:aded7afc r4:af3ca010 r3:aded7800
> >     [<804e2dd8>] (drm_crtc_helper_set_config) from [<804fea6c>] (drm_mode_set_config_internal+0x6c/0xf4)
> >      r10:adca6000 r9:af1af700 r8:8155a290 r7:af3c9800 r6:00000001 r5:ae4f2c18
> >      r4:ae77fcc0
> >     [<804fea00>] (drm_mode_set_config_internal) from [<8050440c>] (drm_mode_setcrtc+0x504/0x57c)
> >      r7:af3c9800 r6:00000001 r5:adb3d940 r4:adca7e50
> >     [<80503f08>] (drm_mode_setcrtc) from [<804f5404>] (drm_ioctl+0x144/0x4dc)
> >      r10:ada2e000 r9:000000a2 r8:af3c9800 r7:8155a290 r6:809320b4 r5:00000051
> >      r4:adca7e50
> >     [<804f52c0>] (drm_ioctl) from [<802725ec>] (do_vfs_ioctl+0xb0/0x9d0)
> >      r10:00000000 r9:adca6000 r8:00000003 r7:80272f50 r6:adfe3b80 r5:af342bb0
> >      r4:7ebd5630
> >     [<8027253c>] (do_vfs_ioctl) from [<80272f50>] (SyS_ioctl+0x44/0x6c)
> >      r10:00000000 r9:adca6000 r8:00000003 r7:c06864a2 r6:adfe3b80 r5:7ebd5630
> >      r4:adfe3b81
> >     [<80272f0c>] (SyS_ioctl) from [<80108840>] (ret_fast_syscall+0x0/0x1c)
> >      r8:801089e4 r7:00000036 r6:01614c10 r5:00000001 r4:00017050 r3:000115bc
> >     Code: 0a00000c e5932004 e1a01003 e1a0a004 (e5842004)
> >     ---[ end trace 9a7257572ccacb16 ]---
> > 
> > Only the reference count of connectors that weren't previously bound to
> > an encoder should be incremented after a call to drm_crtc_helper_set_config.
> > And only the reference count of connectors that were previously bound to
> > an encoder and are unbound afterwards should ever be decremented.
> > The reference counts of the temporary copies in the save_connectors
> > should not be touched at all.
> > 
> > This patch fixes the above error by only incrementing the reference count
> > of those connectors in the set that are initially not bound to any encoder,
> > and also by restoring the reference count of only those connectors in the
> > set in the failure case.
> > 
> > Fixes: 0955c1250e96 ("drm/crtc: take references to connectors used in a modeset. (v2)")
> > Signed-off-by: Philipp Zabel <p.zabel@xxxxxxxxxxxxxx>
> 
> Commit message needs to be augmented:
> 
> "Note that this can only be hit when fbdev emulation is disabled, since
> then the refcount drops from 1 to 0 and we call the connector destroy
> functions on the backup copy, which eventually results in tears. With
> fbdev emulation the refcount only goes down from 2 to 1 ever. And since we
> unconditionally increment the refcount on the real object, the refcount of
> that will slowly increase. The backup connector's refcount doesn't matter,
> since we kfree() that either way in the end of
> drm_crtc_helper_set_config()."
> > ---
> >  drivers/gpu/drm/drm_crtc_helper.c | 18 ++++++++++--------
> >  1 file changed, 10 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
> > index a6e4243..1c4d674 100644
> > --- a/drivers/gpu/drm/drm_crtc_helper.c
> > +++ b/drivers/gpu/drm/drm_crtc_helper.c
> > @@ -631,8 +631,12 @@ int drm_crtc_helper_set_config(struct drm_mode_set *set)
> >  		mode_changed = true;
> >  	}
> >  
> > -	/* take a reference on all connectors in set */
> > +	/* take a reference on all unbound connectors in set, reuse the
> > +	 * already taken reference for bound connectors
> > +	 */
> >  	for (ro = 0; ro < set->num_connectors; ro++) {
> > +		if (set->connectors[ro]->encoder)
> > +			continue;
> 
> The real bugfix is that you're switching from save_connectors to
> set->connectors. This filtering here should be irrelevant.
> 
> >  		drm_connector_reference(set->connectors[ro]);
> >  	}
> >  
> > @@ -754,12 +758,6 @@ int drm_crtc_helper_set_config(struct drm_mode_set *set)
> >  		}
> >  	}
> >  
> > -	/* after fail drop reference on all connectors in save set */
> > -	count = 0;
> > -	drm_for_each_connector(connector, dev) {
> > -		drm_connector_unreference(&save_connectors[count++]);
> 
> And we need to keep the unreference here (but on set->connectors instead),
> otherwise this will leak badly. In short this is the only place you need
> to fix up, the other hunks shouldn't be needed.
> 
> > -	}
> > -
> >  	kfree(save_connectors);
> >  	kfree(save_encoders);
> >  	return 0;
> > @@ -776,8 +774,12 @@ fail:
> >  		*connector = save_connectors[count++];
> >  	}
> >  
> > -	/* after fail drop reference on all connectors in set */
> > +	/* after fail drop reference on all unbound connectors in set, let
> > +	 * bound connectors keep their reference
> > +	 */
> >  	for (ro = 0; ro < set->num_connectors; ro++) {
> > +		if (set->connectors[ro]->encoder)
> > +			continue;
> 
> Again, filtering on ->encoder is superflous.
> 
> >  		drm_connector_unreference(set->connectors[ro]);
> >  	}
> 
> With the bugs in the code fixed and the commit message augmented:

Ok, I thought about this some more while eating, and I was totally off the
tracks. The patch is sound, but still needs the commit message ammended.
With that:
 
Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux