drm/atomic: Fix freeing connector/plane state too early by tracking commits, v3.

Currently we neatly track the crtc state, but forget to look at
plane/connector state.

When doing a nonblocking modeset, immediately followed by a setprop
before the modeset completes, the setprop will see the modesets new
state as the old state and free it.

This has to be solved by waiting for hw_done on the connector, even
if it's not assigned to a crtc. When a connector is unbound we take
the last crtc commit, and when it stays unbound we create a new
fake crtc commit for that gets signaled on hw_done for all the
planes/connectors.

We wait for it the same way as we do for crtc's, which will make
sure we never run into a use-after-free situation.

Changes since v1:
- Only create a single disable commit. (danvet)
- Fix leak in intel_legacy_cursor_update.
Changes since v2:
- Make reference counting in drm_atomic_helper_setup_commit
  more obvious. (pinchartl)
- Call cleanup_done for fake commit. (danvet)
- Add comments to drm_atomic_helper_setup_commit. (danvet, pinchartl)
- Add comment to drm_atomic_helper_swap_state. (pinchartl)

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Testcase: kms_atomic_transition.plane-use-after-nonblocking-unbind*
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20170904104838.23822-6-maarten.lankhorst@linux.intel.com
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 58df70a..98f4239 100644 (file)
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -192,6 +192,10 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state)
        }
        state->num_private_objs = 0;
 
+       if (state->fake_commit) {
+               drm_crtc_commit_put(state->fake_commit);
+               state->fake_commit = NULL;
+       }
 }
 EXPORT_SYMBOL(drm_atomic_state_default_clear);
 

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 8bc2f15..820adcd 100644 (file)
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1667,6 +1667,38 @@ static void release_crtc_commit(struct completion *completion)
        drm_crtc_commit_put(commit);
 }
 
+static void init_commit(struct drm_crtc_commit *commit, struct drm_crtc *crtc)
+{
+       init_completion(&commit->flip_done);
+       init_completion(&commit->hw_done);
+       init_completion(&commit->cleanup_done);
+       INIT_LIST_HEAD(&commit->commit_entry);
+       kref_init(&commit->ref);
+       commit->crtc = crtc;
+}
+
+static struct drm_crtc_commit *
+crtc_or_fake_commit(struct drm_atomic_state *state, struct drm_crtc *crtc)
+{
+       if (crtc) {
+               struct drm_crtc_state *new_crtc_state;
+
+               new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
+
+               return new_crtc_state->commit;
+       }
+
+       if (!state->fake_commit) {
+               state->fake_commit = kzalloc(sizeof(*state->fake_commit), GFP_KERNEL);
+               if (!state->fake_commit)
+                       return NULL;
+
+               init_commit(state->fake_commit, NULL);
+       }
+
+       return state->fake_commit;
+}
+
 /**
  * drm_atomic_helper_setup_commit - setup possibly nonblocking commit
  * @state: new modeset state to be committed
@@ -1715,6 +1747,10 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
 {
        struct drm_crtc *crtc;
        struct drm_crtc_state *old_crtc_state, *new_crtc_state;
+       struct drm_connector *conn;
+       struct drm_connector_state *old_conn_state, *new_conn_state;
+       struct drm_plane *plane;
+       struct drm_plane_state *old_plane_state, *new_plane_state;
        struct drm_crtc_commit *commit;
        int i, ret;
 
@@ -1723,12 +1759,7 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
                if (!commit)
                        return -ENOMEM;
 
-               init_completion(&commit->flip_done);
-               init_completion(&commit->hw_done);
-               init_completion(&commit->cleanup_done);
-               INIT_LIST_HEAD(&commit->commit_entry);
-               kref_init(&commit->ref);
-               commit->crtc = crtc;
+               init_commit(commit, crtc);
 
                new_crtc_state->commit = commit;
 
@@ -1764,6 +1795,42 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
                drm_crtc_commit_get(commit);
        }
 
+       for_each_oldnew_connector_in_state(state, conn, old_conn_state, new_conn_state, i) {
+               /* commit tracked through new_crtc_state->commit, no need to do it explicitly */
+               if (new_conn_state->crtc)
+                       continue;
+
+               /* Userspace is not allowed to get ahead of the previous
+                * commit with nonblocking ones. */
+               if (nonblock && old_conn_state->commit &&
+                   !try_wait_for_completion(&old_conn_state->commit->flip_done))
+                       return -EBUSY;
+
+               commit = crtc_or_fake_commit(state, old_conn_state->crtc);
+               if (!commit)
+                       return -ENOMEM;
+
+               new_conn_state->commit = drm_crtc_commit_get(commit);
+       }
+
+       for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i) {
+               /* commit tracked through new_crtc_state->commit, no need to do it explicitly */
+               if (new_plane_state->crtc)
+                       continue;
+
+               /* Userspace is not allowed to get ahead of the previous
+                * commit with nonblocking ones. */
+               if (nonblock && old_plane_state->commit &&
+                   !try_wait_for_completion(&old_plane_state->commit->flip_done))
+                       return -EBUSY;
+
+               commit = crtc_or_fake_commit(state, old_plane_state->crtc);
+               if (!commit)
+                       return -ENOMEM;
+
+               new_plane_state->commit = drm_crtc_commit_get(commit);
+       }
+
        return 0;
 }
 EXPORT_SYMBOL(drm_atomic_helper_setup_commit);
@@ -1784,6 +1851,10 @@ void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *old_state)
 {
        struct drm_crtc *crtc;
        struct drm_crtc_state *old_crtc_state;
+       struct drm_plane *plane;
+       struct drm_plane_state *old_plane_state;
+       struct drm_connector *conn;
+       struct drm_connector_state *old_conn_state;
        struct drm_crtc_commit *commit;
        int i;
        long ret;
@@ -1808,6 +1879,48 @@ void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *old_state)
                        DRM_ERROR("[CRTC:%d:%s] flip_done timed out\n",
                                  crtc->base.id, crtc->name);
        }
+
+       for_each_old_connector_in_state(old_state, conn, old_conn_state, i) {
+               commit = old_conn_state->commit;
+
+               if (!commit)
+                       continue;
+
+               ret = wait_for_completion_timeout(&commit->hw_done,
+                                                 10*HZ);
+               if (ret == 0)
+                       DRM_ERROR("[CONNECTOR:%d:%s] hw_done timed out\n",
+                                 conn->base.id, conn->name);
+
+               /* Currently no support for overwriting flips, hence
+                * stall for previous one to execute completely. */
+               ret = wait_for_completion_timeout(&commit->flip_done,
+                                                 10*HZ);
+               if (ret == 0)
+                       DRM_ERROR("[CONNECTOR:%d:%s] flip_done timed out\n",
+                                 conn->base.id, conn->name);
+       }
+
+       for_each_old_plane_in_state(old_state, plane, old_plane_state, i) {
+               commit = old_plane_state->commit;
+
+               if (!commit)
+                       continue;
+
+               ret = wait_for_completion_timeout(&commit->hw_done,
+                                                 10*HZ);
+               if (ret == 0)
+                       DRM_ERROR("[PLANE:%d:%s] hw_done timed out\n",
+                                 plane->base.id, plane->name);
+
+               /* Currently no support for overwriting flips, hence
+                * stall for previous one to execute completely. */
+               ret = wait_for_completion_timeout(&commit->flip_done,
+                                                 10*HZ);
+               if (ret == 0)
+                       DRM_ERROR("[PLANE:%d:%s] flip_done timed out\n",
+                                 plane->base.id, plane->name);
+       }
 }
 EXPORT_SYMBOL(drm_atomic_helper_wait_for_dependencies);
 
@@ -1852,6 +1965,11 @@ void drm_atomic_helper_commit_hw_done(struct drm_atomic_state *old_state)
                WARN_ON(new_crtc_state->event);
                complete_all(&commit->hw_done);
        }
+
+       if (old_state->fake_commit) {
+               complete_all(&old_state->fake_commit->hw_done);
+               complete_all(&old_state->fake_commit->flip_done);
+       }
 }
 EXPORT_SYMBOL(drm_atomic_helper_commit_hw_done);
 
@@ -1885,6 +2003,9 @@ void drm_atomic_helper_commit_cleanup_done(struct drm_atomic_state *old_state)
                list_del(&commit->commit_entry);
                spin_unlock(&crtc->commit_lock);
        }
+
+       if (old_state->fake_commit)
+               complete_all(&old_state->fake_commit->cleanup_done);
 }
 EXPORT_SYMBOL(drm_atomic_helper_commit_cleanup_done);
 
