[PATCH] drm/mst: check connector state before dereference
Lyude Paul
lyude at redhat.com
Fri Sep 22 19:46:31 UTC 2023
(sorry if there's been discussion on the gitlab about this or related issues,
I've been trying to let amd folks handle more issues like this)
Warnings would be fine but - I'm pretty sure the solution to this is already
mentioned in the documentation for these functions. Explanations below
On Tue, 2023-09-19 at 16:51 -0400, Fangzhi Zuo wrote:
> We are seeing the crash in the wild that we cannot repro ourselves.
> We want to be able to gather more data and the code should never be
> allowed to crash.
>
> [ 8.433306] BUG: kernel NULL pointer dereference, address: 0000000000000008
> [ 8.433318] #PF: supervisor read access in kernel mode
> [ 8.433323] #PF: error_code(0x0000) - not-present page
> [ 8.433327] PGD 0 P4D 0
> [ 8.433333] Oops: 0000 [#1] PREEMPT SMP NOPTI
> [ 8.433339] CPU: 7 PID: 488 Comm: Xorg Tainted: G OE 6.2.10-arch1-1-00004-g72efbf0a04ca #2 cb04c5bbf595f3de9363c870cd584da0b91be458
> [ 8.433348] Hardware name: HP HP ProBook 445 G6/85D9, BIOS R80 Ver. 01.21.01 07/28/2022
> [ 8.433351] RIP: 0010:drm_dp_atomic_find_time_slots+0x5e/0x260 [drm_display_helper]
> [ 8.433387] Code: 01 00 00 48 8b 85 60 05 00 00 48 63 80 88 00 00 00 3b 43 28 0f 8d 2e 01 00 00 48 8b 53 30 48 8d 04 80 48 8d 04 c2 48 8b 40 18 <48> 8b 40 08 4d 8d 65 38 8b 88 90 00 00 00 b8 01 00 00 00 d3 e0 41
> [ 8.433392] RSP: 0018:ffffb7b540ee36b0 EFLAGS: 00010293
> [ 8.433397] RAX: 0000000000000000 RBX: ffff90d6064ae780 RCX: 0000000000000224
> [ 8.433401] RDX: ffff90d6069e0400 RSI: ffff90d60c496568 RDI: ffff90d6064ae780
> [ 8.433405] RBP: ffff90d60c483000 R08: 0000000000000407 R09: ffff90d608c8e850
> [ 8.433408] R10: 0000000000000002 R11: 0000000000000000 R12: ffffb7b540ee3798
> [ 8.433411] R13: ffff90d607ab9660 R14: ffff90d60c496568 R15: 0000000000000224
> [ 8.433415] FS: 00007fead406e440(0000) GS:ffff90d9201c0000(0000) knlGS:0000000000000000
> [ 8.433419] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 8.433423] CR2: 0000000000000008 CR3: 0000000102f96000 CR4: 00000000003506e0
> [ 8.433427] Call Trace:
> [ 8.433431] <TASK>
> [ 8.433437] compute_mst_dsc_configs_for_link+0x31a/0xab0 [amdgpu b041282416fbbcc9f3f3583485c4c54bacfbbcf9]
> [ 8.434321] ? get_page_from_freelist+0x14a5/0x1630
> [ 8.434338] compute_mst_dsc_configs_for_state+0x1e1/0x250 [amdgpu b041282416fbbcc9f3f3583485c4c54bacfbbcf9]
This is misuse of the drm_dp_atomic_release_time_slots() function - the docs
specifically mention it should be called from the
drm_connector_helper_funcs.atomic_check() callback - which would ensure that
this function can't get called in any atomic check that doesn't already have
the connector state.
/**
* drm_dp_atomic_release_time_slots() - Release allocated time slots
* @state: global atomic state
* @mgr: MST topology manager for the port
* @port: The port to release the time slots from
*
* Releases any time slots that have been allocated to a port in the atomic
* state. Any atomic drivers which support MST must call this function
* unconditionally in their &drm_connector_helper_funcs.atomic_check() callback.
* This helper will check whether time slots would be released by the new state and
* respond accordingly, along with ensuring the MST state is always added to the
* atomic state whenever a new state would modify the state of payloads on the
* topology.
*
* It is OK to call this even if @port has been removed from the system.
* Additionally, it is OK to call this function multiple times on the same
* @port as needed. It is not OK however, to call this function and
* drm_dp_atomic_find_time_slots() on the same @port in a single atomic check
* phase.
*
* See also:
* drm_dp_atomic_find_time_slots()
* drm_dp_mst_atomic_check()
*
* Returns:
* 0 on success, negative error code otherwise
*/
could you please fix that issue in amdgpu first and then re-evaluate things?
> [ 8.435205] amdgpu_dm_atomic_check+0xf33/0x11b0 [amdgpu b041282416fbbcc9f3f3583485c4c54bacfbbcf9]
> [ 8.435985] drm_atomic_check_only+0x5c0/0xa30
> [ 8.435994] drm_atomic_commit+0x5a/0xd0
> [ 8.436001] ? __pfx___drm_printfn_info+0x10/0x10
> [ 8.436008] drm_atomic_helper_set_config+0x74/0xb0
> [ 8.436014] drm_mode_setcrtc+0x515/0x7e0
> [ 8.436023] ? __pfx_drm_mode_setcrtc+0x10/0x10
> [ 8.436029] drm_ioctl_kernel+0xcd/0x170
> [ 8.436036] drm_ioctl+0x233/0x410
> [ 8.436040] ? __pfx_drm_mode_setcrtc+0x10/0x10
> [ 8.436049] amdgpu_drm_ioctl+0x4e/0x90 [amdgpu b041282416fbbcc9f3f3583485c4c54bacfbbcf9]
> [ 8.436755] __x64_sys_ioctl+0x94/0xd0
> [ 8.436763] do_syscall_64+0x5f/0x90
> [ 8.436772] ? amdgpu_drm_ioctl+0x71/0x90 [amdgpu b041282416fbbcc9f3f3583485c4c54bacfbbcf9]
> [ 8.437477] ? __x64_sys_ioctl+0xac/0xd0
> [ 8.437484] ? syscall_exit_to_user_mode+0x1b/0x40
> [ 8.437492] ? do_syscall_64+0x6b/0x90
> [ 8.437499] ? amdgpu_drm_ioctl+0x71/0x90 [amdgpu b041282416fbbcc9f3f3583485c4c54bacfbbcf9]
> [ 8.438193] ? __x64_sys_ioctl+0xac/0xd0
> [ 8.438199] ? syscall_exit_to_user_mode+0x1b/0x40
> [ 8.438205] ? do_syscall_64+0x6b/0x90
> [ 8.438211] ? syscall_exit_to_user_mode+0x1b/0x40
> [ 8.438217] ? do_syscall_64+0x6b/0x90
> [ 8.438223] entry_SYSCALL_64_after_hwframe+0x72/0xdc
> [ 8.438231] RIP: 0033:0x7fead4a3f53f
> [ 8.438258] Code: 00 48 89 44 24 18 31 c0 48 8d 44 24 60 c7 04 24 10 00 00 00 48 89 44 24 08 48 8d 44 24 20 48 89 44 24 10 b8 10 00 00 00 0f 05 <89> c2 3d 00 f0 ff ff 77 18 48 8b 44 24 18 64 48 2b 04 25 28 00 00
> [ 8.438262] RSP: 002b:00007ffd20e26be0 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
> [ 8.438268] RAX: ffffffffffffffda RBX: 0000564cc75abfa0 RCX: 00007fead4a3f53f
> [ 8.438271] RDX: 00007ffd20e26c70 RSI: 00000000c06864a2 RDI: 000000000000000f
> [ 8.438273] RBP: 00007ffd20e26c70 R08: 0000000000000000 R09: 0000564cc75dec90
> [ 8.438276] R10: 0000000000000000 R11: 0000000000000246 R12: 00000000c06864a2
> [ 8.438278] R13: 000000000000000f R14: 0000564cc6cb7f00 R15: 0000564cc6ab4620
> [ 8.438286] </TASK>
> [ 8.438288] Modules linked in: cmac algif_hash algif_skcipher af_alg bnep rtw88_8822be snd_hda_codec_realtek intel_rapl_msr intel_rapl_common rtw88_8822b snd_hda_codec_generic edac_mce_amd ledtrig_audio snd_hda_codec_hdmi rtw88_pci kvm_amd rtw88_core snd_hda_intel kvm snd_intel_dspcfg mac80211 nls_iso8859_1 snd_intel_sdw_acpi uvcvideo btusb vfat snd_hda_codec irqbypass fat btrtl videobuf2_vmalloc crct10dif_pclmul crc32_pclmul videobuf2_memops polyval_clmulni btbcm libarc4 snd_hda_core videobuf2_v4l2 polyval_generic btintel gf128mul snd_hwdep btmtk ghash_clmulni_intel hid_multitouch sha512_ssse3 videodev r8169 snd_pcm cfg80211 bluetooth aesni_intel ucsi_acpi realtek typec_ucsi hp_wmi videobuf2_common crypto_simd mdio_devres sparse_keymap snd_timer sp5100_tco cryptd typec mc ecdh_generic mousedev joydev rapl platform_profile snd psmouse rfkill k10temp wmi_bmof i2c_piix4 soundcore libphy ccp roles hp_accel lis3lv02d i2c_hid_acpi i2c_amd_mp2_plat i2c_hid wireless_hotkey i2c_amd_mp2_pci
> [ 8.438385] acpi_cpufreq mac_hid vboxnetflt(OE) vboxnetadp(OE) vboxdrv(OE) sg crypto_user dm_mod fuse loop bpf_preload ip_tables x_tables ext4 crc32c_generic crc16 mbcache jbd2 hid_logitech_hidpp hid_logitech_dj usbhid amdgpu rtsx_pci_sdmmc drm_ttm_helper serio_raw atkbd ttm mmc_core libps2 drm_buddy vivaldi_fmap gpu_sched crc32c_intel xhci_pci drm_display_helper i8042 xhci_pci_renesas rtsx_pci cec video serio wmi
> [ 8.438436] CR2: 0000000000000008
> [ 8.438440] ---[ end trace 0000000000000000 ]---
> [ 8.438443] RIP: 0010:drm_dp_atomic_find_time_slots+0x5e/0x260 [drm_display_helper]
> [ 8.438470] Code: 01 00 00 48 8b 85 60 05 00 00 48 63 80 88 00 00 00 3b 43 28 0f 8d 2e 01 00 00 48 8b 53 30 48 8d 04 80 48 8d 04 c2 48 8b 40 18 <48> 8b 40 08 4d 8d 65 38 8b 88 90 00 00 00 b8 01 00 00 00 d3 e0 41
> [ 8.438473] RSP: 0018:ffffb7b540ee36b0 EFLAGS: 00010293
> [ 8.438477] RAX: 0000000000000000 RBX: ffff90d6064ae780 RCX: 0000000000000224
> [ 8.438480] RDX: ffff90d6069e0400 RSI: ffff90d60c496568 RDI: ffff90d6064ae780
> [ 8.438482] RBP: ffff90d60c483000 R08: 0000000000000407 R09: ffff90d608c8e850
> [ 8.438485] R10: 0000000000000002 R11: 0000000000000000 R12: ffffb7b540ee3798
> [ 8.438487] R13: ffff90d607ab9660 R14: ffff90d60c496568 R15: 0000000000000224
> [ 8.438490] FS: 00007fead406e440(0000) GS:ffff90d9201c0000(0000) knlGS:0000000000000000
> [ 8.438493] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 8.438496] CR2: 0000000000000008 CR3: 0000000102f96000 CR4: 00000000003506e0
>
> Link: https://gitlab.freedesktop.org/drm/amd/-/issues/2314#note_2080187
> Suggested-by: John Lindgren <john at jlindgren.net>
> Signed-off-by: Fangzhi Zuo <Jerry.Zuo at amd.com>
> ---
> drivers/gpu/drm/display/drm_dp_mst_topology.c | 12 ++++++++----
> 1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> index c490e8befc2f..995bf34154ec 100644
> --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> @@ -4314,7 +4314,9 @@ int drm_dp_atomic_find_time_slots(struct drm_atomic_state *state,
> return PTR_ERR(topology_state);
>
> conn_state = drm_atomic_get_new_connector_state(state, port->connector);
> - topology_state->pending_crtc_mask |= drm_crtc_mask(conn_state->crtc);
> + WARN_ON(!conn_state);
> + if (conn_state)
> + topology_state->pending_crtc_mask |= drm_crtc_mask(conn_state->crtc);
>
> /* Find the current allocation for this port, if any */
> payload = drm_atomic_get_mst_payload_state(topology_state, port);
> @@ -4400,12 +4402,14 @@ int drm_dp_atomic_release_time_slots(struct drm_atomic_state *state,
> bool update_payload = true;
>
> old_conn_state = drm_atomic_get_old_connector_state(state, port->connector);
> - if (!old_conn_state->crtc)
> + WARN_ON(!old_conn_state);
> + if (!old_conn_state || !old_conn_state->crtc)
> return 0;
>
> /* If the CRTC isn't disabled by this state, don't release it's payload */
> new_conn_state = drm_atomic_get_new_connector_state(state, port->connector);
> - if (new_conn_state->crtc) {
> + WARN_ON(!new_conn_state);
> + if (new_conn_state && new_conn_state->crtc) {
> struct drm_crtc_state *crtc_state =
> drm_atomic_get_new_crtc_state(state, new_conn_state->crtc);
>
> @@ -4432,7 +4436,7 @@ int drm_dp_atomic_release_time_slots(struct drm_atomic_state *state,
> return -EINVAL;
> }
>
> - if (new_conn_state->crtc)
> + if (new_conn_state && new_conn_state->crtc)
> return 0;
>
> drm_dbg_atomic(mgr->dev, "[MST PORT:%p] TU %d -> 0\n", port, payload->time_slots);
--
Cheers,
Lyude Paul (she/her)
Software Engineer at Red Hat
More information about the dri-devel
mailing list