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

[Leo Li]

Updated IGT results seem sane:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7698/shards.html

Would someone be able to apply this patch?

Thanks,
Leo

On 2018-01-17 03:18 PM, Sean Paul wrote:
> 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
>>