Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 21a01abb authored by Maarten Lankhorst's avatar Maarten Lankhorst
Browse files

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: default avatarMaarten 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: default avatarDaniel Vetter <daniel.vetter@ffwll.ch>
parent de39bec1

drivers/gpu/drm/drm_atomic.c

+4 −0
Original line number Diff line number Diff line
@@ -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);

drivers/gpu/drm/drm_atomic_helper.c

+166 −6
Original line number Diff line number Diff line
@@ -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);

drivers/gpu/drm/i915/intel_display.c

+2 −0
Original line number Diff line number Diff line
@@ -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;

include/drm/drm_atomic.h

+12 −0
Original line number Diff line number Diff line
@@ -235,6 +235,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:

	 *

include/drm/drm_connector.h

+7 −0
Original line number Diff line number Diff line
@@ -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;



	/**