LKML Archive on lore.kernel.org
 help / color / Atom feed
From: Sebastian Siewior <bigeasy@linutronix.de>
To: Thomas Gleixner <tglx@linutronix.de>,
	Peter Zijlstra <peterz@infradead.org>
Cc: LKML <linux-kernel@vger.kernel.org>,
	Ingo Molnar <mingo@kernel.org>,
	Steven Rostedt <rostedt@goodmis.org>
Subject: [RFC PATCH] trace/perf: cure locking issue in perf_event_open() error path
Date: Fri, 28 Apr 2017 16:24:56 +0200
Message-ID: <20170428142456.5xh44ef3fv7w2kkh@linutronix.de> (raw)
In-Reply-To: <20170418170442.665445272@linutronix.de>

As trinity figured out, there is a recursive get_online_cpus() in
perf_event_open()'s error path:
| Call Trace:
|  dump_stack+0x86/0xce
|  __lock_acquire+0x2520/0x2cd0
|  lock_acquire+0x27c/0x2f0
|  get_online_cpus+0x3d/0x80
|  static_key_slow_dec+0x5a/0x70
|  sw_perf_event_destroy+0x8e/0x100
|  _free_event+0x61b/0x800
|  free_event+0x68/0x70
|  SyS_perf_event_open+0x19db/0x1d80

In order to cure, I am moving free_event() after the put_online_cpus()
block.
Besides that one, there also the error path in perf_event_alloc() which
also invokes event->destory. Here I delayed the destory work to
schedule_work().

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---

I am not quite happy with the schedule_work() part.

 include/linux/perf_event.h |  1 +
 kernel/events/core.c       | 52 +++++++++++++++++++++++++++++++++-------------
 2 files changed, 38 insertions(+), 15 deletions(-)

diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index 24a635887f28..d6a874dbbd21 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -718,6 +718,7 @@ struct perf_event {
 #endif
 
 	struct list_head		sb_list;
+	struct work_struct		destroy_work;
 #endif /* CONFIG_PERF_EVENTS */
 };
 
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 7aed78b516fc..3358889609f8 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -9320,6 +9320,16 @@ static void account_event(struct perf_event *event)
 	account_pmu_sb_event(event);
 }
 
+static void perf_alloc_destroy_ev(struct work_struct *work)
+{
+	struct perf_event *event;
+
+	event = container_of(work, struct perf_event, destroy_work);
+	event->destroy(event);
+	module_put(event->pmu->module);
+	kfree(event);
+}
+
 /*
  * Allocate and initialize a event structure
  */
@@ -9334,6 +9344,7 @@ perf_event_alloc(struct perf_event_attr *attr, int cpu,
 	struct pmu *pmu;
 	struct perf_event *event;
 	struct hw_perf_event *hwc;
+	bool delay_destroy = false;
 	long err = -EINVAL;
 
 	if ((unsigned)cpu >= nr_cpu_ids) {
@@ -9497,15 +9508,22 @@ perf_event_alloc(struct perf_event_attr *attr, int cpu,
 	exclusive_event_destroy(event);
 
 err_pmu:
-	if (event->destroy)
-		event->destroy(event);
-	module_put(pmu->module);
+	if (event->destroy) {
+		/* delay ->destroy due to nested get_online_cpus() */
+		INIT_WORK(&event->destroy_work, perf_alloc_destroy_ev);
+		delay_destroy = true;
+	} else {
+		module_put(pmu->module);
+	}
 err_ns:
 	if (is_cgroup_event(event))
 		perf_detach_cgroup(event);
 	if (event->ns)
 		put_pid_ns(event->ns);
-	kfree(event);
+	if (delay_destroy)
+		schedule_work(&event->destroy_work);
+	else
+		kfree(event);
 
 	return ERR_PTR(err);
 }
@@ -9798,7 +9816,7 @@ SYSCALL_DEFINE5(perf_event_open,
 		pid_t, pid, int, cpu, int, group_fd, unsigned long, flags)
 {
 	struct perf_event *group_leader = NULL, *output_event = NULL;
-	struct perf_event *event, *sibling;
+	struct perf_event *event = NULL, *sibling;
 	struct perf_event_attr attr;
 	struct perf_event_context *ctx, *uninitialized_var(gctx);
 	struct file *event_file = NULL;
@@ -9908,13 +9926,14 @@ SYSCALL_DEFINE5(perf_event_open,
 				 NULL, NULL, cgroup_fd);
 	if (IS_ERR(event)) {
 		err = PTR_ERR(event);
+		event = NULL;
 		goto err_cred;
 	}
 
 	if (is_sampling_event(event)) {
 		if (event->pmu->capabilities & PERF_PMU_CAP_NO_INTERRUPT) {
 			err = -EOPNOTSUPP;
-			goto err_alloc;
+			goto err_cred;
 		}
 	}
 
@@ -9927,7 +9946,7 @@ SYSCALL_DEFINE5(perf_event_open,
 	if (attr.use_clockid) {
 		err = perf_event_set_clock(event, attr.clockid);
 		if (err)
-			goto err_alloc;
+			goto err_cred;
 	}
 
 	if (pmu->task_ctx_nr == perf_sw_context)
@@ -9962,7 +9981,7 @@ SYSCALL_DEFINE5(perf_event_open,
 	ctx = find_get_context(pmu, task, event);
 	if (IS_ERR(ctx)) {
 		err = PTR_ERR(ctx);
-		goto err_alloc;
+		goto err_cred;
 	}
 
 	if ((pmu->capabilities & PERF_PMU_CAP_EXCLUSIVE) && group_leader) {
@@ -10186,18 +10205,21 @@ SYSCALL_DEFINE5(perf_event_open,
 err_context:
 	perf_unpin_context(ctx);
 	put_ctx(ctx);
-err_alloc:
-	/*
-	 * If event_file is set, the fput() above will have called ->release()
-	 * and that will take care of freeing the event.
-	 */
-	if (!event_file)
-		free_event(event);
 err_cred:
 	if (task)
 		mutex_unlock(&task->signal->cred_guard_mutex);
 err_cpus:
 	put_online_cpus();
+	/*
+	 * The event cleanup should happen earlier (as per cleanup in reverse
+	 * allocation order). It is delayed after the put_online_cpus() section
+	 * so we don't invoke event->destroy in it and risk recursive invocation
+	 * of it via static_key_slow_dec().
+	 * If event_file is set, the fput() above will have called ->release()
+	 * and that will take care of freeing the event.
+	 */
+	if (event && !event_file)
+		free_event(event);
 err_task:
 	if (task)
 		put_task_struct(task);
-- 
2.11.0

  parent reply index

Thread overview: 72+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-04-18 17:04 [patch V2 00/24] cpu/hotplug: Convert get_online_cpus() to a percpu_rwsem Thomas Gleixner
2017-04-18 17:04 ` [patch V2 01/24] cpu/hotplug: Provide cpuhp_setup/remove_state[_nocalls]_cpuslocked() Thomas Gleixner
2017-04-20 11:18   ` [tip:smp/hotplug] " tip-bot for Sebastian Andrzej Siewior
2017-04-18 17:04 ` [patch V2 02/24] stop_machine: Provide stop_machine_cpuslocked() Thomas Gleixner
2017-04-20 11:19   ` [tip:smp/hotplug] " tip-bot for Sebastian Andrzej Siewior
2017-04-18 17:04 ` [patch V2 03/24] padata: Make padata_alloc() static Thomas Gleixner
2017-04-20 11:19   ` [tip:smp/hotplug] " tip-bot for Thomas Gleixner
2017-04-18 17:04 ` [patch V2 04/24] padata: Avoid nested calls to get_online_cpus() in pcrypt_init_padata() Thomas Gleixner
2017-04-20 11:20   ` [tip:smp/hotplug] " tip-bot for Sebastian Andrzej Siewior
2017-04-18 17:04 ` [patch V2 05/24] x86/mtrr: Remove get_online_cpus() from mtrr_save_state() Thomas Gleixner
2017-04-20 11:20   ` [tip:smp/hotplug] " tip-bot for Sebastian Andrzej Siewior
2017-04-18 17:04 ` [patch V2 06/24] cpufreq: Use cpuhp_setup_state_nocalls_cpuslocked() Thomas Gleixner
2017-04-20 11:21   ` [tip:smp/hotplug] " tip-bot for Sebastian Andrzej Siewior
2017-04-18 17:04 ` [patch V2 07/24] KVM/PPC/Book3S HV: " Thomas Gleixner
2017-04-20 11:21   ` [tip:smp/hotplug] " tip-bot for Sebastian Andrzej Siewior
2017-04-18 17:04 ` [patch V2 08/24] hwtracing/coresight-etm3x: " Thomas Gleixner
2017-04-20 11:22   ` [tip:smp/hotplug] " tip-bot for Sebastian Andrzej Siewior
2017-04-20 15:14   ` [patch V2 08/24] " Mathieu Poirier
2017-04-20 15:32   ` Mathieu Poirier
2017-04-18 17:04 ` [patch V2 09/24] hwtracing/coresight-etm4x: " Thomas Gleixner
2017-04-20 11:22   ` [tip:smp/hotplug] " tip-bot for Sebastian Andrzej Siewior
2017-04-18 17:04 ` [patch V2 10/24] perf/x86/intel/cqm: Use cpuhp_setup_state_cpuslocked() Thomas Gleixner
2017-04-20 11:23   ` [tip:smp/hotplug] " tip-bot for Sebastian Andrzej Siewior
2017-04-18 17:04 ` [patch V2 11/24] ARM/hw_breakpoint: " Thomas Gleixner
2017-04-19 17:54   ` Mark Rutland
2017-04-19 18:20     ` Thomas Gleixner
2017-04-20 11:23   ` [tip:smp/hotplug] " tip-bot for Sebastian Andrzej Siewior
2017-04-18 17:04 ` [patch V2 12/24] s390/kernel: Use stop_machine_cpuslocked() Thomas Gleixner
2017-04-20 11:24   ` [tip:smp/hotplug] " tip-bot for Sebastian Andrzej Siewior
2017-04-18 17:04 ` [patch V2 13/24] powerpc/powernv: " Thomas Gleixner
2017-04-20 11:24   ` [tip:smp/hotplug] " tip-bot for Sebastian Andrzej Siewior
2017-04-18 17:04 ` [patch V2 14/24] cpu/hotplug: Use stop_machine_cpuslocked() in takedown_cpu() Thomas Gleixner
2017-04-20 11:25   ` [tip:smp/hotplug] " tip-bot for Sebastian Andrzej Siewior
2017-04-18 17:04 ` [patch V2 15/24] x86/perf: Drop EXPORT of perf_check_microcode Thomas Gleixner
2017-04-20 11:25   ` [tip:smp/hotplug] " tip-bot for Thomas Gleixner
2017-04-18 17:04 ` [patch V2 16/24] perf/x86/intel: Drop get_online_cpus() in intel_snb_check_microcode() Thomas Gleixner
2017-04-20 11:26   ` [tip:smp/hotplug] " tip-bot for Sebastian Andrzej Siewior
2017-04-18 17:04 ` [patch V2 17/24] PCI: Use cpu_hotplug_disable() instead of get_online_cpus() Thomas Gleixner
2017-04-20 11:27   ` [tip:smp/hotplug] " tip-bot for Thomas Gleixner
2017-04-18 17:05 ` [patch V2 18/24] PCI: Replace the racy recursion prevention Thomas Gleixner
2017-04-20 11:27   ` [tip:smp/hotplug] " tip-bot for Thomas Gleixner
2017-04-18 17:05 ` [patch V2 19/24] ACPI/processor: Use cpu_hotplug_disable() instead of get_online_cpus() Thomas Gleixner
2017-04-20 11:28   ` [tip:smp/hotplug] " tip-bot for Thomas Gleixner
2017-04-18 17:05 ` [patch V2 20/24] perf/core: Remove redundant get_online_cpus() Thomas Gleixner
2017-04-20 11:28   ` [tip:smp/hotplug] " tip-bot for Thomas Gleixner
2017-04-18 17:05 ` [patch V2 21/24] jump_label: Pull get_online_cpus() into generic code Thomas Gleixner
2017-04-18 17:05 ` [patch V2 22/24] jump_label: Provide static_key_slow_inc_cpuslocked() Thomas Gleixner
2017-04-18 17:05 ` [patch V2 23/24] perf: Avoid cpu_hotplug_lock r-r recursion Thomas Gleixner
2017-04-18 17:05 ` [patch V2 24/24] cpu/hotplug: Convert hotplug locking to percpu rwsem Thomas Gleixner
2017-04-20 11:30   ` [tip:smp/hotplug] " tip-bot for Thomas Gleixner
2017-05-10  4:59   ` [patch V2 24/24] " Michael Ellerman
2017-05-10  8:49     ` Thomas Gleixner
2017-05-10 16:30       ` Steven Rostedt
2017-05-10 17:15         ` Steven Rostedt
2017-05-11  5:49       ` Michael Ellerman
2017-04-25 16:10 ` [patch V2 00/24] cpu/hotplug: Convert get_online_cpus() to a percpu_rwsem Mark Rutland
2017-04-25 17:28   ` Sebastian Siewior
2017-04-26  8:59     ` Mark Rutland
2017-04-26  9:40       ` Suzuki K Poulose
2017-04-26 10:32         ` Mark Rutland
2017-04-27  8:27           ` Sebastian Siewior
2017-04-27  9:57             ` Mark Rutland
2017-04-27 10:01               ` Thomas Gleixner
2017-04-27 12:30                 ` Mark Rutland
2017-04-27 15:48                   ` [PATCH] arm64: cpufeature: use static_branch_enable_cpuslocked() (was: Re: [patch V2 00/24] cpu/hotplug: Convert get_online_cpus() to a percpu_rwsem) Mark Rutland
2017-04-27 16:35                     ` Suzuki K Poulose
2017-04-27 17:03                       ` [PATCH] arm64: cpufeature: use static_branch_enable_cpuslocked() Suzuki K Poulose
2017-04-27 17:17                         ` Mark Rutland
2017-04-28 14:24 ` Sebastian Siewior [this message]
2017-04-28 14:27   ` [RFC PATCH] trace/perf: cure locking issue in perf_event_open() error path Sebastian Siewior
2017-05-01 12:57   ` [tip:smp/hotplug] perf: Reorder cpu hotplug rwsem against cred_guard_mutex tip-bot for Thomas Gleixner
2017-05-01 12:58   ` [tip:smp/hotplug] perf: Push hotplug protection down to callers tip-bot for Thomas Gleixner

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=20170428142456.5xh44ef3fv7w2kkh@linutronix.de \
    --to=bigeasy@linutronix.de \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mingo@kernel.org \
    --cc=peterz@infradead.org \
    --cc=rostedt@goodmis.org \
    --cc=tglx@linutronix.de \
    /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

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git
	git clone --mirror https://lore.kernel.org/lkml/8 lkml/git/8.git
	git clone --mirror https://lore.kernel.org/lkml/9 lkml/git/9.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org
	public-inbox-index lkml

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git