drm/atomic: Fix memleak on ERESTARTSYS during non-blocking commits, v2.
diff mbox

Message ID 20180110110016.64632-1-maarten.lankhorst@linux.intel.com
State New
Headers show

Commit Message

Maarten Lankhorst Jan. 10, 2018, 11 a.m. UTC
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().

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 | 9 +++++++++
 1 file changed, 9 insertions(+)

Comments

Alex Deucher Jan. 10, 2018, 8:21 p.m. UTC | #1
On Wed, Jan 10, 2018 at 6:00 AM, Maarten Lankhorst
<maarten.lankhorst@linux.intel.com> 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().
>
> Fixes: 24835e442f28 ("drm: reference count event->completion")

Thanks for fixing this up.  You mentioned on IRC that this version
still caused problems.  What were those?

Thanks,

Alex

> 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 | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index b16f1d69a0bb..1d43f3e85a7d 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -3327,6 +3327,15 @@ 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.
> +                */
> +               if (state->event && state->event->base.completion)
> +                       drm_crtc_commit_put(state->commit);
> +
>                 kfree(state->commit->event);
>                 state->commit->event = NULL;
>                 drm_crtc_commit_put(state->commit);
> --
> 2.15.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Patch
diff mbox

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index b16f1d69a0bb..1d43f3e85a7d 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -3327,6 +3327,15 @@  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.
+		 */
+		if (state->event && state->event->base.completion)
+			drm_crtc_commit_put(state->commit);
+
 		kfree(state->commit->event);
 		state->commit->event = NULL;
 		drm_crtc_commit_put(state->commit);