[Intel-gfx] [PATCH v4 1/1] drm/fb-helper: Avoid race with DRM userspace

Noralf Trønnes noralf at tronnes.org
Sat May 4 12:34:05 UTC 2019



Den 25.04.2019 10.31, skrev Noralf Trønnes:
> 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.
> 
> v2:
> - Remove drm_fb_helper_is_bound() (Daniel Vetter)
> - No need to check fb_helper->dev->master in
>   drm_fb_helper_single_fb_probe(), restore_fbdev_mode() has the check.
> 
> Suggested-by: Daniel Vetter <daniel.vetter at ffwll.ch>
> Signed-off-by: Noralf Trønnes <noralf at tronnes.org>
> Reviewed-by: Daniel Vetter <daniel.vetter at ffwll.ch>
> ---
>  drivers/gpu/drm/drm_auth.c      | 20 ++++++++
>  drivers/gpu/drm/drm_fb_helper.c | 90 ++++++++++++++++-----------------
>  drivers/gpu/drm/drm_internal.h  |  2 +
>  3 files changed, 67 insertions(+), 45 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 2339f0f8f5a8..578428461391 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -44,6 +44,7 @@
>  
>  #include "drm_crtc_internal.h"
>  #include "drm_crtc_helper_internal.h"
> +#include "drm_internal.h"
>  
>  static bool drm_fbdev_emulation = true;
>  module_param_named(fbdev_emulation, drm_fbdev_emulation, bool, 0600);
> @@ -509,7 +510,7 @@ static int restore_fbdev_mode_legacy(struct drm_fb_helper *fb_helper)
>  	return ret;
>  }
>  
> -static int restore_fbdev_mode(struct drm_fb_helper *fb_helper)
> +static int restore_fbdev_mode_force(struct drm_fb_helper *fb_helper)
>  {
>  	struct drm_device *dev = fb_helper->dev;
>  
> @@ -519,6 +520,21 @@ static int restore_fbdev_mode(struct drm_fb_helper *fb_helper)
>  		return restore_fbdev_mode_legacy(fb_helper);
>  }
>  
> +static int restore_fbdev_mode(struct drm_fb_helper *fb_helper)
> +{
> +	struct drm_device *dev = fb_helper->dev;
> +	int ret;
> +
> +	if (!drm_master_internal_acquire(dev))
> +		return -EBUSY;
> +
> +	ret = restore_fbdev_mode_force(fb_helper);
> +
> +	drm_master_internal_release(dev);
> +
> +	return ret;
> +}
> +
>  /**
>   * drm_fb_helper_restore_fbdev_mode_unlocked - restore fbdev configuration
>   * @fb_helper: driver-allocated fbdev helper, can be NULL

The Intel CI doesn't like this patch. AFAICT the reason is that the
igt at kms_fbcon_fbt@psr-suspend test expects fbcon to work while it has an
open fd that is master. This doesn't match the new rule of bailing out
if there's a master.

Adding this debug output:

@@ -558,6 +558,17 @@  int
drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper)
 		return 0;

 	mutex_lock(&fb_helper->lock);
+if (READ_ONCE(fb_helper->dev->master)) {
+	int level = default_message_loglevel;
+
+	default_message_loglevel = LOGLEVEL_DEBUG;
+	printk("\n");
+	printk("\n");
+	printk("%s\n", __func__);
+	printk("    THERE IS A MASTER\n");
+	dump_stack();
+	default_message_loglevel = level;
+}
 	ret = restore_fbdev_mode_force(fb_helper);

 	do_delayed = fb_helper->delayed_hotplug;

Gives these log entries:

<7> [1857.940072] drm_fb_helper_restore_fbdev_mode_unlocked
<7> [1857.940074]     THERE IS A MASTER
<7> [1857.940079] CPU: 4 PID: 8209 Comm: kms_fbcon_fbt Tainted: G     U
          5.1.0-rc7-CI-Trybot_4252+ #1
<7> [1857.940081] Hardware name: Intel Corporation Ice Lake Client
Platform/IceLake U DDR4 SODIMM PD RVP, BIOS
ICLSFWR1.R00.3121.A00.1903190527 03/19/2019
<7> [1857.940083] Call Trace:
<7> [1857.940091]  dump_stack+0x67/0x9b
<7> [1857.940099]  drm_fb_helper_restore_fbdev_mode_unlocked+0xda/0xf0
<7> [1857.940104]  drm_fb_helper_set_par+0x24/0x50
<7> [1857.940188]  intel_fbdev_set_par+0x11/0x40 [i915]
<7> [1857.940197]  fb_set_var+0x17a/0x3f0
<7> [1857.940212]  ? __lock_acquire+0x49f/0x1590
<7> [1857.940230]  fbcon_blank+0x192/0x2e0
<7> [1857.940235]  ? __lock_acquire+0x49f/0x1590
<7> [1857.940254]  do_unblank_screen+0xa1/0x170
<7> [1857.940260]  vt_ioctl+0x505/0x11d0
<7> [1857.940270]  tty_ioctl+0xee/0x940
<7> [1857.940274]  ? lockdep_hardirqs_off+0x94/0xd0
<7> [1857.940278]  ? __slab_free+0x24e/0x4f0
<7> [1857.940287]  ? _raw_spin_unlock_irqrestore+0x39/0x60
<7> [1857.940291]  ? kmem_cache_free+0x279/0x2e0
<7> [1857.940299]  do_vfs_ioctl+0xa0/0x6e0
<7> [1857.940305]  ? do_sys_open+0x13b/0x250
<7> [1857.940311]  ? rcu_read_lock_sched_held+0x6f/0x80
<7> [1857.940313]  ? kmem_cache_free+0x283/0x2e0
<7> [1857.940321]  ksys_ioctl+0x35/0x60
<7> [1857.940328]  __x64_sys_ioctl+0x11/0x20
<7> [1857.940332]  do_syscall_64+0x55/0x190
<7> [1857.940337]  entry_SYSCALL_64_after_hwframe+0x49/0xbe
<7> [1857.940341] RIP: 0033:0x7faf8f0825d7
<7> [1857.940345] Code: b3 66 90 48 8b 05 b1 48 2d 00 64 c7 00 26 00 00
00 48 c7 c0 ff ff ff ff c3 66 2e 0f 1f 84 00 00 00 00 00 b8 10 00 00 00
0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 81 48 2d 00 f7 d8 64 89 01 48
<7> [1857.940347] RSP: 002b:00007ffddf6a94b8 EFLAGS: 00000246 ORIG_RAX:
0000000000000010
<7> [1857.940351] RAX: ffffffffffffffda RBX: 0000000000000000 RCX:
00007faf8f0825d7
<7> [1857.940353] RDX: 0000000000000000 RSI: 0000000000004b3a RDI:
000000000000000a
<7> [1857.940355] RBP: 0000000000000000 R08: 0000000000000000 R09:
00000000000000ab
<7> [1857.940357] R10: 0000000000000000 R11: 0000000000000246 R12:
0000000000004b3a
<7> [1857.940359] R13: 000000000000000a R14: 0000000000000000 R15:
0000000000000000
<6> [1861.481078] [IGT] kms_fbcon_fbt: exiting, ret=99

Patches:
https://patchwork.freedesktop.org/series/59951/#rev8
Failing test:
https://intel-gfx-ci.01.org/tree/drm-tip/Trybot_4252/shard-iclb7/igt@kms_fbcon_fbt@psr-suspend.html


If I make this change and don't check for master, then it's fine:

@@ -542,7 +558,7 @@  int
drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper)
 		return 0;

 	mutex_lock(&fb_helper->lock);
-	ret = restore_fbdev_mode(fb_helper);
+	ret = restore_fbdev_mode_force(fb_helper);

 	do_delayed = fb_helper->delayed_hotplug;
 	if (do_delayed)

https://patchwork.freedesktop.org/series/59951/#rev9

Should I make this change, or should the test change?

Noralf.

> @@ -556,34 +572,6 @@ int drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper)
>  }
>  EXPORT_SYMBOL(drm_fb_helper_restore_fbdev_mode_unlocked);
>  
> -static bool drm_fb_helper_is_bound(struct drm_fb_helper *fb_helper)
> -{
> -	struct drm_device *dev = fb_helper->dev;
> -	struct drm_crtc *crtc;
> -	int bound = 0, crtcs_bound = 0;
> -
> -	/*
> -	 * Sometimes user space wants everything disabled, so don't steal the
> -	 * display if there's a master.
> -	 */
> -	if (READ_ONCE(dev->master))
> -		return false;
> -
> -	drm_for_each_crtc(crtc, dev) {
> -		drm_modeset_lock(&crtc->mutex, NULL);
> -		if (crtc->primary->fb)
> -			crtcs_bound++;
> -		if (crtc->primary->fb == fb_helper->fb)
> -			bound++;
> -		drm_modeset_unlock(&crtc->mutex);
> -	}
> -
> -	if (bound < crtcs_bound)
> -		return false;
> -
> -	return true;
> -}
> -
>  #ifdef CONFIG_MAGIC_SYSRQ
>  /*
>   * restore fbcon display for all kms driver's using this helper, used for sysrq
> @@ -604,7 +592,7 @@ static bool drm_fb_helper_force_kernel_mode(void)
>  			continue;
>  
>  		mutex_lock(&helper->lock);
> -		ret = restore_fbdev_mode(helper);
> +		ret = restore_fbdev_mode_force(helper);
>  		if (ret)
>  			error = true;
>  		mutex_unlock(&helper->lock);
> @@ -663,20 +651,22 @@ static void dpms_legacy(struct drm_fb_helper *fb_helper, int dpms_mode)
>  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)) {
> -		mutex_unlock(&fb_helper->lock);
> -		return;
> -	}
> +	if (!drm_master_internal_acquire(dev))
> +		goto unlock;
>  
> -	if (drm_drv_uses_atomic_modeset(fb_helper->dev))
> +	if (drm_drv_uses_atomic_modeset(dev))
>  		restore_fbdev_mode_atomic(fb_helper, dpms_mode == DRM_MODE_DPMS_ON);
>  	else
>  		dpms_legacy(fb_helper, dpms_mode);
> +
> +	drm_master_internal_release(dev);
> +unlock:
>  	mutex_unlock(&fb_helper->lock);
>  }
>  
> @@ -1509,6 +1499,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)
> @@ -1516,9 +1507,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)
> @@ -1528,7 +1519,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;
> @@ -1548,12 +1540,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_mode_set *mode_set;
>  	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;
>  	}
> @@ -1591,11 +1584,12 @@ 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;
> @@ -1847,15 +1841,18 @@ 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;
> @@ -2014,7 +2011,7 @@ 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))
> +		if (!fb_helper->deferred_setup)
>  			restore_fbdev_mode(fb_helper);
>  		return -EAGAIN;
>  	}
> @@ -3028,6 +3025,7 @@ EXPORT_SYMBOL(drm_fb_helper_initial_config);
>   */
>  int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper)
>  {
> +	struct drm_device *dev = fb_helper->dev;
>  	int err = 0;
>  
>  	if (!drm_fbdev_emulation || !fb_helper)
> @@ -3040,12 +3038,14 @@ int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper)
>  		return err;
>  	}
>  
> -	if (!fb_helper->fb || !drm_fb_helper_is_bound(fb_helper)) {
> +	if (!fb_helper->fb || !drm_master_internal_acquire(dev)) {
>  		fb_helper->delayed_hotplug = true;
>  		mutex_unlock(&fb_helper->lock);
>  		return err;
>  	}
>  
> +	drm_master_internal_release(dev);
> +
>  	DRM_DEBUG_KMS("\n");
>  
>  	drm_setup_crtcs(fb_helper, fb_helper->fb->width, fb_helper->fb->height);
> diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
> index d9a483a5fce0..3ee97c9998a2 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;
> 


More information about the Intel-gfx mailing list