[PATCH v2] drm: Block fb changes for async plane updates

[PATCH v2] drm: Block fb changes for async plane updates

[Nicholas Kazlauskas]

The prepare_fb call always happens on new_plane_state.

The drm_atomic_helper_cleanup_planes checks to see if
plane state pointer has changed when deciding to call cleanup_fb on
either the new_plane_state or the old_plane_state.

For a non-async atomic commit the state pointer is swapped, so this
helper calls prepare_fb on the new_plane_state and cleanup_fb on the
old_plane_state. This makes sense, since we want to prepare the
framebuffer we are going to use and cleanup the the framebuffer we are
no longer using.

For the async atomic update helpers this differs. The async atomic
update helpers perform in-place updates on the existing state. They call
drm_atomic_helper_cleanup_planes but the state pointer is not swapped.
This means that prepare_fb is called on the new_plane_state and
cleanup_fb is called on the new_plane_state (not the old).

In the case where old_plane_state->fb == new_plane_state->fb then
there should be no behavioral difference between an async update
and a non-async commit. But there are issues that arise when
old_plane_state->fb != new_plane_state->fb.

The first is that the new_plane_state->fb is immediately cleaned up
after it has been prepared, so we're using a fb that we shouldn't
be.

The second occurs during a sequence of async atomic updates and
non-async regular atomic commits. Suppose there are two framebuffers
being interleaved in a double-buffering scenario, fb1 and fb2:

- Async update, oldfb = NULL, newfb = fb1, prepare fb1, cleanup fb1
- Async update, oldfb = fb1, newfb = fb2, prepare fb2, cleanup fb2
- Non-async commit, oldfb = fb2, newfb = fb1, prepare fb1, cleanup fb2

We call cleanup_fb on fb2 twice in this example scenario, and any
further use will result in use-after-free.

The simple fix to this problem is to block framebuffer changes
in the drm_atomic_helper_async_check function for now.

Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Harry Wentland <harry.wentland@amd.com>
Cc: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
Cc: <stable@vger.kernel.org> # v4.14+
Fixes: fef9df8b5945 ("drm/atomic: initial support for asynchronous plane update")
Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.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 54e2ae614dcc..f4290f6b0c38 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1602,6 +1602,15 @@ int drm_atomic_helper_async_check(struct drm_device *dev,
 	    old_plane_state->crtc != new_plane_state->crtc)
 		return -EINVAL;
 
+	/*
+	 * FIXME: Since prepare_fb and cleanup_fb are always called on
+	 * the new_plane_state for async updates we need to block framebuffer
+	 * changes. This prevents use of a fb that's been cleaned up and
+	 * double cleanups from occuring.
+	 */
+	if (old_plane_state->fb != new_plane_state->fb)
+		return -EINVAL;
+
 	funcs = plane->helper_private;
 	if (!funcs->atomic_async_update)
 		return -EINVAL;
-- 
2.17.1

Re: [PATCH v2] drm: Block fb changes for async plane updates

[Wentland, Harry]

On 2019-01-07 12:41 p.m., Nicholas Kazlauskas wrote:
> The prepare_fb call always happens on new_plane_state.
> 
> The drm_atomic_helper_cleanup_planes checks to see if
> plane state pointer has changed when deciding to call cleanup_fb on
> either the new_plane_state or the old_plane_state.
> 
> For a non-async atomic commit the state pointer is swapped, so this
> helper calls prepare_fb on the new_plane_state and cleanup_fb on the
> old_plane_state. This makes sense, since we want to prepare the
> framebuffer we are going to use and cleanup the the framebuffer we are
> no longer using.
> 
> For the async atomic update helpers this differs. The async atomic
> update helpers perform in-place updates on the existing state. They call
> drm_atomic_helper_cleanup_planes but the state pointer is not swapped.
> This means that prepare_fb is called on the new_plane_state and
> cleanup_fb is called on the new_plane_state (not the old).
> 
> In the case where old_plane_state->fb == new_plane_state->fb then
> there should be no behavioral difference between an async update
> and a non-async commit. But there are issues that arise when
> old_plane_state->fb != new_plane_state->fb.
> 
> The first is that the new_plane_state->fb is immediately cleaned up
> after it has been prepared, so we're using a fb that we shouldn't
> be.
> 
> The second occurs during a sequence of async atomic updates and
> non-async regular atomic commits. Suppose there are two framebuffers
> being interleaved in a double-buffering scenario, fb1 and fb2:
> 
> - Async update, oldfb = NULL, newfb = fb1, prepare fb1, cleanup fb1
> - Async update, oldfb = fb1, newfb = fb2, prepare fb2, cleanup fb2
> - Non-async commit, oldfb = fb2, newfb = fb1, prepare fb1, cleanup fb2
> 
> We call cleanup_fb on fb2 twice in this example scenario, and any
> further use will result in use-after-free.
> 
> The simple fix to this problem is to block framebuffer changes
> in the drm_atomic_helper_async_check function for now.
> 
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Harry Wentland <harry.wentland@amd.com>
> Cc: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
> Cc: <stable@vger.kernel.org> # v4.14+
> Fixes: fef9df8b5945 ("drm/atomic: initial support for asynchronous plane update")
> Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>

Patch is merged to drm-misc-fixes with Daniel's RB from IRC and Andrey's and my AB.

Harry

> ---
>  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 54e2ae614dcc..f4290f6b0c38 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1602,6 +1602,15 @@ int drm_atomic_helper_async_check(struct drm_device *dev,
>  	    old_plane_state->crtc != new_plane_state->crtc)
>  		return -EINVAL;
>  
> +	/*
> +	 * FIXME: Since prepare_fb and cleanup_fb are always called on
> +	 * the new_plane_state for async updates we need to block framebuffer
> +	 * changes. This prevents use of a fb that's been cleaned up and
> +	 * double cleanups from occuring.
> +	 */
> +	if (old_plane_state->fb != new_plane_state->fb)
> +		return -EINVAL;
> +
>  	funcs = plane->helper_private;
>  	if (!funcs->atomic_async_update)
>  		return -EINVAL;
>