[PATCH] drm/amd/display: Skip fast cursor updates for fb changes

[Kazlauskas, Nicholas]

On 12/14/18 12:34 PM, Grodzovsky, Andrey wrote:
> 
> 
> On 12/14/2018 12:26 PM, Nicholas Kazlauskas wrote:
>> [Why]
>> The behavior of drm_atomic_helper_cleanup_planes 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 is updated
>> in place and has not been swapped the cleanup_fb call affects the new
>> plane state.
>>
>> This results in a use after free for the given sequence:
>>
>> - Fast update, fb1 pin/ref, fb1 unpin/unref
>> - Fast update, fb2 pin/ref, fb2 unpin/unref
>> - Slow update, fb1 pin/ref, fb2 unpin/unref
> 
> Shouldn't you have use after free already at this point ?
> 
> Andrey

This assumes there was 1 reference on the bo. You would be getting use 
after free on every single update (be it fast or slow) if this wasn't 
the case.

So in the case where there was 1 reference on fb2 it's actually freed at 
the end of slow update since the ref count is now 0. Then the use after 
free happens:

> 
>> - Fast update, fb2 pin/ref -> use after free. bug

...here.

You can see this exact sequence happening in Michel's log.

Nicholas Kazlauskas

>>
>> [How]
>> Disallow framebuffer changes in the fast path. Since this includes
>> a NULL framebuffer, this means that only framebuffers that have
>> been previously pin+ref at least once will be used, preventing a
>> use after free.
>>
>> This has a significant throughput reduction for cursor updates where
>> the framebuffer changes. For most desktop usage this isn't a problem,
>> but it does introduce performance regressions for two specific IGT
>> tests:
>>
>> - cursor-vs-flip-toggle
>> - cursor-vs-flip-varying-size
>>
>> Cc: Leo Li <sunpeng.li at amd.com>
>> Cc: Harry Wentland <harry.wentland at amd.com>
>> Cc: Michel Dänzer <michel at daenzer.net>
>> Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas at amd.com>
>> ---
>>    drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 10 ++++++++++
>>    1 file changed, 10 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> index d01315965af0..dc1eb9ec2c38 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> @@ -3628,10 +3628,20 @@ static int dm_plane_atomic_check(struct drm_plane *plane,
>>    static int dm_plane_atomic_async_check(struct drm_plane *plane,
>>    				       struct drm_plane_state *new_plane_state)
>>    {
>> +	struct drm_plane_state *old_plane_state =
>> +		drm_atomic_get_old_plane_state(new_plane_state->state, plane);
>> +
>>    	/* Only support async updates on cursor planes. */
>>    	if (plane->type != DRM_PLANE_TYPE_CURSOR)
>>    		return -EINVAL;
>>    
>> +	/*
>> +	 * DRM calls prepare_fb and cleanup_fb on new_plane_state for
>> +	 * async commits so don't allow fb changes.
>> +	 */
>> +	if (old_plane_state->fb != new_plane_state->fb)
>> +		return -EINVAL;
>> +
>>    	return 0;
>>    }
>>    
>