On Wed, Nov 23, 2016 at 02:04:15PM +0000, Chris Wilson wrote: > 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] [<ffffffff814b72aa>] dump_stack+0x67/0x9d > [ 1250.873106] [<ffffffff8124ff3c>] kasan_object_err+0x1c/0x70 > [ 1250.873113] [<ffffffff812501e4>] kasan_report_error+0x204/0x4f0 > [ 1250.873120] [<ffffffff81698df0>] ? drm_dev_printk+0x140/0x140 > [ 1250.873127] [<ffffffff81250ac3>] kasan_report+0x53/0x60 > [ 1250.873134] [<ffffffff81688b40>] ? drm_setup_crtcs+0x320/0xf80 > [ 1250.873142] [<ffffffff8124f18e>] check_memory_region+0x13e/0x1a0 > [ 1250.873147] [<ffffffff8124f5f3>] memset+0x23/0x40 > [ 1250.873154] [<ffffffff81688b40>] drm_setup_crtcs+0x320/0xf80 > [ 1250.873161] [<ffffffff810be7c5>] ? wake_up_q+0x45/0x80 > [ 1250.873169] [<ffffffff81b0c180>] ? mutex_lock_nested+0x5a0/0x5a0 > [ 1250.873176] [<ffffffff8168a0e6>] drm_fb_helper_initial_config+0x206/0x7a0 > [ 1250.873183] [<ffffffff81689ee0>] ? drm_fb_helper_set_par+0x90/0x90 > [ 1250.873303] [<ffffffffa0b68690>] ? intel_fbdev_fini+0x140/0x140 [i915] > [ 1250.873387] [<ffffffffa0b686b2>] intel_fbdev_initial_config+0x22/0x40 [i915] > [ 1250.873391] [<ffffffff810b50ff>] async_run_entry_fn+0x7f/0x270 > [ 1250.873394] [<ffffffff810a64b0>] process_one_work+0x3d0/0x960 > [ 1250.873398] [<ffffffff810a641d>] ? process_one_work+0x33d/0x960 > [ 1250.873401] [<ffffffff810a60e0>] ? max_active_store+0xf0/0xf0 > [ 1250.873406] [<ffffffff810f6f9d>] ? do_raw_spin_lock+0x10d/0x1a0 > [ 1250.873413] [<ffffffff810a767d>] worker_thread+0x8d/0x840 > [ 1250.873419] [<ffffffff810a75f0>] ? create_worker+0x2e0/0x2e0 > [ 1250.873426] [<ffffffff810b0454>] kthread+0x194/0x1c0 > [ 1250.873432] [<ffffffff810b02c0>] ? kthread_park+0x60/0x60 > [ 1250.873438] [<ffffffff810f095d>] ? trace_hardirqs_on+0xd/0x10 > [ 1250.873446] [<ffffffff810b02c0>] ? kthread_park+0x60/0x60 > [ 1250.873453] [<ffffffff810b02c0>] ? kthread_park+0x60/0x60 > [ 1250.873457] [<ffffffff81b12277>] ret_from_fork+0x27/0x40 > [ 1250.873460] Object at ffff88074cdd2608, in cache kmalloc-32 size: 32 > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=98826 > Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> > --- > 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 14547817566d..d13c85682891 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); 1;}), \ > + i__ = 0; i__ < (fbh)->connector_count; i__++) > + +1 on Jani's question. > /** > * 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); > - Why move this hunk? kernel_fb_helper_list seems entirely unprotected, so shouldn't matter really where we touch it ;-) Otherwise lgtm. -Daniel > 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; > @@ -1964,7 +1950,7 @@ static bool drm_target_preferred(struct drm_fb_helper *fb_helper, > int tile_pass = 0; > mask = (1 << fb_helper->connector_count) - 1; > 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 & (1 << i)) > @@ -2127,6 +2113,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, > @@ -2140,7 +2129,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 && > @@ -2169,7 +2157,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]; > @@ -2246,7 +2234,9 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper) > 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; > @@ -2255,7 +2245,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. > */ > @@ -2263,8 +2252,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.10.2 > > _______________________________________________ > dri-devel mailing list > dri-devel@xxxxxxxxxxxxxxxxxxxxx > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
