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/[email protected]/
> > [2] https://github.com/notro/gud/wiki/GUD-Protocol#display-state
> >
> > Reported-by: kernel test robot <[email protected]>
> > Reported-by: Dan Carpenter <[email protected]>
> > Closes: https://lore.kernel.org/r/[email protected]/
> > Fixes: 73cfd166e045 ("drm/gud: Replace simple display pipe with DRM atomic
> > helpers")
> > Signed-off-by: Shenghao Yang <[email protected]>
>
> AFAICT this looks good.
>
> Reviewed-by: Thomas Zimmermann <[email protected]>Acked-by: Ruben Wauters <[email protected]> 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/[email protected]/ > > [v3]: > > https://lore.kernel.org/lkml/[email protected]/ > > [v2]: > > https://lore.kernel.org/lkml/[email protected]/ > > [v1]: > > https://lore.kernel.org/lkml/[email protected]/ > > > > 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); > > }
signature.asc
Description: This is a digitally signed message part
