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

[Daniel Vetter]

On Mon, Mar 20, 2017 at 09:38:52AM +0100, Maarten Lankhorst wrote:
> Op 20-03-17 om 09:18 schreef 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
> 
> Yes, but I'm still not completely convinced it's required for connector state to use RCU. During detect()
> we would take the connection_mutex anyway, so I can probably  do that for mode_valid as well.

Why do you want to take the connection_mutex there? If we just go with
taking that everywhere we touch shared state, we might as well push that
up in the callchain ... With the connector_iter stuff there's no reason at
all anymore to hold the mode_config.mutex for anything really around
connector probing.

The only thing we need is that we pass the acquire_ctx down into callars
somehow, so that the load detect stuff still works.

But my idea was kinda that we'd do the same for probe -> modeset data
flows like here for the other way round: Just a bunch of READ_ONCE and
maybe lookup the edid with rcu too. That way it's clear to anybody that
probe and modeset are entirely not synchronized.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch