[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