NULL ptr deref in perf/filter_match

NULL ptr deref in perf/filter_match

[Vegard Nossum]

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

Re: NULL ptr deref in perf/filter_match

[Vegard Nossum]

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

Re: NULL ptr deref in perf/filter_match

[Vegard Nossum]

[-- Attachment #1: Type: text/plain, Size: 1926 bytes --]

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

[-- Attachment #2: 0001-Revert-perf-core-Rename-the-perf_event_aux-APIs-to-p.patch --]
[-- Type: text/x-patch, Size: 8846 bytes --]

From 18abcfa13157ae840e425da21047c734442bee57 Mon Sep 17 00:00:00 2001
From: Vegard Nossum <vegard.nossum@oracle.com>
Date: Fri, 29 Jul 2016 14:32:44 -0700
Subject: [PATCH] Revert "perf/core: Rename the perf_event_aux*() APIs to
 perf_event_sb*(), to separate them from AUX ring-buffer records" and
 "perf/core: Optimize side-band event delivery"

This reverts commit aab5b71ef2b5c62323b9abe397e2db57b18e1f78.
This reverts commit f2fb6bef92514432398a653df1c2f1041d79ac46.
---
 include/linux/perf_event.h |   6 --
 kernel/events/core.c       | 144 ++++++++++++---------------------------------
 2 files changed, 39 insertions(+), 111 deletions(-)

diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index e1f921c..1f37113 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -530,11 +530,6 @@ struct swevent_hlist {
 struct perf_cgroup;
 struct ring_buffer;
 
-struct pmu_event_list {
-	raw_spinlock_t		lock;
-	struct list_head	list;
-};
-
 /**
  * struct perf_event - performance event kernel representation:
  */
@@ -693,7 +688,6 @@ struct perf_event {
 	int				cgrp_defer_enabled;
 #endif
 
-	struct list_head		sb_list;
 #endif /* CONFIG_PERF_EVENTS */
 };
 
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 09ae27b..54baf9b 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -335,7 +335,6 @@ static atomic_t perf_sched_count;
 
 static DEFINE_PER_CPU(atomic_t, perf_cgroup_events);
 static DEFINE_PER_CPU(int, perf_sched_cb_usages);
-static DEFINE_PER_CPU(struct pmu_event_list, pmu_sb_events);
 
 static atomic_t nr_mmap_events __read_mostly;
 static atomic_t nr_comm_events __read_mostly;
@@ -3694,39 +3693,6 @@ static void free_event_rcu(struct rcu_head *head)
 static void ring_buffer_attach(struct perf_event *event,
 			       struct ring_buffer *rb);
 
-static void detach_sb_event(struct perf_event *event)
-{
-	struct pmu_event_list *pel = per_cpu_ptr(&pmu_sb_events, event->cpu);
-
-	raw_spin_lock(&pel->lock);
-	list_del_rcu(&event->sb_list);
-	raw_spin_unlock(&pel->lock);
-}
-
-static bool is_sb_event(struct perf_event *event)
-{
-	struct perf_event_attr *attr = &event->attr;
-
-	if (event->parent)
-		return false;
-
-	if (event->attach_state & PERF_ATTACH_TASK)
-		return false;
-
-	if (attr->mmap || attr->mmap_data || attr->mmap2 ||
-	    attr->comm || attr->comm_exec ||
-	    attr->task ||
-	    attr->context_switch)
-		return true;
-	return false;
-}
-
-static void unaccount_pmu_sb_event(struct perf_event *event)
-{
-	if (is_sb_event(event))
-		detach_sb_event(event);
-}
-
 static void unaccount_event_cpu(struct perf_event *event, int cpu)
 {
 	if (event->parent)
@@ -3790,8 +3756,6 @@ static void unaccount_event(struct perf_event *event)
 	}
 
 	unaccount_event_cpu(event, event->cpu);
-
-	unaccount_pmu_sb_event(event);
 }
 
 static void perf_sched_delayed(struct work_struct *work)
@@ -5942,11 +5906,11 @@ perf_event_read_event(struct perf_event *event,
 	perf_output_end(&handle);
 }
 
-typedef void (perf_iterate_f)(struct perf_event *event, void *data);
+typedef void (perf_event_aux_output_cb)(struct perf_event *event, void *data);
 
 static void
-perf_iterate_ctx(struct perf_event_context *ctx,
-		   perf_iterate_f output,
+perf_event_aux_ctx(struct perf_event_context *ctx,
+		   perf_event_aux_output_cb output,
 		   void *data, bool all)
 {
 	struct perf_event *event;
@@ -5963,55 +5927,52 @@ perf_iterate_ctx(struct perf_event_context *ctx,
 	}
 }
 
-static void perf_iterate_sb_cpu(perf_iterate_f output, void *data)
+static void
+perf_event_aux_task_ctx(perf_event_aux_output_cb output, void *data,
+			struct perf_event_context *task_ctx)
 {
-	struct pmu_event_list *pel = this_cpu_ptr(&pmu_sb_events);
-	struct perf_event *event;
-
-	list_for_each_entry_rcu(event, &pel->list, sb_list) {
-		if (event->state < PERF_EVENT_STATE_INACTIVE)
-			continue;
-		if (!event_filter_match(event))
-			continue;
-		output(event, data);
-	}
+	rcu_read_lock();
+	preempt_disable();
+	perf_event_aux_ctx(task_ctx, output, data, false);
+	preempt_enable();
+	rcu_read_unlock();
 }
 
-/*
- * Iterate all events that need to receive side-band events.
- *
- * For new callers; ensure that account_pmu_sb_event() includes
- * your event, otherwise it might not get delivered.
- */
 static void
-perf_iterate_sb(perf_iterate_f output, void *data,
+perf_event_aux(perf_event_aux_output_cb output, void *data,
 	       struct perf_event_context *task_ctx)
 {
+	struct perf_cpu_context *cpuctx;
 	struct perf_event_context *ctx;
+	struct pmu *pmu;
 	int ctxn;
 
-	rcu_read_lock();
-	preempt_disable();
-
 	/*
-	 * If we have task_ctx != NULL we only notify the task context itself.
-	 * The task_ctx is set only for EXIT events before releasing task
+	 * If we have task_ctx != NULL we only notify
+	 * the task context itself. The task_ctx is set
+	 * only for EXIT events before releasing task
 	 * context.
 	 */
 	if (task_ctx) {
-		perf_iterate_ctx(task_ctx, output, data, false);
-		goto done;
+		perf_event_aux_task_ctx(output, data, task_ctx);
+		return;
 	}
 
-	perf_iterate_sb_cpu(output, data);
-
-	for_each_task_context_nr(ctxn) {
+	rcu_read_lock();
+	list_for_each_entry_rcu(pmu, &pmus, entry) {
+		cpuctx = get_cpu_ptr(pmu->pmu_cpu_context);
+		if (cpuctx->unique_pmu != pmu)
+			goto next;
+		perf_event_aux_ctx(&cpuctx->ctx, output, data, false);
+		ctxn = pmu->task_ctx_nr;
+		if (ctxn < 0)
+			goto next;
 		ctx = rcu_dereference(current->perf_event_ctxp[ctxn]);
 		if (ctx)
-			perf_iterate_ctx(ctx, output, data, false);
+			perf_event_aux_ctx(ctx, output, data, false);
+next:
+		put_cpu_ptr(pmu->pmu_cpu_context);
 	}
-done:
-	preempt_enable();
 	rcu_read_unlock();
 }
 
@@ -6060,7 +6021,7 @@ void perf_event_exec(void)
 
 		perf_event_enable_on_exec(ctxn);
 
-		perf_iterate_ctx(ctx, perf_event_addr_filters_exec, NULL,
+		perf_event_aux_ctx(ctx, perf_event_addr_filters_exec, NULL,
 				   true);
 	}
 	rcu_read_unlock();
@@ -6104,9 +6065,9 @@ static int __perf_pmu_output_stop(void *info)
 	};
 
 	rcu_read_lock();
-	perf_iterate_ctx(&cpuctx->ctx, __perf_event_output_stop, &ro, false);
+	perf_event_aux_ctx(&cpuctx->ctx, __perf_event_output_stop, &ro, false);
 	if (cpuctx->task_ctx)
-		perf_iterate_ctx(cpuctx->task_ctx, __perf_event_output_stop,
+		perf_event_aux_ctx(cpuctx->task_ctx, __perf_event_output_stop,
 				   &ro, false);
 	rcu_read_unlock();
 
@@ -6235,7 +6196,7 @@ static void perf_event_task(struct task_struct *task,
 		},
 	};
 
-	perf_iterate_sb(perf_event_task_output,
+	perf_event_aux(perf_event_task_output,
 		       &task_event,
 		       task_ctx);
 }
@@ -6314,7 +6275,7 @@ static void perf_event_comm_event(struct perf_comm_event *comm_event)
 
 	comm_event->event_id.header.size = sizeof(comm_event->event_id) + size;
 
-	perf_iterate_sb(perf_event_comm_output,
+	perf_event_aux(perf_event_comm_output,
 		       comm_event,
 		       NULL);
 }
@@ -6545,7 +6506,7 @@ got_name:
 
 	mmap_event->event_id.header.size = sizeof(mmap_event->event_id) + size;
 
-	perf_iterate_sb(perf_event_mmap_output,
+	perf_event_aux(perf_event_mmap_output,
 		       mmap_event,
 		       NULL);
 
@@ -6628,7 +6589,7 @@ static void perf_addr_filters_adjust(struct vm_area_struct *vma)
 		if (!ctx)
 			continue;
 
-		perf_iterate_ctx(ctx, __perf_addr_filters_adjust, vma, true);
+		perf_event_aux_ctx(ctx, __perf_addr_filters_adjust, vma, true);
 	}
 	rcu_read_unlock();
 }
@@ -6815,7 +6776,7 @@ static void perf_event_switch(struct task_struct *task,
 		},
 	};
 
-	perf_iterate_sb(perf_event_switch_output,
+	perf_event_aux(perf_event_switch_output,
 		       &switch_event,
 		       NULL);
 }
@@ -8739,28 +8700,6 @@ unlock:
 	return pmu;
 }
 
-static void attach_sb_event(struct perf_event *event)
-{
-	struct pmu_event_list *pel = per_cpu_ptr(&pmu_sb_events, event->cpu);
-
-	raw_spin_lock(&pel->lock);
-	list_add_rcu(&event->sb_list, &pel->list);
-	raw_spin_unlock(&pel->lock);
-}
-
-/*
- * We keep a list of all !task (and therefore per-cpu) events
- * that need to receive side-band records.
- *
- * This avoids having to scan all the various PMU per-cpu contexts
- * looking for them.
- */
-static void account_pmu_sb_event(struct perf_event *event)
-{
-	if (is_sb_event(event))
-		attach_sb_event(event);
-}
-
 static void account_event_cpu(struct perf_event *event, int cpu)
 {
 	if (event->parent)
@@ -8841,8 +8780,6 @@ static void account_event(struct perf_event *event)
 enabled:
 
 	account_event_cpu(event, event->cpu);
-
-	account_pmu_sb_event(event);
 }
 
 /*
@@ -10351,9 +10288,6 @@ static void __init perf_event_init_all_cpus(void)
 		swhash = &per_cpu(swevent_htable, cpu);
 		mutex_init(&swhash->hlist_mutex);
 		INIT_LIST_HEAD(&per_cpu(active_ctx_list, cpu));
-
-		INIT_LIST_HEAD(&per_cpu(pmu_sb_events.list, cpu));
-		raw_spin_lock_init(&per_cpu(pmu_sb_events.lock, cpu));
 	}
 }
 
-- 
2.7.4

Re: NULL ptr deref in perf/filter_match

[Peter Zijlstra]

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))

Re: NULL ptr deref in perf/filter_match

[Vegard Nossum]

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

Re: NULL ptr deref in perf/filter_match

[Vegard Nossum]

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

Re: NULL ptr deref in perf/filter_match

[Peter Zijlstra]

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 ]---

Re: NULL ptr deref in perf/filter_match

[Vince Weaver]

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

Re: NULL ptr deref in perf/filter_match

[Vegard Nossum]

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

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

[tip-bot for Peter Zijlstra]

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))