lkml.org 
[lkml]   [2022]   [Jul]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH] drm: Prevent modeset helpers to access an uninitialized drm_mode_config
From
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
[unhandled content-type:application/pgp-signature]
\
 
 \ /
  Last update: 2022-07-25 09:13    [W:0.580 / U:0.024 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site