diff mbox

drm: allocate crtc mutex separately from crtc

Message ID 1381966519-4185-1-git-send-email-ihadzic@research.bell-labs.com (mailing list archive)
State New, archived
Headers show

Commit Message

Ilija Hadzic Oct. 16, 2013, 11:35 p.m. UTC 
Embedding a mutex inside drm_crtc structure evokes a
subtle and rare corruption in drm_crtc_helper_set_config
failure-recovery path.

Namely, if drm_crtc_helper_set_config takes the
path under 'fail:' label *and* some other process
has attempted to grab the crtc mutex (and got blocked),
restoring the CRTC structure (which is done by bit-copying
the entire structure from saved_crtcs array) will overwrite
the mutex state and the waiters list pointer within the mutex
structure. Consequently the blocked process will never
be scheduled.

This patch fixes the issue by un-embeding the mutex structure
and allocating it separately from the drm_crtc structure
when the CRTC is initialized. The bit-copying in
drm_crtc_helper_set_config helper will now overwrite the pointer
which is never modified during the CRTC's lifetime, thus
avoiding corruption.

Signed-off-by: Ilija Hadzic <ihadzic@research.bell-labs.com>
Cc: stable@vger.kernel.org
---
 drivers/gpu/drm/drm_crtc.c           | 22 +++++++++++++---------
 drivers/gpu/drm/i915/intel_display.c | 16 ++++++++--------
 drivers/gpu/drm/omapdrm/omap_crtc.c  | 10 +++++-----
 drivers/gpu/drm/vmwgfx/vmwgfx_kms.c  |  8 ++++----
 include/drm/drm_crtc.h               |  2 +-
 5 files changed, 31 insertions(+), 27 deletions(-)

Comments

Daniel Vetter Oct. 17, 2013, 7:16 a.m. UTC | #1 
On Thu, Oct 17, 2013 at 1:35 AM, Ilija Hadzic <ilijahadzic@gmail.com> wrote:
> Embedding a mutex inside drm_crtc structure evokes a
> subtle and rare corruption in drm_crtc_helper_set_config
> failure-recovery path.
>
> Namely, if drm_crtc_helper_set_config takes the
> path under 'fail:' label *and* some other process
> has attempted to grab the crtc mutex (and got blocked),
> restoring the CRTC structure (which is done by bit-copying
> the entire structure from saved_crtcs array) will overwrite
> the mutex state and the waiters list pointer within the mutex
> structure. Consequently the blocked process will never
> be scheduled.
>
> This patch fixes the issue by un-embeding the mutex structure
> and allocating it separately from the drm_crtc structure
> when the CRTC is initialized. The bit-copying in
> drm_crtc_helper_set_config helper will now overwrite the pointer
> which is never modified during the CRTC's lifetime, thus
> avoiding corruption.
>
> Signed-off-by: Ilija Hadzic <ihadzic@research.bell-labs.com>
> Cc: stable@vger.kernel.org

Can't we instead just copy the few things we need to restore back?
This wholesale structure copying has rather tricky semantics and is
bound to trick up someone else in the future. And indeed we seem to
have a similar (but less catastrophic) thing going on with crtc->fb I
think.

I've looked a bit through the code and I think we don't actually need
to restore anything for crtcs. We pass the mode, fb and offsets in
explicitly, and everything else in drm_crtc is derived state. This is
also the same that i915 does, we only restore the connector/encoder
links even.
-Daniel
Ilija Hadzic Oct. 17, 2013, 12:58 p.m. UTC | #2 
> Can't we instead just copy the few things we need to restore back?
> This wholesale structure copying has rather tricky semantics and is
> bound to trick up someone else in the future. And indeed we seem to
> have a similar (but less catastrophic) thing going on with crtc->fb I
> think.

Sure, it would be possible. If that's a preferred solution, I'll cut
the alternative patch.

I.
Ilija Hadzic Oct. 18, 2013, 1:27 a.m. UTC | #3 
(dropping stable@... until we get the fix we can agree on)

While looking through that function (drm_crtc_helper_set_config) to
figure out what we really need to save and restore, I came across a
piece of code that you added in 25f397a4. The 'if (connector->dpms !=
DRM_MODE_DPMS_ON)'  (line 719 in the version that is on the top of
Dave's drm-next branch), comes right after the unconditional break, is
unreachable code (and removing it produces the same object code). Can
you explain what your intent was here? Also, the comment above the
break that reads "don't break so fail path works correct[sic]" doesn't
make much sense to me either.

-- Ilija
Daniel Vetter Oct. 19, 2013, 10:41 a.m. UTC | #4 
On Thu, Oct 17, 2013 at 09:27:41PM -0400, Ilija Hadzic wrote:
> (dropping stable@... until we get the fix we can agree on)
> 
> While looking through that function (drm_crtc_helper_set_config) to
> figure out what we really need to save and restore, I came across a
> piece of code that you added in 25f397a4. The 'if (connector->dpms !=
> DRM_MODE_DPMS_ON)'  (line 719 in the version that is on the top of
> Dave's drm-next branch), comes right after the unconditional break, is
> unreachable code (and removing it produces the same object code). Can
> you explain what your intent was here? Also, the comment above the
> break that reads "don't break so fail path works correct[sic]" doesn't
> make much sense to me either.

The idea was to also remove the break there. I just haven't had the time
yet to fix this fumble and test it a bit.

For the funny commment I think this is just to avoid the code in the outer
for loop wrestling with the connector->encoder pointer. Removing the
comment and inline the ret = -EINVAL; goto fail; would be clearer code
imo.
-Daniel
diff mbox

Patch

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index d7a8370..16b4001 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -52,7 +52,7 @@  void drm_modeset_lock_all(struct drm_device *dev)
 	mutex_lock(&dev->mode_config.mutex);
 
 	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head)
-		mutex_lock_nest_lock(&crtc->mutex, &dev->mode_config.mutex);
+		mutex_lock_nest_lock(crtc->mutex, &dev->mode_config.mutex);
 }
 EXPORT_SYMBOL(drm_modeset_lock_all);
 
@@ -65,7 +65,7 @@  void drm_modeset_unlock_all(struct drm_device *dev)
 	struct drm_crtc *crtc;
 
 	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head)
-		mutex_unlock(&crtc->mutex);
+		mutex_unlock(crtc->mutex);
 
 	mutex_unlock(&dev->mode_config.mutex);
 }
@@ -84,7 +84,7 @@  void drm_warn_on_modeset_not_all_locked(struct drm_device *dev)
 		return;
 
 	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head)
-		WARN_ON(!mutex_is_locked(&crtc->mutex));
+		WARN_ON(!mutex_is_locked(crtc->mutex));
 
 	WARN_ON(!mutex_is_locked(&dev->mode_config.mutex));
 }
@@ -634,8 +634,11 @@  int drm_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
 	crtc->invert_dimensions = false;
 
 	drm_modeset_lock_all(dev);
-	mutex_init(&crtc->mutex);
-	mutex_lock_nest_lock(&crtc->mutex, &dev->mode_config.mutex);
+	crtc->mutex = kmalloc(sizeof(struct mutex), GFP_KERNEL);
+	if (!crtc->mutex)
+		return -ENOMEM;
+	mutex_init(crtc->mutex);
+	mutex_lock_nest_lock(crtc->mutex, &dev->mode_config.mutex);
 
 	ret = drm_mode_object_get(dev, &crtc->base, DRM_MODE_OBJECT_CRTC);
 	if (ret)
@@ -670,6 +673,7 @@  void drm_crtc_cleanup(struct drm_crtc *crtc)
 
 	drm_mode_object_put(dev, &crtc->base);
 	list_del(&crtc->head);
+	kfree(crtc->mutex);
 	dev->mode_config.num_crtc--;
 }
 EXPORT_SYMBOL(drm_crtc_cleanup);
@@ -2284,7 +2288,7 @@  static int drm_mode_cursor_common(struct drm_device *dev,
 	}
 	crtc = obj_to_crtc(obj);
 
-	mutex_lock(&crtc->mutex);
+	mutex_lock(crtc->mutex);
 	if (req->flags & DRM_MODE_CURSOR_BO) {
 		if (!crtc->funcs->cursor_set && !crtc->funcs->cursor_set2) {
 			ret = -ENXIO;
@@ -2308,7 +2312,7 @@  static int drm_mode_cursor_common(struct drm_device *dev,
 		}
 	}
 out:
-	mutex_unlock(&crtc->mutex);
+	mutex_unlock(crtc->mutex);
 
 	return ret;
 
@@ -3618,7 +3622,7 @@  int drm_mode_page_flip_ioctl(struct drm_device *dev,
 		return -EINVAL;
 	crtc = obj_to_crtc(obj);
 
-	mutex_lock(&crtc->mutex);
+	mutex_lock(crtc->mutex);
 	if (crtc->fb == NULL) {
 		/* The framebuffer is currently unbound, presumably
 		 * due to a hotplug event, that userspace has not
@@ -3700,7 +3704,7 @@  out:
 		drm_framebuffer_unreference(fb);
 	if (old_fb)
 		drm_framebuffer_unreference(old_fb);
-	mutex_unlock(&crtc->mutex);
+	mutex_unlock(crtc->mutex);
 
 	return ret;
 }
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 617b963..330c0ae 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2229,11 +2229,11 @@  void intel_display_handle_reset(struct drm_device *dev)
 	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
 		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 
-		mutex_lock(&crtc->mutex);
+		mutex_lock(crtc->mutex);
 		if (intel_crtc->active)
 			dev_priv->display.update_plane(crtc, crtc->fb,
 						       crtc->x, crtc->y);
-		mutex_unlock(&crtc->mutex);
+		mutex_unlock(crtc->mutex);
 	}
 }
 
@@ -7375,7 +7375,7 @@  bool intel_get_load_detect_pipe(struct drm_connector *connector,
 	if (encoder->crtc) {
 		crtc = encoder->crtc;
 
-		mutex_lock(&crtc->mutex);
+		mutex_lock(crtc->mutex);
 
 		old->dpms_mode = connector->dpms;
 		old->load_detect_temp = false;
@@ -7406,7 +7406,7 @@  bool intel_get_load_detect_pipe(struct drm_connector *connector,
 		return false;
 	}
 
-	mutex_lock(&crtc->mutex);
+	mutex_lock(crtc->mutex);
 	intel_encoder->new_crtc = to_intel_crtc(crtc);
 	to_intel_connector(connector)->new_encoder = intel_encoder;
 
@@ -7434,7 +7434,7 @@  bool intel_get_load_detect_pipe(struct drm_connector *connector,
 		DRM_DEBUG_KMS("reusing fbdev for load-detection framebuffer\n");
 	if (IS_ERR(fb)) {
 		DRM_DEBUG_KMS("failed to allocate framebuffer for load-detection\n");
-		mutex_unlock(&crtc->mutex);
+		mutex_unlock(crtc->mutex);
 		return false;
 	}
 
@@ -7442,7 +7442,7 @@  bool intel_get_load_detect_pipe(struct drm_connector *connector,
 		DRM_DEBUG_KMS("failed to set mode on load-detect pipe\n");
 		if (old->release_fb)
 			old->release_fb->funcs->destroy(old->release_fb);
-		mutex_unlock(&crtc->mutex);
+		mutex_unlock(crtc->mutex);
 		return false;
 	}
 
@@ -7473,7 +7473,7 @@  void intel_release_load_detect_pipe(struct drm_connector *connector,
 			drm_framebuffer_unreference(old->release_fb);
 		}
 
-		mutex_unlock(&crtc->mutex);
+		mutex_unlock(crtc->mutex);
 		return;
 	}
 
@@ -7481,7 +7481,7 @@  void intel_release_load_detect_pipe(struct drm_connector *connector,
 	if (old->dpms_mode != DRM_MODE_DPMS_ON)
 		connector->funcs->dpms(connector, old->dpms_mode);
 
-	mutex_unlock(&crtc->mutex);
+	mutex_unlock(crtc->mutex);
 }
 
 static int i9xx_pll_refclk(struct drm_device *dev,
diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c
index 0fd2eb1..7b402a3 100644
--- a/drivers/gpu/drm/omapdrm/omap_crtc.c
+++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
@@ -307,13 +307,13 @@  static void page_flip_worker(struct work_struct *work)
 	struct drm_display_mode *mode = &crtc->mode;
 	struct drm_gem_object *bo;
 
-	mutex_lock(&crtc->mutex);
+	mutex_lock(crtc->mutex);
 	omap_plane_mode_set(omap_crtc->plane, crtc, crtc->fb,
 			0, 0, mode->hdisplay, mode->vdisplay,
 			crtc->x << 16, crtc->y << 16,
 			mode->hdisplay << 16, mode->vdisplay << 16,
 			vblank_cb, crtc);
-	mutex_unlock(&crtc->mutex);
+	mutex_unlock(crtc->mutex);
 
 	bo = omap_framebuffer_bo(crtc->fb, 0);
 	drm_gem_object_unreference_unlocked(bo);
@@ -446,7 +446,7 @@  static void apply_worker(struct work_struct *work)
 	 * the callbacks and list modification all serialized
 	 * with respect to modesetting ioctls from userspace.
 	 */
-	mutex_lock(&crtc->mutex);
+	mutex_lock(crtc->mutex);
 	dispc_runtime_get();
 
 	/*
@@ -491,7 +491,7 @@  static void apply_worker(struct work_struct *work)
 
 out:
 	dispc_runtime_put();
-	mutex_unlock(&crtc->mutex);
+	mutex_unlock(crtc->mutex);
 }
 
 int omap_crtc_apply(struct drm_crtc *crtc,
@@ -499,7 +499,7 @@  int omap_crtc_apply(struct drm_crtc *crtc,
 {
 	struct omap_crtc *omap_crtc = to_omap_crtc(crtc);
 
-	WARN_ON(!mutex_is_locked(&crtc->mutex));
+	WARN_ON(!mutex_is_locked(crtc->mutex));
 
 	/* no need to queue it again if it is already queued: */
 	if (apply->queued)
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
index fc43c06..7874313 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
@@ -186,7 +186,7 @@  int vmw_du_crtc_cursor_set(struct drm_crtc *crtc, struct drm_file *file_priv,
 	 * can do this since the caller in the drm core doesn't check anything
 	 * which is protected by any looks.
 	 */
-	mutex_unlock(&crtc->mutex);
+	mutex_unlock(crtc->mutex);
 	drm_modeset_lock_all(dev_priv->dev);
 
 	/* A lot of the code assumes this */
@@ -251,7 +251,7 @@  int vmw_du_crtc_cursor_set(struct drm_crtc *crtc, struct drm_file *file_priv,
 	ret = 0;
 out:
 	drm_modeset_unlock_all(dev_priv->dev);
-	mutex_lock(&crtc->mutex);
+	mutex_lock(crtc->mutex);
 
 	return ret;
 }
@@ -272,7 +272,7 @@  int vmw_du_crtc_cursor_move(struct drm_crtc *crtc, int x, int y)
 	 * can do this since the caller in the drm core doesn't check anything
 	 * which is protected by any looks.
 	 */
-	mutex_unlock(&crtc->mutex);
+	mutex_unlock(crtc->mutex);
 	drm_modeset_lock_all(dev_priv->dev);
 
 	vmw_cursor_update_position(dev_priv, shown,
@@ -280,7 +280,7 @@  int vmw_du_crtc_cursor_move(struct drm_crtc *crtc, int x, int y)
 				   du->cursor_y + du->hotspot_y);
 
 	drm_modeset_unlock_all(dev_priv->dev);
-	mutex_lock(&crtc->mutex);
+	mutex_lock(crtc->mutex);
 
 	return 0;
 }
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index ba407f6..c8543f2 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -416,7 +416,7 @@  struct drm_crtc {
 	 * state, ...) and a write lock for everything which can be update
 	 * without a full modeset (fb, cursor data, ...)
 	 */
-	struct mutex mutex;
+	struct mutex *mutex;
 
 	struct drm_mode_object base;