[Intel-gfx] [PATCH] drm/atomic: Fix memleak on ERESTARTSYS during non-blocking commits

[Maarten Lankhorst]

Op 17-01-18 om 19:29 schreef Sean Paul:
> On Wed, Jan 17, 2018 at 12:51:08PM +0100, Maarten Lankhorst wrote:
>> From: "Leo (Sunpeng) Li" <sunpeng.li at amd.com>
>>
>> During a non-blocking commit, it is possible to return before the
>> commit_tail work is queued (-ERESTARTSYS, for example).
>>
>> Since a reference on the crtc commit object is obtained for the pending
>> vblank event when preparing the commit, the above situation will leave
>> us with an extra reference.
>>
>> Therefore, if the commit_tail worker has not consumed the event at the
>> end of a commit, release it's reference.
>>
>> Changes since v1:
>> - Also check for state->event->base.completion being set, to
>>   handle the case where stall_checks() fails in setup_crtc_commit().
>> Changes since v2:
>> - Add a flag to drm_crtc_commit, to prevent dereferencing a freed event.
>>   i915 may unreference the state in a worker.
>>
>> Fixes: 24835e442f28 ("drm: reference count event->completion")
>> Cc: <stable at vger.kernel.org> # v4.11+
>> Signed-off-by: Leo (Sunpeng) Li <sunpeng.li at amd.com>
>> Acked-by: Harry Wentland <harry.wentland at amd.com> #v1
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
>> ---
>>  drivers/gpu/drm/drm_atomic_helper.c | 15 +++++++++++++++
>>  include/drm/drm_atomic.h            |  9 +++++++++
>>  2 files changed, 24 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
>> index ab4032167094..ae3cbfe9e01c 100644
>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>> @@ -1878,6 +1878,8 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
>>  		new_crtc_state->event->base.completion = &commit->flip_done;
>>  		new_crtc_state->event->base.completion_release = release_crtc_commit;
>>  		drm_crtc_commit_get(commit);
>> +
>> +		commit->abort_completion = true;
>>  	}
>>  
>>  	for_each_oldnew_connector_in_state(state, conn, old_conn_state, new_conn_state, i) {
>> @@ -3421,8 +3423,21 @@ EXPORT_SYMBOL(drm_atomic_helper_crtc_duplicate_state);
>>  void __drm_atomic_helper_crtc_destroy_state(struct drm_crtc_state *state)
>>  {
>>  	if (state->commit) {
>> +		/*
>> +		 * In the event that a non-blocking commit returns
>> +		 * -ERESTARTSYS before the commit_tail work is queued, we will
>> +		 * have an extra reference to the commit object. Release it, if
>> +		 * the event has not been consumed by the worker.
>> +		 *
>> +		 * state->event may be freed, so we can't directly look at
>> +		 * state->event->base.completion.
>> +		 */
>> +		if (state->event && state->commit->abort_completion)
>> +			drm_crtc_commit_put(state->commit);
>> +
>>  		kfree(state->commit->event);
>>  		state->commit->event = NULL;
>> +
>>  		drm_crtc_commit_put(state->commit);
>>  	}
>>  
>> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
>> index 1c27526c499e..cf13842a6dbd 100644
>> --- a/include/drm/drm_atomic.h
>> +++ b/include/drm/drm_atomic.h
>> @@ -134,6 +134,15 @@ struct drm_crtc_commit {
>>  	 * &drm_pending_vblank_event pointer to clean up private events.
>>  	 */
>>  	struct drm_pending_vblank_event *event;
>> +
>> +	/**
>> +	 * @abort_completion:
>> +	 *
>> +	 * A flag that's set after drm_atomic_helper_setup_commit takes a second
>> +	 * reference for the completion of $drm_crtc_state.event. It's used by
>> +	 * the free code to remove the second reference if commit fails.
>> +	 */
> Perhaps it's just me, or I'm oversimplifying the problem. I think this would
> be easier to understand if we just dropped the additional reference at the point
> of failure (ie: in swap_state). That way we don't have to add Yet Another Piece
> Of State.

That's assuming nothing can fail between setup_commit() and swap_state() and
also that the driver implementing atomci puts no calls in between. And also
assumes that even setup_commit has proper rollback. I think it's overkill,
and we have no choice but to do it like this. :(

~Maarten