Re: [Intel-gfx] [PATCH 4/5] drm/atomic: Fix freeing connector/plane state too early by tracking commits, v2.

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

On Wednesday, 30 August 2017 17:17:36 EEST Daniel Vetter wrote:
> On Wed, Aug 30, 2017 at 05:10:43PM +0300, Laurent Pinchart wrote:
> > Hi Maarten,
> > 
> > Thank you for the patch.
> > 
> > On Wednesday, 30 August 2017 15:17:51 EEST 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.
> > > 
> > > 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  | 156
> > >  ++++++++++++++++++++++++++++++--
> > >  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, 182 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 8ccb8b6536c0..034f563fb130
> > > 100644
> > > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > @@ -1644,6 +1644,40 @@ 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)
> > > +{
> > 
> > You could allocate the commit in this function too, the kzalloc() is
> > currently duplicated. The function should probably be called
> > alloc_commit() then.> 
> > > +	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 *
> > > +fake_or_crtc_commit(struct drm_atomic_state *state, struct drm_crtc
> > > *crtc)
> > > +{
> > > +	struct drm_crtc_commit *commit;
> > > +
> > > +	if (crtc) {
> > > +		struct drm_crtc_state *new_crtc_state;
> > > +
> > > +		new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
> > > +
> > > +		commit = new_crtc_state->commit;
> > > +	} else if (!state->fake_commit) {
> > > +		state->fake_commit = commit = kzalloc(sizeof(*commit), 
GFP_KERNEL);
> > > +		if (!commit)
> > > +			return NULL;
> > > +
> > > +		init_commit(commit, NULL);
> > > +	} else
> > > +		commit = state->fake_commit;
> > > +
> > > +	drm_crtc_commit_get(commit);
> > 
> > I believe the reference counting is right. The double reference in the
> > second case (kref_init() when initializing the commit and
> > drm_crtc_commit_get()) should not cause a leak. The kref_init() takes a
> > reference to store the commit in state->fake_commit, released in
> > drm_atomic_state_default_clear(), and the drm_crtc_commit_get() takes a
> > reference returned by the function, stored in new_*_state->commit by the
> > caller.
> > 
> > This being said, I think the reference counting is confusing, as proved by
> > Daniel thinking there was a leak here (or by me thinking there's no leak
> > while there's one :-)). To make the implementation clearer, I propose
> > turning the definition of drm_crtc_commit_get() to
> > 
> > static inline struct drm_crtc_commit *
> > drm_crtc_commit_get(struct drm_crtc_commit *commit)
> > {
> > 
> > 	kref_get(&commit->ref);
> > 	return commit;
> > 
> > }
> > 
> > and writing this function as
> > 
> > /* Return a new reference to the commit object */
> > static struct drm_crtc_commit *
> > fake_or_crtc_commit(struct drm_atomic_state *state, struct drm_crtc *crtc)
> > {
> > 
> > 	struct drm_crtc_commit *commit;
> > 	
> > 	if (crtc) {
> > 	
> > 		struct drm_crtc_state *new_crtc_state;
> > 		
> > 		new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
> > 		
> > 		commit = new_crtc_state->commit;
> > 	
> > 	} else {
> > 	
> > 		if (!state->fake_commit)
> > 		
> > 			state->fake_commit = alloc_commit(NULL);
> > 		
> > 		commit = state->fake_commit;
> > 	
> > 	}
> > 	
> > 	return drm_crtc_commit_get(commit);
> > 
> > }
> 
> +1 on something like this, the compressed layout with assigning both
> commit and ->fake_commit is what tricked me into seeing a leak.

What I think makes it clearer is to store the return value of functions 
returning a reference directly in the pointer that stores the reference. 
That's why I prefer

1.		if (!state->fake_commit)
2.			state->fake_commit = alloc_commit(NULL);
3.
4.		commit = state->fake_commit;

Line 2 stores the pointer in an object that thus requires a reference, which 
is returned by alloc_commit(). Line 4 stores the pointer in a local variable, 
and thus doesn't require a reference.

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel