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

[Maarten Lankhorst]

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.

:(