From 966a6a13c6660b499caf2932de22ae70c1317786 Mon Sep 17 00:00:00 2001 From: Chris Wilson Date: Tue, 29 Nov 2016 12:02:15 +0000 Subject: [PATCH] drm: Hold mode_config.lock to prevent hotplug whilst setting up crtcs The fb_helper->connector_count is modified when a new connector is constructed following a hotplug event (e.g. DP-MST). This causes trouble for drm_setup_crtcs() and friends that assume that fb_helper is constant: [ 1250.872997] BUG: KASAN: slab-out-of-bounds in drm_setup_crtcs+0x320/0xf80 at addr ffff88074cdd2608 [ 1250.873020] Write of size 40 by task kworker/u8:3/480 [ 1250.873039] CPU: 2 PID: 480 Comm: kworker/u8:3 Tainted: G U 4.9.0-rc6+ #285 [ 1250.873043] Hardware name: /NUC6i3SYB, BIOS SYSKLi35.86A.0024.2015.1027.2142 10/27/2015 [ 1250.873050] Workqueue: events_unbound async_run_entry_fn [ 1250.873056] ffff88070f9d78f0 ffffffff814b72aa ffff88074e40c5c0 ffff88074cdd2608 [ 1250.873067] ffff88070f9d7918 ffffffff8124ff3c ffff88070f9d79b0 ffff88074cdd2600 [ 1250.873079] ffff88074e40c5c0 ffff88070f9d79a0 ffffffff812501e4 0000000000000005 [ 1250.873090] Call Trace: [ 1250.873099] [] dump_stack+0x67/0x9d [ 1250.873106] [] kasan_object_err+0x1c/0x70 [ 1250.873113] [] kasan_report_error+0x204/0x4f0 [ 1250.873120] [] ? drm_dev_printk+0x140/0x140 [ 1250.873127] [] kasan_report+0x53/0x60 [ 1250.873134] [] ? drm_setup_crtcs+0x320/0xf80 [ 1250.873142] [] check_memory_region+0x13e/0x1a0 [ 1250.873147] [] memset+0x23/0x40 [ 1250.873154] [] drm_setup_crtcs+0x320/0xf80 [ 1250.873161] [] ? wake_up_q+0x45/0x80 [ 1250.873169] [] ? mutex_lock_nested+0x5a0/0x5a0 [ 1250.873176] [] drm_fb_helper_initial_config+0x206/0x7a0 [ 1250.873183] [] ? drm_fb_helper_set_par+0x90/0x90 [ 1250.873303] [] ? intel_fbdev_fini+0x140/0x140 [i915] [ 1250.873387] [] intel_fbdev_initial_config+0x22/0x40 [i915] [ 1250.873391] [] async_run_entry_fn+0x7f/0x270 [ 1250.873394] [] process_one_work+0x3d0/0x960 [ 1250.873398] [] ? process_one_work+0x33d/0x960 [ 1250.873401] [] ? max_active_store+0xf0/0xf0 [ 1250.873406] [] ? do_raw_spin_lock+0x10d/0x1a0 [ 1250.873413] [] worker_thread+0x8d/0x840 [ 1250.873419] [] ? create_worker+0x2e0/0x2e0 [ 1250.873426] [] kthread+0x194/0x1c0 [ 1250.873432] [] ? kthread_park+0x60/0x60 [ 1250.873438] [] ? trace_hardirqs_on+0xd/0x10 [ 1250.873446] [] ? kthread_park+0x60/0x60 [ 1250.873453] [] ? kthread_park+0x60/0x60 [ 1250.873457] [] ret_from_fork+0x27/0x40 [ 1250.873460] Object at ffff88074cdd2608, in cache kmalloc-32 size: 32 However, when holding the mode_config.lock around the fb_helper, we have to be careful of any callbacks that may reenter the fb_helper and so try to reacquire the mode_config.lock (e.g. register_framebuffer). To avoid the mutex recursion, we have to rearrange the sequence to move the registration into the caller outside of the mode_config.lock. v2: drop the 1; following the lockdep assertion inside the for(;;), I anticipated an error that doesn't happen! Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=98826 Signed-off-by: Chris Wilson Cc: Daniel Vetter Signed-off-by: Sean Paul Link: http://patchwork.freedesktop.org/patch/msgid/20161129120217.7344-1-chris@chris-wilson.co.uk --- drivers/gpu/drm/drm_fb_helper.c | 73 ++++++++++++++++++--------------- 1 file changed, 40 insertions(+), 33 deletions(-) diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 1f26634f53d8..380a0ec01033 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -97,6 +97,10 @@ static LIST_HEAD(kernel_fb_helper_list); * mmap page writes. */ +#define drm_fb_helper_for_each_connector(fbh, i__) \ + for (({ lockdep_assert_held(&(fbh)->dev->mode_config.mutex); }), \ + i__ = 0; i__ < (fbh)->connector_count; i__++) + /** * drm_fb_helper_single_add_all_connectors() - add all connectors to fbdev * emulation helper @@ -130,7 +134,7 @@ int drm_fb_helper_single_add_all_connectors(struct drm_fb_helper *fb_helper) mutex_unlock(&dev->mode_config.mutex); return 0; fail: - for (i = 0; i < fb_helper->connector_count; i++) { + drm_fb_helper_for_each_connector(fb_helper, i) { struct drm_fb_helper_connector *fb_helper_connector = fb_helper->connector_info[i]; @@ -565,7 +569,7 @@ static void drm_fb_helper_dpms(struct fb_info *info, int dpms_mode) continue; /* Walk the connectors & encoders on this fb turning them on/off */ - for (j = 0; j < fb_helper->connector_count; j++) { + drm_fb_helper_for_each_connector(fb_helper, j) { connector = fb_helper->connector_info[j]->connector; connector->funcs->dpms(connector, dpms_mode); drm_object_property_set_value(&connector->base, @@ -1469,7 +1473,6 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper, int ret = 0; int crtc_count = 0; int i; - struct fb_info *info; struct drm_fb_helper_surface_size sizes; int gamma_size = 0; @@ -1485,7 +1488,7 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper, sizes.surface_depth = sizes.surface_bpp = preferred_bpp; /* first up get a count of crtcs now in use and new min/maxes width/heights */ - for (i = 0; i < fb_helper->connector_count; i++) { + drm_fb_helper_for_each_connector(fb_helper, i) { struct drm_fb_helper_connector *fb_helper_conn = fb_helper->connector_info[i]; struct drm_cmdline_mode *cmdline_mode; @@ -1572,8 +1575,6 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper, if (ret < 0) return ret; - info = fb_helper->fbdev; - /* * Set the fb pointer - usually drm_setup_crtcs does this for hotplug * events, but at init time drm_setup_crtcs needs to be called before @@ -1585,20 +1586,6 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper, if (fb_helper->crtc_info[i].mode_set.num_connectors) fb_helper->crtc_info[i].mode_set.fb = fb_helper->fb; - - info->var.pixclock = 0; - if (register_framebuffer(info) < 0) - return -EINVAL; - - dev_info(fb_helper->dev->dev, "fb%d: %s frame buffer device\n", - info->node, info->fix.id); - - if (list_empty(&kernel_fb_helper_list)) { - register_sysrq_key('v', &sysrq_drm_fb_helper_restore_op); - } - - list_add(&fb_helper->kernel_fb_list, &kernel_fb_helper_list); - return 0; } @@ -1730,7 +1717,7 @@ static int drm_fb_helper_probe_connector_modes(struct drm_fb_helper *fb_helper, int count = 0; int i; - for (i = 0; i < fb_helper->connector_count; i++) { + drm_fb_helper_for_each_connector(fb_helper, i) { connector = fb_helper->connector_info[i]->connector; count += connector->funcs->fill_modes(connector, maxX, maxY); } @@ -1830,7 +1817,7 @@ static void drm_enable_connectors(struct drm_fb_helper *fb_helper, struct drm_connector *connector; int i = 0; - for (i = 0; i < fb_helper->connector_count; i++) { + drm_fb_helper_for_each_connector(fb_helper, i) { connector = fb_helper->connector_info[i]->connector; enabled[i] = drm_connector_enabled(connector, true); DRM_DEBUG_KMS("connector %d enabled? %s\n", connector->base.id, @@ -1841,7 +1828,7 @@ static void drm_enable_connectors(struct drm_fb_helper *fb_helper, if (any_enabled) return; - for (i = 0; i < fb_helper->connector_count; i++) { + drm_fb_helper_for_each_connector(fb_helper, i) { connector = fb_helper->connector_info[i]->connector; enabled[i] = drm_connector_enabled(connector, false); } @@ -1862,7 +1849,7 @@ static bool drm_target_cloned(struct drm_fb_helper *fb_helper, return false; count = 0; - for (i = 0; i < fb_helper->connector_count; i++) { + drm_fb_helper_for_each_connector(fb_helper, i) { if (enabled[i]) count++; } @@ -1873,7 +1860,7 @@ static bool drm_target_cloned(struct drm_fb_helper *fb_helper, /* check the command line or if nothing common pick 1024x768 */ can_clone = true; - for (i = 0; i < fb_helper->connector_count; i++) { + drm_fb_helper_for_each_connector(fb_helper, i) { if (!enabled[i]) continue; fb_helper_conn = fb_helper->connector_info[i]; @@ -1899,8 +1886,7 @@ static bool drm_target_cloned(struct drm_fb_helper *fb_helper, can_clone = true; dmt_mode = drm_mode_find_dmt(fb_helper->dev, 1024, 768, 60, false); - for (i = 0; i < fb_helper->connector_count; i++) { - + drm_fb_helper_for_each_connector(fb_helper, i) { if (!enabled[i]) continue; @@ -1931,7 +1917,7 @@ static int drm_get_tile_offsets(struct drm_fb_helper *fb_helper, int i; int hoffset = 0, voffset = 0; - for (i = 0; i < fb_helper->connector_count; i++) { + drm_fb_helper_for_each_connector(fb_helper, i) { fb_helper_conn = fb_helper->connector_info[i]; if (!fb_helper_conn->connector->has_tile) continue; @@ -1965,7 +1951,7 @@ static bool drm_target_preferred(struct drm_fb_helper *fb_helper, int i; retry: - for (i = 0; i < fb_helper->connector_count; i++) { + drm_fb_helper_for_each_connector(fb_helper, i) { fb_helper_conn = fb_helper->connector_info[i]; if (conn_configured & BIT_ULL(i)) @@ -2128,6 +2114,9 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper) width = dev->mode_config.max_width; height = dev->mode_config.max_height; + /* prevent concurrent modification of connector_count by hotplug */ + lockdep_assert_held(&fb_helper->dev->mode_config.mutex); + crtcs = kcalloc(fb_helper->connector_count, sizeof(struct drm_fb_helper_crtc *), GFP_KERNEL); modes = kcalloc(fb_helper->connector_count, @@ -2141,7 +2130,6 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper) goto out; } - drm_enable_connectors(fb_helper, enabled); if (!(fb_helper->funcs->initial_config && @@ -2170,7 +2158,7 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper) drm_fb_helper_modeset_release(fb_helper, &fb_helper->crtc_info[i].mode_set); - for (i = 0; i < fb_helper->connector_count; i++) { + drm_fb_helper_for_each_connector(fb_helper, i) { struct drm_display_mode *mode = modes[i]; struct drm_fb_helper_crtc *fb_crtc = crtcs[i]; struct drm_fb_offset *offset = &offsets[i]; @@ -2247,7 +2235,9 @@ out: int drm_fb_helper_initial_config(struct drm_fb_helper *fb_helper, int bpp_sel) { struct drm_device *dev = fb_helper->dev; + struct fb_info *info; int count = 0; + int ret; if (!drm_fbdev_emulation) return 0; @@ -2256,7 +2246,6 @@ int drm_fb_helper_initial_config(struct drm_fb_helper *fb_helper, int bpp_sel) count = drm_fb_helper_probe_connector_modes(fb_helper, dev->mode_config.max_width, dev->mode_config.max_height); - mutex_unlock(&dev->mode_config.mutex); /* * we shouldn't end up with no modes here. */ @@ -2264,8 +2253,26 @@ int drm_fb_helper_initial_config(struct drm_fb_helper *fb_helper, int bpp_sel) dev_info(fb_helper->dev->dev, "No connectors reported connected with modes\n"); drm_setup_crtcs(fb_helper); + ret = drm_fb_helper_single_fb_probe(fb_helper, bpp_sel); + mutex_unlock(&dev->mode_config.mutex); + if (ret) + return ret; - return drm_fb_helper_single_fb_probe(fb_helper, bpp_sel); + info = fb_helper->fbdev; + info->var.pixclock = 0; + ret = register_framebuffer(info); + if (ret < 0) + return ret; + + dev_info(dev->dev, "fb%d: %s frame buffer device\n", + info->node, info->fix.id); + + if (list_empty(&kernel_fb_helper_list)) + register_sysrq_key('v', &sysrq_drm_fb_helper_restore_op); + + list_add(&fb_helper->kernel_fb_list, &kernel_fb_helper_list); + + return 0; } EXPORT_SYMBOL(drm_fb_helper_initial_config); -- 2.20.1