| Message ID | 20170904104838.23822-6-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Sep 04, 2017 at 12:48:37PM +0200, Maarten Lankhorst wrote: > Currently we neatly track the crtc state, but forget to look at > plane/connector state. > > When doing a nonblocking modeset, immediately followed by a setprop > before the modeset completes, the setprop will see the modesets new > state as the old state and free it. > > This has to be solved by waiting for hw_done on the connector, even > if it's not assigned to a crtc. When a connector is unbound we take > the last crtc commit, and when it stays unbound we create a new > fake crtc commit for that gets signaled on hw_done for all the > planes/connectors. > > We wait for it the same way as we do for crtc's, which will make > sure we never run into a use-after-free situation. > > Changes since v1: > - Only create a single disable commit. (danvet) > - Fix leak in intel_legacy_cursor_update. > Changes since v2: > - Make reference counting in drm_atomic_helper_setup_commit > more obvious. (pinchartl) > - Call cleanup_done for fake commit. (danvet) > - Add comments to drm_atomic_helper_setup_commit. (danvet, pinchartl) > - Add comment to drm_atomic_helper_swap_state. (pinchartl) > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Testcase: kms_atomic_transition.plane-use-after-nonblocking-unbind* > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > drivers/gpu/drm/drm_atomic.c | 4 + > drivers/gpu/drm/drm_atomic_helper.c | 172 +++++++++++++++++++++++++++++++++-- > drivers/gpu/drm/i915/intel_display.c | 2 + > include/drm/drm_atomic.h | 12 +++ > include/drm/drm_connector.h | 7 ++ > include/drm/drm_plane.h | 7 ++ > 6 files changed, 198 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > index 2cce48f203e0..75f5f74de9bf 100644 > --- a/drivers/gpu/drm/drm_atomic.c > +++ b/drivers/gpu/drm/drm_atomic.c > @@ -192,6 +192,10 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state) > } > state->num_private_objs = 0; > > + if (state->fake_commit) { > + drm_crtc_commit_put(state->fake_commit); > + state->fake_commit = NULL; > + } > } > EXPORT_SYMBOL(drm_atomic_state_default_clear); > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > index 04629d883114..c81d46927a74 100644 > --- a/drivers/gpu/drm/drm_atomic_helper.c > +++ b/drivers/gpu/drm/drm_atomic_helper.c > @@ -1667,6 +1667,38 @@ static void release_crtc_commit(struct completion *completion) > drm_crtc_commit_put(commit); > } > > +static void init_commit(struct drm_crtc_commit *commit, struct drm_crtc *crtc) > +{ > + init_completion(&commit->flip_done); > + init_completion(&commit->hw_done); > + init_completion(&commit->cleanup_done); > + INIT_LIST_HEAD(&commit->commit_entry); > + kref_init(&commit->ref); > + commit->crtc = crtc; > +} > + > +static struct drm_crtc_commit * > +crtc_or_fake_commit(struct drm_atomic_state *state, struct drm_crtc *crtc) Bikeshed: Would be nice if this function directly increases the refcount, instead of imposing this on all callers. Would need a rename too like crtc_or_fake_commit_get(). But since this bug is randomly killing our hsw CI and causing lots of noise better to push as-is and polish later on. Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > +{ > + if (crtc) { > + struct drm_crtc_state *new_crtc_state; > + > + new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc); > + > + return new_crtc_state->commit; > + } > + > + if (!state->fake_commit) { > + state->fake_commit = kzalloc(sizeof(*state->fake_commit), GFP_KERNEL); > + if (!state->fake_commit) > + return NULL; > + > + init_commit(state->fake_commit, NULL); > + } > + > + return state->fake_commit; > +} > + > /** > * drm_atomic_helper_setup_commit - setup possibly nonblocking commit > * @state: new modeset state to be committed > @@ -1715,6 +1747,10 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state, > { > struct drm_crtc *crtc; > struct drm_crtc_state *old_crtc_state, *new_crtc_state; > + struct drm_connector *conn; > + struct drm_connector_state *old_conn_state, *new_conn_state; > + struct drm_plane *plane; > + struct drm_plane_state *old_plane_state, *new_plane_state; > struct drm_crtc_commit *commit; > int i, ret; > > @@ -1723,12 +1759,7 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state, > if (!commit) > return -ENOMEM; > > - init_completion(&commit->flip_done); > - init_completion(&commit->hw_done); > - init_completion(&commit->cleanup_done); > - INIT_LIST_HEAD(&commit->commit_entry); > - kref_init(&commit->ref); > - commit->crtc = crtc; > + init_commit(commit, crtc); > > new_crtc_state->commit = commit; > > @@ -1764,6 +1795,42 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state, > drm_crtc_commit_get(commit); > } > > + for_each_oldnew_connector_in_state(state, conn, old_conn_state, new_conn_state, i) { > + /* commit tracked through new_crtc_state->commit, no need to do it explicitly */ > + if (new_conn_state->crtc) > + continue; > + > + /* Userspace is not allowed to get ahead of the previous > + * commit with nonblocking ones. */ > + if (nonblock && old_conn_state->commit && > + !try_wait_for_completion(&old_conn_state->commit->flip_done)) > + return -EBUSY; > + > + commit = crtc_or_fake_commit(state, old_conn_state->crtc); > + if (!commit) > + return -ENOMEM; > + > + new_conn_state->commit = drm_crtc_commit_get(commit); > + } > + > + for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i) { > + /* commit tracked through new_crtc_state->commit, no need to do it explicitly */ > + if (new_plane_state->crtc) > + continue; > + > + /* Userspace is not allowed to get ahead of the previous > + * commit with nonblocking ones. */ > + if (nonblock && old_plane_state->commit && > + !try_wait_for_completion(&old_plane_state->commit->flip_done)) > + return -EBUSY; > + > + commit = crtc_or_fake_commit(state, old_plane_state->crtc); > + if (!commit) > + return -ENOMEM; > + > + new_plane_state->commit = drm_crtc_commit_get(commit); > + } > + > return 0; > } > EXPORT_SYMBOL(drm_atomic_helper_setup_commit); > @@ -1784,6 +1851,10 @@ void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *old_state) > { > struct drm_crtc *crtc; > struct drm_crtc_state *old_crtc_state; > + struct drm_plane *plane; > + struct drm_plane_state *old_plane_state; > + struct drm_connector *conn; > + struct drm_connector_state *old_conn_state; > struct drm_crtc_commit *commit; > int i; > long ret; > @@ -1808,6 +1879,48 @@ void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *old_state) > DRM_ERROR("[CRTC:%d:%s] flip_done timed out\n", > crtc->base.id, crtc->name); > } > + > + for_each_old_connector_in_state(old_state, conn, old_conn_state, i) { > + commit = old_conn_state->commit; > + > + if (!commit) > + continue; > + > + ret = wait_for_completion_timeout(&commit->hw_done, > + 10*HZ); > + if (ret == 0) > + DRM_ERROR("[CONNECTOR:%d:%s] hw_done timed out\n", > + conn->base.id, conn->name); > + > + /* Currently no support for overwriting flips, hence > + * stall for previous one to execute completely. */ > + ret = wait_for_completion_timeout(&commit->flip_done, > + 10*HZ); > + if (ret == 0) > + DRM_ERROR("[CONNECTOR:%d:%s] flip_done timed out\n", > + conn->base.id, conn->name); > + } > + > + for_each_old_plane_in_state(old_state, plane, old_plane_state, i) { > + commit = old_plane_state->commit; > + > + if (!commit) > + continue; > + > + ret = wait_for_completion_timeout(&commit->hw_done, > + 10*HZ); > + if (ret == 0) > + DRM_ERROR("[PLANE:%d:%s] hw_done timed out\n", > + plane->base.id, plane->name); > + > + /* Currently no support for overwriting flips, hence > + * stall for previous one to execute completely. */ > + ret = wait_for_completion_timeout(&commit->flip_done, > + 10*HZ); > + if (ret == 0) > + DRM_ERROR("[PLANE:%d:%s] flip_done timed out\n", > + plane->base.id, plane->name); > + } > } > EXPORT_SYMBOL(drm_atomic_helper_wait_for_dependencies); > > @@ -1842,6 +1955,11 @@ void drm_atomic_helper_commit_hw_done(struct drm_atomic_state *old_state) > WARN_ON(new_crtc_state->event); > complete_all(&commit->hw_done); > } > + > + if (old_state->fake_commit) { > + complete_all(&old_state->fake_commit->hw_done); > + complete_all(&old_state->fake_commit->flip_done); > + } > } > EXPORT_SYMBOL(drm_atomic_helper_commit_hw_done); > > @@ -1875,6 +1993,9 @@ void drm_atomic_helper_commit_cleanup_done(struct drm_atomic_state *old_state) > list_del(&commit->commit_entry); > spin_unlock(&crtc->commit_lock); > } > + > + if (old_state->fake_commit) > + complete_all(&old_state->fake_commit->cleanup_done); > } > EXPORT_SYMBOL(drm_atomic_helper_commit_cleanup_done); > > @@ -2254,6 +2375,15 @@ int drm_atomic_helper_swap_state(struct drm_atomic_state *state, > struct drm_private_state *old_obj_state, *new_obj_state; > > if (stall) { > + /* > + * We have to stall for hw_done here before > + * drm_atomic_helper_wait_for_dependencies() because flip > + * depth > 1 is not yet supported by all drivers. As long as > + * obj->state is directly dereferenced anywhere in the drivers > + * atomic_commit_tail function, then it's unsafe to swap state > + * before drm_atomic_helper_commit_hw_done() is called. > + */ > + > for_each_old_crtc_in_state(state, crtc, old_crtc_state, i) { > commit = old_crtc_state->commit; > > @@ -2264,6 +2394,28 @@ int drm_atomic_helper_swap_state(struct drm_atomic_state *state, > if (ret) > return ret; > } > + > + for_each_old_connector_in_state(state, connector, old_conn_state, i) { > + commit = old_conn_state->commit; > + > + if (!commit) > + continue; > + > + ret = wait_for_completion_interruptible(&commit->hw_done); > + if (ret) > + return ret; > + } > + > + for_each_old_plane_in_state(state, plane, old_plane_state, i) { > + commit = old_plane_state->commit; > + > + if (!commit) > + continue; > + > + ret = wait_for_completion_interruptible(&commit->hw_done); > + if (ret) > + return ret; > + } > } > > for_each_oldnew_connector_in_state(state, connector, old_conn_state, new_conn_state, i) { > @@ -3246,6 +3398,7 @@ void __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane, > drm_framebuffer_get(state->fb); > > state->fence = NULL; > + state->commit = NULL; > } > EXPORT_SYMBOL(__drm_atomic_helper_plane_duplicate_state); > > @@ -3287,6 +3440,9 @@ void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state) > > if (state->fence) > dma_fence_put(state->fence); > + > + if (state->commit) > + drm_crtc_commit_put(state->commit); > } > EXPORT_SYMBOL(__drm_atomic_helper_plane_destroy_state); > > @@ -3365,6 +3521,7 @@ __drm_atomic_helper_connector_duplicate_state(struct drm_connector *connector, > memcpy(state, connector->state, sizeof(*state)); > if (state->crtc) > drm_connector_get(connector); > + state->commit = NULL; > } > EXPORT_SYMBOL(__drm_atomic_helper_connector_duplicate_state); > > @@ -3491,6 +3648,9 @@ __drm_atomic_helper_connector_destroy_state(struct drm_connector_state *state) > { > if (state->crtc) > drm_connector_put(state->connector); > + > + if (state->commit) > + drm_crtc_commit_put(state->commit); > } > EXPORT_SYMBOL(__drm_atomic_helper_connector_destroy_state); > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index a6cf1c20c712..7abbc761a635 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -13132,8 +13132,10 @@ intel_legacy_cursor_update(struct drm_plane *plane, > > /* Swap plane state */ > new_plane_state->fence = old_plane_state->fence; > + new_plane_state->commit = old_plane_state->commit; > *to_intel_plane_state(old_plane_state) = *to_intel_plane_state(new_plane_state); > new_plane_state->fence = NULL; > + new_plane_state->commit = NULL; > new_plane_state->fb = old_fb; > to_intel_plane_state(new_plane_state)->vma = NULL; > > diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h > index a80a8dadef00..07a71daa3582 100644 > --- a/include/drm/drm_atomic.h > +++ b/include/drm/drm_atomic.h > @@ -236,6 +236,18 @@ struct drm_atomic_state { > struct drm_modeset_acquire_ctx *acquire_ctx; > > /** > + * @fake_commit: > + * > + * Used for signaling unbound planes/connectors. > + * When a connector or plane is not bound to any CRTC, it's still important > + * to preserve linearity to prevent the atomic states from being freed to early. > + * > + * This commit (if set) is not bound to any crtc, but will be completed when > + * drm_atomic_helper_commit_hw_done() is called. > + */ > + struct drm_crtc_commit *fake_commit; > + > + /** > * @commit_work: > * > * Work item which can be used by the driver or helpers to execute the > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h > index ea8da401c93c..8837649d16e8 100644 > --- a/include/drm/drm_connector.h > +++ b/include/drm/drm_connector.h > @@ -347,6 +347,13 @@ struct drm_connector_state { > > struct drm_atomic_state *state; > > + /** > + * @commit: Tracks the pending commit to prevent use-after-free conditions. > + * > + * Is only set when @crtc is NULL. > + */ > + struct drm_crtc_commit *commit; > + > struct drm_tv_connector_state tv; > > /** > diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h > index 73f90f9d057f..7d96116fd4c4 100644 > --- a/include/drm/drm_plane.h > +++ b/include/drm/drm_plane.h > @@ -123,6 +123,13 @@ struct drm_plane_state { > */ > bool visible; > > + /** > + * @commit: Tracks the pending commit to prevent use-after-free conditions. > + * > + * Is only set when @crtc is NULL. > + */ > + struct drm_crtc_commit *commit; > + > struct drm_atomic_state *state; > }; > > -- > 2.11.0 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
Op 07-09-17 om 12:05 schreef Daniel Vetter: > On Mon, Sep 04, 2017 at 12:48:37PM +0200, Maarten Lankhorst wrote: >> Currently we neatly track the crtc state, but forget to look at >> plane/connector state. >> >> When doing a nonblocking modeset, immediately followed by a setprop >> before the modeset completes, the setprop will see the modesets new >> state as the old state and free it. >> >> This has to be solved by waiting for hw_done on the connector, even >> if it's not assigned to a crtc. When a connector is unbound we take >> the last crtc commit, and when it stays unbound we create a new >> fake crtc commit for that gets signaled on hw_done for all the >> planes/connectors. >> >> We wait for it the same way as we do for crtc's, which will make >> sure we never run into a use-after-free situation. >> >> Changes since v1: >> - Only create a single disable commit. (danvet) >> - Fix leak in intel_legacy_cursor_update. >> Changes since v2: >> - Make reference counting in drm_atomic_helper_setup_commit >> more obvious. (pinchartl) >> - Call cleanup_done for fake commit. (danvet) >> - Add comments to drm_atomic_helper_setup_commit. (danvet, pinchartl) >> - Add comment to drm_atomic_helper_swap_state. (pinchartl) >> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >> Testcase: kms_atomic_transition.plane-use-after-nonblocking-unbind* >> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >> --- >> drivers/gpu/drm/drm_atomic.c | 4 + >> drivers/gpu/drm/drm_atomic_helper.c | 172 +++++++++++++++++++++++++++++++++-- >> drivers/gpu/drm/i915/intel_display.c | 2 + >> include/drm/drm_atomic.h | 12 +++ >> include/drm/drm_connector.h | 7 ++ >> include/drm/drm_plane.h | 7 ++ >> 6 files changed, 198 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c >> index 2cce48f203e0..75f5f74de9bf 100644 >> --- a/drivers/gpu/drm/drm_atomic.c >> +++ b/drivers/gpu/drm/drm_atomic.c >> @@ -192,6 +192,10 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state) >> } >> state->num_private_objs = 0; >> >> + if (state->fake_commit) { >> + drm_crtc_commit_put(state->fake_commit); >> + state->fake_commit = NULL; >> + } >> } >> EXPORT_SYMBOL(drm_atomic_state_default_clear); >> >> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c >> index 04629d883114..c81d46927a74 100644 >> --- a/drivers/gpu/drm/drm_atomic_helper.c >> +++ b/drivers/gpu/drm/drm_atomic_helper.c >> @@ -1667,6 +1667,38 @@ static void release_crtc_commit(struct completion *completion) >> drm_crtc_commit_put(commit); >> } >> >> +static void init_commit(struct drm_crtc_commit *commit, struct drm_crtc *crtc) >> +{ >> + init_completion(&commit->flip_done); >> + init_completion(&commit->hw_done); >> + init_completion(&commit->cleanup_done); >> + INIT_LIST_HEAD(&commit->commit_entry); >> + kref_init(&commit->ref); >> + commit->crtc = crtc; >> +} >> + >> +static struct drm_crtc_commit * >> +crtc_or_fake_commit(struct drm_atomic_state *state, struct drm_crtc *crtc) > Bikeshed: Would be nice if this function directly increases the refcount, > instead of imposing this on all callers. Would need a rename too like > crtc_or_fake_commit_get(). > > But since this bug is randomly killing our hsw CI and causing lots of > noise better to push as-is and polish later on. > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> I chose not to, to make it explicit that a extra refcount is used on the state object. But sending one final version to trybot to make sure that things don't blow up with the merge conflicts in patch 6. :) >> +{ >> + if (crtc) { >> + struct drm_crtc_state *new_crtc_state; >> + >> + new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc); >> + >> + return new_crtc_state->commit; >> + } >> + >> + if (!state->fake_commit) { >> + state->fake_commit = kzalloc(sizeof(*state->fake_commit), GFP_KERNEL); >> + if (!state->fake_commit) >> + return NULL; >> + >> + init_commit(state->fake_commit, NULL); >> + } >> + >> + return state->fake_commit; >> +} >> + >> /** >> * drm_atomic_helper_setup_commit - setup possibly nonblocking commit >> * @state: new modeset state to be committed >> @@ -1715,6 +1747,10 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state, >> { >> struct drm_crtc *crtc; >> struct drm_crtc_state *old_crtc_state, *new_crtc_state; >> + struct drm_connector *conn; >> + struct drm_connector_state *old_conn_state, *new_conn_state; >> + struct drm_plane *plane; >> + struct drm_plane_state *old_plane_state, *new_plane_state; >> struct drm_crtc_commit *commit; >> int i, ret; >> >> @@ -1723,12 +1759,7 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state, >> if (!commit) >> return -ENOMEM; >> >> - init_completion(&commit->flip_done); >> - init_completion(&commit->hw_done); >> - init_completion(&commit->cleanup_done); >> - INIT_LIST_HEAD(&commit->commit_entry); >> - kref_init(&commit->ref); >> - commit->crtc = crtc; >> + init_commit(commit, crtc); >> >> new_crtc_state->commit = commit; >> >> @@ -1764,6 +1795,42 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state, >> drm_crtc_commit_get(commit); >> } >> >> + for_each_oldnew_connector_in_state(state, conn, old_conn_state, new_conn_state, i) { >> + /* commit tracked through new_crtc_state->commit, no need to do it explicitly */ >> + if (new_conn_state->crtc) >> + continue; >> + >> + /* Userspace is not allowed to get ahead of the previous >> + * commit with nonblocking ones. */ >> + if (nonblock && old_conn_state->commit && >> + !try_wait_for_completion(&old_conn_state->commit->flip_done)) >> + return -EBUSY; >> + >> + commit = crtc_or_fake_commit(state, old_conn_state->crtc); >> + if (!commit) >> + return -ENOMEM; >> + >> + new_conn_state->commit = drm_crtc_commit_get(commit); >> + } >> + >> + for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i) { >> + /* commit tracked through new_crtc_state->commit, no need to do it explicitly */ >> + if (new_plane_state->crtc) >> + continue; >> + >> + /* Userspace is not allowed to get ahead of the previous >> + * commit with nonblocking ones. */ >> + if (nonblock && old_plane_state->commit && >> + !try_wait_for_completion(&old_plane_state->commit->flip_done)) >> + return -EBUSY; >> + >> + commit = crtc_or_fake_commit(state, old_plane_state->crtc); >> + if (!commit) >> + return -ENOMEM; >> + >> + new_plane_state->commit = drm_crtc_commit_get(commit); >> + } >> + >> return 0; >> } >> EXPORT_SYMBOL(drm_atomic_helper_setup_commit); >> @@ -1784,6 +1851,10 @@ void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *old_state) >> { >> struct drm_crtc *crtc; >> struct drm_crtc_state *old_crtc_state; >> + struct drm_plane *plane; >> + struct drm_plane_state *old_plane_state; >> + struct drm_connector *conn; >> + struct drm_connector_state *old_conn_state; >> struct drm_crtc_commit *commit; >> int i; >> long ret; >> @@ -1808,6 +1879,48 @@ void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *old_state) >> DRM_ERROR("[CRTC:%d:%s] flip_done timed out\n", >> crtc->base.id, crtc->name); >> } >> + >> + for_each_old_connector_in_state(old_state, conn, old_conn_state, i) { >> + commit = old_conn_state->commit; >> + >> + if (!commit) >> + continue; >> + >> + ret = wait_for_completion_timeout(&commit->hw_done, >> + 10*HZ); >> + if (ret == 0) >> + DRM_ERROR("[CONNECTOR:%d:%s] hw_done timed out\n", >> + conn->base.id, conn->name); >> + >> + /* Currently no support for overwriting flips, hence >> + * stall for previous one to execute completely. */ >> + ret = wait_for_completion_timeout(&commit->flip_done, >> + 10*HZ); >> + if (ret == 0) >> + DRM_ERROR("[CONNECTOR:%d:%s] flip_done timed out\n", >> + conn->base.id, conn->name); >> + } >> + >> + for_each_old_plane_in_state(old_state, plane, old_plane_state, i) { >> + commit = old_plane_state->commit; >> + >> + if (!commit) >> + continue; >> + >> + ret = wait_for_completion_timeout(&commit->hw_done, >> + 10*HZ); >> + if (ret == 0) >> + DRM_ERROR("[PLANE:%d:%s] hw_done timed out\n", >> + plane->base.id, plane->name); >> + >> + /* Currently no support for overwriting flips, hence >> + * stall for previous one to execute completely. */ >> + ret = wait_for_completion_timeout(&commit->flip_done, >> + 10*HZ); >> + if (ret == 0) >> + DRM_ERROR("[PLANE:%d:%s] flip_done timed out\n", >> + plane->base.id, plane->name); >> + } >> } >> EXPORT_SYMBOL(drm_atomic_helper_wait_for_dependencies); >> >> @@ -1842,6 +1955,11 @@ void drm_atomic_helper_commit_hw_done(struct drm_atomic_state *old_state) >> WARN_ON(new_crtc_state->event); >> complete_all(&commit->hw_done); >> } >> + >> + if (old_state->fake_commit) { >> + complete_all(&old_state->fake_commit->hw_done); >> + complete_all(&old_state->fake_commit->flip_done); >> + } >> } >> EXPORT_SYMBOL(drm_atomic_helper_commit_hw_done); >> >> @@ -1875,6 +1993,9 @@ void drm_atomic_helper_commit_cleanup_done(struct drm_atomic_state *old_state) >> list_del(&commit->commit_entry); >> spin_unlock(&crtc->commit_lock); >> } >> + >> + if (old_state->fake_commit) >> + complete_all(&old_state->fake_commit->cleanup_done); >> } >> EXPORT_SYMBOL(drm_atomic_helper_commit_cleanup_done); >> >> @@ -2254,6 +2375,15 @@ int drm_atomic_helper_swap_state(struct drm_atomic_state *state, >> struct drm_private_state *old_obj_state, *new_obj_state; >> >> if (stall) { >> + /* >> + * We have to stall for hw_done here before >> + * drm_atomic_helper_wait_for_dependencies() because flip >> + * depth > 1 is not yet supported by all drivers. As long as >> + * obj->state is directly dereferenced anywhere in the drivers >> + * atomic_commit_tail function, then it's unsafe to swap state >> + * before drm_atomic_helper_commit_hw_done() is called. >> + */ >> + >> for_each_old_crtc_in_state(state, crtc, old_crtc_state, i) { >> commit = old_crtc_state->commit; >> >> @@ -2264,6 +2394,28 @@ int drm_atomic_helper_swap_state(struct drm_atomic_state *state, >> if (ret) >> return ret; >> } >> + >> + for_each_old_connector_in_state(state, connector, old_conn_state, i) { >> + commit = old_conn_state->commit; >> + >> + if (!commit) >> + continue; >> + >> + ret = wait_for_completion_interruptible(&commit->hw_done); >> + if (ret) >> + return ret; >> + } >> + >> + for_each_old_plane_in_state(state, plane, old_plane_state, i) { >> + commit = old_plane_state->commit; >> + >> + if (!commit) >> + continue; >> + >> + ret = wait_for_completion_interruptible(&commit->hw_done); >> + if (ret) >> + return ret; >> + } >> } >> >> for_each_oldnew_connector_in_state(state, connector, old_conn_state, new_conn_state, i) { >> @@ -3246,6 +3398,7 @@ void __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane, >> drm_framebuffer_get(state->fb); >> >> state->fence = NULL; >> + state->commit = NULL; >> } >> EXPORT_SYMBOL(__drm_atomic_helper_plane_duplicate_state); >> >> @@ -3287,6 +3440,9 @@ void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state) >> >> if (state->fence) >> dma_fence_put(state->fence); >> + >> + if (state->commit) >> + drm_crtc_commit_put(state->commit); >> } >> EXPORT_SYMBOL(__drm_atomic_helper_plane_destroy_state); >> >> @@ -3365,6 +3521,7 @@ __drm_atomic_helper_connector_duplicate_state(struct drm_connector *connector, >> memcpy(state, connector->state, sizeof(*state)); >> if (state->crtc) >> drm_connector_get(connector); >> + state->commit = NULL; >> } >> EXPORT_SYMBOL(__drm_atomic_helper_connector_duplicate_state); >> >> @@ -3491,6 +3648,9 @@ __drm_atomic_helper_connector_destroy_state(struct drm_connector_state *state) >> { >> if (state->crtc) >> drm_connector_put(state->connector); >> + >> + if (state->commit) >> + drm_crtc_commit_put(state->commit); >> } >> EXPORT_SYMBOL(__drm_atomic_helper_connector_destroy_state); >> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >> index a6cf1c20c712..7abbc761a635 100644 >> --- a/drivers/gpu/drm/i915/intel_display.c >> +++ b/drivers/gpu/drm/i915/intel_display.c >> @@ -13132,8 +13132,10 @@ intel_legacy_cursor_update(struct drm_plane *plane, >> >> /* Swap plane state */ >> new_plane_state->fence = old_plane_state->fence; >> + new_plane_state->commit = old_plane_state->commit; >> *to_intel_plane_state(old_plane_state) = *to_intel_plane_state(new_plane_state); >> new_plane_state->fence = NULL; >> + new_plane_state->commit = NULL; >> new_plane_state->fb = old_fb; >> to_intel_plane_state(new_plane_state)->vma = NULL; >> >> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h >> index a80a8dadef00..07a71daa3582 100644 >> --- a/include/drm/drm_atomic.h >> +++ b/include/drm/drm_atomic.h >> @@ -236,6 +236,18 @@ struct drm_atomic_state { >> struct drm_modeset_acquire_ctx *acquire_ctx; >> >> /** >> + * @fake_commit: >> + * >> + * Used for signaling unbound planes/connectors. >> + * When a connector or plane is not bound to any CRTC, it's still important >> + * to preserve linearity to prevent the atomic states from being freed to early. >> + * >> + * This commit (if set) is not bound to any crtc, but will be completed when >> + * drm_atomic_helper_commit_hw_done() is called. >> + */ >> + struct drm_crtc_commit *fake_commit; >> + >> + /** >> * @commit_work: >> * >> * Work item which can be used by the driver or helpers to execute the >> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h >> index ea8da401c93c..8837649d16e8 100644 >> --- a/include/drm/drm_connector.h >> +++ b/include/drm/drm_connector.h >> @@ -347,6 +347,13 @@ struct drm_connector_state { >> >> struct drm_atomic_state *state; >> >> + /** >> + * @commit: Tracks the pending commit to prevent use-after-free conditions. >> + * >> + * Is only set when @crtc is NULL. >> + */ >> + struct drm_crtc_commit *commit; >> + >> struct drm_tv_connector_state tv; >> >> /** >> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h >> index 73f90f9d057f..7d96116fd4c4 100644 >> --- a/include/drm/drm_plane.h >> +++ b/include/drm/drm_plane.h >> @@ -123,6 +123,13 @@ struct drm_plane_state { >> */ >> bool visible; >> >> + /** >> + * @commit: Tracks the pending commit to prevent use-after-free conditions. >> + * >> + * Is only set when @crtc is NULL. >> + */ >> + struct drm_crtc_commit *commit; >> + >> struct drm_atomic_state *state; >> }; >> >> -- >> 2.11.0 >> >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 2cce48f203e0..75f5f74de9bf 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -192,6 +192,10 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state) } state->num_private_objs = 0; + if (state->fake_commit) { + drm_crtc_commit_put(state->fake_commit); + state->fake_commit = NULL; + } } EXPORT_SYMBOL(drm_atomic_state_default_clear); diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 04629d883114..c81d46927a74 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -1667,6 +1667,38 @@ static void release_crtc_commit(struct completion *completion) drm_crtc_commit_put(commit); } +static void init_commit(struct drm_crtc_commit *commit, struct drm_crtc *crtc) +{ + init_completion(&commit->flip_done); + init_completion(&commit->hw_done); + init_completion(&commit->cleanup_done); + INIT_LIST_HEAD(&commit->commit_entry); + kref_init(&commit->ref); + commit->crtc = crtc; +} + +static struct drm_crtc_commit * +crtc_or_fake_commit(struct drm_atomic_state *state, struct drm_crtc *crtc) +{ + if (crtc) { + struct drm_crtc_state *new_crtc_state; + + new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc); + + return new_crtc_state->commit; + } + + if (!state->fake_commit) { + state->fake_commit = kzalloc(sizeof(*state->fake_commit), GFP_KERNEL); + if (!state->fake_commit) + return NULL; + + init_commit(state->fake_commit, NULL); + } + + return state->fake_commit; +} + /** * drm_atomic_helper_setup_commit - setup possibly nonblocking commit * @state: new modeset state to be committed @@ -1715,6 +1747,10 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state, { struct drm_crtc *crtc; struct drm_crtc_state *old_crtc_state, *new_crtc_state; + struct drm_connector *conn; + struct drm_connector_state *old_conn_state, *new_conn_state; + struct drm_plane *plane; + struct drm_plane_state *old_plane_state, *new_plane_state; struct drm_crtc_commit *commit; int i, ret; @@ -1723,12 +1759,7 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state, if (!commit) return -ENOMEM; - init_completion(&commit->flip_done); - init_completion(&commit->hw_done); - init_completion(&commit->cleanup_done); - INIT_LIST_HEAD(&commit->commit_entry); - kref_init(&commit->ref); - commit->crtc = crtc; + init_commit(commit, crtc); new_crtc_state->commit = commit; @@ -1764,6 +1795,42 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state, drm_crtc_commit_get(commit); } + for_each_oldnew_connector_in_state(state, conn, old_conn_state, new_conn_state, i) { + /* commit tracked through new_crtc_state->commit, no need to do it explicitly */ + if (new_conn_state->crtc) + continue; + + /* Userspace is not allowed to get ahead of the previous + * commit with nonblocking ones. */ + if (nonblock && old_conn_state->commit && + !try_wait_for_completion(&old_conn_state->commit->flip_done)) + return -EBUSY; + + commit = crtc_or_fake_commit(state, old_conn_state->crtc); + if (!commit) + return -ENOMEM; + + new_conn_state->commit = drm_crtc_commit_get(commit); + } + + for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i) { + /* commit tracked through new_crtc_state->commit, no need to do it explicitly */ + if (new_plane_state->crtc) + continue; + + /* Userspace is not allowed to get ahead of the previous + * commit with nonblocking ones. */ + if (nonblock && old_plane_state->commit && + !try_wait_for_completion(&old_plane_state->commit->flip_done)) + return -EBUSY; + + commit = crtc_or_fake_commit(state, old_plane_state->crtc); + if (!commit) + return -ENOMEM; + + new_plane_state->commit = drm_crtc_commit_get(commit); + } + return 0; } EXPORT_SYMBOL(drm_atomic_helper_setup_commit); @@ -1784,6 +1851,10 @@ void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *old_state) { struct drm_crtc *crtc; struct drm_crtc_state *old_crtc_state; + struct drm_plane *plane; + struct drm_plane_state *old_plane_state; + struct drm_connector *conn; + struct drm_connector_state *old_conn_state; struct drm_crtc_commit *commit; int i; long ret; @@ -1808,6 +1879,48 @@ void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *old_state) DRM_ERROR("[CRTC:%d:%s] flip_done timed out\n", crtc->base.id, crtc->name); } + + for_each_old_connector_in_state(old_state, conn, old_conn_state, i) { + commit = old_conn_state->commit; + + if (!commit) + continue; + + ret = wait_for_completion_timeout(&commit->hw_done, + 10*HZ); + if (ret == 0) + DRM_ERROR("[CONNECTOR:%d:%s] hw_done timed out\n", + conn->base.id, conn->name); + + /* Currently no support for overwriting flips, hence + * stall for previous one to execute completely. */ + ret = wait_for_completion_timeout(&commit->flip_done, + 10*HZ); + if (ret == 0) + DRM_ERROR("[CONNECTOR:%d:%s] flip_done timed out\n", + conn->base.id, conn->name); + } + + for_each_old_plane_in_state(old_state, plane, old_plane_state, i) { + commit = old_plane_state->commit; + + if (!commit) + continue; + + ret = wait_for_completion_timeout(&commit->hw_done, + 10*HZ); + if (ret == 0) + DRM_ERROR("[PLANE:%d:%s] hw_done timed out\n", + plane->base.id, plane->name); + + /* Currently no support for overwriting flips, hence + * stall for previous one to execute completely. */ + ret = wait_for_completion_timeout(&commit->flip_done, + 10*HZ); + if (ret == 0) + DRM_ERROR("[PLANE:%d:%s] flip_done timed out\n", + plane->base.id, plane->name); + } } EXPORT_SYMBOL(drm_atomic_helper_wait_for_dependencies); @@ -1842,6 +1955,11 @@ void drm_atomic_helper_commit_hw_done(struct drm_atomic_state *old_state) WARN_ON(new_crtc_state->event); complete_all(&commit->hw_done); } + + if (old_state->fake_commit) { + complete_all(&old_state->fake_commit->hw_done); + complete_all(&old_state->fake_commit->flip_done); + } } EXPORT_SYMBOL(drm_atomic_helper_commit_hw_done); @@ -1875,6 +1993,9 @@ void drm_atomic_helper_commit_cleanup_done(struct drm_atomic_state *old_state) list_del(&commit->commit_entry); spin_unlock(&crtc->commit_lock); } + + if (old_state->fake_commit) + complete_all(&old_state->fake_commit->cleanup_done); } EXPORT_SYMBOL(drm_atomic_helper_commit_cleanup_done); @@ -2254,6 +2375,15 @@ int drm_atomic_helper_swap_state(struct drm_atomic_state *state, struct drm_private_state *old_obj_state, *new_obj_state; if (stall) { + /* + * We have to stall for hw_done here before + * drm_atomic_helper_wait_for_dependencies() because flip + * depth > 1 is not yet supported by all drivers. As long as + * obj->state is directly dereferenced anywhere in the drivers + * atomic_commit_tail function, then it's unsafe to swap state + * before drm_atomic_helper_commit_hw_done() is called. + */ + for_each_old_crtc_in_state(state, crtc, old_crtc_state, i) { commit = old_crtc_state->commit; @@ -2264,6 +2394,28 @@ int drm_atomic_helper_swap_state(struct drm_atomic_state *state, if (ret) return ret; } + + for_each_old_connector_in_state(state, connector, old_conn_state, i) { + commit = old_conn_state->commit; + + if (!commit) + continue; + + ret = wait_for_completion_interruptible(&commit->hw_done); + if (ret) + return ret; + } + + for_each_old_plane_in_state(state, plane, old_plane_state, i) { + commit = old_plane_state->commit; + + if (!commit) + continue; + + ret = wait_for_completion_interruptible(&commit->hw_done); + if (ret) + return ret; + } } for_each_oldnew_connector_in_state(state, connector, old_conn_state, new_conn_state, i) { @@ -3246,6 +3398,7 @@ void __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane, drm_framebuffer_get(state->fb); state->fence = NULL; + state->commit = NULL; } EXPORT_SYMBOL(__drm_atomic_helper_plane_duplicate_state); @@ -3287,6 +3440,9 @@ void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state) if (state->fence) dma_fence_put(state->fence); + + if (state->commit) + drm_crtc_commit_put(state->commit); } EXPORT_SYMBOL(__drm_atomic_helper_plane_destroy_state); @@ -3365,6 +3521,7 @@ __drm_atomic_helper_connector_duplicate_state(struct drm_connector *connector, memcpy(state, connector->state, sizeof(*state)); if (state->crtc) drm_connector_get(connector); + state->commit = NULL; } EXPORT_SYMBOL(__drm_atomic_helper_connector_duplicate_state); @@ -3491,6 +3648,9 @@ __drm_atomic_helper_connector_destroy_state(struct drm_connector_state *state) { if (state->crtc) drm_connector_put(state->connector); + + if (state->commit) + drm_crtc_commit_put(state->commit); } EXPORT_SYMBOL(__drm_atomic_helper_connector_destroy_state); diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index a6cf1c20c712..7abbc761a635 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -13132,8 +13132,10 @@ intel_legacy_cursor_update(struct drm_plane *plane, /* Swap plane state */ new_plane_state->fence = old_plane_state->fence; + new_plane_state->commit = old_plane_state->commit; *to_intel_plane_state(old_plane_state) = *to_intel_plane_state(new_plane_state); new_plane_state->fence = NULL; + new_plane_state->commit = NULL; new_plane_state->fb = old_fb; to_intel_plane_state(new_plane_state)->vma = NULL; diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h index a80a8dadef00..07a71daa3582 100644 --- a/include/drm/drm_atomic.h +++ b/include/drm/drm_atomic.h @@ -236,6 +236,18 @@ struct drm_atomic_state { struct drm_modeset_acquire_ctx *acquire_ctx; /** + * @fake_commit: + * + * Used for signaling unbound planes/connectors. + * When a connector or plane is not bound to any CRTC, it's still important + * to preserve linearity to prevent the atomic states from being freed to early. + * + * This commit (if set) is not bound to any crtc, but will be completed when + * drm_atomic_helper_commit_hw_done() is called. + */ + struct drm_crtc_commit *fake_commit; + + /** * @commit_work: * * Work item which can be used by the driver or helpers to execute the diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h index ea8da401c93c..8837649d16e8 100644 --- a/include/drm/drm_connector.h +++ b/include/drm/drm_connector.h @@ -347,6 +347,13 @@ struct drm_connector_state { struct drm_atomic_state *state; + /** + * @commit: Tracks the pending commit to prevent use-after-free conditions. + * + * Is only set when @crtc is NULL. + */ + struct drm_crtc_commit *commit; + struct drm_tv_connector_state tv; /** diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h index 73f90f9d057f..7d96116fd4c4 100644 --- a/include/drm/drm_plane.h +++ b/include/drm/drm_plane.h @@ -123,6 +123,13 @@ struct drm_plane_state { */ bool visible; + /** + * @commit: Tracks the pending commit to prevent use-after-free conditions. + * + * Is only set when @crtc is NULL. + */ + struct drm_crtc_commit *commit; + struct drm_atomic_state *state; };
Currently we neatly track the crtc state, but forget to look at plane/connector state. When doing a nonblocking modeset, immediately followed by a setprop before the modeset completes, the setprop will see the modesets new state as the old state and free it. This has to be solved by waiting for hw_done on the connector, even if it's not assigned to a crtc. When a connector is unbound we take the last crtc commit, and when it stays unbound we create a new fake crtc commit for that gets signaled on hw_done for all the planes/connectors. We wait for it the same way as we do for crtc's, which will make sure we never run into a use-after-free situation. Changes since v1: - Only create a single disable commit. (danvet) - Fix leak in intel_legacy_cursor_update. Changes since v2: - Make reference counting in drm_atomic_helper_setup_commit more obvious. (pinchartl) - Call cleanup_done for fake commit. (danvet) - Add comments to drm_atomic_helper_setup_commit. (danvet, pinchartl) - Add comment to drm_atomic_helper_swap_state. (pinchartl) Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Testcase: kms_atomic_transition.plane-use-after-nonblocking-unbind* Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- drivers/gpu/drm/drm_atomic.c | 4 + drivers/gpu/drm/drm_atomic_helper.c | 172 +++++++++++++++++++++++++++++++++-- drivers/gpu/drm/i915/intel_display.c | 2 + include/drm/drm_atomic.h | 12 +++ include/drm/drm_connector.h | 7 ++ include/drm/drm_plane.h | 7 ++ 6 files changed, 198 insertions(+), 6 deletions(-)