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

[Ruben Wauters <rubenru09@aol.com>]

[-- Attachment #1: Type: text/plain, Size: 9932 bytes --]

On Wed, 2026-02-25 at 11:52 +0000, Ruben Wauters wrote:
> On Wed, 2026-02-25 at 09:52 +0100, Thomas Zimmermann wrote:
> > Hi
> > 
> > Am 22.02.26 um 06:45 schrieb Shenghao Yang:
> > > gud_plane_atomic_update() currently handles both crtc state and
> > > framebuffer updates - the complexity has led to a few accidental
> > > NULL pointer dereferences.
> > > 
> > > Commit dc2d5ddb193e ("drm/gud: fix NULL fb and crtc dereferences
> > > on USB disconnect") [1] fixed an earlier dereference but planes
> > > can also be disabled in non-hotplug paths (e.g. display disables
> > > via the desktop environment). The drm_dev_enter() call would not
> > > cause an early return in those and 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
> > > 
> > > Split out crtc handling from gud_plane_atomic_update() into
> > > atomic_enable() and atomic_disable() functions to delegate
> > > crtc state transitioning work to the DRM helpers.
> > > 
> > > To preserve the gud state commit sequence [2], switch to
> > > the runtime PM version of drm_atomic_helper_commit_tail() which
> > > ensures that crtcs are enabled (hence sending the
> > > GUD_REQ_SET_CONTROLLER_ENABLE and GUD_REQ_SET_DISPLAY_ENABLE
> > > requests) before a framebuffer update is sent.
> > > 
> > > [1] https://lore.kernel.org/all/20251231055039.44266-1-me@shenghaoyang.info/
> > > [2] https://github.com/notro/gud/wiki/GUD-Protocol#display-state
> > > 
> > > Reported-by: kernel test robot <lkp@intel.com>
> > > Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> > > Closes: https://lore.kernel.org/r/202601142159.0v8ilfVs-lkp@intel.com/
> > > Fixes: 73cfd166e045 ("drm/gud: Replace simple display pipe with DRM atomic helpers")
> > > Signed-off-by: Shenghao Yang <me@shenghaoyang.info>
> > 
> > AFAICT this looks good.
> > 
> > Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
> 
> Acked-by: Ruben Wauters <rubenru09@aol.com>
> 
> This will likely require a CC stable for 6.19 at least, possibly also
> 6.18, though I'm not 100% sure how long that one will last with 7.0-rc1
> being released.

Just as a note, it seems that 6.18 has been designated a longterm
support release, and as such it would be a good idea/appropriate to
also backport this to 6.18, since the bug exists in that version too.

Shenghao, Would you prefer for me to backport this on merge when it
fails to apply it, or would you like me to add the cc stable tags, and
on failure, for you to backport it yourself?

Ruben
> 
> I can add this when I merge it.
> > 
> > Best regards
> > Thomas
> > 
> > > ---
> > > v5: Send SET_CONTROLLER_ENABLE and SET_STATE_COMMIT unconditionally on
> > >      crtc enable
> > > v4: Send SET_DISPLAY_ENABLE=1 unconditionally on crtc enable
> > > v3: Dropped stable AUTOSEL opt out
> > > v2: Moved controller and display control commands to crtc
> > >      enable / disable functions.
> > > 
> > > [v4]: https://lore.kernel.org/lkml/20260218054711.63982-1-me@shenghaoyang.info/
> > > [v3]: https://lore.kernel.org/lkml/20260203172630.10077-1-me@shenghaoyang.info/
> > > [v2]: https://lore.kernel.org/lkml/20260201095956.21042-1-me@shenghaoyang.info/
> > > [v1]: https://lore.kernel.org/lkml/20260118125044.54467-1-me@shenghaoyang.info/
> > > 
> > >   drivers/gpu/drm/gud/gud_drv.c      |  9 ++++-
> > >   drivers/gpu/drm/gud/gud_internal.h |  4 +++
> > >   drivers/gpu/drm/gud/gud_pipe.c     | 54 ++++++++++++++++++++----------
> > >   3 files changed, 48 insertions(+), 19 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/gud/gud_drv.c b/drivers/gpu/drm/gud/gud_drv.c
> > > index d0122d477610..17c2dead2c13 100644
> > > --- a/drivers/gpu/drm/gud/gud_drv.c
> > > +++ b/drivers/gpu/drm/gud/gud_drv.c
> > > @@ -339,7 +339,9 @@ static int gud_stats_debugfs(struct seq_file *m, void *data)
> > >   }
> > >   
> > >   static const struct drm_crtc_helper_funcs gud_crtc_helper_funcs = {
> > > -	.atomic_check = drm_crtc_helper_atomic_check
> > > +	.atomic_check = drm_crtc_helper_atomic_check,
> > > +	.atomic_enable = gud_crtc_atomic_enable,
> > > +	.atomic_disable = gud_crtc_atomic_disable,
> > >   };
> > >   
> > >   static const struct drm_crtc_funcs gud_crtc_funcs = {
> > > @@ -364,6 +366,10 @@ static const struct drm_plane_funcs gud_plane_funcs = {
> > >   	DRM_GEM_SHADOW_PLANE_FUNCS,
> > >   };
> > >   
> > > +static const struct drm_mode_config_helper_funcs gud_mode_config_helpers = {
> > > +	.atomic_commit_tail = drm_atomic_helper_commit_tail_rpm,
> > > +};
> > > +
> > >   static const struct drm_mode_config_funcs gud_mode_config_funcs = {
> > >   	.fb_create = drm_gem_fb_create_with_dirty,
> > >   	.atomic_check = drm_atomic_helper_check,
> > > @@ -499,6 +505,7 @@ static int gud_probe(struct usb_interface *intf, const struct usb_device_id *id)
> > >   	drm->mode_config.min_height = le32_to_cpu(desc.min_height);
> > >   	drm->mode_config.max_height = le32_to_cpu(desc.max_height);
> > >   	drm->mode_config.funcs = &gud_mode_config_funcs;
> > > +	drm->mode_config.helper_private = &gud_mode_config_helpers;
> > >   
> > >   	/* Format init */
> > >   	formats_dev = devm_kmalloc(dev, GUD_FORMATS_MAX_NUM, GFP_KERNEL);
> > > diff --git a/drivers/gpu/drm/gud/gud_internal.h b/drivers/gpu/drm/gud/gud_internal.h
> > > index d27c31648341..a5b7e53cf79c 100644
> > > --- a/drivers/gpu/drm/gud/gud_internal.h
> > > +++ b/drivers/gpu/drm/gud/gud_internal.h
> > > @@ -62,6 +62,10 @@ int gud_usb_set_u8(struct gud_device *gdrm, u8 request, u8 val);
> > >   
> > >   void gud_clear_damage(struct gud_device *gdrm);
> > >   void gud_flush_work(struct work_struct *work);
> > > +void gud_crtc_atomic_enable(struct drm_crtc *crtc,
> > > +			   struct drm_atomic_state *state);
> > > +void gud_crtc_atomic_disable(struct drm_crtc *crtc,
> > > +			   struct drm_atomic_state *state);
> > >   int gud_plane_atomic_check(struct drm_plane *plane,
> > >   			   struct drm_atomic_state *state);
> > >   void gud_plane_atomic_update(struct drm_plane *plane,
> > > diff --git a/drivers/gpu/drm/gud/gud_pipe.c b/drivers/gpu/drm/gud/gud_pipe.c
> > > index 4b77be94348d..587d6dd2b32e 100644
> > > --- a/drivers/gpu/drm/gud/gud_pipe.c
> > > +++ b/drivers/gpu/drm/gud/gud_pipe.c
> > > @@ -580,6 +580,39 @@ int gud_plane_atomic_check(struct drm_plane *plane,
> > >   	return ret;
> > >   }
> > >   
> > > +void gud_crtc_atomic_enable(struct drm_crtc *crtc,
> > > +			   struct drm_atomic_state *state)
> > > +{
> > > +	struct drm_device *drm = crtc->dev;
> > > +	struct gud_device *gdrm = to_gud_device(drm);
> > > +	int idx;
> > > +
> > > +	if (!drm_dev_enter(drm, &idx))
> > > +		return;
> > > +
> > > +	gud_usb_set_u8(gdrm, GUD_REQ_SET_CONTROLLER_ENABLE, 1);
> > > +	gud_usb_set(gdrm, GUD_REQ_SET_STATE_COMMIT, 0, NULL, 0);
> > > +	gud_usb_set_u8(gdrm, GUD_REQ_SET_DISPLAY_ENABLE, 1);
> > > +
> > > +	drm_dev_exit(idx);
> > > +}
> > > +
> > > +void gud_crtc_atomic_disable(struct drm_crtc *crtc,
> > > +			   struct drm_atomic_state *state)
> > > +{
> > > +	struct drm_device *drm = crtc->dev;
> > > +	struct gud_device *gdrm = to_gud_device(drm);
> > > +	int idx;
> > > +
> > > +	if (!drm_dev_enter(drm, &idx))
> > > +		return;
> > > +
> > > +	gud_usb_set_u8(gdrm, GUD_REQ_SET_DISPLAY_ENABLE, 0);
> > > +	gud_usb_set_u8(gdrm, GUD_REQ_SET_CONTROLLER_ENABLE, 0);
> > > +
> > > +	drm_dev_exit(idx);
> > > +}
> > > +
> > >   void gud_plane_atomic_update(struct drm_plane *plane,
> > >   			     struct drm_atomic_state *atomic_state)
> > >   {
> > > @@ -607,24 +640,12 @@ void gud_plane_atomic_update(struct drm_plane *plane,
> > >   		mutex_unlock(&gdrm->damage_lock);
> > >   	}
> > >   
> > > -	if (!drm_dev_enter(drm, &idx))
> > > +	if (!crtc || !drm_dev_enter(drm, &idx))
> > >   		return;
> > >   
> > > -	if (!old_state->fb)
> > > -		gud_usb_set_u8(gdrm, GUD_REQ_SET_CONTROLLER_ENABLE, 1);
> > > -
> > > -	if (fb && (crtc->state->mode_changed || crtc->state->connectors_changed))
> > > -		gud_usb_set(gdrm, GUD_REQ_SET_STATE_COMMIT, 0, NULL, 0);
> > > -
> > > -	if (crtc->state->active_changed)
> > > -		gud_usb_set_u8(gdrm, GUD_REQ_SET_DISPLAY_ENABLE, crtc->state->active);
> > > -
> > > -	if (!fb)
> > > -		goto ctrl_disable;
> > > -
> > >   	ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
> > >   	if (ret)
> > > -		goto ctrl_disable;
> > > +		goto out;
> > >   
> > >   	drm_atomic_helper_damage_iter_init(&iter, old_state, new_state);
> > >   	drm_atomic_for_each_plane_damage(&iter, &damage)
> > > @@ -632,9 +653,6 @@ 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)
> > > -		gud_usb_set_u8(gdrm, GUD_REQ_SET_CONTROLLER_ENABLE, 0);
> > > -
> > > +out:
> > >   	drm_dev_exit(idx);
> > >   }

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 870 bytes --]