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 --- 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;
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;@@ -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;-- 2.16.1
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
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;@@ -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;-- 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;@@ -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;-- 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
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?
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Maarten Lankhorst -
Ville Syrjala -
Ville Syrjälä