Hi Ruben,
> 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?
I'm good with either! Maybe we go with what's more convenient for you?
The backport resolution (I think) should just be a single line move:
+ drm->mode_config.helper_private = &gud_mode_config_helpers;
Thanks,
Shenghao
On 4/3/26 21:53, Ruben Wauters wrote:
> 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/[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.
>
> 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/[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);
>>>> }
