drm/fb-helper: Avoid race with DRM userspace

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.

Two igt tests fail with the new 'bail out if master' rule. Work around
this by relaxing this rule for drm_fb_helper_restore_fbdev_mode_unlocked()
until the tests have been fixed. Add todo entry for this.

Locking rule: Always take the fb-helper lock first.

v5: drm_fb_helper_restore_fbdev_mode_unlocked(): Use
    restore_fbdev_mode_force()

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@ffwll.ch>
Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Link: https://patchwork.freedesktop.org/patch/msgid/20190506180139.6913-3-noralf@tronnes.org

diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
index 06ccff9165d5dbf5563f270b3476f9bf71b35da7..66f05f4e469f322dd91d4e4bdf3a254d95f15559 100644 (file)
--- a/Documentation/gpu/todo.rst
+++ b/Documentation/gpu/todo.rst
@@ -281,6 +281,14 @@ it to use drm_mode_hsync() instead.
 
 Contact: Sean Paul
 
+drm_fb_helper tasks
+-------------------
+
+- drm_fb_helper_restore_fbdev_mode_unlocked() should call restore_fbdev_mode()
+  not the _force variant so it can bail out if there is a master. But first
+  these igt tests need to be fixed: kms_fbcon_fbt@psr and
+  kms_fbcon_fbt@psr-suspend.
+
 Core refactorings
 =================
 

diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c
index 22c7a104b802eb3bad5619ab1f47d4a6d2457564..bf98402f3210da1189703270ca3bbd2944d5a77c 100644 (file)
--- a/drivers/gpu/drm/drm_auth.c
+++ b/drivers/gpu/drm/drm_auth.c
@@ -351,3 +351,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 498f95c3e81d7c95b717075ba9656f2e126ec24f..c521e7490380c09e8a3c435a51c756fd4e020b5e 100644 (file)
--- 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 @@ out:
        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
@@ -542,7 +558,17 @@ 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);
+       /*
+        * TODO:
+        * We should bail out here if there is a master by dropping _force.
+        * Currently these igt tests fail if we do that:
+        * - kms_fbcon_fbt@psr
+        * - kms_fbcon_fbt@psr-suspend
+        *
+        * So first these tests need to be fixed so they drop master or don't
+        * have an fd open.
+        */
+       ret = restore_fbdev_mode_force(fb_helper);
 
        do_delayed = fb_helper->delayed_hotplug;
        if (do_delayed)
@@ -556,34 +582,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 +602,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 +661,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 +1509,7 @@ backoff:
 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 +1517,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 +1529,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 +1550,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 +1594,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 +1851,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 +2021,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;
        }
@@ -3029,6 +3036,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)
@@ -3041,12 +3049,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 e19ac7ca602d45be9f5c995821fa968229b59f2c..e6281d9f9c875f9f19d2b9904dab07434a66c092 100644 (file)
--- a/drivers/gpu/drm/drm_internal.h
+++ b/drivers/gpu/drm/drm_internal.h
@@ -93,6 +93,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;