[PATCH] [rfc] drm: close race in connector registration

[Daniel Vetter]

On Thu, Jul 24, 2014 at 11:53:45AM +1000, Dave Airlie wrote:
> From: Dave Airlie <airlied at redhat.com>
> 
> Daniel pointed out with hotplug that userspace could be trying to oops us
> as root for lols, and that to be correct we shouldn't register the object
> with the idr before we have fully set the connector object up.
> 
> His proposed solution was a lot more life changing, this seemed like a simpler
> proposition to me, get the connector object id from the idr, but don't
> register the object until the drm_connector_register callback.
> 
> The open question is whether the drm_mode_object_register needs a bigger lock
> than just the idr one, but I can't see why it would, but I can be locking
> challenged.
> 
> Signed-off-by: Dave Airlie <airlied at redhat.com>

The noreg=false double negation is a bit a bender, I'd invert the meaning
of that. But otherwise I couldn't poke holes into this (but found another
one around the idr_mutex). Fix for that is on its way.

With noreg negated and the kerneldoc/EXPORT_SYMBOL moved to the new place
this is Reviewed-by: Daniel Vetter <daniel.vetter at ffwll.ch>

Cheers, Daniel
> ---
>  drivers/gpu/drm/drm_crtc.c | 25 +++++++++++++++++++++----
>  1 file changed, 21 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 1ccf5cb..9f8cc1a 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -376,13 +376,14 @@ EXPORT_SYMBOL(drm_get_format_name);
>   * New unique (relative to other objects in @dev) integer identifier for the
>   * object.
>   */
> -int drm_mode_object_get(struct drm_device *dev,
> -			struct drm_mode_object *obj, uint32_t obj_type)
> +static int drm_mode_object_get_noreg(struct drm_device *dev,
> +				     struct drm_mode_object *obj, uint32_t obj_type,
> +				     bool noreg)
>  {
>  	int ret;
>  
>  	mutex_lock(&dev->mode_config.idr_mutex);
> -	ret = idr_alloc(&dev->mode_config.crtc_idr, obj, 1, 0, GFP_KERNEL);
> +	ret = idr_alloc(&dev->mode_config.crtc_idr, noreg ? NULL : obj, 1, 0, GFP_KERNEL);
>  	if (ret >= 0) {
>  		/*
>  		 * Set up the object linking under the protection of the idr
> @@ -396,6 +397,20 @@ int drm_mode_object_get(struct drm_device *dev,
>  	return ret < 0 ? ret : 0;
>  }
>  
> +int drm_mode_object_get(struct drm_device *dev,
> +			struct drm_mode_object *obj, uint32_t obj_type)
> +{
> +	return drm_mode_object_get_noreg(dev, obj, obj_type, false);
> +}
> +
> +static void drm_mode_object_register(struct drm_device *dev,
> +				     struct drm_mode_object *obj)
> +{
> +	mutex_lock(&dev->mode_config.idr_mutex);
> +	idr_replace(&dev->mode_config.crtc_idr, obj, obj->id);
> +	mutex_unlock(&dev->mode_config.idr_mutex);
> +}
> +
>  /**
>   * drm_mode_object_put - free a modeset identifer
>   * @dev: DRM device
> @@ -849,7 +864,7 @@ int drm_connector_init(struct drm_device *dev,
>  
>  	drm_modeset_lock_all(dev);
>  
> -	ret = drm_mode_object_get(dev, &connector->base, DRM_MODE_OBJECT_CONNECTOR);
> +	ret = drm_mode_object_get_noreg(dev, &connector->base, DRM_MODE_OBJECT_CONNECTOR, true);
>  	if (ret)
>  		goto out_unlock;
>  
> @@ -942,6 +957,8 @@ int drm_connector_register(struct drm_connector *connector)
>  {
>  	int ret;
>  
> +	drm_mode_object_register(connector->dev, &connector->base);
> +
>  	ret = drm_sysfs_connector_add(connector);
>  	if (ret)
>  		return ret;
> -- 
> 1.9.3
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch