Re: [PATCH] drm: Add checks for NULL drm_*_helper_funcs

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, May 29, 2018 at 12:03:24PM +0300, Haneen Mohammed wrote:
> On Tue, May 29, 2018 at 10:03:53AM +0200, Daniel Vetter wrote:
> > On Fri, May 25, 2018 at 05:20:08AM +0300, Haneen Mohammed wrote:
> > > This patch add checks for NULL drm_[connector/crtc/plane]_helper_funcs
> > > pointers before derefrencing the variable to avoid NULL pointer
> > > dereference and make the helper functions as optional as possible.
> > > 
> > > Signed-off-by: Haneen Mohammed <hamohammed.sa@gmail.com>
> > 
> > I started reviewing this, and then realized it's a bit a can of worms.
> > E.g. connector->funcs->detect shouldn't be there, it's a helper callback
> > really (but placed in the wrong function table). So connector->funcs isn't
> > optional, but connector->funcs->detect maybe should be optional.
> > 
> > So I'm not sure anymore whether doing this holesale is a good idea. Do we
> > still need this for vkms? If yes, then a more focused patch to just make
> > the things optional that vkms does not (yet) provide might be better.
> > Including an explanation of what exactly blows up in vkms.
> > 
> 
> hm only for running the igt tests & modetest in libdrm. 
> 
> It was easier to add check for all the funcs, but I will check again the
> least amount of checks needed for vkms.

To clarify what I'm looking for: I think a more specific approach like
"vkms doesn have funcs for $object, and that makes sense because $reason."
I think making some of the funcs optional might not make any sense at all,
since any working driver needs them. vkms atm is special, since it just
absolutely nothing right now (but that will change).

The goal here is to make driver writing easier by only requiring what's
really needed, not mislead them by not requiring stuff that really should
be there for a working driver :-)
-Daniel

> 
> Thank you!
> Haneen
> 
> > Thanks, Daniel
> > > ---
> > >  drivers/gpu/drm/drm_atomic_helper.c | 42 +++++++++++++++--------------
> > >  drivers/gpu/drm/drm_probe_helper.c  | 11 ++++----
> > >  2 files changed, 28 insertions(+), 25 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > > index c35654591c12..52092deb741d 100644
> > > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > @@ -112,9 +112,9 @@ static int handle_conflicting_encoders(struct drm_atomic_state *state,
> > >  		if (!new_conn_state->crtc)
> > >  			continue;
> > >  
> > > -		if (funcs->atomic_best_encoder)
> > > +		if (funcs && funcs->atomic_best_encoder)
> > >  			new_encoder = funcs->atomic_best_encoder(connector, new_conn_state);
> > > -		else if (funcs->best_encoder)
> > > +		else if (funcs && funcs->best_encoder)
> > >  			new_encoder = funcs->best_encoder(connector);
> > >  		else
> > >  			new_encoder = drm_atomic_helper_best_encoder(connector);
> > > @@ -308,10 +308,10 @@ update_connector_routing(struct drm_atomic_state *state,
> > >  
> > >  	funcs = connector->helper_private;
> > >  
> > > -	if (funcs->atomic_best_encoder)
> > > +	if (funcs && funcs->atomic_best_encoder)
> > >  		new_encoder = funcs->atomic_best_encoder(connector,
> > >  							 new_connector_state);
> > > -	else if (funcs->best_encoder)
> > > +	else if (funcs && funcs->best_encoder)
> > >  		new_encoder = funcs->best_encoder(connector);
> > >  	else
> > >  		new_encoder = drm_atomic_helper_best_encoder(connector);
> > > @@ -438,7 +438,7 @@ mode_fixup(struct drm_atomic_state *state)
> > >  			continue;
> > >  
> > >  		funcs = crtc->helper_private;
> > > -		if (!funcs->mode_fixup)
> > > +		if (!funcs || !funcs->mode_fixup)
> > >  			continue;
> > >  
> > >  		ret = funcs->mode_fixup(crtc, &new_crtc_state->mode,
> > > @@ -639,7 +639,7 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
> > >  				new_crtc_state->connectors_changed = true;
> > >  		}
> > >  
> > > -		if (funcs->atomic_check)
> > > +		if (funcs && funcs->atomic_check)
> > >  			ret = funcs->atomic_check(connector, new_connector_state);
> > >  		if (ret)
> > >  			return ret;
> > > @@ -681,7 +681,7 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
> > >  		if (connectors_mask & BIT(i))
> > >  			continue;
> > >  
> > > -		if (funcs->atomic_check)
> > > +		if (funcs && funcs->atomic_check)
> > >  			ret = funcs->atomic_check(connector, new_connector_state);
> > >  		if (ret)
> > >  			return ret;
> > > @@ -972,14 +972,16 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
> > >  
> > >  
> > >  		/* Right function depends upon target state. */
> > > -		if (new_crtc_state->enable && funcs->prepare)
> > > -			funcs->prepare(crtc);
> > > -		else if (funcs->atomic_disable)
> > > -			funcs->atomic_disable(crtc, old_crtc_state);
> > > -		else if (funcs->disable)
> > > -			funcs->disable(crtc);
> > > -		else
> > > -			funcs->dpms(crtc, DRM_MODE_DPMS_OFF);
> > > +		if (funcs) {
> > > +			if (new_crtc_state->enable && funcs->prepare)
> > > +				funcs->prepare(crtc);
> > > +			else if (funcs->atomic_disable)
> > > +				funcs->atomic_disable(crtc, old_crtc_state);
> > > +			else if (funcs->disable)
> > > +				funcs->disable(crtc);
> > > +			else
> > > +				funcs->dpms(crtc, DRM_MODE_DPMS_OFF);
> > > +		}
> > >  
> > >  		if (!(dev->irq_enabled && dev->num_crtcs))
> > >  			continue;
> > > @@ -1093,7 +1095,7 @@ crtc_set_mode(struct drm_device *dev, struct drm_atomic_state *old_state)
> > >  
> > >  		funcs = crtc->helper_private;
> > >  
> > > -		if (new_crtc_state->enable && funcs->mode_set_nofb) {
> > > +		if (new_crtc_state->enable && funcs && funcs->mode_set_nofb) {
> > >  			DRM_DEBUG_ATOMIC("modeset on [CRTC:%d:%s]\n",
> > >  					 crtc->base.id, crtc->name);
> > >  
> > > @@ -1197,7 +1199,7 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
> > >  
> > >  		funcs = crtc->helper_private;
> > >  
> > > -		if (new_crtc_state->enable) {
> > > +		if (funcs && new_crtc_state->enable) {
> > >  			DRM_DEBUG_ATOMIC("enabling [CRTC:%d:%s]\n",
> > >  					 crtc->base.id, crtc->name);
> > >  
> > > @@ -2117,7 +2119,7 @@ int drm_atomic_helper_prepare_planes(struct drm_device *dev,
> > >  
> > >  		funcs = plane->helper_private;
> > >  
> > > -		if (funcs->prepare_fb) {
> > > +		if (funcs && funcs->prepare_fb) {
> > >  			ret = funcs->prepare_fb(plane, new_plane_state);
> > >  			if (ret)
> > >  				goto fail;
> > > @@ -2135,7 +2137,7 @@ int drm_atomic_helper_prepare_planes(struct drm_device *dev,
> > >  
> > >  		funcs = plane->helper_private;
> > >  
> > > -		if (funcs->cleanup_fb)
> > > +		if (funcs && funcs->cleanup_fb)
> > >  			funcs->cleanup_fb(plane, new_plane_state);
> > >  	}
> > >  
> > > @@ -2412,7 +2414,7 @@ void drm_atomic_helper_cleanup_planes(struct drm_device *dev,
> > >  
> > >  		funcs = plane->helper_private;
> > >  
> > > -		if (funcs->cleanup_fb)
> > > +		if (funcs && funcs->cleanup_fb)
> > >  			funcs->cleanup_fb(plane, plane_state);
> > >  	}
> > >  }
> > > diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> > > index 527743394150..42507aa7f763 100644
> > > --- a/drivers/gpu/drm/drm_probe_helper.c
> > > +++ b/drivers/gpu/drm/drm_probe_helper.c
> > > @@ -272,9 +272,9 @@ drm_helper_probe_detect_ctx(struct drm_connector *connector, bool force)
> > >  retry:
> > >  	ret = drm_modeset_lock(&connector->dev->mode_config.connection_mutex, &ctx);
> > >  	if (!ret) {
> > > -		if (funcs->detect_ctx)
> > > +		if (funcs && funcs->detect_ctx)
> > >  			ret = funcs->detect_ctx(connector, &ctx, force);
> > > -		else if (connector->funcs->detect)
> > > +		else if (connector->funcs && connector->funcs->detect)
> > >  			ret = connector->funcs->detect(connector, force);
> > >  		else
> > >  			ret = connector_status_connected;
> > > @@ -320,9 +320,9 @@ drm_helper_probe_detect(struct drm_connector *connector,
> > >  	if (ret)
> > >  		return ret;
> > >  
> > > -	if (funcs->detect_ctx)
> > > +	if (funcs && funcs->detect_ctx)
> > >  		return funcs->detect_ctx(connector, ctx, force);
> > > -	else if (connector->funcs->detect)
> > > +	else if (connector->funcs && connector->funcs->detect)
> > >  		return connector->funcs->detect(connector, force);
> > >  	else
> > >  		return connector_status_connected;
> > > @@ -480,7 +480,8 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
> > >  		goto prune;
> > >  	}
> > >  
> > > -	count = (*connector_funcs->get_modes)(connector);
> > > +	if (connector_funcs && connector_funcs->get_modes)
> > > +		count = (*connector_funcs->get_modes)(connector);
> > >  
> > >  	if (count == 0 && connector->status == connector_status_connected)
> > >  		count = drm_add_modes_noedid(connector, 1024, 768);
> > > -- 
> > > 2.17.0
> > > 
> > 
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel