Re: [PATCH 1/3] drm: Hold mode_config.lock to prevent hotplug whilst setting up crtcs

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux