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

[Daniel Vetter]

On Fri, Dec 21, 2018 at 09:33:24AM -0500, Nicholas Kazlauskas wrote:
> The behavior of drm_atomic_helper_cleanup_planes differs depending on
> whether the commit was an asynchronous update or not.
> 
> For a typical (non-async) atomic commit prepare_fb is called on the
> new_plane_state and cleanup_fb is called on the old_plane_state.
> 
> However, async commits are performed in place and don't swap the state
> for the plane. The call to prepare_fb happens on the new_plane_state
> and the call to cleanup_fb is also called on the new_plane_state in
> this case (since the state hasn't swapped).
> 
> This behavior can lead to use-after-free or unpin of an active fb.
> 
> Consider the following sequence of events for interleaving fbs:
> 
> - Async update, fb1 prepare, fb1 cleanup_fb
> - Async update, fb2 prepare, fb2 cleanup_fb
> - Non-async update, fb1 prepare, fb2 cleanup_fb
> - Async update, fb2 cleanup_fb -> double cleanup, use-after-free

I think I see your bug, but I'm completely lost in your description above.

I think this is ok as a short-term gap, but imo better if it's a separate
if condition with a FIXME comment.

Long-term we want to fix this, and I think simplest way to do that is if
we expect drivers to store the old fb in the new_plane_state (and check
that with a WARN_ON like the others). I think that should work.

We probably also need some locking on top, to prevent races with the
cleanup_fb calls done by non-blocking commits, to make sure those clean up
the right fb.
-Daniel

> This situation has been observed in practice for a double buffered
> cursor when closing an X client. The non-async update occurs because
> the new_plane_state->crtc != old_plane_state->crtc which forces the
> non-async path to occur.
> 
> The simplest fix for this is to block fb updates in
> drm_atomic_helper_async_check. This guarantees that the framebuffer
> will have previously been prepared and any subsequent async updates
> will always call prepare and cleanup_fb like the non-async atomic
> commit path would.
> 
> Cc: Michel Dänzer <michel at daenzer.net>
> Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> Cc: Andrey Grodzovsky <andrey.grodzovsky at amd.com>
> Cc: Harry Wentland <harry.wentland at amd.com>
> Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas at amd.com>
> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 54e2ae614dcc..d2f80bf14f86 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1599,7 +1599,8 @@ int drm_atomic_helper_async_check(struct drm_device *dev,
>  		return -EINVAL;
>  
>  	if (!new_plane_state->crtc ||
> -	    old_plane_state->crtc != new_plane_state->crtc)
> +	    old_plane_state->crtc != new_plane_state->crtc ||
> +	    old_plane_state->fb != new_plane_state->fb)
>  		return -EINVAL;
>  
>  	funcs = plane->helper_private;
> -- 
> 2.17.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch