mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
* [PATCH] drm/gud: fix NULL fb and crtc dereferences on USB disconnect
@ 2025-12-31  5:50 Shenghao Yang
  2026-01-03 17:23 ` Ruben Wauters
  0 siblings, 1 reply; 10+ messages in thread
From: Shenghao Yang @ 2025-12-31  5:50 UTC (permalink / raw)
  To: Ruben Wauters, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, David Airlie, Simona Vetter
  Cc: dri-devel, linux-kernel, Shenghao Yang, stable

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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] drm/gud: fix NULL fb and crtc dereferences on USB disconnect
  2025-12-31  5:50 [PATCH] drm/gud: fix NULL fb and crtc dereferences on USB disconnect Shenghao Yang
@ 2026-01-03 17:23 ` Ruben Wauters
  2026-01-03 17:47   ` Shenghao Yang
  0 siblings, 1 reply; 10+ messages in thread
From: Ruben Wauters @ 2026-01-03 17:23 UTC (permalink / raw)
  To: Shenghao Yang, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, David Airlie, Simona Vetter
  Cc: dri-devel, linux-kernel, stable

[-- Attachment #1: Type: text/plain, Size: 3079 bytes --]

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) {

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] drm/gud: fix NULL fb and crtc dereferences on USB disconnect
  2026-01-03 17:23 ` Ruben Wauters
@ 2026-01-03 17:47   ` Shenghao Yang
  2026-01-03 19:18     ` Ruben Wauters
  0 siblings, 1 reply; 10+ messages in thread
From: Shenghao Yang @ 2026-01-03 17:47 UTC (permalink / raw)
  To: Ruben Wauters, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, David Airlie, Simona Vetter
  Cc: dri-devel, linux-kernel, stable

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] drm/gud: fix NULL fb and crtc dereferences on USB disconnect
  2026-01-03 17:47   ` Shenghao Yang
@ 2026-01-03 19:18     ` Ruben Wauters
  2026-01-04 16:30       ` Shenghao Yang
  2026-01-07  7:46       ` Thomas Zimmermann
  0 siblings, 2 replies; 10+ messages in thread
From: Ruben Wauters @ 2026-01-03 19:18 UTC (permalink / raw)
  To: Shenghao Yang, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, David Airlie, Simona Vetter
  Cc: dri-devel, linux-kernel, stable

[-- Attachment #1: Type: text/plain, Size: 2169 bytes --]

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

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] drm/gud: fix NULL fb and crtc dereferences on USB disconnect
  2026-01-03 19:18     ` Ruben Wauters
@ 2026-01-04 16:30       ` Shenghao Yang
  2026-01-07  7:46       ` Thomas Zimmermann
  1 sibling, 0 replies; 10+ messages in thread
From: Shenghao Yang @ 2026-01-04 16:30 UTC (permalink / raw)
  To: Ruben Wauters, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, David Airlie, Simona Vetter
  Cc: dri-devel, linux-kernel, stable

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] drm/gud: fix NULL fb and crtc dereferences on USB disconnect
  2026-01-03 19:18     ` Ruben Wauters
  2026-01-04 16:30       ` Shenghao Yang
@ 2026-01-07  7:46       ` Thomas Zimmermann
  2026-01-07 15:02         ` Ruben Wauters
  1 sibling, 1 reply; 10+ messages in thread
From: Thomas Zimmermann @ 2026-01-07  7:46 UTC (permalink / raw)
  To: Ruben Wauters, Shenghao Yang, Maarten Lankhorst, Maxime Ripard,
	David Airlie, Simona Vetter
  Cc: dri-devel, linux-kernel, stable

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)



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] drm/gud: fix NULL fb and crtc dereferences on USB disconnect
  2026-01-07  7:46       ` Thomas Zimmermann
@ 2026-01-07 15:02         ` Ruben Wauters
  2026-01-07 15:56           ` Thomas Zimmermann
  0 siblings, 1 reply; 10+ messages in thread
From: Ruben Wauters @ 2026-01-07 15:02 UTC (permalink / raw)
  To: Thomas Zimmermann, Shenghao Yang, Maarten Lankhorst,
	Maxime Ripard, David Airlie, Simona Vetter
  Cc: dri-devel, linux-kernel, stable

[-- Attachment #1: Type: text/plain, Size: 3647 bytes --]

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

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] drm/gud: fix NULL fb and crtc dereferences on USB disconnect
  2026-01-07 15:02         ` Ruben Wauters
@ 2026-01-07 15:56           ` Thomas Zimmermann
  2026-01-08 13:39             ` Shenghao Yang
  0 siblings, 1 reply; 10+ messages in thread
From: Thomas Zimmermann @ 2026-01-07 15:56 UTC (permalink / raw)
  To: Ruben Wauters, Shenghao Yang, Maarten Lankhorst, Maxime Ripard,
	David Airlie, Simona Vetter
  Cc: dri-devel, linux-kernel, stable

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)



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] drm/gud: fix NULL fb and crtc dereferences on USB disconnect
  2026-01-07 15:56           ` Thomas Zimmermann
@ 2026-01-08 13:39             ` Shenghao Yang
  2026-01-13 14:05               ` Ruben Wauters
  0 siblings, 1 reply; 10+ messages in thread
From: Shenghao Yang @ 2026-01-08 13:39 UTC (permalink / raw)
  To: Thomas Zimmermann, Ruben Wauters, Maarten Lankhorst,
	Maxime Ripard, David Airlie, Simona Vetter
  Cc: dri-devel, linux-kernel, stable

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] drm/gud: fix NULL fb and crtc dereferences on USB disconnect
  2026-01-08 13:39             ` Shenghao Yang
@ 2026-01-13 14:05               ` Ruben Wauters
  0 siblings, 0 replies; 10+ messages in thread
From: Ruben Wauters @ 2026-01-13 14:05 UTC (permalink / raw)
  To: Shenghao Yang, Thomas Zimmermann, Maarten Lankhorst,
	Maxime Ripard, David Airlie, Simona Vetter
  Cc: dri-devel, linux-kernel, stable

[-- Attachment #1: Type: text/plain, Size: 783 bytes --]

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

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 870 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2026-01-13 14:26 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2025-12-31  5:50 [PATCH] drm/gud: fix NULL fb and crtc dereferences on USB disconnect Shenghao Yang
2026-01-03 17:23 ` Ruben Wauters
2026-01-03 17:47   ` Shenghao Yang
2026-01-03 19:18     ` Ruben Wauters
2026-01-04 16:30       ` Shenghao Yang
2026-01-07  7:46       ` Thomas Zimmermann
2026-01-07 15:02         ` Ruben Wauters
2026-01-07 15:56           ` Thomas Zimmermann
2026-01-08 13:39             ` Shenghao Yang
2026-01-13 14:05               ` Ruben Wauters

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®