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

From: Sean Paul <seanpaul@chromium.org>
To: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: dri-devel@lists.freedesktop.org,
	"Leo (Sunpeng) Li" <sunpeng.li@amd.com>,
	intel-gfx@lists.freedesktop.org,
	Harry Wentland <harry.wentland@amd.com>,
	stable@vger.kernel.org
Subject: Re: [Intel-gfx] [PATCH] drm/atomic: Fix memleak on ERESTARTSYS during non-blocking commits
Date: Wed, 17 Jan 2018 13:29:31 -0500	[thread overview]
Message-ID: <20180117182931.tv7x3zmc7ltx3w3w@art_vandelay> (raw)
In-Reply-To: <20180117115108.29608-1-maarten.lankhorst@linux.intel.com>

On Wed, Jan 17, 2018 at 12:51:08PM +0100, Maarten Lankhorst wrote:
> From: "Leo (Sunpeng) Li" <sunpeng.li@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@vger.kernel.org> # v4.11+
> Signed-off-by: Leo (Sunpeng) Li <sunpeng.li@amd.com>
> Acked-by: Harry Wentland <harry.wentland@amd.com> #v1
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@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.

Sean

> +	bool abort_completion;
>  };
>  
>  struct __drm_planes_state {
> -- 
> 2.15.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Sean Paul, Software Engineer, Google / Chromium OS