| Message ID | 20180502183247.5746-1-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, May 02, 2018 at 09:32:47PM +0300, Ville Syrjala wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Clear the old_state and new_state pointers for every object in > drm_atomic_state_default_clear(). Otherwise > drm_atomic_get_{new,old}_*_state() will hand out stale pointers to > anyone who hasn't first confirmed that the object is in fact part of > the current atomic transcation, if they are called after we've done > the ww backoff dance while hanging on to the same drm_atomic_state. > > For example, handle_conflicting_encoders() looks like it could hit > this since it iterates the full connector list and just calls > drm_atomic_get_new_connector_state() for each. > > And I believe we have now witnessed this happening at least once in > i915 check_digital_port_conflicts(). Commit 8b69449d2663 ("drm/i915: > Remove last references to drm_atomic_get_existing* macros") changed > the safe drm_atomic_get_existing_connector_state() to the unsafe > drm_atomic_get_new_connector_state(), which opened the doors for > this particular bug there as well. > > Cc: stable@vger.kernel.org > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Cc: Abhay Kumar <abhay.kumar@intel.com> > Fixes: 581e49fe6b41 ("drm/atomic: Add new iterators over all state, v3.") > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> Uh ... that's some bad oversight :-/ Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> btw for stable we might want to split this into 1 patch for core objects and 1 patch for driver private stuff. Feel free to do so while applying and keep my r-b. The fixes line for the 2nd patch would be: Fixes: a4370c777406 ("drm/atomic: Make private objs proper objects") Cc: <stable@vger.kernel.org> # v4.14+ Cheers, Daniel > --- > drivers/gpu/drm/drm_atomic.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > index 7d25c42f22db..c825c76edc1d 100644 > --- a/drivers/gpu/drm/drm_atomic.c > +++ b/drivers/gpu/drm/drm_atomic.c > @@ -155,6 +155,8 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state) > state->connectors[i].state); > state->connectors[i].ptr = NULL; > state->connectors[i].state = NULL; > + state->connectors[i].old_state = NULL; > + state->connectors[i].new_state = NULL; > drm_connector_put(connector); > } > > @@ -169,6 +171,8 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state) > > state->crtcs[i].ptr = NULL; > state->crtcs[i].state = NULL; > + state->crtcs[i].old_state = NULL; > + state->crtcs[i].new_state = NULL; > } > > for (i = 0; i < config->num_total_plane; i++) { > @@ -181,6 +185,8 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state) > state->planes[i].state); > state->planes[i].ptr = NULL; > state->planes[i].state = NULL; > + state->planes[i].old_state = NULL; > + state->planes[i].new_state = NULL; > } > > for (i = 0; i < state->num_private_objs; i++) { > @@ -190,6 +196,8 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state) > state->private_objs[i].state); > state->private_objs[i].ptr = NULL; > state->private_objs[i].state = NULL; > + state->private_objs[i].old_state = NULL; > + state->private_objs[i].new_state = NULL; > } > state->num_private_objs = 0; > > -- > 2.16.1 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
Op 02-05-18 om 20:32 schreef Ville Syrjala: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Clear the old_state and new_state pointers for every object in > drm_atomic_state_default_clear(). Otherwise > drm_atomic_get_{new,old}_*_state() will hand out stale pointers to > anyone who hasn't first confirmed that the object is in fact part of > the current atomic transcation, if they are called after we've done > the ww backoff dance while hanging on to the same drm_atomic_state. > > For example, handle_conflicting_encoders() looks like it could hit > this since it iterates the full connector list and just calls > drm_atomic_get_new_connector_state() for each. > > And I believe we have now witnessed this happening at least once in > i915 check_digital_port_conflicts(). Commit 8b69449d2663 ("drm/i915: > Remove last references to drm_atomic_get_existing* macros") changed > the safe drm_atomic_get_existing_connector_state() to the unsafe > drm_atomic_get_new_connector_state(), which opened the doors for > this particular bug there as well. > > Cc: stable@vger.kernel.org > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Cc: Abhay Kumar <abhay.kumar@intel.com> > Fixes: 581e49fe6b41 ("drm/atomic: Add new iterators over all state, v3.") > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- OUCH! Good catch.. ~Maarten Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> How come KASAN didn't complain?
On Thu, May 03, 2018 at 08:35:30AM +0200, Daniel Vetter wrote: > On Wed, May 02, 2018 at 09:32:47PM +0300, Ville Syrjala wrote: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Clear the old_state and new_state pointers for every object in > > drm_atomic_state_default_clear(). Otherwise > > drm_atomic_get_{new,old}_*_state() will hand out stale pointers to > > anyone who hasn't first confirmed that the object is in fact part of > > the current atomic transcation, if they are called after we've done > > the ww backoff dance while hanging on to the same drm_atomic_state. > > > > For example, handle_conflicting_encoders() looks like it could hit > > this since it iterates the full connector list and just calls > > drm_atomic_get_new_connector_state() for each. > > > > And I believe we have now witnessed this happening at least once in > > i915 check_digital_port_conflicts(). Commit 8b69449d2663 ("drm/i915: > > Remove last references to drm_atomic_get_existing* macros") changed > > the safe drm_atomic_get_existing_connector_state() to the unsafe > > drm_atomic_get_new_connector_state(), which opened the doors for > > this particular bug there as well. > > > > Cc: stable@vger.kernel.org > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Cc: Abhay Kumar <abhay.kumar@intel.com> > > Fixes: 581e49fe6b41 ("drm/atomic: Add new iterators over all state, v3.") > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Uh ... that's some bad oversight :-/ > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > btw for stable we might want to split this into 1 patch for core objects > and 1 patch for driver private stuff. Good point. I forgot we did that after the new iterators were introduced. I'll do the split. > Feel free to do so while applying > and keep my r-b. The fixes line for the 2nd patch would be: > > Fixes: a4370c777406 ("drm/atomic: Make private objs proper objects") > Cc: <stable@vger.kernel.org> # v4.14+ Ta. > > Cheers, Daniel > > --- > > drivers/gpu/drm/drm_atomic.c | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > > index 7d25c42f22db..c825c76edc1d 100644 > > --- a/drivers/gpu/drm/drm_atomic.c > > +++ b/drivers/gpu/drm/drm_atomic.c > > @@ -155,6 +155,8 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state) > > state->connectors[i].state); > > state->connectors[i].ptr = NULL; > > state->connectors[i].state = NULL; > > + state->connectors[i].old_state = NULL; > > + state->connectors[i].new_state = NULL; > > drm_connector_put(connector); > > } > > > > @@ -169,6 +171,8 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state) > > > > state->crtcs[i].ptr = NULL; > > state->crtcs[i].state = NULL; > > + state->crtcs[i].old_state = NULL; > > + state->crtcs[i].new_state = NULL; > > } > > > > for (i = 0; i < config->num_total_plane; i++) { > > @@ -181,6 +185,8 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state) > > state->planes[i].state); > > state->planes[i].ptr = NULL; > > state->planes[i].state = NULL; > > + state->planes[i].old_state = NULL; > > + state->planes[i].new_state = NULL; > > } > > > > for (i = 0; i < state->num_private_objs; i++) { > > @@ -190,6 +196,8 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state) > > state->private_objs[i].state); > > state->private_objs[i].ptr = NULL; > > state->private_objs[i].state = NULL; > > + state->private_objs[i].old_state = NULL; > > + state->private_objs[i].new_state = NULL; > > } > > state->num_private_objs = 0; > > > > -- > > 2.16.1 > > > > _______________________________________________ > > dri-devel mailing list > > dri-devel@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/dri-devel > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
On Thu, May 03, 2018 at 01:41:52PM +0300, Ville Syrjälä wrote: > On Thu, May 03, 2018 at 08:35:30AM +0200, Daniel Vetter wrote: > > On Wed, May 02, 2018 at 09:32:47PM +0300, Ville Syrjala wrote: > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > Clear the old_state and new_state pointers for every object in > > > drm_atomic_state_default_clear(). Otherwise > > > drm_atomic_get_{new,old}_*_state() will hand out stale pointers to > > > anyone who hasn't first confirmed that the object is in fact part of > > > the current atomic transcation, if they are called after we've done > > > the ww backoff dance while hanging on to the same drm_atomic_state. > > > > > > For example, handle_conflicting_encoders() looks like it could hit > > > this since it iterates the full connector list and just calls > > > drm_atomic_get_new_connector_state() for each. > > > > > > And I believe we have now witnessed this happening at least once in > > > i915 check_digital_port_conflicts(). Commit 8b69449d2663 ("drm/i915: > > > Remove last references to drm_atomic_get_existing* macros") changed > > > the safe drm_atomic_get_existing_connector_state() to the unsafe > > > drm_atomic_get_new_connector_state(), which opened the doors for > > > this particular bug there as well. > > > > > > Cc: stable@vger.kernel.org > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > Cc: Abhay Kumar <abhay.kumar@intel.com> > > > Fixes: 581e49fe6b41 ("drm/atomic: Add new iterators over all state, v3.") > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Uh ... that's some bad oversight :-/ > > > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > > > btw for stable we might want to split this into 1 patch for core objects > > and 1 patch for driver private stuff. > > Good point. I forgot we did that after the new iterators were > introduced. I'll do the split. Pushed the split version for drm-misc-fixes. As mentioned on irc we don't have any get_{new,old}_private_state() functions, so the private objs part is pretty theoretical. However splitting does have the benefit of (hopefully) not requiring manual intervention when someone tries to cherry-pick this to 4.12/4.13. Thanks for the reviews. > > > Feel free to do so while applying > > and keep my r-b. The fixes line for the 2nd patch would be: > > > > Fixes: a4370c777406 ("drm/atomic: Make private objs proper objects") > > Cc: <stable@vger.kernel.org> # v4.14+ > > Ta. > > > > > Cheers, Daniel > > > --- > > > drivers/gpu/drm/drm_atomic.c | 8 ++++++++ > > > 1 file changed, 8 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > > > index 7d25c42f22db..c825c76edc1d 100644 > > > --- a/drivers/gpu/drm/drm_atomic.c > > > +++ b/drivers/gpu/drm/drm_atomic.c > > > @@ -155,6 +155,8 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state) > > > state->connectors[i].state); > > > state->connectors[i].ptr = NULL; > > > state->connectors[i].state = NULL; > > > + state->connectors[i].old_state = NULL; > > > + state->connectors[i].new_state = NULL; > > > drm_connector_put(connector); > > > } > > > > > > @@ -169,6 +171,8 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state) > > > > > > state->crtcs[i].ptr = NULL; > > > state->crtcs[i].state = NULL; > > > + state->crtcs[i].old_state = NULL; > > > + state->crtcs[i].new_state = NULL; > > > } > > > > > > for (i = 0; i < config->num_total_plane; i++) { > > > @@ -181,6 +185,8 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state) > > > state->planes[i].state); > > > state->planes[i].ptr = NULL; > > > state->planes[i].state = NULL; > > > + state->planes[i].old_state = NULL; > > > + state->planes[i].new_state = NULL; > > > } > > > > > > for (i = 0; i < state->num_private_objs; i++) { > > > @@ -190,6 +196,8 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state) > > > state->private_objs[i].state); > > > state->private_objs[i].ptr = NULL; > > > state->private_objs[i].state = NULL; > > > + state->private_objs[i].old_state = NULL; > > > + state->private_objs[i].new_state = NULL; > > > } > > > state->num_private_objs = 0; > > > > > > -- > > > 2.16.1 > > > > > > _______________________________________________ > > > dri-devel mailing list > > > dri-devel@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel > > > > -- > > Daniel Vetter > > Software Engineer, Intel Corporation > > http://blog.ffwll.ch > > -- > Ville Syrjälä > Intel > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 7d25c42f22db..c825c76edc1d 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -155,6 +155,8 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state) state->connectors[i].state); state->connectors[i].ptr = NULL; state->connectors[i].state = NULL; + state->connectors[i].old_state = NULL; + state->connectors[i].new_state = NULL; drm_connector_put(connector); } @@ -169,6 +171,8 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state) state->crtcs[i].ptr = NULL; state->crtcs[i].state = NULL; + state->crtcs[i].old_state = NULL; + state->crtcs[i].new_state = NULL; } for (i = 0; i < config->num_total_plane; i++) { @@ -181,6 +185,8 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state) state->planes[i].state); state->planes[i].ptr = NULL; state->planes[i].state = NULL; + state->planes[i].old_state = NULL; + state->planes[i].new_state = NULL; } for (i = 0; i < state->num_private_objs; i++) { @@ -190,6 +196,8 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state) state->private_objs[i].state); state->private_objs[i].ptr = NULL; state->private_objs[i].state = NULL; + state->private_objs[i].old_state = NULL; + state->private_objs[i].new_state = NULL; } state->num_private_objs = 0;