drm/atomic: Fix memleak on ERESTARTSYS during non-blocking commits

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@vger.kernel.org> # v4.11+
Signed-off-by: Leo (Sunpeng) Li <sunpeng.li@amd.com>
Acked-by: Harry Wentland <harry.wentland@amd.com> #v1
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20180117115108.29608-1-maarten.lankhorst@linux.intel.com
Reviewed-by: Sean Paul <seanpaul@chromium.org>

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index b16f1d69a0bbf345e33e277de5ce73a38974bf73..e8c249361d7e4acebd8e6f2dcf5d50d8d94935dc 100644 (file)
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1778,6 +1778,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) {
@@ -3327,8 +3329,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 5afd6e364fb6774b846134e05a1e33b010672cc1..c63b0b48e884cc9dbc38ef0428b2d0ad761a2775 100644 (file)
--- 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.
+        */
+       bool abort_completion;
 };
 
 struct __drm_planes_state {