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

Daniel Vetter daniel at ffwll.ch
Mon Jun 13 18:18:47 UTC 2016


On Mon, Jun 13, 2016 at 06:49:57PM +0200, Lucas Stach wrote:
> Am Donnerstag, den 02.06.2016, 22:28 +0200 schrieb Daniel Vetter:
> > 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 at pengutronix.de>
> > 
> > I'd like to analyze this bug first a bit more (since it seems to be imx
> > specific) before review/merging, per our ongoing discussion. The current
> > code is definitely wrong, but looking at it I more expected a leak (since
> > we decrement saved structures, not the real ones), not an explosion.
> > -Daniel
> > 
> There is a regression bug open against nouveau, which shows the same
> symptoms and is fixed by these patches.
> 
> https://bugzilla.kernel.org/show_bug.cgi?id=119861
> 
> It is not specific to imx-drm, but doesn't explode on desktop system
> right away, probably due to the FB emulation holding a reference to all
> connectors.

Sensible theory. Can you confirm that enabling fbdev emulation on imx
works around this?
-Daniel

> 
> Regards,
> Lucas
> 
> > > ---
> > >  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;
> > >  		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++]);
> > > -	}
> > > -
> > >  	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;
> > >  		drm_connector_unreference(set->connectors[ro]);
> > >  	}
> > >  
> > > -- 
> > > 2.8.1
> > > 
> > 
> 
> 

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


More information about the dri-devel mailing list