[PATCH v2 1/2] drm/crtc: fix connector reference counting mismatch in drm_crtc_helper_set_config
Daniel Vetter
daniel at ffwll.ch
Thu Jun 2 20:28:21 UTC 2016
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
> ---
> 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