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@xxxxxxx> > Cc: Harry Wentland <harry.wentland@xxxxxxx> > Cc: Michel Dänzer <michel.daenzer@xxxxxxx> > Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@xxxxxxx> > > [...] > > @@ -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 _______________________________________________ amd-gfx mailing list amd-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/amd-gfx
