[PATCH] drm: allocate crtc mutex separately from crtc
Ilija Hadzic
ilijahadzic at gmail.com
Thu Oct 17 01:35:19 CEST 2013
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 at research.bell-labs.com>
Cc: stable at 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(-)
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;
--
1.8.2.1
More information about the dri-devel
mailing list