drivers/gpu/drm/gud/gud_pipe.c | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-)
On disconnect drm_atomic_helper_disable_all() is called which
sets both the fb and crtc for a plane to NULL before invoking a commit.
This causes a kernel oops on every display disconnect.
Add guards for those dereferences.
Cc: <stable@vger.kernel.org> # 6.18.x
Signed-off-by: Shenghao Yang <me@shenghaoyang.info>
---
drivers/gpu/drm/gud/gud_pipe.c | 20 ++++++++------------
1 file changed, 8 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/gud/gud_pipe.c b/drivers/gpu/drm/gud/gud_pipe.c
index 76d77a736d84..4b77be94348d 100644
--- a/drivers/gpu/drm/gud/gud_pipe.c
+++ b/drivers/gpu/drm/gud/gud_pipe.c
@@ -457,27 +457,20 @@ int gud_plane_atomic_check(struct drm_plane *plane,
struct drm_plane_state *old_plane_state = drm_atomic_get_old_plane_state(state, plane);
struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state, plane);
struct drm_crtc *crtc = new_plane_state->crtc;
- struct drm_crtc_state *crtc_state;
+ struct drm_crtc_state *crtc_state = NULL;
const struct drm_display_mode *mode;
struct drm_framebuffer *old_fb = old_plane_state->fb;
struct drm_connector_state *connector_state = NULL;
struct drm_framebuffer *fb = new_plane_state->fb;
- const struct drm_format_info *format = fb->format;
+ const struct drm_format_info *format;
struct drm_connector *connector;
unsigned int i, num_properties;
struct gud_state_req *req;
int idx, ret;
size_t len;
- if (drm_WARN_ON_ONCE(plane->dev, !fb))
- return -EINVAL;
-
- if (drm_WARN_ON_ONCE(plane->dev, !crtc))
- return -EINVAL;
-
- crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
-
- mode = &crtc_state->mode;
+ if (crtc)
+ crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
ret = drm_atomic_helper_check_plane_state(new_plane_state, crtc_state,
DRM_PLANE_NO_SCALING,
@@ -492,6 +485,9 @@ int gud_plane_atomic_check(struct drm_plane *plane,
if (old_plane_state->rotation != new_plane_state->rotation)
crtc_state->mode_changed = true;
+ mode = &crtc_state->mode;
+ format = fb->format;
+
if (old_fb && old_fb->format != format)
crtc_state->mode_changed = true;
@@ -598,7 +594,7 @@ void gud_plane_atomic_update(struct drm_plane *plane,
struct drm_atomic_helper_damage_iter iter;
int ret, idx;
- if (crtc->state->mode_changed || !crtc->state->enable) {
+ if (!crtc || crtc->state->mode_changed || !crtc->state->enable) {
cancel_work_sync(&gdrm->work);
mutex_lock(&gdrm->damage_lock);
if (gdrm->fb) {
--
2.52.0
Hi
On Wed, 2025-12-31 at 13:50 +0800, Shenghao Yang wrote:
> On disconnect drm_atomic_helper_disable_all() is called which
> sets both the fb and crtc for a plane to NULL before invoking a commit.
>
> This causes a kernel oops on every display disconnect.
>
> Add guards for those dereferences.
>
> Cc: <stable@vger.kernel.org> # 6.18.x
> Signed-off-by: Shenghao Yang <me@shenghaoyang.info>
> ---
> drivers/gpu/drm/gud/gud_pipe.c | 20 ++++++++------------
> 1 file changed, 8 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/gud/gud_pipe.c b/drivers/gpu/drm/gud/gud_pipe.c
> index 76d77a736d84..4b77be94348d 100644
> --- a/drivers/gpu/drm/gud/gud_pipe.c
> +++ b/drivers/gpu/drm/gud/gud_pipe.c
> @@ -457,27 +457,20 @@ int gud_plane_atomic_check(struct drm_plane *plane,
> struct drm_plane_state *old_plane_state = drm_atomic_get_old_plane_state(state, plane);
> struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state, plane);
> struct drm_crtc *crtc = new_plane_state->crtc;
> - struct drm_crtc_state *crtc_state;
> + struct drm_crtc_state *crtc_state = NULL;
> const struct drm_display_mode *mode;
> struct drm_framebuffer *old_fb = old_plane_state->fb;
> struct drm_connector_state *connector_state = NULL;
> struct drm_framebuffer *fb = new_plane_state->fb;
> - const struct drm_format_info *format = fb->format;
> + const struct drm_format_info *format;
> struct drm_connector *connector;
> unsigned int i, num_properties;
> struct gud_state_req *req;
> int idx, ret;
> size_t len;
>
> - if (drm_WARN_ON_ONCE(plane->dev, !fb))
> - return -EINVAL;
> -
> - if (drm_WARN_ON_ONCE(plane->dev, !crtc))
> - return -EINVAL;
With the elimination of these two WARN_ON_ONCEs, it's possible that
crtc_state may not be assigned below, and therefore may be read/passed
to functions when it is NULL (e.g. line 488). Either protection for a
null crtc_state should be added to the rest of the function, or the
function shouldn't continue if crtc is NULL.
Ruben
> - crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
> -
> - mode = &crtc_state->mode;
> + if (crtc)
> + crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
>
> ret = drm_atomic_helper_check_plane_state(new_plane_state, crtc_state,
> DRM_PLANE_NO_SCALING,
> @@ -492,6 +485,9 @@ int gud_plane_atomic_check(struct drm_plane *plane,
> if (old_plane_state->rotation != new_plane_state->rotation)
> crtc_state->mode_changed = true;
>
> + mode = &crtc_state->mode;
> + format = fb->format;
> +
> if (old_fb && old_fb->format != format)
> crtc_state->mode_changed = true;
>
> @@ -598,7 +594,7 @@ void gud_plane_atomic_update(struct drm_plane *plane,
> struct drm_atomic_helper_damage_iter iter;
> int ret, idx;
>
> - if (crtc->state->mode_changed || !crtc->state->enable) {
> + if (!crtc || crtc->state->mode_changed || !crtc->state->enable) {
> cancel_work_sync(&gdrm->work);
> mutex_lock(&gdrm->damage_lock);
> if (gdrm->fb) {
Hi Ruben, On 4/1/26 01:23, Ruben Wauters wrote: > With the elimination of these two WARN_ON_ONCEs, it's possible that > crtc_state may not be assigned below, and therefore may be read/passed > to functions when it is NULL (e.g. line 488). Either protection for a > null crtc_state should be added to the rest of the function, or the > function shouldn't continue if crtc is NULL. > > Ruben >> - crtc_state = drm_atomic_get_new_crtc_state(state, crtc); >> - >> - mode = &crtc_state->mode; >> + if (crtc) >> + crtc_state = drm_atomic_get_new_crtc_state(state, crtc); >> >> ret = drm_atomic_helper_check_plane_state(new_plane_state, crtc_state, >> DRM_PLANE_NO_SCALING, >> @@ -492,6 +485,9 @@ int gud_plane_atomic_check(struct drm_plane *plane, >> if (old_plane_state->rotation != new_plane_state->rotation) >> crtc_state->mode_changed = true; >> >> + mode = &crtc_state->mode; >> + format = fb->format; Yup - in this case I'm relying on drm_atomic_helper_check_plane_state() bailing out early after seeing that fb is NULL (since a NULL crtc should imply no fb) and setting plane_state->visible to false. That would cause an early return in gud_plane_atomic_check() without dereferencing crtc_state. Would a more explicit check be preferred? Thanks, Shenghao
Hi On Sun, 2026-01-04 at 01:47 +0800, Shenghao Yang wrote: > Hi Ruben, > > On 4/1/26 01:23, Ruben Wauters wrote: > > > With the elimination of these two WARN_ON_ONCEs, it's possible that > > crtc_state may not be assigned below, and therefore may be read/passed > > to functions when it is NULL (e.g. line 488). Either protection for a > > null crtc_state should be added to the rest of the function, or the > > function shouldn't continue if crtc is NULL. > > > > Ruben > > > - crtc_state = drm_atomic_get_new_crtc_state(state, crtc); > > > - > > > - mode = &crtc_state->mode; > > > + if (crtc) > > > + crtc_state = drm_atomic_get_new_crtc_state(state, crtc); > > > > > > ret = drm_atomic_helper_check_plane_state(new_plane_state, crtc_state, > > > DRM_PLANE_NO_SCALING, > > > @@ -492,6 +485,9 @@ int gud_plane_atomic_check(struct drm_plane *plane, > > > if (old_plane_state->rotation != new_plane_state->rotation) > > > crtc_state->mode_changed = true; > > > > > > + mode = &crtc_state->mode; > > > + format = fb->format; > > Yup - in this case I'm relying on drm_atomic_helper_check_plane_state() > bailing out early after seeing that fb is NULL (since a NULL crtc should > imply no fb) and setting plane_state->visible to false. > > That would cause an early return in gud_plane_atomic_check() without > dereferencing crtc_state. This does work, however this ends up returning 0, which implies that the atomic check succeded. In my opinion in this case, -EINVAL should be returned, as both the crtc and fb don't exist, therefore the check should not succeed. I would personally prefer a more explicit check that does return -EINVAL instead of 0 from drm_atomic_helper_check_planes() As a side note, I'm not sure if there's a reasoning as to why drm_atomic_helper_check_planes() returns 0 if fb is NULL instead of -EINVAL, I'm assuming it's not designed to come across this specific case. Either way it's not too much of an issue but maybe one of the drm maintainers can clarify why it's this way. Ruben > > Would a more explicit check be preferred? > > Thanks, > > Shenghao
Hi Ruben Am 03.01.26 um 20:18 schrieb Ruben Wauters: > Hi > > On Sun, 2026-01-04 at 01:47 +0800, Shenghao Yang wrote: >> Hi Ruben, >> >> On 4/1/26 01:23, Ruben Wauters wrote: >> >>> With the elimination of these two WARN_ON_ONCEs, it's possible that >>> crtc_state may not be assigned below, and therefore may be read/passed >>> to functions when it is NULL (e.g. line 488). Either protection for a >>> null crtc_state should be added to the rest of the function, or the >>> function shouldn't continue if crtc is NULL. >>> >>> Ruben >>>> - crtc_state = drm_atomic_get_new_crtc_state(state, crtc); >>>> - >>>> - mode = &crtc_state->mode; >>>> + if (crtc) >>>> + crtc_state = drm_atomic_get_new_crtc_state(state, crtc); >>>> >>>> ret = drm_atomic_helper_check_plane_state(new_plane_state, crtc_state, >>>> DRM_PLANE_NO_SCALING, >>>> @@ -492,6 +485,9 @@ int gud_plane_atomic_check(struct drm_plane *plane, >>>> if (old_plane_state->rotation != new_plane_state->rotation) >>>> crtc_state->mode_changed = true; >>>> >>>> + mode = &crtc_state->mode; >>>> + format = fb->format; >> Yup - in this case I'm relying on drm_atomic_helper_check_plane_state() >> bailing out early after seeing that fb is NULL (since a NULL crtc should >> imply no fb) and setting plane_state->visible to false. This is correct behavior. >> >> That would cause an early return in gud_plane_atomic_check() without >> dereferencing crtc_state. > This does work, however this ends up returning 0, which implies that > the atomic check succeded. In my opinion in this case, -EINVAL should > be returned, as both the crtc and fb don't exist, therefore the check > should not succeed. I would personally prefer a more explicit check > that does return -EINVAL instead of 0 from > drm_atomic_helper_check_planes() If the plane has been disbabled, fb and crtc are NULL. So this is correct. drm_atomic_helper_check_plane_state() is the place to do this testing before doing much else in the atomic_check handler. If drm_atomic_helper_check_plane_state() gives an error, the error should be returns. But if it returns 0 and sets ->visible to false, the atomic_check should return 0. That means that the plane can successfully be disabled. > > As a side note, I'm not sure if there's a reasoning as to why > drm_atomic_helper_check_planes() returns 0 if fb is NULL instead of > -EINVAL, I'm assuming it's not designed to come across this specific > case. Either way it's not too much of an issue but maybe one of the drm > maintainers can clarify why it's this way. Disabling a plane is not an error, but a common operation. I think the patch is fine and IIRC we have similar logic in other drivers. Best regards Thomas > > Ruben >> Would a more explicit check be preferred? >> >> Thanks, >> >> Shenghao -- -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com GF: Jochen Jaser, Andrew McDonald, Werner Knoblich, (HRB 36809, AG Nürnberg)
Hi, On Wed, 2026-01-07 at 08:46 +0100, Thomas Zimmermann wrote: > Hi Ruben > > Am 03.01.26 um 20:18 schrieb Ruben Wauters: > > Hi > > > > On Sun, 2026-01-04 at 01:47 +0800, Shenghao Yang wrote: > > > Hi Ruben, > > > > > > On 4/1/26 01:23, Ruben Wauters wrote: > > > > > > > With the elimination of these two WARN_ON_ONCEs, it's possible that > > > > crtc_state may not be assigned below, and therefore may be read/passed > > > > to functions when it is NULL (e.g. line 488). Either protection for a > > > > null crtc_state should be added to the rest of the function, or the > > > > function shouldn't continue if crtc is NULL. > > > > > > > > Ruben > > > > > - crtc_state = drm_atomic_get_new_crtc_state(state, crtc); > > > > > - > > > > > - mode = &crtc_state->mode; > > > > > + if (crtc) > > > > > + crtc_state = drm_atomic_get_new_crtc_state(state, crtc); > > > > > > > > > > ret = drm_atomic_helper_check_plane_state(new_plane_state, crtc_state, > > > > > DRM_PLANE_NO_SCALING, > > > > > @@ -492,6 +485,9 @@ int gud_plane_atomic_check(struct drm_plane *plane, > > > > > if (old_plane_state->rotation != new_plane_state->rotation) > > > > > crtc_state->mode_changed = true; > > > > > > > > > > + mode = &crtc_state->mode; > > > > > + format = fb->format; > > > Yup - in this case I'm relying on drm_atomic_helper_check_plane_state() > > > bailing out early after seeing that fb is NULL (since a NULL crtc should > > > imply no fb) and setting plane_state->visible to false. > > This is correct behavior. > > > > > > > That would cause an early return in gud_plane_atomic_check() without > > > dereferencing crtc_state. > > This does work, however this ends up returning 0, which implies that > > the atomic check succeded. In my opinion in this case, -EINVAL should > > be returned, as both the crtc and fb don't exist, therefore the check > > should not succeed. I would personally prefer a more explicit check > > that does return -EINVAL instead of 0 from > > drm_atomic_helper_check_planes() > > If the plane has been disbabled, fb and crtc are NULL. So this is > correct. drm_atomic_helper_check_plane_state() is the place to do this > testing before doing much else in the atomic_check handler. If > drm_atomic_helper_check_plane_state() gives an error, the error should > be returns. But if it returns 0 and sets ->visible to false, the > atomic_check should return 0. That means that the plane can > successfully be disabled. > > > > > As a side note, I'm not sure if there's a reasoning as to why > > drm_atomic_helper_check_planes() returns 0 if fb is NULL instead of > > -EINVAL, I'm assuming it's not designed to come across this specific > > case. Either way it's not too much of an issue but maybe one of the drm > > maintainers can clarify why it's this way. > > Disabling a plane is not an error, but a common operation. I think I may have misunderstood the drm docs on this, if having crtc and fb be null and returning 0 then is ok, I am happy for this patch to be applied. I have tested it and it indeed works, and removes the oops present in the driver before this. > > I think the patch is fine and IIRC we have similar logic in other drivers. Reviewed-by: Ruben Wauters <rubenru09@aol.com> I believe Shenghao mentioned another oops that is present? if so it may be best to submit that in a separate patch rather than a v2 of this one. Ruben > > Best regards > Thomas > > > > > Ruben > > > Would a more explicit check be preferred? > > > > > > Thanks, > > > > > > Shenghao
Hi Am 07.01.26 um 16:02 schrieb Ruben Wauters: > Hi, > > On Wed, 2026-01-07 at 08:46 +0100, Thomas Zimmermann wrote: >> Hi Ruben >> >> Am 03.01.26 um 20:18 schrieb Ruben Wauters: >>> Hi >>> >>> On Sun, 2026-01-04 at 01:47 +0800, Shenghao Yang wrote: >>>> Hi Ruben, >>>> >>>> On 4/1/26 01:23, Ruben Wauters wrote: >>>> >>>>> With the elimination of these two WARN_ON_ONCEs, it's possible that >>>>> crtc_state may not be assigned below, and therefore may be read/passed >>>>> to functions when it is NULL (e.g. line 488). Either protection for a >>>>> null crtc_state should be added to the rest of the function, or the >>>>> function shouldn't continue if crtc is NULL. >>>>> >>>>> Ruben >>>>>> - crtc_state = drm_atomic_get_new_crtc_state(state, crtc); >>>>>> - >>>>>> - mode = &crtc_state->mode; >>>>>> + if (crtc) >>>>>> + crtc_state = drm_atomic_get_new_crtc_state(state, crtc); >>>>>> >>>>>> ret = drm_atomic_helper_check_plane_state(new_plane_state, crtc_state, >>>>>> DRM_PLANE_NO_SCALING, >>>>>> @@ -492,6 +485,9 @@ int gud_plane_atomic_check(struct drm_plane *plane, >>>>>> if (old_plane_state->rotation != new_plane_state->rotation) >>>>>> crtc_state->mode_changed = true; >>>>>> >>>>>> + mode = &crtc_state->mode; >>>>>> + format = fb->format; >>>> Yup - in this case I'm relying on drm_atomic_helper_check_plane_state() >>>> bailing out early after seeing that fb is NULL (since a NULL crtc should >>>> imply no fb) and setting plane_state->visible to false. >> This is correct behavior. >> >>>> That would cause an early return in gud_plane_atomic_check() without >>>> dereferencing crtc_state. >>> This does work, however this ends up returning 0, which implies that >>> the atomic check succeded. In my opinion in this case, -EINVAL should >>> be returned, as both the crtc and fb don't exist, therefore the check >>> should not succeed. I would personally prefer a more explicit check >>> that does return -EINVAL instead of 0 from >>> drm_atomic_helper_check_planes() >> If the plane has been disbabled, fb and crtc are NULL. So this is >> correct. drm_atomic_helper_check_plane_state() is the place to do this >> testing before doing much else in the atomic_check handler. If >> drm_atomic_helper_check_plane_state() gives an error, the error should >> be returns. But if it returns 0 and sets ->visible to false, the >> atomic_check should return 0. That means that the plane can >> successfully be disabled. >> >>> As a side note, I'm not sure if there's a reasoning as to why >>> drm_atomic_helper_check_planes() returns 0 if fb is NULL instead of >>> -EINVAL, I'm assuming it's not designed to come across this specific >>> case. Either way it's not too much of an issue but maybe one of the drm >>> maintainers can clarify why it's this way. >> Disabling a plane is not an error, but a common operation. > I think I may have misunderstood the drm docs on this, if having crtc > and fb be null and returning 0 then is ok, I am happy for this patch to > be applied. I have tested it and it indeed works, and removes the oops > present in the driver before this. No worries, DRM semantics can be murky. This is one of the cases that is impossible to know unless you came across a patch like this one. Best regards Thomas >> I think the patch is fine and IIRC we have similar logic in other drivers. > Reviewed-by: Ruben Wauters <rubenru09@aol.com> > > I believe Shenghao mentioned another oops that is present? if so it may > be best to submit that in a separate patch rather than a v2 of this > one. > > Ruben >> Best regards >> Thomas >> >>> Ruben >>>> Would a more explicit check be preferred? >>>> >>>> Thanks, >>>> >>>> Shenghao -- -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com GF: Jochen Jaser, Andrew McDonald, Werner Knoblich, (HRB 36809, AG Nürnberg)
Hi Ruben, Thomas, On 7/1/26 23:56, Thomas Zimmermann wrote: > > No worries, DRM semantics can be murky. This is one of the cases that is impossible to know unless you came across a patch like this one. > > Best regards > Thomas > >>> I think the patch is fine and IIRC we have similar logic in other drivers. >> Reviewed-by: Ruben Wauters <rubenru09@aol.com> >> >> I believe Shenghao mentioned another oops that is present? if so it may >> be best to submit that in a separate patch rather than a v2 of this >> one. >> >> Ruben Thanks both! I'll split the patch for the second oops. Shenghao
On Thu, 2026-01-08 at 21:39 +0800, Shenghao Yang wrote Hi > Hi Ruben, Thomas, > > On 7/1/26 23:56, Thomas Zimmermann wrote: > > > > > No worries, DRM semantics can be murky. This is one of the cases that is impossible to know unless you came across a patch like this one. > > > > Best regards > > Thomas > > > > > > I think the patch is fine and IIRC we have similar logic in other drivers. > > > Reviewed-by: Ruben Wauters <rubenru09@aol.com> Patch applied to drm-misc-fixes, thanks > > > > > > I believe Shenghao mentioned another oops that is present? if so it may > > > be best to submit that in a separate patch rather than a v2 of this > > > one. > > > > > > Ruben > > > Thanks both! I'll split the patch for the second oops. > > Shenghao
Hi Ruben, On 4/1/26 03:18, Ruben Wauters wrote: > Hi > > This does work, however this ends up returning 0, which implies that > the atomic check succeded. In my opinion in this case, -EINVAL should > be returned, as both the crtc and fb don't exist, therefore the check > should not succeed. I would personally prefer a more explicit check > that does return -EINVAL instead of 0 from > drm_atomic_helper_check_planes() > As a side note, I'm not sure if there's a reasoning as to why > drm_atomic_helper_check_planes() returns 0 if fb is NULL instead of > -EINVAL, I'm assuming it's not designed to come across this specific > case. Either way it's not too much of an issue but maybe one of the drm > maintainers can clarify why it's this way. Maybe this is a result of the atomic conversions? It's possible that now we get passed NULLs on hotplug and display disables. (I didn't know enough about DRM to be sure and didn't reference that commit in the previous email). I think a return of 0 should be it - both exynos_plane_atomic_check()[1] and virtio_gpu_plane_atomic_check()[2] return 0 on either a NULL fb or crtc - I've tried returning -EINVAL and KDE can no longer disable the display because the rejection is being propagated back to userspace. I'll respin this patch to return 0 after an explicit check and include another NULL dereference fix in the plane update path. Thanks, Shenghao [1] https://elixir.bootlin.com/linux/v6.18.2/source/drivers/gpu/drm/exynos/exynos_drm_plane.c#L231 [2] https://elixir.bootlin.com/linux/v6.18.2/C/ident/virtio_gpu_plane_atomic_check
© 2016 - 2026 Red Hat, Inc.