All of lore.kernel.org
 help / color / mirror / Atom feed
From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
To: intel-gfx@lists.freedesktop.org
Subject: [Intel-gfx] [PATCH 01/23] perf/core: Only copy-to-user after completely unlocking all locks, v3.
Date: Wed,  8 Apr 2020 13:10:09 +0200	[thread overview]
Message-ID: <20200408111031.2330026-1-maarten.lankhorst@linux.intel.com> (raw)

We inadvertently create a dependency on mmap_sem with a whole chain.

This breaks any user who wants to take a lock and call rcu_barrier(),
while also taking that lock inside mmap_sem:

<4> [604.892532] ======================================================
<4> [604.892534] WARNING: possible circular locking dependency detected
<4> [604.892536] 5.6.0-rc7-CI-Patchwork_17096+ #1 Tainted: G     U
<4> [604.892537] ------------------------------------------------------
<4> [604.892538] kms_frontbuffer/2595 is trying to acquire lock:
<4> [604.892540] ffffffff8264a558 (rcu_state.barrier_mutex){+.+.}, at: rcu_barrier+0x23/0x190
<4> [604.892547]
but task is already holding lock:
<4> [604.892547] ffff888484716050 (reservation_ww_class_mutex){+.+.}, at: i915_gem_object_pin_to_display_plane+0x89/0x270 [i915]
<4> [604.892592]
which lock already depends on the new lock.
<4> [604.892593]
the existing dependency chain (in reverse order) is:
<4> [604.892594]
-> #6 (reservation_ww_class_mutex){+.+.}:
<4> [604.892597]        __ww_mutex_lock.constprop.15+0xc3/0x1090
<4> [604.892598]        ww_mutex_lock+0x39/0x70
<4> [604.892600]        dma_resv_lockdep+0x10e/0x1f5
<4> [604.892602]        do_one_initcall+0x58/0x300
<4> [604.892604]        kernel_init_freeable+0x17b/0x1dc
<4> [604.892605]        kernel_init+0x5/0x100
<4> [604.892606]        ret_from_fork+0x24/0x50
<4> [604.892607]
-> #5 (reservation_ww_class_acquire){+.+.}:
<4> [604.892609]        dma_resv_lockdep+0xec/0x1f5
<4> [604.892610]        do_one_initcall+0x58/0x300
<4> [604.892610]        kernel_init_freeable+0x17b/0x1dc
<4> [604.892611]        kernel_init+0x5/0x100
<4> [604.892612]        ret_from_fork+0x24/0x50
<4> [604.892613]
-> #4 (&mm->mmap_sem#2){++++}:
<4> [604.892615]        __might_fault+0x63/0x90
<4> [604.892617]        _copy_to_user+0x1e/0x80
<4> [604.892619]        perf_read+0x200/0x2b0
<4> [604.892621]        vfs_read+0x96/0x160
<4> [604.892622]        ksys_read+0x9f/0xe0
<4> [604.892623]        do_syscall_64+0x4f/0x220
<4> [604.892624]        entry_SYSCALL_64_after_hwframe+0x49/0xbe
<4> [604.892625]
-> #3 (&cpuctx_mutex){+.+.}:
<4> [604.892626]        __mutex_lock+0x9a/0x9c0
<4> [604.892627]        perf_event_init_cpu+0xa4/0x140
<4> [604.892629]        perf_event_init+0x19d/0x1cd
<4> [604.892630]        start_kernel+0x362/0x4e4
<4> [604.892631]        secondary_startup_64+0xa4/0xb0
<4> [604.892631]
-> #2 (pmus_lock){+.+.}:
<4> [604.892633]        __mutex_lock+0x9a/0x9c0
<4> [604.892633]        perf_event_init_cpu+0x6b/0x140
<4> [604.892635]        cpuhp_invoke_callback+0x9b/0x9d0
<4> [604.892636]        _cpu_up+0xa2/0x140
<4> [604.892637]        do_cpu_up+0x61/0xa0
<4> [604.892639]        smp_init+0x57/0x96
<4> [604.892639]        kernel_init_freeable+0x87/0x1dc
<4> [604.892640]        kernel_init+0x5/0x100
<4> [604.892642]        ret_from_fork+0x24/0x50
<4> [604.892642]
-> #1 (cpu_hotplug_lock.rw_sem){++++}:
<4> [604.892643]        cpus_read_lock+0x34/0xd0
<4> [604.892644]        rcu_barrier+0xaa/0x190
<4> [604.892645]        kernel_init+0x21/0x100
<4> [604.892647]        ret_from_fork+0x24/0x50
<4> [604.892647]
-> #0 (rcu_state.barrier_mutex){+.+.}:
<4> [604.892649]        __lock_acquire+0x1328/0x15d0
<4> [604.892650]        lock_acquire+0xa7/0x1c0
<4> [604.892651]        __mutex_lock+0x9a/0x9c0
<4> [604.892652]        rcu_barrier+0x23/0x190
<4> [604.892680]        i915_gem_object_unbind+0x29d/0x3f0 [i915]
<4> [604.892707]        i915_gem_object_pin_to_display_plane+0x141/0x270 [i915]
<4> [604.892737]        intel_pin_and_fence_fb_obj+0xec/0x1f0 [i915]
<4> [604.892767]        intel_plane_pin_fb+0x3f/0xd0 [i915]
<4> [604.892797]        intel_prepare_plane_fb+0x13b/0x5c0 [i915]
<4> [604.892798]        drm_atomic_helper_prepare_planes+0x85/0x110
<4> [604.892827]        intel_atomic_commit+0xda/0x390 [i915]
<4> [604.892828]        drm_atomic_helper_set_config+0x57/0xa0
<4> [604.892830]        drm_mode_setcrtc+0x1c4/0x720
<4> [604.892830]        drm_ioctl_kernel+0xb0/0xf0
<4> [604.892831]        drm_ioctl+0x2e1/0x390
<4> [604.892833]        ksys_ioctl+0x7b/0x90
<4> [604.892835]        __x64_sys_ioctl+0x11/0x20
<4> [604.892835]        do_syscall_64+0x4f/0x220
<4> [604.892836]        entry_SYSCALL_64_after_hwframe+0x49/0xbe
<4> [604.892837]

Changes since v1:
- Use (*values)[n++] in perf_read_one().
Changes since v2:
- Centrally allocate values.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

fixup perf patch

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 kernel/events/core.c | 45 +++++++++++++++++++++-----------------------
 1 file changed, 21 insertions(+), 24 deletions(-)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index 085d9263d595..4921b64a0abd 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -4926,20 +4926,16 @@ static int __perf_read_group_add(struct perf_event *leader,
 }
 
 static int perf_read_group(struct perf_event *event,
-				   u64 read_format, char __user *buf)
+				   u64 read_format, char __user *buf,
+				   u64 *values)
 {
 	struct perf_event *leader = event->group_leader, *child;
 	struct perf_event_context *ctx = leader->ctx;
 	int ret;
-	u64 *values;
 
 	lockdep_assert_held(&ctx->mutex);
 
-	values = kzalloc(event->read_size, GFP_KERNEL);
-	if (!values)
-		return -ENOMEM;
-
-	values[0] = 1 + leader->nr_siblings;
+	*values = 1 + leader->nr_siblings;
 
 	/*
 	 * By locking the child_mutex of the leader we effectively
@@ -4957,25 +4953,17 @@ static int perf_read_group(struct perf_event *event,
 			goto unlock;
 	}
 
-	mutex_unlock(&leader->child_mutex);
-
 	ret = event->read_size;
-	if (copy_to_user(buf, values, event->read_size))
-		ret = -EFAULT;
-	goto out;
-
 unlock:
 	mutex_unlock(&leader->child_mutex);
-out:
-	kfree(values);
 	return ret;
 }
 
 static int perf_read_one(struct perf_event *event,
-				 u64 read_format, char __user *buf)
+				 u64 read_format, char __user *buf,
+				 u64 *values)
 {
 	u64 enabled, running;
-	u64 values[4];
 	int n = 0;
 
 	values[n++] = __perf_event_read_value(event, &enabled, &running);
@@ -4986,9 +4974,6 @@ static int perf_read_one(struct perf_event *event,
 	if (read_format & PERF_FORMAT_ID)
 		values[n++] = primary_event_id(event);
 
-	if (copy_to_user(buf, values, n * sizeof(u64)))
-		return -EFAULT;
-
 	return n * sizeof(u64);
 }
 
@@ -5009,7 +4994,8 @@ static bool is_event_hup(struct perf_event *event)
  * Read the performance event - simple non blocking version for now
  */
 static ssize_t
-__perf_read(struct perf_event *event, char __user *buf, size_t count)
+__perf_read(struct perf_event *event, char __user *buf,
+		    size_t count, u64 *values)
 {
 	u64 read_format = event->attr.read_format;
 	int ret;
@@ -5027,9 +5013,9 @@ __perf_read(struct perf_event *event, char __user *buf, size_t count)
 
 	WARN_ON_ONCE(event->ctx->parent_ctx);
 	if (read_format & PERF_FORMAT_GROUP)
-		ret = perf_read_group(event, read_format, buf);
+		ret = perf_read_group(event, read_format, buf, values);
 	else
-		ret = perf_read_one(event, read_format, buf);
+		ret = perf_read_one(event, read_format, buf, values);
 
 	return ret;
 }
@@ -5039,6 +5025,7 @@ perf_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
 {
 	struct perf_event *event = file->private_data;
 	struct perf_event_context *ctx;
+	u64 *values;
 	int ret;
 
 	ret = security_perf_event_read(event);
@@ -5046,9 +5033,19 @@ perf_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
 		return ret;
 
 	ctx = perf_event_ctx_lock(event);
-	ret = __perf_read(event, buf, count);
+	values = kzalloc(event->read_size, GFP_KERNEL);
+	if (values)
+		ret = __perf_read(event, buf, count, values);
+	else
+		ret = -ENOMEM;
 	perf_event_ctx_unlock(event, ctx);
 
+	if (ret > 0) {
+		if (copy_to_user(buf, values, ret))
+			ret = -EFAULT;
+	}
+	kfree(values);
+
 	return ret;
 }
 
-- 
2.25.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

             reply	other threads:[~2020-04-08 11:10 UTC|newest]

Thread overview: 26+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-04-08 11:10 Maarten Lankhorst [this message]
2020-04-08 11:10 ` [Intel-gfx] [PATCH 02/23] Revert "drm/i915/gem: Drop relocation slowpath" Maarten Lankhorst
2020-04-08 11:10 ` [Intel-gfx] [PATCH 03/23] drm/i915: Add an implementation for i915_gem_ww_ctx locking, v2 Maarten Lankhorst
2020-04-08 11:10 ` [Intel-gfx] [PATCH 04/23] drm/i915: Remove locking from i915_gem_object_prepare_read/write Maarten Lankhorst
2020-04-08 11:10 ` [Intel-gfx] [PATCH 05/23] drm/i915: Parse command buffer earlier in eb_relocate(slow) Maarten Lankhorst
2020-04-08 11:10 ` [Intel-gfx] [PATCH 06/23] Revert "drm/i915/gem: Split eb_vma into its own allocation" Maarten Lankhorst
2020-04-08 11:10 ` [Intel-gfx] [PATCH 07/23] drm/i915: Use per object locking in execbuf, v7 Maarten Lankhorst
2020-04-08 11:10 ` [Intel-gfx] [PATCH 08/23] drm/i915: Use ww locking in intel_renderstate Maarten Lankhorst
2020-04-08 11:10 ` [Intel-gfx] [PATCH 09/23] drm/i915: Add ww context handling to context_barrier_task Maarten Lankhorst
2020-04-08 11:10 ` [Intel-gfx] [PATCH 10/23] drm/i915: Nuke arguments to eb_pin_engine Maarten Lankhorst
2020-04-08 11:10 ` [Intel-gfx] [PATCH 11/23] drm/i915: Pin engine before pinning all objects, v3 Maarten Lankhorst
2020-04-08 11:10 ` [Intel-gfx] [PATCH 12/23] drm/i915: Rework intel_context pinning to do everything outside of pin_mutex Maarten Lankhorst
2020-04-08 11:10 ` [Intel-gfx] [PATCH 13/23] drm/i915: Make sure execbuffer always passes ww state to i915_vma_pin Maarten Lankhorst
2020-04-08 11:10 ` [Intel-gfx] [PATCH 14/23] drm/i915: Convert i915_gem_object/client_blt.c to use ww locking as well, v2 Maarten Lankhorst
2020-04-08 11:10 ` [Intel-gfx] [PATCH 15/23] drm/i915: Kill last user of intel_context_create_request outside of selftests Maarten Lankhorst
2020-04-08 11:10 ` [Intel-gfx] [PATCH 16/23] drm/i915: Convert i915_perf to ww locking as well Maarten Lankhorst
2020-04-08 11:10 ` [Intel-gfx] [PATCH 17/23] drm/i915: Dirty hack to fix selftests locking inversion Maarten Lankhorst
2020-04-08 11:10 ` [Intel-gfx] [PATCH 18/23] drm/i915/selftests: Fix locking inversion in lrc selftest Maarten Lankhorst
2020-04-08 11:10 ` [Intel-gfx] [PATCH 19/23] drm/i915: Use ww pinning for intel_context_create_request() Maarten Lankhorst
2020-04-08 11:10 ` [Intel-gfx] [PATCH 20/23] drm/i915: Move i915_vma_lock in the selftests to avoid lock inversion, v2 Maarten Lankhorst
2020-04-08 11:10 ` [Intel-gfx] [PATCH 21/23] drm/i915: Add ww locking to vm_fault_gtt Maarten Lankhorst
2020-04-08 11:10 ` [Intel-gfx] [PATCH 22/23] drm/i915: Add ww locking to pin_to_display_plane Maarten Lankhorst
2020-04-08 11:10 ` [Intel-gfx] [PATCH 23/23] drm/i915: Ensure we hold the pin mutex Maarten Lankhorst
2020-04-08 14:18 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for series starting with [01/23] perf/core: Only copy-to-user after completely unlocking all locks, v3 Patchwork
2020-04-08 14:44 ` [Intel-gfx] ✗ Fi.CI.BAT: failure " Patchwork
2020-04-10  3:26 ` [Intel-gfx] ✗ Fi.CI.IGT: " Patchwork

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=20200408111031.2330026-1-maarten.lankhorst@linux.intel.com \
    --to=maarten.lankhorst@linux.intel.com \
    --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.