[PATCH] drm/amd/display: Don't cleanup new cursor fb in fast cursor update

[Michel Dänzer]

On 2018-12-14 3:48 p.m., Nicholas Kazlauskas wrote:
> [Why]
> The behavior of dm_plane_helper_cleanup_fb differs depending on
> whether the commit was asynchronous or not. When it's called from
> amdgpu_dm_atomic_commit_tail during a typical atomic commit the
> plane state has been swapped so it calls cleanup_fb on the old plane
> state.
> 
> However, in the asynchronous commit codepath the call to
> drm_atomic_helper_commit also calls dm_plane_helper_cleanup_fb after
> atomic_async_update has been called. Since the plane state has not
> been swapped, the cleanup_fb call affects the new plane state with the
> active fb.
> 
> The slow, full path is only ever used for the cursor update when the
> cursor CRTC changes. If there was previously a fb in the state before
> that had gone through the fast asynchronous path then cleanup_fb will
> be called a second time on an fb that was previously unpinned/unref'd.
> 
> This results in a use after free on the next cursor update.
> 
> [How]
> Regardless of whether this was intentional behavior in DRM or not,
> the fix is to manually call cleanup_fb on the old plane state's fb.
> 
> Since the pointer to the old_plane_state is actually the current
> plane->state in this function, a backup is needed. This has a minor
> change to behavior for handle_cursor_update since it now correctly uses
> the old state.
> 
> The next step is to prevent the cleanup_fb call from DRM from unpinning
> the active new_state->fb. Setting new_state->fb to NULL would be
> enough to handle this, but DRM has a warning in the
> drm_atomic_helper_async_commit helper if
> plane->state->fb != new_state->fb.
> 
> A new field was added (cursor_update) to dm_plane_state that's only
> ever set to true during the fast path. If it's true, then cleanup_fb
> doesn't unpin and unref the fb.
> 
> Cc: Leo Li <sunpeng.li at amd.com>
> Cc: Harry Wentland <harry.wentland at amd.com>
> Cc: Michel Dänzer <michel.daenzer at amd.com>
> Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas at amd.com>
> 
> [...]
>  
> @@ -3653,7 +3654,15 @@ static void dm_plane_atomic_async_update(struct drm_plane *plane,
>  	plane->state->crtc_w = new_state->crtc_w;
>  	plane->state->crtc_h = new_state->crtc_h;
>  
> -	handle_cursor_update(plane, old_state);
> +	handle_cursor_update(plane, &old_state);
> +
> +	/*
> +	 * While DRM already takes care of drm_atomic_helper_cleanup_planes,
> +	 * it does it on the wrong state (new_state instead of old_state).
> +	 * This is a workaround for that behavior.
> +	 */
> +	dm_plane_helper_cleanup_fb(plane, &old_state);
> +	dm_plane_state->cursor_update = true;

We shouldn't work around DRM core code in driver code. Please work on a
solution with the core code developers on the dri-devel list, and revert
the cursor fast path for now.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer