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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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


-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel