From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga03.intel.com ([134.134.136.65]:45402 "EHLO mga03.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751772AbeECKl5 (ORCPT ); Thu, 3 May 2018 06:41:57 -0400 Date: Thu, 3 May 2018 13:41:52 +0300 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= To: Daniel Vetter Cc: dri-devel@lists.freedesktop.org, intel-gfx@lists.freedesktop.org, Abhay Kumar , Laurent Pinchart , stable@vger.kernel.org Subject: Re: [PATCH] drm/atomic: Clean old_state/new_state in drm_atomic_state_default_clear() Message-ID: <20180503104152.GP23723@intel.com> References: <20180502183247.5746-1-ville.syrjala@linux.intel.com> <20180503063530.GE12521@phenom.ffwll.local> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20180503063530.GE12521@phenom.ffwll.local> Sender: stable-owner@vger.kernel.org List-ID: 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� > > > > 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 > > Cc: Laurent Pinchart > > Cc: Abhay Kumar > > Fixes: 581e49fe6b41 ("drm/atomic: Add new iterators over all state, v3.") > > Signed-off-by: Ville Syrj�l� > > Uh ... that's some bad oversight :-/ > > Reviewed-by: Daniel Vetter > > 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: # 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