Re: [PATCH 13/16] drm/fb-helper: Avoid race with DRM userspace

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Mar 26, 2019 at 06:55:43PM +0100, Noralf Trønnes wrote:
> drm_fb_helper_is_bound() is used to check if DRM userspace is in control.
> This is done by looking at the fb on the primary plane. By the time
> fb-helper gets around to committing, it's possible that the facts have
> changed.
> 
> Avoid this race by holding the drm_device->master_mutex lock while
> committing. When DRM userspace does its first open, it will now wait
> until fb-helper is done. The helper will stay away if there's a master.
> 
> Locking rule: Always take the fb-helper lock first.
> 
> Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>

I think it'd be good to reorder this earlier in the series. And I'm
wondering why you didn't replace all occurences of the _is_bound()
function. With the consistent master state check we're doing with this I
don't think any of the fbdev checking is still needed. Plus looking at
crtc->primary->fb without any locks is racy, so would be good to ditch
that hack.
-Daniel

> ---
>  drivers/gpu/drm/drm_auth.c      | 20 ++++++++++++++
>  drivers/gpu/drm/drm_fb_helper.c | 49 ++++++++++++++++++++++++---------
>  drivers/gpu/drm/drm_internal.h  |  2 ++
>  3 files changed, 58 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c
> index 1669c42c40ed..db199807b7dc 100644
> --- a/drivers/gpu/drm/drm_auth.c
> +++ b/drivers/gpu/drm/drm_auth.c
> @@ -368,3 +368,23 @@ void drm_master_put(struct drm_master **master)
>  	*master = NULL;
>  }
>  EXPORT_SYMBOL(drm_master_put);
> +
> +/* Used by drm_client and drm_fb_helper */
> +bool drm_master_internal_acquire(struct drm_device *dev)
> +{
> +	mutex_lock(&dev->master_mutex);
> +	if (dev->master) {
> +		mutex_unlock(&dev->master_mutex);
> +		return false;
> +	}
> +
> +	return true;
> +}
> +EXPORT_SYMBOL(drm_master_internal_acquire);
> +
> +/* Used by drm_client and drm_fb_helper */
> +void drm_master_internal_release(struct drm_device *dev)
> +{
> +	mutex_unlock(&dev->master_mutex);
> +}
> +EXPORT_SYMBOL(drm_master_internal_release);
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 4a073cd4e423..9f253fcf3f79 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -41,6 +41,8 @@
>  #include <drm/drm_crtc_helper.h>
>  #include <drm/drm_atomic.h>
>  
> +#include "drm_internal.h"
> +
>  static bool drm_fbdev_emulation = true;
>  module_param_named(fbdev_emulation, drm_fbdev_emulation, bool, 0600);
>  MODULE_PARM_DESC(fbdev_emulation,
> @@ -235,7 +237,12 @@ int drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper)
>  		return 0;
>  
>  	mutex_lock(&fb_helper->lock);
> -	ret = drm_client_modesets_commit(fb_helper->dev, fb_helper->modesets);
> +	if (drm_master_internal_acquire(fb_helper->dev)) {
> +		ret = drm_client_modesets_commit(fb_helper->dev, fb_helper->modesets);
> +		drm_master_internal_release(fb_helper->dev);
> +	} else {
> +		ret = -EBUSY;
> +	}
>  
>  	do_delayed = fb_helper->delayed_hotplug;
>  	if (do_delayed)
> @@ -332,13 +339,16 @@ static struct sysrq_key_op sysrq_drm_fb_helper_restore_op = { };
>  static void drm_fb_helper_dpms(struct fb_info *info, int dpms_mode)
>  {
>  	struct drm_fb_helper *fb_helper = info->par;
> +	struct drm_device *dev = fb_helper->dev;
>  
>  	/*
>  	 * For each CRTC in this fb, turn the connectors on/off.
>  	 */
>  	mutex_lock(&fb_helper->lock);
> -	if (drm_fb_helper_is_bound(fb_helper))
> -		drm_client_modesets_dpms(fb_helper->dev, fb_helper->modesets, dpms_mode);
> +	if (drm_master_internal_acquire(dev)) {
> +		drm_client_modesets_dpms(dev, fb_helper->modesets, dpms_mode);
> +		drm_master_internal_release(dev);
> +	}
>  	mutex_unlock(&fb_helper->lock);
>  }
>  
> @@ -1097,6 +1107,7 @@ static int setcmap_atomic(struct fb_cmap *cmap, struct fb_info *info)
>  int drm_fb_helper_setcmap(struct fb_cmap *cmap, struct fb_info *info)
>  {
>  	struct drm_fb_helper *fb_helper = info->par;
> +	struct drm_device *dev = fb_helper->dev;
>  	int ret;
>  
>  	if (oops_in_progress)
> @@ -1104,9 +1115,9 @@ int drm_fb_helper_setcmap(struct fb_cmap *cmap, struct fb_info *info)
>  
>  	mutex_lock(&fb_helper->lock);
>  
> -	if (!drm_fb_helper_is_bound(fb_helper)) {
> +	if (!drm_master_internal_acquire(dev)) {
>  		ret = -EBUSY;
> -		goto out;
> +		goto unlock;
>  	}
>  
>  	if (info->fix.visual == FB_VISUAL_TRUECOLOR)
> @@ -1116,7 +1127,8 @@ int drm_fb_helper_setcmap(struct fb_cmap *cmap, struct fb_info *info)
>  	else
>  		ret = setcmap_legacy(cmap, info);
>  
> -out:
> +	drm_master_internal_release(dev);
> +unlock:
>  	mutex_unlock(&fb_helper->lock);
>  
>  	return ret;
> @@ -1136,11 +1148,13 @@ int drm_fb_helper_ioctl(struct fb_info *info, unsigned int cmd,
>  			unsigned long arg)
>  {
>  	struct drm_fb_helper *fb_helper = info->par;
> +	struct drm_device *dev = fb_helper->dev;
>  	struct drm_crtc *crtc;
>  	int ret = 0;
>  
>  	mutex_lock(&fb_helper->lock);
> -	if (!drm_fb_helper_is_bound(fb_helper)) {
> +
> +	if (!drm_master_internal_acquire(dev)) {
>  		ret = -EBUSY;
>  		goto unlock;
>  	}
> @@ -1177,13 +1191,15 @@ int drm_fb_helper_ioctl(struct fb_info *info, unsigned int cmd,
>  		}
>  
>  		ret = 0;
> -		goto unlock;
> +		break;
>  	default:
>  		ret = -ENOTTY;
>  	}
>  
> +	drm_master_internal_release(dev);
>  unlock:
>  	mutex_unlock(&fb_helper->lock);
> +
>  	return ret;
>  }
>  EXPORT_SYMBOL(drm_fb_helper_ioctl);
> @@ -1426,15 +1442,19 @@ int drm_fb_helper_pan_display(struct fb_var_screeninfo *var,
>  		return -EBUSY;
>  
>  	mutex_lock(&fb_helper->lock);
> -	if (!drm_fb_helper_is_bound(fb_helper)) {
> -		mutex_unlock(&fb_helper->lock);
> -		return -EBUSY;
> +
> +	if (!drm_master_internal_acquire(dev)) {
> +		ret = -EBUSY;
> +		goto unlock;
>  	}
>  
>  	if (drm_drv_uses_atomic_modeset(dev))
>  		ret = pan_display_atomic(var, info);
>  	else
>  		ret = pan_display_legacy(var, info);
> +
> +	drm_master_internal_release(dev);
> +unlock:
>  	mutex_unlock(&fb_helper->lock);
>  
>  	return ret;
> @@ -1451,6 +1471,7 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
>  	int ret = 0;
>  	int crtc_count = 0;
>  	struct drm_connector_list_iter conn_iter;
> +	struct drm_device *dev = fb_helper->dev;
>  	struct drm_fb_helper_surface_size sizes;
>  	struct drm_connector *connector;
>  	struct drm_mode_set *mode_set;
> @@ -1593,8 +1614,10 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
>  		DRM_INFO("Cannot find any crtc or sizes\n");
>  
>  		/* First time: disable all crtc's.. */
> -		if (!fb_helper->deferred_setup && !READ_ONCE(fb_helper->dev->master))
> -			drm_client_modesets_commit(fb_helper->dev, fb_helper->modesets);
> +		if (!fb_helper->deferred_setup && drm_master_internal_acquire(dev)) {
> +			drm_client_modesets_commit(dev, fb_helper->modesets);
> +			drm_master_internal_release(dev);
> +		}
>  		return -EAGAIN;
>  	}
>  
> diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
> index 251d67e04c2d..6d5d1184c084 100644
> --- a/drivers/gpu/drm/drm_internal.h
> +++ b/drivers/gpu/drm/drm_internal.h
> @@ -91,6 +91,8 @@ int drm_dropmaster_ioctl(struct drm_device *dev, void *data,
>  			 struct drm_file *file_priv);
>  int drm_master_open(struct drm_file *file_priv);
>  void drm_master_release(struct drm_file *file_priv);
> +bool drm_master_internal_acquire(struct drm_device *dev);
> +void drm_master_internal_release(struct drm_device *dev);
>  
>  /* drm_sysfs.c */
>  extern struct class *drm_class;
> -- 
> 2.20.1
> 

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