Message-ID: <YxeeOJJ17L95LruS@phenom.ffwll.local>
Date:   Tue, 6 Sep 2022 21:23:36 +0200
From:   Daniel Vetter <daniel@...ll.ch>
To:     Javier Martinez Canillas <javierm@...hat.com>
Cc:     Thomas Zimmermann <tzimmermann@...e.de>,
        linux-kernel@...r.kernel.org, David Airlie <airlied@...ux.ie>,
        Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
        dri-devel@...ts.freedesktop.org
Subject: Re: [PATCH] drm: Prevent modeset helpers to access an uninitialized
 drm_mode_config

On Mon, Jul 25, 2022 at 10:28:21AM +0200, Javier Martinez Canillas wrote:
> 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. 

Way late on this, but because it's a bit more fundamental discussions I
figured maybe still useful when I chime in here ...

Imo the fix for this is drmm_ and aggressively removing driver cleanup
code everywhere by using drmm_ and devm_ for everything.

Trying to make these functions robust against arbitrary driver bugs is a
lost play imo, and the fundamental fix is making cleanup code fool proof
by making it all unecessary.

Ideally we'd be able to unexport all the explicit cleanup functions so
that drivers just cannot get anything wrong here. We're a long way from
that world though :-(
-Daniel


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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Powered by blists - more mailing lists