Re: [PATCH] drm/gud: fix NULL fb and crtc dereferences on USB disconnect

[Thomas Zimmermann <tzimmermann@suse.de>]

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)