Re: [PATCH 4/5] drm/atomic: Fix freeing connector/plane state too early by tracking commits, v2.

Hi Maarten,

Thank you for the patch.

On Wednesday, 30 August 2017 15:17:51 EEST Maarten Lankhorst wrote:

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.

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

---

drivers/gpu/drm/drm_atomic.c | 4 + drivers/gpu/drm/drm_atomic_helper.c | 156 ++++++++++++++++++++++++++++++-- drivers/gpu/drm/i915/intel_display.c | 2 + include/drm/drm_atomic.h | 12 +++ include/drm/drm_connector.h | 7 ++ include/drm/drm_plane.h | 7 ++ 6 files changed, 182 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 2cce48f203e0..75f5f74de9bf 100644 --- 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 8ccb8b6536c0..034f563fb130 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -1644,6 +1644,40 @@ 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) +{

You could allocate the commit in this function too, the kzalloc() is currently duplicated. The function should probably be called alloc_commit() then.

• 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 * +fake_or_crtc_commit(struct drm_atomic_state *state, struct drm_crtc *crtc) +{

• struct drm_crtc_commit *commit;
• if (crtc) {

• struct drm_crtc_state *new_crtc_state;
  

• new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
  

• commit = new_crtc_state->commit;
  

• } else if (!state->fake_commit) {

• state->fake_commit = commit = kzalloc(sizeof(*commit), GFP_KERNEL);
  

• if (!commit)
  

• 	return NULL;
  

• init_commit(commit, NULL);
  

• } else

• commit = state->fake_commit;
  

• drm_crtc_commit_get(commit);

I believe the reference counting is right. The double reference in the second case (kref_init() when initializing the commit and drm_crtc_commit_get()) should not cause a leak. The kref_init() takes a reference to store the commit in state->fake_commit, released in drm_atomic_state_default_clear(), and the drm_crtc_commit_get() takes a reference returned by the function, stored in new_*_state->commit by the caller.

This being said, I think the reference counting is confusing, as proved by Daniel thinking there was a leak here (or by me thinking there's no leak while there's one :-)). To make the implementation clearer, I propose turning the definition of drm_crtc_commit_get() to

static inline struct drm_crtc_commit * drm_crtc_commit_get(struct drm_crtc_commit *commit) { kref_get(&commit->ref); return commit; }

and writing this function as

/* Return a new reference to the commit object */ static struct drm_crtc_commit * fake_or_crtc_commit(struct drm_atomic_state *state, struct drm_crtc *crtc) { struct drm_crtc_commit *commit;

if (crtc) { struct drm_crtc_state *new_crtc_state;

new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc);

commit = new_crtc_state->commit; } else { if (!state->fake_commit) state->fake_commit = alloc_commit(NULL);

commit = state->fake_commit; }

return drm_crtc_commit_get(commit); }

• return commit;

+}

/**

• drm_atomic_helper_setup_commit - setup possibly nonblocking commit
• @state: new modeset state to be committed

@@ -1692,6 +1726,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;

@@ -1700,12 +1738,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;

@@ -1741,6 +1774,36 @@ 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) {

• if (new_conn_state->crtc)
  

• 	continue;
  

• if (nonblock && old_conn_state->commit &&
  

•     !try_wait_for_completion(&old_conn_state->commit->flip_done))
  

• 	return -EBUSY;
  

flip_done only, not hw_done ? A comment that explains why would be helpful.

• commit = fake_or_crtc_commit(state, old_conn_state->crtc);
  

• if (!commit)
  

• 	return -ENOMEM;
  

• new_conn_state->commit = commit;
  

• }
• for_each_oldnew_plane_in_state(state, plane, old_plane_state,

new_plane_state, i) {

• if (new_plane_state->crtc)
  

• 	continue;
  

• if (nonblock && old_plane_state->commit &&
  

•     !try_wait_for_completion(&old_plane_state->commit->flip_done))
  

• 	return -EBUSY;
  

Same here.

• commit = fake_or_crtc_commit(state, old_plane_state->crtc);
  

• if (!commit)
  

• 	return -ENOMEM;
  

• new_plane_state->commit = commit;
  

• }
• return 0;

} EXPORT_SYMBOL(drm_atomic_helper_setup_commit); @@ -1761,6 +1824,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;

@@ -1785,6 +1852,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);
  

There's probably something I don't get, but given that the connector state -

commit pointer is only set for commits that have no CRTC, do we have a

guarantee that there will be a page flip to be signalled ? Can't we for instance disable a connector without a page flip ? Or set a property on an already disabled connector ?

• }
• 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);

@@ -1819,6 +1928,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);

@@ -2241,6 +2355,28 @@ int drm_atomic_helper_swap_state(struct drm_atomic_state *state, if (ret) return ret; }

A comment would be helpful here too. In particular explaining why you wait on hw_done only isn't straightforward.

• 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) { @@ -3223,6 +3359,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);

@@ -3264,6 +3401,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);

@@ -3342,6 +3482,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);

@@ -3468,6 +3609,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 3f3cb96aa11e..70ce02753177 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -13109,8 +13109,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 = NULL;

diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h index e76d9f95c191..ba976f4a4938 100644 --- 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 ea8da401c93c..8837649d16e8 100644 --- 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 73f90f9d057f..7d96116fd4c4 100644 --- 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;

};