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

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

Op 07-09-17 om 12:05 schreef Daniel Vetter:
> On Mon, Sep 04, 2017 at 12:48:37PM +0200, 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.
>> 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>
>> ---
>>  drivers/gpu/drm/drm_atomic.c         |   4 +
>>  drivers/gpu/drm/drm_atomic_helper.c  | 172 +++++++++++++++++++++++++++++++++--
>>  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, 198 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 04629d883114..c81d46927a74 100644
>> --- 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)
> Bikeshed: Would be nice if this function directly increases the refcount,
> instead of imposing this on all callers. Would need a rename too like
> crtc_or_fake_commit_get().
>
> But since this bug is randomly killing our hsw CI and causing lots of
> noise better to push as-is and polish later on.
>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
I chose not to, to make it explicit that a extra refcount is used on the state object.
But sending one final version to trybot to make sure that things don't blow up with the merge conflicts in patch 6. :)
>> +{
>> +	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);
>>  
>> @@ -1842,6 +1955,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);
>>  
>> @@ -1875,6 +1993,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);
>>  
>> @@ -2254,6 +2375,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;
>>  
>> @@ -2264,6 +2394,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) {
>> @@ -3246,6 +3398,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);
>>  
>> @@ -3287,6 +3440,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);
>>  
>> @@ -3365,6 +3521,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);
>>  
>> @@ -3491,6 +3648,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 a6cf1c20c712..7abbc761a635 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -13132,8 +13132,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 a80a8dadef00..07a71daa3582 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;
>>  };
>>  
>> -- 
>> 2.11.0
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel


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