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@xxxxxxxx>
> Cc: Harry Wentland <harry.wentland@xxxxxxx>
> Cc: Andrey Grodzovsky <andrey.grodzovsky@xxxxxxx>
> Cc: <stable@xxxxxxxxxxxxxxx> # v4.14+
> Fixes: fef9df8b5945 ("drm/atomic: initial support for asynchronous plane update")
> Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@xxxxxxx>
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;
>
