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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

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: OpenPGP_signature
Description: OpenPGP digital signature


[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux