perf: Fix issues reported by perf fuzzer
Subsystems (3)
Jiri Olsa
reviewercover perf: Fix issues reported by perf fuzzer
hi,
Vince reported several issues with perf code hit by perf fuzzer, in here:
http://marc.info/?l=linux-kernel&m=141806390822670&w=2
I run the fuzzer, but wasn't able to hit issues reported by Vince. But.. I was lucky enough to hit other 2 issues described in following patches ;-)
As I dont follow the uncore code much, it is sort of RFC patchset. However I'm running the fuzzer now for several hours and can't hit any other issue.
thanks,
jirka
---
Jiri Olsa (2):
perf/x86/intel/uncore: Make sure only uncore events are collected
perf: Fix events installation during moving group
arch/x86/kernel/cpu/perf_event_intel_uncore.c | 22 +++++++++++++++++++--- kernel/events/core.c | 4 ++-- 2 files changed, 21 insertions(+), 5 deletions(-)
Jiri Olsa
reviewerPATCH 1/2 perf/x86/intel/uncore: Make sure only uncore events are collected
The uncore_collect_events functions assumes that event group might contain only uncore events which is wrong, because it might contain any type of events.
This bug leads to uncore framework touching 'not' uncore events, which could end up all sorts of bugs.
One was triggered by Vince's perf fuzzer, when the uncore code touched breakpoint event private event space as if it was uncore event and caused BUG:
---
[ 4696.010643] BUG: unable to handle kernel paging request at ffffffff82822068 [ 4696.018433] IP: [<ffffffff81020338>] uncore_assign_events+0x188/0x250 [ 4696.025633] PGD 1812067 PUD 1813063 PMD 0 [ 4696.030228] Oops: 0000 [#1] SMP [ 4696.033844] Modules linked in: igb ptp ioatdma pps_core i2c_algo_bit i2c_core x86_pkg_temp_thermal crc32c_intel dca microcode lpc_ich mfd_core megaraid_sas [ 4696.049503] CPU: 1 PID: 9199 Comm: perf_fuzzer Not tainted 3.18.0fuzzer #117 [ 4696.057368] Hardware name: IBM System x3650 M4 : -[7915E2G]-/00Y7683, BIOS -[VVE124AUS-1.30]- 11/21/2012 [ 4696.067947] task: ffff88026be9da00 ti: ffff88026b9b0000 task.ti: ffff88026b9b0000 [ 4696.076295] RIP: 0010:[<ffffffff81020338>] [<ffffffff81020338>] uncore_assign_events+0x188/0x250 [ 4696.086207] RSP: 0018:ffff88026b9b3d48 EFLAGS: 00010246 [ 4696.092133] RAX: 0000000000000000 RBX: ffff88007a3eda00 RCX: ffffffff81821fa0 [ 4696.100095] RDX: ffff8802719a1000 RSI: 0000000008000446 RDI: 0000000000000000 [ 4696.108057] RBP: ffff88026b9b3db8 R08: ffffffff81821fe0 R09: ffff880277803600 [ 4696.116018] R10: ffffffff81821fa0 R11: ffffffff81020429 R12: ffff88007a3eda10 [ 4696.123980] R13: 0000000000000004 R14: 0000000000000000 R15: 0000000000000004 [ 4696.131942] FS: 00007ff960a2d740(0000) GS:ffff880277c20000(0000) knlGS:0000000000000000 [ 4696.140971] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 4696.147380] CR2: ffffffff82822068 CR3: 0000000273ef0000 CR4: 00000000000407e0 [ 4696.155341] DR0: 000000003af8b4b4 DR1: 0000000000000000 DR2: 0000000000000000 [ 4696.163302] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000600 [ 4696.171264] Stack: [ 4696.173505] ffff88026ca5e000 0000000200000000 ffff88007a3eda10 0000000000000001 [ 4696.181797] ffff88026b9b3d98 ffff88026ca5e000 ffff88007a3eda00 0000000000000000 [ 4696.190089] 0000000000000000 ffff88026ca5e000 ffff880473280100 ffff88007a3eda00 [ 4696.198381] Call Trace: [ 4696.201109] [<ffffffff81020c4c>] uncore_pmu_event_init+0x1cc/0x260 [ 4696.208104] [<ffffffff81109053>] perf_init_event+0x93/0x130 [ 4696.214419] [<ffffffff81109488>] perf_event_alloc+0x398/0x440 [ 4696.220928] [<ffffffff811098f1>] SYSC_perf_event_open+0x3c1/0xbb0 [ 4696.227825] [<ffffffff8110a539>] SyS_perf_event_open+0x9/0x10 [ 4696.234335] [<ffffffff8154d712>] system_call_fastpath+0x12/0x17
---
The code in uncore_assign_events fucntion was looking for event->hw.idx data while the event was initialized as a breakpoint with different members in event->hw union.
This patch forces uncore_collect_events to collect only uncore events.
--- arch/x86/kernel/cpu/perf_event_intel_uncore.c | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-)
arch/x86/kernel/cpu/perf_event_intel_uncore.c +19 -3
@@ -276,6 +276,17 @@ static struct intel_uncore_box *uncore_alloc_box(struct intel_uncore_type *type,return box; } +/* +* Using uncore_pmu_event_init pmu event_init callback +* as a detection point for uncore events. +*/ +static int uncore_pmu_event_init (struct perf_event *event); + +static bool is_uncore_event (struct perf_event *event) +{ +return event->pmu->event_init == uncore_pmu_event_init; +} +static int uncore_collect_events (struct intel_uncore_box *box,struct perf_event *leader,bool dogrp) {
@@ -290,13 +301,18 @@ uncore_collect_events(struct intel_uncore_box *box, struct perf_event *leader, breturn -EINVAL ; n = box->n_events; - box->event_list[n] = leader; - n++; + +if (is_uncore_event (leader)) { + box->event_list[n] = leader; + n++; + } +if (!dogrp)return n;list_for_each_entry (event, &leader->sibling_list, group_entry) { -if (event->state <=PERF_EVENT_STATE_OFF ) +if (!is_uncore_event (event) || + event->state <=PERF_EVENT_STATE_OFF )continue ;if (n >= max_count)
Signature · 1 line
1.9.3
Re: [PATCH 1/2] perf/x86/intel/uncore: Make sure only uncore events are collected
Quoted 6 lines · Jiri Olsa
The
uncore_collect_eventsfunctions assumes that event group might contain only uncore events which is wrong, because it might contain any type of events.This bug leads to uncore framework touching 'not' uncore events, which could end up all sorts of bugs.
Thanks for tracking that down. Looks good to me.
-Andi
tip-bot for Jiri Olsa
merge bot[tip:perf/urgent] perf/x86/intel/uncore: Make sure only uncore events are collected
In reply to Jiri Olsaperf/x86/intel/uncore: Make sure only uncore events are collected
Commit-ID: af91568e762d04931dcbdd6bef4655433d8b9418 Gitweb: http://git.kernel.org/tip/af91568e762d04931dcbdd6bef4655433d8b9418 Author: Jiri Olsa <[email protected]> AuthorDate: Wed, 10 Dec 2014 21:23:50 +0100 Committer: Ingo Molnar <[email protected]> CommitDate: Thu, 11 Dec 2014 11:24:14 +0100
perf/x86/intel/uncore: Make sure only uncore events are collected
The uncore_collect_events functions assumes that event group might contain only uncore events which is wrong, because it might contain any type of events.
This bug leads to uncore framework touching 'not' uncore events, which could end up all sorts of bugs.
One was triggered by Vince's perf fuzzer, when the uncore code touched breakpoint event private event space as if it was uncore event and caused BUG:
BUG: unable to handle kernel paging request at ffffffff82822068 IP: [<ffffffff81020338>] uncore_assign_events+0x188/0x250 ...
The code in uncore_assign_events() function was looking for event->hw.idx data while the event was initialized as a breakpoint with different members in event->hw union.
This patch forces uncore_collect_events() to collect only uncore events.
--- arch/x86/kernel/cpu/perf_event_intel_uncore.c | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-)
arch/x86/kernel/cpu/perf_event_intel_uncore.c +19 -3
@@ -276,6 +276,17 @@ static struct intel_uncore_box *uncore_alloc_box(struct intel_uncore_type *type,return box; } +/* +* Using uncore_pmu_event_init pmu event_init callback +* as a detection point for uncore events. +*/ +static int uncore_pmu_event_init (struct perf_event *event); + +static bool is_uncore_event (struct perf_event *event) +{ +return event->pmu->event_init == uncore_pmu_event_init; +} +static int uncore_collect_events (struct intel_uncore_box *box,struct perf_event *leader,bool dogrp) {
@@ -290,13 +301,18 @@ uncore_collect_events(struct intel_uncore_box *box, struct perf_event *leader, breturn -EINVAL ; n = box->n_events; - box->event_list[n] = leader; - n++; + +if (is_uncore_event (leader)) { + box->event_list[n] = leader; + n++; + } +if (!dogrp)return n;list_for_each_entry (event, &leader->sibling_list, group_entry) { -if (event->state <=PERF_EVENT_STATE_OFF ) +if (!is_uncore_event (event) || + event->state <=PERF_EVENT_STATE_OFF )continue ;if (n >= max_count)
Jiri Olsa
reviewerPATCH 2/2 perf: Fix events installation during moving group
In reply to Jiri OlsaWe allow PMU driver to change the cpu on which the event
should be installed to. This happened in patch:
e2d37cd213dc perf: Allow the PMU driver to choose the CPU on which to install events
This patch also forces all the group members to follow the currently opened events cpu if the group happened to be moved.
This and the change of event->cpu in perf_install_in_context function introduced in:
0cda4c023132 perf: Introduce perf_pmu_migrate_context()
forces group members to change their event->cpu, if the currently-opened-event's PMU changed the cpu and there is a group move.
Above behaviour causes problem for breakpoint events, which uses event->cpu to touch cpu specific data for breakpoints accounting. By changing event->cpu, some breakpoints slots were wrongly accounted for given cpu.
Vinces's perf fuzzer hit this issue and caused following WARN on my setup:
[ 7113.758779] WARNING: CPU: 0 PID: 20214 at arch/x86/kernel/hw_breakpoint.c:119 arch_install_hw_breakpoint+0x142/0x150() [ 7113.759262] Can't find any breakpoint slot [ 7113.759433] Modules linked in: [ 7113.759433] CPU: 0 PID: 20214 Comm: perf_fuzzer Not tainted 3.18.0 #31 [ 7113.759433] Hardware name: Intel Corporation Montevina platform/To be filled by O.E.M., BIOS AMVACRB1.86C.0066.B00.0805070703 05/07/2008 [ 7113.759433] 0000000000000009 ffff880050783ab8 ffffffff8157b70b 0000000000000004 [ 7113.759433] ffff880050783b08 ffff880050783af8 ffffffff8104b771 0000000000000021 [ 7113.759433] 0000000000000004 ffff88007a60a958 000000000000a960 ffff88002ef42000 [ 7113.759433] Call Trace: [ 7113.759433] [<ffffffff8157b70b>] dump_stack+0x4f/0x7c [ 7113.759433] [<ffffffff8104b771>] warn_slowpath_common+0x81/0xa0 [ 7113.759433] [<ffffffff8104b7d6>] warn_slowpath_fmt+0x46/0x50 [ 7113.759433] [<ffffffff8100b262>] arch_install_hw_breakpoint+0x142/0x150 [ 7113.759433] [<ffffffff81117e48>] hw_breakpoint_add+0x48/0x50 [ 7113.759433] [<ffffffff81111611>] event_sched_in.isra.81+0xa1/0x270 [ 7113.759433] [<ffffffff81111892>] group_sched_in+0xb2/0x1d0 [ 7113.759433] [<ffffffff81111b90>] ctx_sched_in+0x1e0/0x390 [ 7113.759433] [<ffffffff81111da4>] perf_event_sched_in+0x64/0x90 [ 7113.759433] [<ffffffff811126a1>] __perf_install_in_context+0x121/0x1c0 [ 7113.759433] [<ffffffff8108b975>] ? mark_held_locks+0x75/0xa0 [ 7113.759433] [<ffffffff8110e930>] ? task_clock_event_add+0x40/0x40 [ 7113.759433] [<ffffffff8110e983>] remote_function+0x53/0x70 [ 7113.759433] [<ffffffff810c0d43>] generic_exec_single+0x123/0x180 [ 7113.759433] [<ffffffff8110e930>] ? task_clock_event_add+0x40/0x40 [ 7113.759433] [<ffffffff810c0dfa>] smp_call_function_single+0x5a/0xc0 [ 7113.759433] [<ffffffff8108ba9d>] ? trace_hardirqs_on_caller+0xfd/0x1c0 [ 7113.759433] [<ffffffff8110bc44>] task_function_call+0x44/0x50 [ 7113.759433] [<ffffffff81112580>] ? perf_cpu_hrtimer_handler+0x210/0x210 [ 7113.759433] [<ffffffff8110e77b>] perf_install_in_context+0x8b/0x110 [ 7113.759433] [<ffffffff811161eb>] SyS_perf_event_open+0x59b/0xcc0 [ 7113.759433] [<ffffffff811361d5>] ? vm_mmap_pgoff+0x75/0xa0 [ 7113.759433] [<ffffffff81584162>] tracesys_phase2+0xd4/0xd9 [ 7113.759433] ---[ end trace 1a0d82aa412e33cf ]---
This patch changes the group moving code to keep events original cpus.
--- kernel/events/core.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
kernel/events/core.c +2 -2
@@ -7477,11 +7477,11 @@ SYSCALL_DEFINE5(perf_event_open,if (move_group) {synchronize_rcu (); -perf_install_in_context (ctx, group_leader, event->cpu); +perf_install_in_context (ctx, group_leader, group_leader->cpu);get_ctx (ctx);list_for_each_entry (sibling, &group_leader->sibling_list, group_entry) { -perf_install_in_context (ctx, sibling, event->cpu); +perf_install_in_context (ctx, sibling, sibling->cpu);get_ctx (ctx); } }
Signature · 1 line
1.9.3
tip-bot for Jiri Olsa
merge bot[tip:perf/urgent] perf: Fix events installation during moving group
perf: Fix events installation during moving group
Commit-ID: 9fc81d87420d0d3fd62d5e5529972c0ad9eab9cc Gitweb: http://git.kernel.org/tip/9fc81d87420d0d3fd62d5e5529972c0ad9eab9cc Author: Jiri Olsa <[email protected]> AuthorDate: Wed, 10 Dec 2014 21:23:51 +0100 Committer: Ingo Molnar <[email protected]> CommitDate: Thu, 11 Dec 2014 11:24:15 +0100
perf: Fix events installation during moving group
We allow PMU driver to change the cpu on which the event should be installed to. This happened in patch:
e2d37cd213dc ("perf: Allow the PMU driver to choose the CPU on which to install events")
This patch also forces all the group members to follow the currently opened events cpu if the group happened to be moved.
This and the change of event->cpu in perf_install_in_context() function introduced in:
0cda4c023132 ("perf: Introduce perf_pmu_migrate_context()")
forces group members to change their event->cpu, if the currently-opened-event's PMU changed the cpu and there is a group move.
Above behaviour causes problem for breakpoint events, which uses event->cpu to touch cpu specific data for breakpoints accounting. By changing event->cpu, some breakpoints slots were wrongly accounted for given cpu.
Vinces's perf fuzzer hit this issue and caused following WARN on my setup:
WARNING: CPU: 0 PID: 20214 at arch/x86/kernel/hw_breakpoint.c:119 arch_install_hw_breakpoint+0x142/0x150() Can't find any breakpoint slot [...]
This patch changes the group moving code to keep the event's original cpu.
--- kernel/events/core.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
kernel/events/core.c +2 -2
@@ -7435,11 +7435,11 @@ SYSCALL_DEFINE5(perf_event_open,if (move_group) {synchronize_rcu (); -perf_install_in_context (ctx, group_leader, event->cpu); +perf_install_in_context (ctx, group_leader, group_leader->cpu);get_ctx (ctx);list_for_each_entry (sibling, &group_leader->sibling_list, group_entry) { -perf_install_in_context (ctx, sibling, event->cpu); +perf_install_in_context (ctx, sibling, sibling->cpu);get_ctx (ctx); } }
Quoted 5 lines · Jiri Olsa
hi,
Vince reported several issues with perf code hit by perf fuzzer, in here:
http://marc.info/?l=linux-kernel&m=141806390822670&w=2
Quoted 7 lines
I run the fuzzer, but wasn't able to hit issues reported by Vince. But.. I was lucky enough to hit other 2 issues described in following patches ;-)
As I dont follow the uncore code much, it is sort of RFC patchset. However I'm running the fuzzer now for several hours and can't hit any other issue.
The changes are looking good to me so I've applied them to tip:perf/urgent to give them more testing.
If they break anything in the uncore code's expectations the uncore people should chime in. In any case, stability comes first.
Thanks,
Ingo