Re: [PATCH] drm/gud: fix NULL crtc dereference on display disable

[Thomas Zimmermann]

Hi,

thanks for the patch.


Am 18.01.26 um 13:50 schrieb Shenghao Yang:
> Commit dc2d5ddb193e ("drm/gud: fix NULL fb and crtc dereferences
> on USB disconnect") [1] only fixed the initial NULL crtc dereference
> in gud_plane_atomic_update().
>
> However, planes can also be disabled in non-hotplug paths (e.g.
> display disables via the DE). The drm_dev_enter() call would not

'DE' ?

> cause an early return in those and we'll subsequently oops on
> dereferencing crtc:
>
> BUG: kernel NULL pointer dereference, address: 00000000000005c8
> CPU: 6 UID: 1000 PID: 3473 Comm: kwin_wayland Not tainted 6.18.2-200.vanilla.gud.fc42.x86_64 #1 PREEMPT(lazy)
> RIP: 0010:gud_plane_atomic_update+0x148/0x470 [gud]
>   <TASK>
>   drm_atomic_helper_commit_planes+0x28e/0x310
>   drm_atomic_helper_commit_tail+0x2a/0x70
>   commit_tail+0xf1/0x150
>   drm_atomic_helper_commit+0x13c/0x180
>   drm_atomic_commit+0xb1/0xe0
> info ? __pfx___drm_printfn_info+0x10/0x10
>   drm_mode_atomic_ioctl+0x70f/0x7c0
>   ? __pfx_drm_mode_atomic_ioctl+0x10/0x10
>   drm_ioctl_kernel+0xae/0x100
>   drm_ioctl+0x2a8/0x550
>   ? __pfx_drm_mode_atomic_ioctl+0x10/0x10
>   __x64_sys_ioctl+0x97/0xe0
>   do_syscall_64+0x7e/0x7f0
>   ? __ct_user_enter+0x56/0xd0
>   ? do_syscall_64+0x158/0x7f0
>   ? __ct_user_enter+0x56/0xd0
>   ? do_syscall_64+0x158/0x7f0
>   entry_SYSCALL_64_after_hwframe+0x76/0x7e
>
> Add an early exit and disable the display controller if crtc is NULL.

It seems to me that all these calls to GUD_REQ_SET_CONTROLLER_ENABLE(^1) 
and GUD_REQ_SET_DISPLAY_ENABLEshould rather go to the CRTC's 
atomic_enable/atomic_disable functions. Those currently seem missing 
from [1]. The atomic_update helper would then be reduced to damage 
handling. Best regards Thomas [1] 
https://elixir.bootlin.com/linux/v6.18.6/source/drivers/gpu/drm/gud/gud_drv.c#L341 

^1: SET_CONTROLLER_ENABLE sounds like it could even be part of device 
probing and runtime PM management, but that is a more invasive change.
> [1] https://lore.kernel.org/all/20251231055039.44266-1-me@xxxxxxxxxxxxxxxxx/
>
> Cc: <stable@xxxxxxxxxxxxxxx> # 6.18.x
> Reported-by: kernel test robot <lkp@xxxxxxxxx>
> Reported-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
> Closes: https://lore.kernel.org/r/202601142159.0v8ilfVs-lkp@xxxxxxxxx/
> Fixes: 73cfd166e045 ("drm/gud: Replace simple display pipe with DRM atomic helpers")
> Signed-off-by: Shenghao Yang <me@xxxxxxxxxxxxxxxxx>
> ---
>   drivers/gpu/drm/gud/gud_pipe.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/gud/gud_pipe.c b/drivers/gpu/drm/gud/gud_pipe.c
> index 4b77be94348d..a69dee71490f 100644
> --- a/drivers/gpu/drm/gud/gud_pipe.c
> +++ b/drivers/gpu/drm/gud/gud_pipe.c
> @@ -610,6 +610,9 @@ void gud_plane_atomic_update(struct drm_plane *plane,
>   	if (!drm_dev_enter(drm, &idx))
>   		return;
>   
> +	if (!crtc)
> +		goto ctrl_disable;
> +
>   	if (!old_state->fb)
>   		gud_usb_set_u8(gdrm, GUD_REQ_SET_CONTROLLER_ENABLE, 1);
>   
> @@ -633,7 +636,7 @@ void gud_plane_atomic_update(struct drm_plane *plane,
>   	drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
>   
>   ctrl_disable:
> -	if (!crtc->state->enable)
> +	if (!crtc || !crtc->state->enable)
>   		gud_usb_set_u8(gdrm, GUD_REQ_SET_CONTROLLER_ENABLE, 0);
>   
>   	drm_dev_exit(idx);

--
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, Werner Knoblich, (HRB 36809, AG Nürnberg)