[Intel-gfx] [PATCH] drm/atomic: Fix memleak on ERESTARTSYS during non-blocking commits

[Sean Paul]

On Wed, Jan 17, 2018 at 10:39 AM, Maarten Lankhorst
<maarten.lankhorst at linux.intel.com> wrote:
> Op 17-01-18 om 19:29 schreef Sean Paul:
>> On Wed, Jan 17, 2018 at 12:51:08PM +0100, Maarten Lankhorst wrote:
>>> From: "Leo (Sunpeng) Li" <sunpeng.li at amd.com>
>>>
>>> During a non-blocking commit, it is possible to return before the
>>> commit_tail work is queued (-ERESTARTSYS, for example).
>>>
>>> Since a reference on the crtc commit object is obtained for the pending
>>> vblank event when preparing the commit, the above situation will leave
>>> us with an extra reference.
>>>
>>> Therefore, if the commit_tail worker has not consumed the event at the
>>> end of a commit, release it's reference.
>>>
>>> Changes since v1:
>>> - Also check for state->event->base.completion being set, to
>>>   handle the case where stall_checks() fails in setup_crtc_commit().
>>> Changes since v2:
>>> - Add a flag to drm_crtc_commit, to prevent dereferencing a freed event.
>>>   i915 may unreference the state in a worker.
>>>
>>> Fixes: 24835e442f28 ("drm: reference count event->completion")
>>> Cc: <stable at vger.kernel.org> # v4.11+
>>> Signed-off-by: Leo (Sunpeng) Li <sunpeng.li at amd.com>
>>> Acked-by: Harry Wentland <harry.wentland at amd.com> #v1
>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
>>> ---
>>>  drivers/gpu/drm/drm_atomic_helper.c | 15 +++++++++++++++
>>>  include/drm/drm_atomic.h            |  9 +++++++++
>>>  2 files changed, 24 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
>>> index ab4032167094..ae3cbfe9e01c 100644
>>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>>> @@ -1878,6 +1878,8 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
>>>              new_crtc_state->event->base.completion = &commit->flip_done;
>>>              new_crtc_state->event->base.completion_release = release_crtc_commit;
>>>              drm_crtc_commit_get(commit);
>>> +
>>> +            commit->abort_completion = true;
>>>      }
>>>
>>>      for_each_oldnew_connector_in_state(state, conn, old_conn_state, new_conn_state, i) {
>>> @@ -3421,8 +3423,21 @@ EXPORT_SYMBOL(drm_atomic_helper_crtc_duplicate_state);
>>>  void __drm_atomic_helper_crtc_destroy_state(struct drm_crtc_state *state)
>>>  {
>>>      if (state->commit) {
>>> +            /*
>>> +             * In the event that a non-blocking commit returns
>>> +             * -ERESTARTSYS before the commit_tail work is queued, we will
>>> +             * have an extra reference to the commit object. Release it, if
>>> +             * the event has not been consumed by the worker.
>>> +             *
>>> +             * state->event may be freed, so we can't directly look at
>>> +             * state->event->base.completion.
>>> +             */
>>> +            if (state->event && state->commit->abort_completion)
>>> +                    drm_crtc_commit_put(state->commit);
>>> +
>>>              kfree(state->commit->event);
>>>              state->commit->event = NULL;
>>> +
>>>              drm_crtc_commit_put(state->commit);
>>>      }
>>>
>>> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
>>> index 1c27526c499e..cf13842a6dbd 100644
>>> --- a/include/drm/drm_atomic.h
>>> +++ b/include/drm/drm_atomic.h
>>> @@ -134,6 +134,15 @@ struct drm_crtc_commit {
>>>       * &drm_pending_vblank_event pointer to clean up private events.
>>>       */
>>>      struct drm_pending_vblank_event *event;
>>> +
>>> +    /**
>>> +     * @abort_completion:
>>> +     *
>>> +     * A flag that's set after drm_atomic_helper_setup_commit takes a second
>>> +     * reference for the completion of $drm_crtc_state.event. It's used by
>>> +     * the free code to remove the second reference if commit fails.
>>> +     */
>> Perhaps it's just me, or I'm oversimplifying the problem. I think this would
>> be easier to understand if we just dropped the additional reference at the point
>> of failure (ie: in swap_state). That way we don't have to add Yet Another Piece
>> Of State.
>
> That's assuming nothing can fail between setup_commit() and swap_state() and
> also that the driver implementing atomci puts no calls in between. And also
> assumes that even setup_commit has proper rollback. I think it's overkill,
> and we have no choice but to do it like this. :(
>

yeah, fair enough.

Reviewed-by: Sean Paul <seanpaul at chromium.org>

> ~Maarten
>