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

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

[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>
---
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);
 }
-- 
2.53.0

Claude review: drm/gud: fix NULL crtc dereference on display disable

[Claude Code Review Bot]

Overall Series Review

Subject: drm/gud: fix NULL crtc dereference on display disable
Author: Shenghao Yang <me@shenghaoyang.info>
Patches: 1
Reviewed: 2026-02-23T05:53:49.308471

---

This is a single-patch fix for a NULL pointer dereference in the GUD (Generic USB Display) driver's `gud_plane_atomic_update()` function. When a display is disabled through normal desktop environment paths (as opposed to USB hotplug), `new_state->crtc` is NULL, and the function proceeds to dereference it after the `drm_dev_enter()` check (which only guards against USB disconnect). The crash was reported by the kernel test robot and Dan Carpenter.

The fix takes the correct architectural approach: instead of adding more NULL checks inline, it refactors the crtc enable/disable logic out of the plane update function and into proper `atomic_enable`/`atomic_disable` CRTC helper callbacks. This delegates the crtc state transitions to the DRM atomic helpers, which is the standard pattern for DRM drivers. The switch to `drm_atomic_helper_commit_tail_rpm` ensures correct ordering (crtc enables before plane updates) and uses `DRM_PLANE_COMMIT_ACTIVE_ONLY` so planes on inactive CRTCs are not flushed.

The patch introduces some behavioral changes compared to the old code: on mode changes, a full disable/re-enable cycle is performed (the old code only sent `SET_STATE_COMMIT` inline), and `SET_STATE_COMMIT` is now sent unconditionally on crtc enable. These are documented in the v5 changelog and appear to be intentional simplifications that the GUD protocol should tolerate. The patch is well-structured and correct.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/gud: fix NULL crtc dereference on display disable

[Claude Code Review Bot]

Patch Review

The new crtc enable/disable callbacks are straightforward:

> +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);
> +}

The `SET_STATE_COMMIT` is sent unconditionally here. In the old code, it was only sent when `mode_changed || connectors_changed`. For the initial enable and mode change paths, `SET_STATE_CHECK` will have been sent during `gud_plane_atomic_check`, so the commit is expected. For DPMS-on (where `active_changed` triggers `atomic_enable` but there's no mode change), `SET_STATE_COMMIT` is sent without a preceding `SET_STATE_CHECK`. This is documented as intentional in the v5 changelog. Worth confirming with the GUD protocol maintainer (Noralf Trønnes) that the device firmware handles a `SET_STATE_COMMIT` without a prior `SET_STATE_CHECK` gracefully -- though re-committing the existing state should be a no-op in practice.

> +void gud_crtc_atomic_disable(struct drm_crtc *crtc,
> +			   struct drm_atomic_state *state)
> +{
> +	...
> +	gud_usb_set_u8(gdrm, GUD_REQ_SET_DISPLAY_ENABLE, 0);
> +	gud_usb_set_u8(gdrm, GUD_REQ_SET_CONTROLLER_ENABLE, 0);
> +	...
> +}

The disable ordering (display off first, then controller off) is correct per the GUD protocol. Note that `atomic_disable` is called for DPMS-off transitions too (where `old_state->active` is true), not just full disables. The old code only disabled the controller when `!crtc->state->enable`, leaving it enabled during DPMS-off. The new code always disables both, which is a behavioral change but a reasonable one -- `atomic_enable` will re-enable both on DPMS-on.

The switch to `drm_atomic_helper_commit_tail_rpm`:

> +static const struct drm_mode_config_helper_funcs gud_mode_config_helpers = {
> +	.atomic_commit_tail = drm_atomic_helper_commit_tail_rpm,
> +};

This changes the commit ordering from the default (disables → planes → enables) to (disables → enables → planes with `ACTIVE_ONLY`). This ensures that `gud_crtc_atomic_enable` runs before `gud_plane_atomic_update`, matching the GUD protocol requirement that the controller and display must be enabled before framebuffer data is sent. The `DRM_PLANE_COMMIT_ACTIVE_ONLY` flag also means planes on inactive CRTCs are skipped, so DPMS-off doesn't trigger an unnecessary framebuffer flush. Good.

The core fix in `gud_plane_atomic_update`:

> -	if (!drm_dev_enter(drm, &idx))
> +	if (!crtc || !drm_dev_enter(drm, &idx))
>  		return;

This is the actual fix for the NULL dereference. When the plane is being disabled (detached from CRTC), `new_state->crtc` is NULL. The DRM atomic core enforces that `crtc` and `fb` are either both set or both NULL, so if `crtc` passes the NULL check, `fb` is guaranteed non-NULL and the subsequent `drm_gem_fb_begin_cpu_access(fb, ...)` is safe.

The removal of the inline enable/disable logic:

> -	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;

> -ctrl_disable:
> -	if (!crtc->state->enable)
> -		gud_usb_set_u8(gdrm, GUD_REQ_SET_CONTROLLER_ENABLE, 0);

This simplification is clean. All the crtc state management is now properly delegated to the `atomic_enable`/`atomic_disable` callbacks, and `atomic_update` only handles framebuffer flushing.

No issues found that would block this patch. The behavioral changes around `SET_STATE_COMMIT` on DPMS-on are the only thing worth a comment from the GUD protocol maintainer, but they should be benign.

---
Generated by Claude Code Patch Reviewer