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

[Ruben Wauters]

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@xxxxxxxxxxxxxxxxx/
> > [2] https://github.com/notro/gud/wiki/GUD-Protocol#display-state
> > 
> > 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>
> 
> AFAICT this looks good.
> 
> Reviewed-by: Thomas Zimmermann <tzimmermann@xxxxxxx>

Acked-by: Ruben Wauters <rubenru09@xxxxxxx>

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.

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@xxxxxxxxxxxxxxxxx/
> > [v3]: https://lore.kernel.org/lkml/20260203172630.10077-1-me@xxxxxxxxxxxxxxxxx/
> > [v2]: https://lore.kernel.org/lkml/20260201095956.21042-1-me@xxxxxxxxxxxxxxxxx/
> > [v1]: https://lore.kernel.org/lkml/20260118125044.54467-1-me@xxxxxxxxxxxxxxxxx/
> > 
> >   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: signature.asc
Description: This is a digitally signed message part