From: Daniel Vetter <daniel@ffwll.ch>
To: Chris Wilson <chris@chris-wilson.co.uk>
Cc: intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 1/3] drm: Hold mode_config.lock to prevent hotplug whilst setting up crtcs
Date: Mon, 28 Nov 2016 08:26:07 +0100 [thread overview]
Message-ID: <20161128072607.66xcjy6kxzn7gwvi@phenom.ffwll.local> (raw)
In-Reply-To: <20161123140417.18538-1-chris@chris-wilson.co.uk>
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@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 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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
next prev parent reply other threads:[~2016-11-28 7:26 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-11-23 14:04 [PATCH 1/3] drm: Hold mode_config.lock to prevent hotplug whilst setting up crtcs Chris Wilson
2016-11-23 14:04 ` [PATCH 2/3] drm: Pull together probe + setup for drm_fb_helper Chris Wilson
2016-11-28 7:55 ` Daniel Vetter
2016-11-23 14:04 ` [PATCH 3/3] drm: Protect fb_helper list manipulation with a mutex Chris Wilson
2016-11-28 7:55 ` Daniel Vetter
2016-11-28 8:38 ` Chris Wilson
2016-11-28 14:16 ` Daniel Vetter
2016-11-23 14:22 ` [PATCH 1/3] drm: Hold mode_config.lock to prevent hotplug whilst setting up crtcs Jani Nikula
2016-11-23 15:47 ` ✓ Fi.CI.BAT: success for series starting with [1/3] " Patchwork
2016-11-28 7:26 ` Daniel Vetter [this message]
2016-11-28 14:59 ` [PATCH 1/3] " Chris Wilson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20161128072607.66xcjy6kxzn7gwvi@phenom.ffwll.local \
--to=daniel@ffwll.ch \
--cc=chris@chris-wilson.co.uk \
--cc=dri-devel@lists.freedesktop.org \
--cc=intel-gfx@lists.freedesktop.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.