[PATCH] drm: allocate crtc mutex separately from crtc

[Daniel Vetter]

On Thu, Oct 17, 2013 at 1:35 AM, Ilija Hadzic <ilijahadzic at gmail.com> wrote:
> Embedding a mutex inside drm_crtc structure evokes a
> subtle and rare corruption in drm_crtc_helper_set_config
> failure-recovery path.
>
> Namely, if drm_crtc_helper_set_config takes the
> path under 'fail:' label *and* some other process
> has attempted to grab the crtc mutex (and got blocked),
> restoring the CRTC structure (which is done by bit-copying
> the entire structure from saved_crtcs array) will overwrite
> the mutex state and the waiters list pointer within the mutex
> structure. Consequently the blocked process will never
> be scheduled.
>
> This patch fixes the issue by un-embeding the mutex structure
> and allocating it separately from the drm_crtc structure
> when the CRTC is initialized. The bit-copying in
> drm_crtc_helper_set_config helper will now overwrite the pointer
> which is never modified during the CRTC's lifetime, thus
> avoiding corruption.
>
> Signed-off-by: Ilija Hadzic <ihadzic at research.bell-labs.com>
> Cc: stable at vger.kernel.org

Can't we instead just copy the few things we need to restore back?
This wholesale structure copying has rather tricky semantics and is
bound to trick up someone else in the future. And indeed we seem to
have a similar (but less catastrophic) thing going on with crtc->fb I
think.

I've looked a bit through the code and I think we don't actually need
to restore anything for crtcs. We pass the mode, fb and offsets in
explicitly, and everything else in drm_crtc is derived state. This is
also the same that i915 does, we only restore the connector/encoder
links even.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch