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

[Thomas Zimmermann <tzimmermann@suse.de>]

[-- Attachment #1.1: Type: text/plain, Size: 4347 bytes --]

Hi Javier

Am 24.07.22 um 20:41 schrieb Javier Martinez Canillas:
> Hello Thomas,
> 
> Thanks for your feedback.
> 
> On 7/24/22 20:24, Thomas Zimmermann wrote:
>> Hi Javier
>>
>> Am 24.07.22 um 14:37 schrieb Javier Martinez Canillas:
>>> DRM drivers initialize the mode configuration with drmm_mode_config_init()
>>> and that function (among other things) initializes mutexes that are later
>>> used by modeset helpers.
>>>
>>> But the helpers should only attempt to grab those locks if the mode config
>>> was properly initialized. Otherwise it can lead to kernel oops. An example
>>> is when a DRM driver using the component framework does not initialize the
>>> drm_mode_config, because its .bind callback was not being executed due one
>>> of its expected sub-devices' driver failing to probe.
>>>
>>> Some drivers check the struct drm_driver.registered field as an indication
>>> on whether their .shutdown callback should call helpers to tearn down the
>>> mode configuration or not, but most drivers just assume that it is always
>>> safe to call helpers such as drm_atomic_helper_shutdown() during shutdown.
>>>
>>> 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.

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.

> 
> 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@redhat.com/

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

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.

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.

Best regards
Thomas



>   
>> Best regards
>> Thomas
>>
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]