[Intel-gfx] [PATCH] drm/atomic: protect crtc|connector->state with rcu

[Daniel Vetter]

On Fri, Mar 17, 2017 at 05:52:32PM +0100, Maarten Lankhorst wrote:
> Op 16-03-17 om 21:15 schreef Daniel Vetter:
> > On Thu, Mar 16, 2017 at 5:09 PM, Maarten Lankhorst
> > <maarten.lankhorst at linux.intel.com> wrote:
> >> Op 16-03-17 om 16:52 schreef Daniel Vetter:
> >>> The vblank code really wants to look at crtc->state without having to
> >>> take a ww_mutex. One option might be to take one of the vblank locks
> >>> right when assigning crtc->state, which would ensure that the vblank
> >>> code doesn't race and access freed memory.
> >> I'm not sure this is the right approach for vblank.
> > It's not, it's just that I've had to resurrect this patch quickly
> > before leaving and accidentally left the vblank stuff in.
> >
> >> crtc->state might not be the same as the current state in case of a nonblocking
> >> modeset that hasn't even disabled the old crtc yet.
> >>> But userspace tends to poke the vblank_ioctl to query the current
> >>> vblank timestamp rather often, and going all in with rcu would help a
> >>> bit. We're not there yet, but also doesn't really hurt.
> >>>
> >>> v2: Maarten needs this to make connector properties atomic, so he can
> >>> peek at state from the various ->mode_valid hooks.
> >>>
> >>> Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> >>> Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
> >>> ---
> >>>  drivers/gpu/drm/drm_atomic.c        | 26 +++++++++++++++++---------
> >>>  drivers/gpu/drm/drm_atomic_helper.c |  2 +-
> >>>  include/drm/drm_atomic.h            |  5 +++++
> >>>  include/drm/drm_connector.h         | 13 ++++++++++++-
> >>>  include/drm/drm_crtc.h              |  9 ++++++++-
> >>>  5 files changed, 43 insertions(+), 12 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> >>> index 9b892af7811a..ba11e31ff9ba 100644
> >>> --- a/drivers/gpu/drm/drm_atomic.c
> >>> +++ b/drivers/gpu/drm/drm_atomic.c
> >>> @@ -213,16 +213,10 @@ void drm_atomic_state_clear(struct drm_atomic_state *state)
> >>>  }
> >>>  EXPORT_SYMBOL(drm_atomic_state_clear);
> >>>
> >>> -/**
> >>> - * __drm_atomic_state_free - free all memory for an atomic state
> >>> - * @ref: This atomic state to deallocate
> >>> - *
> >>> - * This frees all memory associated with an atomic state, including all the
> >>> - * per-object state for planes, crtcs and connectors.
> >>> - */
> >>> -void __drm_atomic_state_free(struct kref *ref)
> >>> +void ___drm_atomic_state_free(struct rcu_head *rhead)
> >>>  {
> >>> -     struct drm_atomic_state *state = container_of(ref, typeof(*state), ref);
> >>> +     struct drm_atomic_state *state =
> >>> +             container_of(rhead, typeof(*state), rhead);
> >>>       struct drm_mode_config *config = &state->dev->mode_config;
> >>>
> >>>       drm_atomic_state_clear(state);
> >>> @@ -236,6 +230,20 @@ void __drm_atomic_state_free(struct kref *ref)
> >>>               kfree(state);
> >>>       }
> >>>  }
> >> whatisRCU.txt:
> >> "This function invokes func(head) after a grace period has elapsed.
> >> This invocation might happen from either softirq or process context,
> >> so the function is not permitted to block."
> >>
> >> Looking at
> >> commit 6f0f02dc56f18760b46dc1bf5b3f7386869d4162
> >> Author: Chris Wilson <chris at chris-wilson.co.uk>
> >> Date:   Mon Jan 23 21:29:39 2017 +0000
> >>
> >>     drm/i915: Move atomic state free from out of fence release
> >>
> >> I would say that drm_atomic_state_free would definitely block..
> >>
> >> The only thing that makes sense IMO is doing kfree_rcu on the object_states.
> >> But I don't think RCU is the answer here, it won't protect you against using
> >> the wrong crtc state.
> >>
> >> I think I would try to use the crtc ww_mutex in the vblank code and serialize it to pending commits, if at all possible.
> > Oops. I guess it should have come with "totally untested". In that
> > case we need a workter which does a synchronize_rcu() before
> > releasing. I don't just want to do a simple kfree_rcu() because by
> > that point we've (partially) destroyed the state alreayd (so it's
> > already unsafe to access, and special ruels are ugly). And doing it
> > here before we release anything in the core would avoid the need for
> > drivers to bother with kfree_rcu().
> >
> > I'll try to respin something less obviously buggy tomorrow :-)
> It will still be buggy tomorrow, since you have no way to know if the current hardware crtc_state is anything like crtc->state.
> 
> :(

Maybe I wasnt' clear, so let me retry:

- this approach doesn't work for vblank and crtc state. Agreed. I'll
  remove all the leftover comments I've forgotten to remove in a hurry.

- the patch itself is broken, so can't be used for connector->state
  peeking in mode_valid either. That one I'll fix.

Does that make sense?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch