Re: [Intel-gfx] [PATCH] drm/atomic: Clean old_state/new_state in drm_atomic_state_default_clear()

From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: stable@vger.kernel.org, intel-gfx@lists.freedesktop.org,
	Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
	dri-devel@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH] drm/atomic: Clean old_state/new_state in drm_atomic_state_default_clear()
Date: Thu, 3 May 2018 17:48:28 +0300	[thread overview]
Message-ID: <20180503144828.GX23723@intel.com> (raw)
In-Reply-To: <20180503104152.GP23723@intel.com>

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

-- 
Ville Syrjï¿½lï¿½
Intel