1. Patchew
  2. linux
  3. View series

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

Shenghao Yang posted 1 patch 5 days, 21 hours ago
There is a newer version of this series
drivers/gpu/drm/gud/gud_drv.c      |  9 ++++-
drivers/gpu/drm/gud/gud_internal.h |  4 ++
drivers/gpu/drm/gud/gud_pipe.c     | 59 +++++++++++++++++++++---------
3 files changed, 53 insertions(+), 19 deletions(-)
[PATCH v2] drm/gud: fix NULL crtc dereference on display disable
Posted by Shenghao Yang 5 days, 21 hours ago 
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

Cc: <stable+noautosel@kernel.org> # won't apply cleanly on 6.18.x
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>
---
 drivers/gpu/drm/gud/gud_drv.c      |  9 ++++-
 drivers/gpu/drm/gud/gud_internal.h |  4 ++
 drivers/gpu/drm/gud/gud_pipe.c     | 59 +++++++++++++++++++++---------
 3 files changed, 53 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/gud/gud_drv.c b/drivers/gpu/drm/gud/gud_drv.c
index 42135a48d92e..a2000991ecbe 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..46b47452b315 100644
--- a/drivers/gpu/drm/gud/gud_pipe.c
+++ b/drivers/gpu/drm/gud/gud_pipe.c
@@ -580,6 +580,44 @@ 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;
+
+	if (crtc->state->mode_changed || crtc->state->connectors_changed) {
+		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, crtc->state->active);
+
+	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);
+
+	if (crtc->state->mode_changed || crtc->state->connectors_changed)
+		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 +645,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 +658,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.52.0
Re: [PATCH v2] drm/gud: fix NULL crtc dereference on display disable
Posted by Dan Carpenter 4 days, 20 hours ago 
On Sun, Feb 01, 2026 at 05:59:56PM +0800, Shenghao Yang wrote:
> 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
> 
> Cc: <stable+noautosel@kernel.org> # won't apply cleanly on 6.18.x

Why are you adding this?

I suspect it's because checkpatch complains that commits with BUG: in
the commit message should be CC'd to stable.  (Although, I can't trigger
that warning now.  Weird).

If a patch doesn't apply, then the stable scripts aren't going to apply
it.  It's not necessary to tell the scripts not to try.  To me the
"noautosel" basically means that it's important to not backport the
patch.  Maybe the API has changed so backporting it will cause a subtle
breakage.

Are you planning to manually backport it? If so, you could CC stable
and then when you get the email that it doesn't apply, then you can do
the manual backport.  Or you could ignore it.

Or if you think it's not worth backporting, you could explain why.

Cc: <stable+noautosel@kernel.org> # too risky for low benefit

regards,
dan carpenter
Re: [PATCH v2] drm/gud: fix NULL crtc dereference on display disable
Posted by Shenghao Yang 3 days, 13 hours ago 
Hi,

> Why are you adding this?
> 
> I suspect it's because checkpatch complains that commits with BUG: in
> the commit message should be CC'd to stable.  (Although, I can't trigger
> that warning now.  Weird).

> Are you planning to manually backport it? If so, you could CC stable
> and then when you get the email that it doesn't apply, then you can do
> the manual backport.  Or you could ignore it.
> 
> Or if you think it's not worth backporting, you could explain why.
> 
> Cc: <stable+noautosel@kernel.org> # too risky for low benefit

Ah - I added that to try and avoid the failed apply email! It's a bug
that only exists in 6.18 and that stable tree will be EOL soon with the 6.19
release, so I figured it's not worth a manual backport.

> If a patch doesn't apply, then the stable scripts aren't going to apply
> it.  It's not necessary to tell the scripts not to try.  To me the
> "noautosel" basically means that it's important to not backport the
> patch.  Maybe the API has changed so backporting it will cause a subtle
> breakage.

Thanks! I wasn't aware of that connotation - I'll resend without it.

Shenghao

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.