@@ -2264,6 +2385,15 @@ int drm_atomic_helper_swap_state(struct drm_atomic_state *state,
        struct drm_private_state *old_obj_state, *new_obj_state;
 
        if (stall) {
+               /*
+                * We have to stall for hw_done here before
+                * drm_atomic_helper_wait_for_dependencies() because flip
+                * depth > 1 is not yet supported by all drivers. As long as
+                * obj->state is directly dereferenced anywhere in the drivers
+                * atomic_commit_tail function, then it's unsafe to swap state
+                * before drm_atomic_helper_commit_hw_done() is called.
+                */
+
                for_each_old_crtc_in_state(state, crtc, old_crtc_state, i) {
                        commit = old_crtc_state->commit;
 
@@ -2274,6 +2404,28 @@ int drm_atomic_helper_swap_state(struct drm_atomic_state *state,
                        if (ret)
                                return ret;
                }
+
+               for_each_old_connector_in_state(state, connector, old_conn_state, i) {
+                       commit = old_conn_state->commit;
+
+                       if (!commit)
+                               continue;
+
+                       ret = wait_for_completion_interruptible(&commit->hw_done);
+                       if (ret)
+                               return ret;
+               }
+
+               for_each_old_plane_in_state(state, plane, old_plane_state, i) {
+                       commit = old_plane_state->commit;
+
+                       if (!commit)
+                               continue;
+
+                       ret = wait_for_completion_interruptible(&commit->hw_done);
+                       if (ret)
+                               return ret;
+               }
        }
 
        for_each_oldnew_connector_in_state(state, connector, old_conn_state, new_conn_state, i) {
@@ -3242,6 +3394,7 @@ void __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane,
                drm_framebuffer_get(state->fb);
 
        state->fence = NULL;
+       state->commit = NULL;
 }
 EXPORT_SYMBOL(__drm_atomic_helper_plane_duplicate_state);
 
@@ -3283,6 +3436,9 @@ void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state)
 
        if (state->fence)
                dma_fence_put(state->fence);
+
+       if (state->commit)
+               drm_crtc_commit_put(state->commit);
 }
 EXPORT_SYMBOL(__drm_atomic_helper_plane_destroy_state);
 
@@ -3361,6 +3517,7 @@ __drm_atomic_helper_connector_duplicate_state(struct drm_connector *connector,
        memcpy(state, connector->state, sizeof(*state));
        if (state->crtc)
                drm_connector_get(connector);
+       state->commit = NULL;
 }
 EXPORT_SYMBOL(__drm_atomic_helper_connector_duplicate_state);
 
@@ -3487,6 +3644,9 @@ __drm_atomic_helper_connector_destroy_state(struct drm_connector_state *state)
 {
        if (state->crtc)
                drm_connector_put(state->connector);
+
+       if (state->commit)
+               drm_crtc_commit_put(state->commit);
 }
 EXPORT_SYMBOL(__drm_atomic_helper_connector_destroy_state);
 

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 67a8b6d..2e7376f 100644 (file)
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13616,8 +13616,10 @@ intel_legacy_cursor_update(struct drm_plane *plane,
 
        /* Swap plane state */
        new_plane_state->fence = old_plane_state->fence;
+       new_plane_state->commit = old_plane_state->commit;
        *to_intel_plane_state(old_plane_state) = *to_intel_plane_state(new_plane_state);
        new_plane_state->fence = NULL;
+       new_plane_state->commit = NULL;
        new_plane_state->fb = old_fb;
        to_intel_plane_state(new_plane_state)->vma = old_vma;
 

diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index a80a8da..07a71da 100644 (file)
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -236,6 +236,18 @@ struct drm_atomic_state {
        struct drm_modeset_acquire_ctx *acquire_ctx;
 
        /**
+        * @fake_commit:
+        *
+        * Used for signaling unbound planes/connectors.
+        * When a connector or plane is not bound to any CRTC, it's still important
+        * to preserve linearity to prevent the atomic states from being freed to early.
+        *
+        * This commit (if set) is not bound to any crtc, but will be completed when
+        * drm_atomic_helper_commit_hw_done() is called.
+        */
+       struct drm_crtc_commit *fake_commit;
+
+       /**
         * @commit_work:
         *
         * Work item which can be used by the driver or helpers to execute the

diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index ea8da40..8837649 100644 (file)
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -347,6 +347,13 @@ struct drm_connector_state {
 
        struct drm_atomic_state *state;
 
+       /**
+        * @commit: Tracks the pending commit to prevent use-after-free conditions.
+        *
+        * Is only set when @crtc is NULL.
+        */
+       struct drm_crtc_commit *commit;
+
        struct drm_tv_connector_state tv;
 
        /**

diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
index 73f90f9..7d96116 100644 (file)
--- a/include/drm/drm_plane.h
+++ b/include/drm/drm_plane.h
@@ -123,6 +123,13 @@ struct drm_plane_state {
         */
        bool visible;
 
+       /**
+        * @commit: Tracks the pending commit to prevent use-after-free conditions.
+        *
+        * Is only set when @crtc is NULL.
+        */
+       struct drm_crtc_commit *commit;
+
        struct drm_atomic_state *state;
 };