---
 tools/perf/Makefile.perf          |   1 +
 tools/perf/tests/builtin-test.c   |   4 +
 tools/perf/tests/group-read-bug.c | 169 ++++++++++++++++++++++++++++++++++++++
 tools/perf/tests/tests.h          |   1 +
 4 files changed, 175 insertions(+)
 create mode 100644 tools/perf/tests/group-read-bug.c

tools/perf/Makefile.perf +1 -0

index 9670a16..909084f 100644
@@ -419,6 +419,7 @@ endif
 endif
 LIB_OBJS += $(OUTPUT)tests/mmap-thread-lookup.o
 LIB_OBJS += $(OUTPUT)tests/thread-mg-share.o
+LIB_OBJS += $(OUTPUT)tests/group-read-bug.o
 
 BUILTIN_OBJS += $(OUTPUT)builtin-annotate.o
 BUILTIN_OBJS += $(OUTPUT)builtin-bench.o

tools/perf/tests/builtin-test.c +4 -0

index 6f8b01b..2b2d544 100644
@@ -154,6 +154,10 @@ static struct test {
 		.func = test__hists_cumulate,
 	},
 	{
+		.desc = "Test group read bug",
+		.func = test__group_read_bug,
+	},
+	{
 		.func = NULL,
 	},
 };

tools/perf/tests/group-read-bug.c new file +169 -0

169 lines not on this page · raw message

tools/perf/tests/tests.h +1 -0

index ed64790..af43c47 100644
@@ -48,6 +48,7 @@ int test__mmap_thread_lookup(void);
 int test__thread_mg_share(void);
 int test__hists_output(void);
 int test__hists_cumulate(void);
+int test__group_read_bug(void);
 
 #if defined(__x86_64__) || defined(__i386__) || defined(__arm__)
 #ifdef HAVE_DWARF_UNWIND_SUPPORT

Signature · 1 line

1.8.3.1

Jiri Olsa

author reviewer

2014-06-25 18:44 UTC #1 lore raw

PATCH 1/1 perf: Prevent race in PERF_SAMPLE_READ group format sample output

From: Jiri Olsa <[email protected]>

While iterating siblings in perf_output_read_group we could race with addition and removal of sibling in perf_group_attach and perf_group_detach respective.

While in perf_output_read_group we are under active context, so the only sibling_list modification could come via IPI in:
perf_install_in_context or perf_remove_from_context

Disable interrupts before iterating siblings to prevent this race.

Cc Arnaldo Carvalho de Melo <[email protected]>

Cc Corey Ashford <[email protected]>

Cc Frederic Weisbecker <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Paul Mackerras <[email protected]>

Cc Peter Zijlstra <[email protected]>

Signed-off-by Jiri Olsa <[email protected]>

---
 kernel/events/core.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

kernel/events/core.c +11 -0

index a33d9a2b..66649d3 100644
@@ -4509,6 +4509,7 @@ static void perf_output_read_group(struct perf_output_handle *handle,
 {
 	struct perf_event *leader = event->group_leader, *sub;
 	u64 read_format = event->attr.read_format;
+	unsigned long flags;
 	u64 values[5];
 	int n = 0;

@@ -4529,6 +4530,15 @@ static void perf_output_read_group(struct perf_output_handle *handle,
 
 	__output_copy(handle, values, n * sizeof(u64));
 
+	/*
+	 * We are now under active context, so the only sibling_list
+	 * modification could come via IPI in:
+	 *   perf_install_in_context and perf_remove_from_context
+	 *
+	 * Disable interrupts to prevent this race.
+	 */
+	local_irq_save(flags);
+
 	list_for_each_entry(sub, &leader->sibling_list, group_entry) {
 		n = 0;

@@ -4542,6 +4552,7 @@ static void perf_output_read_group(struct perf_output_handle *handle,
 
 		__output_copy(handle, values, n * sizeof(u64));
 	}
+	local_irq_restore(flags);
 }
 
 #define PERF_FORMAT_TOTAL_TIMES (PERF_FORMAT_TOTAL_TIME_ENABLED|\

Signature · 1 line

1.8.3.1

Jiri Olsa

2014-07-02 07:56 UTC #2 lore raw

Re: [PATCH 1/1] perf: Prevent race in PERF_SAMPLE_READ group format sample output

hi,
any feedback?

thanks,
jirka

Quoted 63 lines · Jiri Olsa

From: Jiri Olsa <[email protected]>
While iterating siblings in perf_output_read_group we could race with addition and removal of sibling in perf_group_attach and perf_group_detach respective.
While in perf_output_read_group we are under active context, so the only sibling_list modification could come via IPI in:
perf_install_in_context or perf_remove_from_context
Disable interrupts before iterating siblings to prevent this race.
Cc Arnaldo Carvalho de Melo <[email protected]>
Cc Corey Ashford <[email protected]>
Cc Frederic Weisbecker <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Paul Mackerras <[email protected]>
Cc Peter Zijlstra <[email protected]>
Signed-off-by Jiri Olsa <[email protected]>
---
 kernel/events/core.c | 11 +++++++++++
 1 file changed, 11 insertions(+)
kernel/events/core.c +11 -0
index a33d9a2b..66649d3 100644
@@ -4509,6 +4509,7 @@ static void perf_output_read_group(struct perf_output_handle *handle,
 {
 	struct perf_event *leader = event->group_leader, *sub;
 	u64 read_format = event->attr.read_format;
+	unsigned long flags;
 	u64 values[5];
 	int n = 0;
 
@@ -4529,6 +4530,15 @@ static void perf_output_read_group(struct perf_output_handle *handle,
 
 	__output_copy(handle, values, n * sizeof(u64));
 
+	/*
+	 * We are now under active context, so the only sibling_list
+	 * modification could come via IPI in:
+	 *   perf_install_in_context and perf_remove_from_context
+	 *
+	 * Disable interrupts to prevent this race.
+	 */
+	local_irq_save(flags);
+
 	list_for_each_entry(sub, &leader->sibling_list, group_entry) {
 		n = 0;
 
@@ -4542,6 +4552,7 @@ static void perf_output_read_group(struct perf_output_handle *handle,
 
 		__output_copy(handle, values, n * sizeof(u64));
 	}
+	local_irq_restore(flags);
 }
 
 #define PERF_FORMAT_TOTAL_TIMES (PERF_FORMAT_TOTAL_TIME_ENABLED|\
Signature · 1 line
1.8.3.1

Peter Zijlstra

maintainer

2014-07-07 09:04 UTC #3 lore raw

Re: [PATCH 1/1] perf: Prevent race in PERF_SAMPLE_READ group format sample output

In reply to Jiri Olsa

Quoted 5 lines · Jiri Olsa

From: Jiri Olsa <[email protected]>
While iterating siblings in perf_output_read_group we could race with addition and removal of sibling in perf_group_attach and perf_group_detach respective.

So why would anybody do this?

Quoted 32 lines

While in perf_output_read_group we are under active context, so the only sibling_list modification could come via IPI in:
perf_install_in_context or perf_remove_from_context
Disable interrupts before iterating siblings to prevent this race.
Cc Arnaldo Carvalho de Melo <[email protected]>
Cc Corey Ashford <[email protected]>
Cc Frederic Weisbecker <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Paul Mackerras <[email protected]>
Cc Peter Zijlstra <[email protected]>
Signed-off-by Jiri Olsa <[email protected]>
---
 kernel/events/core.c | 11 +++++++++++
 1 file changed, 11 insertions(+)
kernel/events/core.c +1 -0
index a33d9a2b..66649d3 100644
@@ -4509,6 +4509,7 @@ static void perf_output_read_group(struct perf_output_handle *handle,
 {
 	struct perf_event *leader = event->group_leader, *sub;
 	u64 read_format = event->attr.read_format;
+	unsigned long flags;
 	u64 values[5];
 	int n = 0;
 
@@ -4529,6 +4530,15 @@ static void perf_output_read_group(struct perf_output_handle *handle,
 

kernel/events/core.c static void perf_output_read_group(struct perf_output_handle *handle, · line 4540

Quoted 10 lines

kernel/events/core.c +8 -0

 	__output_copy(handle, values, n * sizeof(u64));
 
+	/*
+	 * We are now under active context, so the only sibling_list
+	 * modification could come via IPI in:
+	 *   perf_install_in_context and perf_remove_from_context
+	 *
+	 * Disable interrupts to prevent this race.
+	 */
+	local_irq_save(flags);

I think this is too late; you want it right at the beginning, before we read ->nr_siblings, as that is also changed by add_event_to_ctx()->perf_group_attach().

That said; it would be nice not to have to poke at the interrupt flag, its expensive.

So is this really a problem, or just a case of: if you do silly things, you get silly results?

Jiri Olsa

2014-07-07 10:20 UTC #4 lore raw

Re: [PATCH 1/1] perf: Prevent race in PERF_SAMPLE_READ group format sample output

Quoted 8 lines · Peter Zijlstra

Quoted 5 lines · Jiri Olsa
From: Jiri Olsa <[email protected]>
While iterating siblings in perf_output_read_group we could race with addition and removal of sibling in perf_group_attach and perf_group_detach respective.
So why would anybody do this?

the test program from 0/1 email hangs up my server but no standard reason AFAICS

Quoted 48 lines

Quoted 33 lines
While in perf_output_read_group we are under active context, so the only sibling_list modification could come via IPI in:
perf_install_in_context or perf_remove_from_context
Disable interrupts before iterating siblings to prevent this race.
Cc Arnaldo Carvalho de Melo <[email protected]>
Cc Corey Ashford <[email protected]>
Cc Frederic Weisbecker <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Paul Mackerras <[email protected]>
Cc Peter Zijlstra <[email protected]>
Signed-off-by Jiri Olsa <[email protected]>
---
 kernel/events/core.c | 11 +++++++++++
 1 file changed, 11 insertions(+)
kernel/events/core.c +1 -0
index a33d9a2b..66649d3 100644
@@ -4509,6 +4509,7 @@ static void perf_output_read_group(struct perf_output_handle *handle,
 {
 	struct perf_event *leader = event->group_leader, *sub;
 	u64 read_format = event->attr.read_format;
+	unsigned long flags;
 	u64 values[5];
 	int n = 0;
 
@@ -4529,6 +4530,15 @@ static void perf_output_read_group(struct perf_output_handle *handle,
 
kernel/events/core.c static void perf_output_read_group(struct perf_output_handle *handle, · line 4540
Quoted 10 lines
kernel/events/core.c +8 -0
 	__output_copy(handle, values, n * sizeof(u64));
 
+	/*
+	 * We are now under active context, so the only sibling_list
+	 * modification could come via IPI in:
+	 *   perf_install_in_context and perf_remove_from_context
+	 *
+	 * Disable interrupts to prevent this race.
+	 */
+	local_irq_save(flags);
I think this is too late; you want it right at the beginning, before we read ->nr_siblings, as that is also changed by add_event_to_ctx()->perf_group_attach().

Quoted 2 lines

That said; it would be nice not to have to poke at the interrupt flag, its expensive.

right.. I'll check if we could use the rcu loop/locking here

Quoted 3 lines

So is this really a problem, or just a case of: if you do silly things, you get silly results?

I've got soft lockup, sometimes ended up with unkillable perf process also few total server hangs

jirka

Peter Zijlstra

maintainer

2014-07-07 11:44 UTC #5 lore raw

Re: [PATCH 1/1] perf: Prevent race in PERF_SAMPLE_READ group format sample output

Quoted 8 lines · Jiri Olsa

Quoted 7 lines · Peter Zijlstra
Quoted 5 lines · Jiri Olsa
From: Jiri Olsa <[email protected]>
While iterating siblings in perf_output_read_group we could race with addition and removal of sibling in perf_group_attach and perf_group_detach respective.

Quoted 7 lines

Quoted 1 line
So why would anybody do this?
the test program from 0/1 email hangs up my server but no standard reason AFAICS
I've got soft lockup, sometimes ended up with unkillable perf process also few total server hangs

OK, so that's useful information to have in a Changelog ;-)

Not immediately obvious how that can happen though. I can see the output getting scrambled.