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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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;
> 
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux