Re: [PATCH] drm: Prevent modeset helpers to access an uninitialized drm_mode_config

[Javier Martinez Canillas]

Hello Thomas,

On 7/25/22 09:12, Thomas Zimmermann wrote:

[...]

>>>>
>>>> Let make the DRM core more robust and prevent this to happen, by marking a
>>>> struct drm_mode_config as initialized during drmm_mode_config_init(). that
>>>> way helpers can check for it and not attempt to grab uninitialized mutexes.
>>>
>>> I disagree. This patch looks like cargo-cult programming and entirely
>>> arbitrary.  The solution here is to fix drivers.  The actual test to
>>> perform is to instrument the mutex implementation to detect
>>> uninitialized mutexes.
>>>
>>
>> While I do agree that drivers should be fixed, IMO we should try to make it
>> hard for the kernel to crash. We already have checks in other DRM helpers to
>> avoid accessing uninitialized data, so I don't see why we couldn't do the
>> same here.
> 
> Code should stand on its own merits, instead of doing something because 
> something else does it. The latter is what is referred to as cargo-cult 
> programming.
>

Yes, I'm familiar with the concept but was thinking about a different paradigm
when writing this patch that is defensive programming. The question is whether
makes sense for the DRM core to defense from buggy drivers. My opinion is that
if possible it should and we should prevent kernel panics at all costs.

Otherwise, let's say a reboot may crash and never finish unless you have a HW
watchdog or something to bring the system again into a functional state.

But I agree that this particular patch is not nice and in fact I considered to
post it as a RFC at the beginning. 
 
> Doing sanity checks on values is not a problem, but putting flag 
> variables throughout the code to question other code's state is. That's 
> not 'The Way of the C.' There's also the problem that a good part of 
> struct drm_mode_config's initialization is open-coded in drivers. So the 
> meaning of is_initialized is somewhat fuzzy.
>

That is true. It is meant to signal that at least drm_mode_config_init() was
called by the driver.

>>
>> I wrote this patch after fixing a bug in the drm/msm driver [0]. By looking
>> at how other drivers handled this case, I'm pretty sure that they have the
>> same problem. A warning is much better than a kernel crash during shutdown.
>>
>> [0]: https://patchwork.kernel.org/project/dri-devel/patch/20220724111327.1195693-1-javierm@xxxxxxxxxx/
> 
> I see. I wasn't aware that missing mode_config_init() is a problem. From 
> the linked URL, I cannot really understand how it's related. msm appears 
> to be calling drm_mode_config_init(), right? The idiomatic solution 
> would be to convert msm to managed code. But that's an entirely 
> different patchset, of course. (I only took a brief look at the link TBH.)
> 

The problem as mentioned is that drivers could call the modeset helpers even
when that init function was never called. In this case due the .bind callback
not being executed because a probe failed for a driver of a expected sub-device
failed.

The .shutdown callback is registered when the platform_driver is registered
and that happens at module_init(), so the shutdown would be executed regardless
of the probe (or bind) doing a proper initialization.

I think we need some way to signal drivers about that. Even better we could use
it in the core helpers to not attempt to access struct drm_device data if the
DRM device was not properly initialized.

Maybe the drm_device .registered field is enough and we could reuse that ?

> Here's a suggestion on how to construct the mode-config code in order to 
> make it hard to misuse:  Driver currently open-code the initialization 
> of many fields in drm_mode_config. Expand the arguments of 
> drm_mode_config_init() to take the pointer to the drm_mode_config_funcs. 
> These functions are essential to do anything, so it's a good candidate 
> for an argument. Drivers are easily converted the the new interface 
> AFAICT.  After the conversion, add a test to drm_mode_config_reset() 
> that tests for the funcs to be set.  drm_mode_config_reset() is also 
> essential during initialization or the driver will fail immediately on 
> the first modeset operation. That gives a test for an initialized 
> mode_config without adding extra fields.
>

I agree that all these are nice things to do but I can't see how it could help
in this case. If all these are still done in the .bind callback then it might
never be executed and the .shutdown callback would still happily call to the
drm_atomic_helper_shutdown() which will try to grab uninitialized mutexes and
crash the kernel.

Yes, I already posted a patch for msm/drm to prevent this but as mentioned a
lot of drivers still have that issue. We could audit all drivers and fix them
but I think we need to make the core more robust.

> As a bit of a sidenote: we should consider making 
> drm_mode_config_reset() and the reset callbacks return errors. The reset 
> functions allocate memory for states and if this fails, we have no way 
> of reporting the failure.
>

Yes, agreed.
 
> Best regards
> Thomas
> 

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat