Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1451295 > unrolled thread

NULL ptr deref in perf/filter_match

Started byVegard Nossum <vegard.nossum@gmail.com>
First post2016-07-27 16:20 +0200
Last post2016-08-10 21:50 +0200
Articles 10 — 4 participants

Back to article view | Back to linux.kernel

Contents

  NULL ptr deref in perf/filter_match Vegard Nossum <vegard.nossum@gmail.com> - 2016-07-27 16:20 +0200
    Re: NULL ptr deref in perf/filter_match Vegard Nossum <vegard.nossum@gmail.com> - 2016-07-29 23:50 +0200
      Re: NULL ptr deref in perf/filter_match Vegard Nossum <vegard.nossum@gmail.com> - 2016-07-30 00:40 +0200
      Re: NULL ptr deref in perf/filter_match Peter Zijlstra <peterz@infradead.org> - 2016-08-04 14:40 +0200
        Re: NULL ptr deref in perf/filter_match Vegard Nossum <vegard.nossum@gmail.com> - 2016-08-04 17:20 +0200
          Re: NULL ptr deref in perf/filter_match Vegard Nossum <vegard.nossum@gmail.com> - 2016-08-04 18:00 +0200
            Re: NULL ptr deref in perf/filter_match Peter Zijlstra <peterz@infradead.org> - 2016-08-04 18:10 +0200
            Re: NULL ptr deref in perf/filter_match Vince Weaver <vincent.weaver@maine.edu> - 2016-08-08 22:10 +0200
              Re: NULL ptr deref in perf/filter_match Vegard Nossum <vegard.nossum@gmail.com> - 2016-08-09 08:20 +0200
        [tip:perf/core] perf/core: Fix sideband list-iteration vs. event  ordering NULL pointer deference crash tip-bot for Peter Zijlstra <tipbot@zytor.com> - 2016-08-10 21:50 +0200

#1451295 — NULL ptr deref in perf/filter_match

FromVegard Nossum <vegard.nossum@gmail.com>
Date2016-07-27 16:20 +0200
SubjectNULL ptr deref in perf/filter_match
Message-ID<rZxQJ-ug-1@gated-at.bofh.it>
Hi,

I'm seeing this on latest linus/master:

kasan: CONFIG_KASAN_INLINE enabled
kasan: GPF could be caused by NULL-ptr deref or user memory access
general protection fault: 0000 [#1] PREEMPT SMP KASAN
Dumping ftrace buffer:
  (ftrace buffer empty)
CPU: 3 PID: 0 Comm: swapper/3 Not tainted 4.7.0+ #50
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS
Ubuntu-1.8.2-1ubuntu1 04/01/2014
task: ffff880119d05400 ti: ffff880119d38000 task.ti: ffff880119d38000
RIP: 0010:[<ffffffff81327820>]  [<ffffffff81327820>] perf_iterate_sb+0x1b0/0x6a0
RSP: 0018:ffff880119d3fc30  EFLAGS: 00010046
RAX: 0000000000000000 RBX: ffff880080af8530 RCX: 0000000000000000
RDX: 1ffff100235f3465 RSI: ffffffff8376a900 RDI: ffff880080af8730
RBP: ffff880119d3fc70 R08: 0000000000000000 R09: 0000000000000000
R10: ffff8800abbfe200 R11: 0000000000000000 R12: ffffffff8131b8e0
R13: ffff880119d3fcf0 R14: dffffc0000000000 R15: 0000000000000000
FS:  0000000000000000(0000) GS:ffff88011af80000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007fad10e87b10 CR3: 00000000a89d5000 CR4: 00000000000006e0
DR0: 00007fad1114b000 DR1: 00007fad0f4a7000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000600
Stack:
ffff88011af9f580 ffff880119d05400 ffff88011af9a328 ffff880119d05400
ffff880119d3fd30 0000000000000003 1ffff100233a7f9a ffff8800abbfe200
ffff880119d3fd58 ffffffff81334670 0000000041b58ab3 ffffffff83bc294e
Call Trace:
[<ffffffff81334670>] __perf_event_task_sched_out+0x2a0/0xec0
[<ffffffff813343d0>] ? perf_event_update_userpage+0x630/0x630
[<ffffffff8115f2bd>] ? finish_task_switch+0x12d/0x580
[<ffffffff8351a881>] __schedule+0x9a1/0x16c0
[<ffffffff83519ee0>] ? pci_mmcfg_check_reserved+0x110/0x110
[<ffffffff81058e37>] ? dump_trace+0x117/0x300
[<ffffffff810771a6>] ? save_stack_trace+0x26/0x50
[<ffffffff8351b86a>] schedule+0x9a/0x1c0
[<ffffffff8351b9e3>] schedule_preempt_disabled+0x13/0x20
[<ffffffff811c35fd>] cpu_startup_entry+0x1cd/0x5a0
[<ffffffff83525d7f>] ? _raw_spin_unlock_irqrestore+0x1f/0x40
[<ffffffff810a76e7>] start_secondary+0x247/0x2d0
Code: 5f ff ff ff 48 8d bb 00 02 00 00 48 89 f8 48 c1 e8 03 42 80 3c
30 00 0f 85 57 04 00 00 4c 8b bb 00 02 00 00 4c 89 f8 48 c1 e8 03 <42>
80 3c 30 00 0f 85 31 04 00 00 4d 8b 3f 49 8d 7f 40 48 89 f8
RIP  [<ffffffff81327820>] perf_iterate_sb+0x1b0/0x6a0
RSP <ffff880119d3fc30>
---[ end trace fc2135c1ac1bf1e9 ]---

That seems to be roughly:

kernel/events/core.c:145
kernel/events/core.c:547 perf_cgroup_match
kernel/events/core.c:1720 event_filter_match
kernel/events/core.c:5950 perf_iterate_sb_cpu
kernel/events/core.c:5982 perf_iterate_sb
kernel/events/core.c:6794 perf_event_switch
kernel/events/core.c:2857 __perf_event_task_sched_out

In particular, it looks to me like event->ctx is NULL.

I haven't seen this before v4.7, so I'm assuming it's new since then.
This would look the most suspicious to me if it weren't for the fact
that it claims no change in functionality:

commit aab5b71ef2b5c62323b9abe397e2db57b18e1f78
Author: Peter Zijlstra <peterz@infradead.org>
Date:   Thu May 12 17:26:46 2016 +0200

   perf/core: Rename the perf_event_aux*() APIs to perf_event_sb*(),
to separate them from AUX ring-buffer record

I don't have time to look any more into this right now, sorry.


Vegard

[toc] | [next] | [standalone]

#1452594

FromVegard Nossum <vegard.nossum@gmail.com>
Date2016-07-29 23:50 +0200
Message-ID<s0nPk-1RL-17@gated-at.bofh.it>
In reply to#1451295
On 27 July 2016 at 16:15, Vegard Nossum <vegard.nossum@gmail.com> wrote:
> Hi,
>
> I'm seeing this on latest linus/master:
>
> kasan: CONFIG_KASAN_INLINE enabled
> kasan: GPF could be caused by NULL-ptr deref or user memory access
> general protection fault: 0000 [#1] PREEMPT SMP KASAN
> Dumping ftrace buffer:
>   (ftrace buffer empty)
> CPU: 3 PID: 0 Comm: swapper/3 Not tainted 4.7.0+ #50
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS
> Ubuntu-1.8.2-1ubuntu1 04/01/2014
> task: ffff880119d05400 ti: ffff880119d38000 task.ti: ffff880119d38000
> RIP: 0010:[<ffffffff81327820>]  [<ffffffff81327820>] perf_iterate_sb+0x1b0/0x6a0
> RSP: 0018:ffff880119d3fc30  EFLAGS: 00010046
> RAX: 0000000000000000 RBX: ffff880080af8530 RCX: 0000000000000000
> RDX: 1ffff100235f3465 RSI: ffffffff8376a900 RDI: ffff880080af8730
> RBP: ffff880119d3fc70 R08: 0000000000000000 R09: 0000000000000000
> R10: ffff8800abbfe200 R11: 0000000000000000 R12: ffffffff8131b8e0
> R13: ffff880119d3fcf0 R14: dffffc0000000000 R15: 0000000000000000
> FS:  0000000000000000(0000) GS:ffff88011af80000(0000) knlGS:0000000000000000
> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 00007fad10e87b10 CR3: 00000000a89d5000 CR4: 00000000000006e0
> DR0: 00007fad1114b000 DR1: 00007fad0f4a7000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000600
> Stack:
> ffff88011af9f580 ffff880119d05400 ffff88011af9a328 ffff880119d05400
> ffff880119d3fd30 0000000000000003 1ffff100233a7f9a ffff8800abbfe200
> ffff880119d3fd58 ffffffff81334670 0000000041b58ab3 ffffffff83bc294e
> Call Trace:
> [<ffffffff81334670>] __perf_event_task_sched_out+0x2a0/0xec0
> [<ffffffff813343d0>] ? perf_event_update_userpage+0x630/0x630
> [<ffffffff8115f2bd>] ? finish_task_switch+0x12d/0x580
> [<ffffffff8351a881>] __schedule+0x9a1/0x16c0
> [<ffffffff83519ee0>] ? pci_mmcfg_check_reserved+0x110/0x110
> [<ffffffff81058e37>] ? dump_trace+0x117/0x300
> [<ffffffff810771a6>] ? save_stack_trace+0x26/0x50
> [<ffffffff8351b86a>] schedule+0x9a/0x1c0
> [<ffffffff8351b9e3>] schedule_preempt_disabled+0x13/0x20
> [<ffffffff811c35fd>] cpu_startup_entry+0x1cd/0x5a0
> [<ffffffff83525d7f>] ? _raw_spin_unlock_irqrestore+0x1f/0x40
> [<ffffffff810a76e7>] start_secondary+0x247/0x2d0
> Code: 5f ff ff ff 48 8d bb 00 02 00 00 48 89 f8 48 c1 e8 03 42 80 3c
> 30 00 0f 85 57 04 00 00 4c 8b bb 00 02 00 00 4c 89 f8 48 c1 e8 03 <42>
> 80 3c 30 00 0f 85 31 04 00 00 4d 8b 3f 49 8d 7f 40 48 89 f8
> RIP  [<ffffffff81327820>] perf_iterate_sb+0x1b0/0x6a0
> RSP <ffff880119d3fc30>
> ---[ end trace fc2135c1ac1bf1e9 ]---
>
> That seems to be roughly:
>
> kernel/events/core.c:145
> kernel/events/core.c:547 perf_cgroup_match
> kernel/events/core.c:1720 event_filter_match
> kernel/events/core.c:5950 perf_iterate_sb_cpu
> kernel/events/core.c:5982 perf_iterate_sb
> kernel/events/core.c:6794 perf_event_switch
> kernel/events/core.c:2857 __perf_event_task_sched_out
>
> In particular, it looks to me like event->ctx is NULL.

Digging a bit deeper into this, it seems the event itself is getting
created by perf_event_open() and it gets added to the pmu_event_list
through:

perf_event_open()
 - perf_event_alloc()
    - account_event()
       - account_pmu_sb_event()
          - attach_sb_event()

so at this point the event is being attached but its ->ctx is still
NULL. It seems like ->ctx is set just a bit later in
perf_event_open(), though.

But before that, __schedule() comes along and creates a stack trace
similar to the one above:

__schedule()
 - __perf_event_task_sched_out()
   - perf_iterate_sb()
     - perf_iterate_sb_cpu()
        - event_filter_match()
          - perf_cgroup_match()
            - __get_cpu_context()
              - (dereference ctx which is NULL)

So I guess the question is... should the event be attached (= put on
the list) before ->ctx gets set? Or should the cgroup code check for a
NULL ->ctx?

I'm seeing the NUL ptr deref in __perf_event_task_sched_in() as well, btw.

I'm thinking this is probably where the bug was introduced:

commit f2fb6bef92514432398a653df1c2f1041d79ac46
Author: Kan Liang <kan.liang@intel.com>
Date:   Wed Mar 23 11:24:37 2016 -0700

   perf/core: Optimize side-band event delivery


Vegard

[toc] | [prev] | [next] | [standalone]

#1452602

FromVegard Nossum <vegard.nossum@gmail.com>
Date2016-07-30 00:40 +0200
Message-ID<s0oBH-2rx-1@gated-at.bofh.it>
In reply to#1452594

[Multipart message — attachments visible in raw view] — view raw

On 29 July 2016 at 23:41, Vegard Nossum <vegard.nossum@gmail.com> wrote:
> On 27 July 2016 at 16:15, Vegard Nossum <vegard.nossum@gmail.com> wrote:
>> Hi,
>>
>> I'm seeing this on latest linus/master:
[...]
>> RIP: 0010:[<ffffffff81327820>]  [<ffffffff81327820>] perf_iterate_sb+0x1b0/0x6a0
[...]
>>
>> In particular, it looks to me like event->ctx is NULL.
>
> Digging a bit deeper into this, it seems the event itself is getting
> created by perf_event_open() and it gets added to the pmu_event_list
> through:
>
> perf_event_open()
>  - perf_event_alloc()
>     - account_event()
>        - account_pmu_sb_event()
>           - attach_sb_event()
>
> so at this point the event is being attached but its ->ctx is still
> NULL. It seems like ->ctx is set just a bit later in
> perf_event_open(), though.
>
> But before that, __schedule() comes along and creates a stack trace
> similar to the one above:
>
> __schedule()
>  - __perf_event_task_sched_out()
>    - perf_iterate_sb()
>      - perf_iterate_sb_cpu()
>         - event_filter_match()
>           - perf_cgroup_match()
>             - __get_cpu_context()
>               - (dereference ctx which is NULL)
>
> So I guess the question is... should the event be attached (= put on
> the list) before ->ctx gets set? Or should the cgroup code check for a
> NULL ->ctx?
>
> I'm seeing the NUL ptr deref in __perf_event_task_sched_in() as well, btw.
>
> I'm thinking this is probably where the bug was introduced:
>
> commit f2fb6bef92514432398a653df1c2f1041d79ac46
> Author: Kan Liang <kan.liang@intel.com>
> Date:   Wed Mar 23 11:24:37 2016 -0700
>
>    perf/core: Optimize side-band event delivery

Reverting aab5b71ef2b5c62323b9abe397e2db57b18e1f78 and
f2fb6bef92514432398a653df1c2f1041d79ac46 does indeed fix the issue for
me.

(Just to be clear, I'm not suggesting a revert as the final fix to
this issue, but it shows quite clearly where the problem is.)


Vegard

[toc] | [prev] | [next] | [standalone]

#1456382

FromPeter Zijlstra <peterz@infradead.org>
Date2016-08-04 14:40 +0200
Message-ID<s2q6m-DV-41@gated-at.bofh.it>
In reply to#1452594
On Fri, Jul 29, 2016 at 11:41:11PM +0200, Vegard Nossum wrote:

> Digging a bit deeper into this, it seems the event itself is getting
> created by perf_event_open() and it gets added to the pmu_event_list
> through:
> 
> perf_event_open()
>  - perf_event_alloc()
>     - account_event()
>        - account_pmu_sb_event()
>           - attach_sb_event()
> 
> so at this point the event is being attached but its ->ctx is still
> NULL. It seems like ->ctx is set just a bit later in
> perf_event_open(), though.
> 
> But before that, __schedule() comes along and creates a stack trace
> similar to the one above:
> 
> __schedule()
>  - __perf_event_task_sched_out()
>    - perf_iterate_sb()
>      - perf_iterate_sb_cpu()
>         - event_filter_match()
>           - perf_cgroup_match()
>             - __get_cpu_context()
>               - (dereference ctx which is NULL)
> 
> So I guess the question is... should the event be attached (= put on
> the list) before ->ctx gets set? Or should the cgroup code check for a
> NULL ->ctx?

Does this fix it? Ordering is a bit of a mess, adding the events to the
list _after_ they've been installed has the risk of missing things I
think, nor does that result in particularly nice code.

Then again, this isn't pretty either.

---
 kernel/events/core.c | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index a19550d80ab1..87d02b8cb87e 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -1716,8 +1716,8 @@ static inline int pmu_filter_match(struct perf_event *event)
 static inline int
 event_filter_match(struct perf_event *event)
 {
-	return (event->cpu == -1 || event->cpu == smp_processor_id())
-	    && perf_cgroup_match(event) && pmu_filter_match(event);
+	return (event->cpu == -1 || event->cpu == smp_processor_id()) &&
+	       perf_cgroup_match(event) && pmu_filter_match(event);
 }
 
 static void
@@ -1737,8 +1737,8 @@ event_sched_out(struct perf_event *event,
 	 * maintained, otherwise bogus information is return
 	 * via read() for time_enabled, time_running:
 	 */
-	if (event->state == PERF_EVENT_STATE_INACTIVE
-	    && !event_filter_match(event)) {
+	if (event->state == PERF_EVENT_STATE_INACTIVE &&
+	    !event_filter_match(event)) {
 		delta = tstamp - event->tstamp_stopped;
 		event->tstamp_running += delta;
 		event->tstamp_stopped = tstamp;
@@ -2236,10 +2236,15 @@ perf_install_in_context(struct perf_event_context *ctx,
 
 	lockdep_assert_held(&ctx->mutex);
 
-	event->ctx = ctx;
 	if (event->cpu != -1)
 		event->cpu = cpu;
 
+	/*
+	 * Ensures that if we can observe event->ctx, both the event and ctx
+	 * will be 'complete'. See perf_iterate_sb_cpu().
+	 */
+	smp_store_release(&event->ctx, ctx);
+
 	if (!task) {
 		cpu_function_call(cpu, __perf_install_in_context, event);
 		return;
@@ -5969,6 +5974,14 @@ static void perf_iterate_sb_cpu(perf_iterate_f output, void *data)
 	struct perf_event *event;
 
 	list_for_each_entry_rcu(event, &pel->list, sb_list) {
+		/*
+		 * Skip events that are not fully formed yet; ensure that
+		 * if we observe event->ctx, both event and ctx will be
+		 * complete enough. See perf_install_in_context().
+		 */
+		if (!smp_load_acquire(&event->ctx))
+			continue;
+
 		if (event->state < PERF_EVENT_STATE_INACTIVE)
 			continue;
 		if (!event_filter_match(event))

[toc] | [prev] | [next] | [standalone]

#1456478

FromVegard Nossum <vegard.nossum@gmail.com>
Date2016-08-04 17:20 +0200
Message-ID<s2sBc-2wI-3@gated-at.bofh.it>
In reply to#1456382
On 4 August 2016 at 14:37, Peter Zijlstra <peterz@infradead.org> wrote:
> On Fri, Jul 29, 2016 at 11:41:11PM +0200, Vegard Nossum wrote:
>
>> Digging a bit deeper into this, it seems the event itself is getting
>> created by perf_event_open() and it gets added to the pmu_event_list
>> through:
>>
>> perf_event_open()
>>  - perf_event_alloc()
>>     - account_event()
>>        - account_pmu_sb_event()
>>           - attach_sb_event()
>>
[...]
>> So I guess the question is... should the event be attached (= put on
>> the list) before ->ctx gets set? Or should the cgroup code check for a
>> NULL ->ctx?
>
> Does this fix it? Ordering is a bit of a mess, adding the events to the
> list _after_ they've been installed has the risk of missing things I
> think, nor does that result in particularly nice code.
>
> Then again, this isn't pretty either.

Patch seems to fix it here, thanks! Feel free to add:

Tested-by: Vegard Nossum <vegard.nossum@oracle.com>


Vegard

[toc] | [prev] | [next] | [standalone]

#1456510

FromVegard Nossum <vegard.nossum@gmail.com>
Date2016-08-04 18:00 +0200
Message-ID<s2tdT-2TC-5@gated-at.bofh.it>
In reply to#1456478
On 4 August 2016 at 17:17, Vegard Nossum <vegard.nossum@gmail.com> wrote:
> On 4 August 2016 at 14:37, Peter Zijlstra <peterz@infradead.org> wrote:
>> On Fri, Jul 29, 2016 at 11:41:11PM +0200, Vegard Nossum wrote:
>>
>>> Digging a bit deeper into this, it seems the event itself is getting
>>> created by perf_event_open() and it gets added to the pmu_event_list
>>> through:
>>>
>>> perf_event_open()
>>>  - perf_event_alloc()
>>>     - account_event()
>>>        - account_pmu_sb_event()
>>>           - attach_sb_event()
>>>
> [...]
>>> So I guess the question is... should the event be attached (= put on
>>> the list) before ->ctx gets set? Or should the cgroup code check for a
>>> NULL ->ctx?
>>
>> Does this fix it? Ordering is a bit of a mess, adding the events to the
>> list _after_ they've been installed has the risk of missing things I
>> think, nor does that result in particularly nice code.
>>
>> Then again, this isn't pretty either.
>
> Patch seems to fix it here, thanks! Feel free to add:
>
> Tested-by: Vegard Nossum <vegard.nossum@oracle.com>

BTW, this seems to show up slightly more frequently although I've seen
it a couple of times before without your patch too so it's probably
unrelated:

WARNING: CPU: 0 PID: 1244 at arch/x86/kernel/hw_breakpoint.c:121
arch_install_hw_breakpoint+0x284/0x2f0
Can't find any breakpoint slot
CPU: 0 PID: 1244 Comm: trinity-c0 Not tainted 4.7.0+ #73
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS
rel-1.9.3-0-ge2fc41e-prebuilt.qemu-project.org 04/01/20
14
ffffffff83a1a5a0 ffff880116b6f860 ffffffff81d75571 ffff880116b6f8d8
0000000000000000 ffff880116b6f8a8 ffffffff810fd28f ffff880115fb5a00
ffff880100000079 ffffed0022d6df17 0000000000000004 ffff88010e202a80
Call Trace:
[<ffffffff81d75571>] dump_stack+0x65/0x84
[<ffffffff810fd28f>] __warn+0x17f/0x1a0
[<ffffffff810fd342>] warn_slowpath_fmt+0x92/0xb0
[<ffffffff810fd2b0>] ? __warn+0x1a0/0x1a0
[<ffffffff8135b5ba>] ? perf_event_update_userpage+0x3ca/0x660
[<ffffffff81064144>] arch_install_hw_breakpoint+0x284/0x2f0
[<ffffffff81371e44>] hw_breakpoint_add+0xd4/0x100
[<ffffffff81354d37>] event_sched_in.isra.100+0x3a7/0xa40
[<ffffffff813554d3>] group_sched_in+0x103/0x3e0
[<ffffffff81066579>] ? sched_clock+0x9/0x10
[<ffffffff81355fbd>] ctx_sched_in+0x80d/0x16f0
[<ffffffff81356f00>] perf_event_sched_in+0x60/0x80
[<ffffffff81356f80>] ctx_resched+0x60/0xa0
[<ffffffff81357d67>] __perf_install_in_context+0x247/0x320
[<ffffffff81357b20>] ? __perf_event_enable+0xb60/0xb60
[<ffffffff81347fa0>] ? perf_duration_warn+0x40/0x40
[<ffffffff813480b5>] remote_function+0x115/0x1a0
[<ffffffff81347fa0>] ? perf_duration_warn+0x40/0x40
[<ffffffff8125d801>] generic_exec_single+0x191/0x290
[<ffffffff81347fa0>] ? perf_duration_warn+0x40/0x40
[<ffffffff8125db2e>] smp_call_function_single+0xde/0x350
[<ffffffff8125da50>] ? generic_smp_call_function_single_interrupt+0x10/0x10
[<ffffffff81341f80>] perf_install_in_context+0x200/0x370
[<ffffffff81341d80>] ? perf_remove_from_context+0xc0/0xc0
[<ffffffff81357b20>] ? __perf_event_enable+0xb60/0xb60
[<ffffffff81361482>] SYSC_perf_event_open+0xd62/0x1cf0
[<ffffffff81360720>] ? perf_event_set_output+0x400/0x400
[<ffffffff81dd4603>] ? __this_cpu_preempt_check+0x13/0x20
[<ffffffff81369e80>] ? perf_pmu_unregister+0x470/0x470
[<ffffffff81369e89>] SyS_perf_event_open+0x9/0x10
[<ffffffff81005391>] do_syscall_64+0x1a1/0x460
[<ffffffff813742fa>] ? __context_tracking_enter+0xaa/0x200
[<ffffffff8389746a>] entry_SYSCALL64_slow_path+0x25/0x25
---[ end trace 966c767fd836202d ]---


Vegard

[toc] | [prev] | [next] | [standalone]

#1456520

FromPeter Zijlstra <peterz@infradead.org>
Date2016-08-04 18:10 +0200
Message-ID<s2tnA-3cA-31@gated-at.bofh.it>
In reply to#1456510
On Thu, Aug 04, 2016 at 05:55:30PM +0200, Vegard Nossum wrote:
> BTW, this seems to show up slightly more frequently although I've seen
> it a couple of times before without your patch too so it's probably
> unrelated:

Frederic, could you have a look?

> WARNING: CPU: 0 PID: 1244 at arch/x86/kernel/hw_breakpoint.c:121
> arch_install_hw_breakpoint+0x284/0x2f0
> Can't find any breakpoint slot
> CPU: 0 PID: 1244 Comm: trinity-c0 Not tainted 4.7.0+ #73
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS
> rel-1.9.3-0-ge2fc41e-prebuilt.qemu-project.org 04/01/20
> 14
> ffffffff83a1a5a0 ffff880116b6f860 ffffffff81d75571 ffff880116b6f8d8
> 0000000000000000 ffff880116b6f8a8 ffffffff810fd28f ffff880115fb5a00
> ffff880100000079 ffffed0022d6df17 0000000000000004 ffff88010e202a80
> Call Trace:
> [<ffffffff81d75571>] dump_stack+0x65/0x84
> [<ffffffff810fd28f>] __warn+0x17f/0x1a0
> [<ffffffff810fd342>] warn_slowpath_fmt+0x92/0xb0
> [<ffffffff810fd2b0>] ? __warn+0x1a0/0x1a0
> [<ffffffff8135b5ba>] ? perf_event_update_userpage+0x3ca/0x660
> [<ffffffff81064144>] arch_install_hw_breakpoint+0x284/0x2f0
> [<ffffffff81371e44>] hw_breakpoint_add+0xd4/0x100
> [<ffffffff81354d37>] event_sched_in.isra.100+0x3a7/0xa40
> [<ffffffff813554d3>] group_sched_in+0x103/0x3e0
> [<ffffffff81066579>] ? sched_clock+0x9/0x10
> [<ffffffff81355fbd>] ctx_sched_in+0x80d/0x16f0
> [<ffffffff81356f00>] perf_event_sched_in+0x60/0x80
> [<ffffffff81356f80>] ctx_resched+0x60/0xa0
> [<ffffffff81357d67>] __perf_install_in_context+0x247/0x320
> [<ffffffff81357b20>] ? __perf_event_enable+0xb60/0xb60
> [<ffffffff81347fa0>] ? perf_duration_warn+0x40/0x40
> [<ffffffff813480b5>] remote_function+0x115/0x1a0
> [<ffffffff81347fa0>] ? perf_duration_warn+0x40/0x40
> [<ffffffff8125d801>] generic_exec_single+0x191/0x290
> [<ffffffff81347fa0>] ? perf_duration_warn+0x40/0x40
> [<ffffffff8125db2e>] smp_call_function_single+0xde/0x350
> [<ffffffff8125da50>] ? generic_smp_call_function_single_interrupt+0x10/0x10
> [<ffffffff81341f80>] perf_install_in_context+0x200/0x370
> [<ffffffff81341d80>] ? perf_remove_from_context+0xc0/0xc0
> [<ffffffff81357b20>] ? __perf_event_enable+0xb60/0xb60
> [<ffffffff81361482>] SYSC_perf_event_open+0xd62/0x1cf0
> [<ffffffff81360720>] ? perf_event_set_output+0x400/0x400
> [<ffffffff81dd4603>] ? __this_cpu_preempt_check+0x13/0x20
> [<ffffffff81369e80>] ? perf_pmu_unregister+0x470/0x470
> [<ffffffff81369e89>] SyS_perf_event_open+0x9/0x10
> [<ffffffff81005391>] do_syscall_64+0x1a1/0x460
> [<ffffffff813742fa>] ? __context_tracking_enter+0xaa/0x200
> [<ffffffff8389746a>] entry_SYSCALL64_slow_path+0x25/0x25
> ---[ end trace 966c767fd836202d ]---

[toc] | [prev] | [next] | [standalone]

#1458209

FromVince Weaver <vincent.weaver@maine.edu>
Date2016-08-08 22:10 +0200
Message-ID<s3Z22-63f-19@gated-at.bofh.it>
In reply to#1456510
On Thu, 4 Aug 2016, Vegard Nossum wrote:
> BTW, this seems to show up slightly more frequently although I've seen
> it a couple of times before without your patch too so it's probably
> unrelated:
> 
> WARNING: CPU: 0 PID: 1244 at arch/x86/kernel/hw_breakpoint.c:121
> arch_install_hw_breakpoint+0x284/0x2f0
> Can't find any breakpoint slot

are you using perf_fuzzer to find these bugs or some other tool?

The "Can't find any breakpoint slot" warning dates back years, and there 
have been a few attempts to fix it but it's never gone away.

Vince

[toc] | [prev] | [next] | [standalone]

#1458428

FromVegard Nossum <vegard.nossum@gmail.com>
Date2016-08-09 08:20 +0200
Message-ID<s48yl-3Q0-7@gated-at.bofh.it>
In reply to#1458209
On 8 August 2016 at 22:07, Vince Weaver <vincent.weaver@maine.edu> wrote:
> On Thu, 4 Aug 2016, Vegard Nossum wrote:
>> BTW, this seems to show up slightly more frequently although I've seen
>> it a couple of times before without your patch too so it's probably
>> unrelated:
>>
>> WARNING: CPU: 0 PID: 1244 at arch/x86/kernel/hw_breakpoint.c:121
>> arch_install_hw_breakpoint+0x284/0x2f0
>> Can't find any breakpoint slot
>
> are you using perf_fuzzer to find these bugs or some other tool?

Just using trinity + fault injection, although passing -c
perf_event_open hits it a lot faster.

> The "Can't find any breakpoint slot" warning dates back years, and there
> have been a few attempts to fix it but it's never gone away.

I'll just comment it out for now.

Thanks,


Vegard

[toc] | [prev] | [next] | [standalone]

#1459727 — [tip:perf/core] perf/core: Fix sideband list-iteration vs. event ordering NULL pointer deference crash

Fromtip-bot for Peter Zijlstra <tipbot@zytor.com>
Date2016-08-10 21:50 +0200
Subject[tip:perf/core] perf/core: Fix sideband list-iteration vs. event ordering NULL pointer deference crash
Message-ID<s4HFN-12V-49@gated-at.bofh.it>
In reply to#1456382
Commit-ID:  0b8f1e2e26bfc6b9abe3f0f3faba2cb0eecb9fb9
Gitweb:     http://git.kernel.org/tip/0b8f1e2e26bfc6b9abe3f0f3faba2cb0eecb9fb9
Author:     Peter Zijlstra <peterz@infradead.org>
AuthorDate: Thu, 4 Aug 2016 14:37:24 +0200
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Wed, 10 Aug 2016 13:05:51 +0200

perf/core: Fix sideband list-iteration vs. event ordering NULL pointer deference crash

Vegard Nossum reported that perf fuzzing generates a NULL
pointer dereference crash:

> Digging a bit deeper into this, it seems the event itself is getting
> created by perf_event_open() and it gets added to the pmu_event_list
> through:
>
> perf_event_open()
>  - perf_event_alloc()
>     - account_event()
>        - account_pmu_sb_event()
>           - attach_sb_event()
>
> so at this point the event is being attached but its ->ctx is still
> NULL. It seems like ->ctx is set just a bit later in
> perf_event_open(), though.
>
> But before that, __schedule() comes along and creates a stack trace
> similar to the one above:
>
> __schedule()
>  - __perf_event_task_sched_out()
>    - perf_iterate_sb()
>      - perf_iterate_sb_cpu()
>         - event_filter_match()
>           - perf_cgroup_match()
>             - __get_cpu_context()
>               - (dereference ctx which is NULL)
>
> So I guess the question is... should the event be attached (= put on
> the list) before ->ctx gets set? Or should the cgroup code check for a
> NULL ->ctx?

The latter seems like the simplest solution. Moving the list-add later
creates a bit of a mess.

Reported-by: Vegard Nossum <vegard.nossum@gmail.com>
Tested-by: Vegard Nossum <vegard.nossum@gmail.com>
Tested-by: Vince Weaver <vincent.weaver@maine.edu>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: David Carrillo-Cisneros <davidcc@google.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Kan Liang <kan.liang@intel.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Stephane Eranian <eranian@google.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Fixes: f2fb6bef9251 ("perf/core: Optimize side-band event delivery")
Link: http://lkml.kernel.org/r/20160804123724.GN6862@twins.programming.kicks-ass.net
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 kernel/events/core.c | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index a19550d..87d02b8 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -1716,8 +1716,8 @@ static inline int pmu_filter_match(struct perf_event *event)
 static inline int
 event_filter_match(struct perf_event *event)
 {
-	return (event->cpu == -1 || event->cpu == smp_processor_id())
-	    && perf_cgroup_match(event) && pmu_filter_match(event);
+	return (event->cpu == -1 || event->cpu == smp_processor_id()) &&
+	       perf_cgroup_match(event) && pmu_filter_match(event);
 }
 
 static void
@@ -1737,8 +1737,8 @@ event_sched_out(struct perf_event *event,
 	 * maintained, otherwise bogus information is return
 	 * via read() for time_enabled, time_running:
 	 */
-	if (event->state == PERF_EVENT_STATE_INACTIVE
-	    && !event_filter_match(event)) {
+	if (event->state == PERF_EVENT_STATE_INACTIVE &&
+	    !event_filter_match(event)) {
 		delta = tstamp - event->tstamp_stopped;
 		event->tstamp_running += delta;
 		event->tstamp_stopped = tstamp;
@@ -2236,10 +2236,15 @@ perf_install_in_context(struct perf_event_context *ctx,
 
 	lockdep_assert_held(&ctx->mutex);
 
-	event->ctx = ctx;
 	if (event->cpu != -1)
 		event->cpu = cpu;
 
+	/*
+	 * Ensures that if we can observe event->ctx, both the event and ctx
+	 * will be 'complete'. See perf_iterate_sb_cpu().
+	 */
+	smp_store_release(&event->ctx, ctx);
+
 	if (!task) {
 		cpu_function_call(cpu, __perf_install_in_context, event);
 		return;
@@ -5969,6 +5974,14 @@ static void perf_iterate_sb_cpu(perf_iterate_f output, void *data)
 	struct perf_event *event;
 
 	list_for_each_entry_rcu(event, &pel->list, sb_list) {
+		/*
+		 * Skip events that are not fully formed yet; ensure that
+		 * if we observe event->ctx, both event and ctx will be
+		 * complete enough. See perf_install_in_context().
+		 */
+		if (!smp_load_acquire(&event->ctx))
+			continue;
+
 		if (event->state < PERF_EVENT_STATE_INACTIVE)
 			continue;
 		if (!event_filter_match(event))

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web