From: Peter Zijlstra <[email protected]>
Make perf_event_exit_event() more robust, such that we can use it from
other contexts. Specifically the up and coming remove_on_exec.
For this to work we need to address a few issues. Remove_on_exec will
not destroy the entire context, so we cannot rely on TASK_TOMBSTONE to
disable event_function_call() and we thus have to use
perf_remove_from_context().
When using perf_remove_from_context(), there's two races to consider.
The first is against close(), where we can have concurrent tear-down
of the event. The second is against child_list iteration, which should
not find a half baked event.
To address this, teach perf_remove_from_context() to special case
!ctx->is_active and about DETACH_CHILD.
Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
Signed-off-by: Marco Elver <[email protected]>
---
v3:
* New dependency for series:
https://lkml.kernel.org/r/YFn/[email protected]
---
include/linux/perf_event.h | 1 +
kernel/events/core.c | 142 +++++++++++++++++++++----------------
2 files changed, 80 insertions(+), 63 deletions(-)
diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index 3f7f89ea5e51..3d478abf411c 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -607,6 +607,7 @@ struct swevent_hlist {
#define PERF_ATTACH_TASK_DATA 0x08
#define PERF_ATTACH_ITRACE 0x10
#define PERF_ATTACH_SCHED_CB 0x20
+#define PERF_ATTACH_CHILD 0x40
struct perf_cgroup;
struct perf_buffer;
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 03db40f6cba9..57de8d436efd 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -2204,6 +2204,26 @@ static void perf_group_detach(struct perf_event *event)
perf_event__header_size(leader);
}
+static void sync_child_event(struct perf_event *child_event);
+
+static void perf_child_detach(struct perf_event *event)
+{
+ struct perf_event *parent_event = event->parent;
+
+ if (!(event->attach_state & PERF_ATTACH_CHILD))
+ return;
+
+ event->attach_state &= ~PERF_ATTACH_CHILD;
+
+ if (WARN_ON_ONCE(!parent_event))
+ return;
+
+ lockdep_assert_held(&parent_event->child_mutex);
+
+ sync_child_event(event);
+ list_del_init(&event->child_list);
+}
+
static bool is_orphaned_event(struct perf_event *event)
{
return event->state == PERF_EVENT_STATE_DEAD;
@@ -2311,6 +2331,7 @@ group_sched_out(struct perf_event *group_event,
}
#define DETACH_GROUP 0x01UL
+#define DETACH_CHILD 0x02UL
/*
* Cross CPU call to remove a performance event
@@ -2334,6 +2355,8 @@ __perf_remove_from_context(struct perf_event *event,
event_sched_out(event, cpuctx, ctx);
if (flags & DETACH_GROUP)
perf_group_detach(event);
+ if (flags & DETACH_CHILD)
+ perf_child_detach(event);
list_del_event(event, ctx);
if (!ctx->nr_events && ctx->is_active) {
@@ -2362,25 +2385,21 @@ static void perf_remove_from_context(struct perf_event *event, unsigned long fla
lockdep_assert_held(&ctx->mutex);
- event_function_call(event, __perf_remove_from_context, (void *)flags);
-
/*
- * The above event_function_call() can NO-OP when it hits
- * TASK_TOMBSTONE. In that case we must already have been detached
- * from the context (by perf_event_exit_event()) but the grouping
- * might still be in-tact.
+ * Because of perf_event_exit_task(), perf_remove_from_context() ought
+ * to work in the face of TASK_TOMBSTONE, unlike every other
+ * event_function_call() user.
*/
- WARN_ON_ONCE(event->attach_state & PERF_ATTACH_CONTEXT);
- if ((flags & DETACH_GROUP) &&
- (event->attach_state & PERF_ATTACH_GROUP)) {
- /*
- * Since in that case we cannot possibly be scheduled, simply
- * detach now.
- */
- raw_spin_lock_irq(&ctx->lock);
- perf_group_detach(event);
+ raw_spin_lock_irq(&ctx->lock);
+ if (!ctx->is_active) {
+ __perf_remove_from_context(event, __get_cpu_context(ctx),
+ ctx, (void *)flags);
raw_spin_unlock_irq(&ctx->lock);
+ return;
}
+ raw_spin_unlock_irq(&ctx->lock);
+
+ event_function_call(event, __perf_remove_from_context, (void *)flags);
}
/*
@@ -12373,14 +12392,17 @@ void perf_pmu_migrate_context(struct pmu *pmu, int src_cpu, int dst_cpu)
}
EXPORT_SYMBOL_GPL(perf_pmu_migrate_context);
-static void sync_child_event(struct perf_event *child_event,
- struct task_struct *child)
+static void sync_child_event(struct perf_event *child_event)
{
struct perf_event *parent_event = child_event->parent;
u64 child_val;
- if (child_event->attr.inherit_stat)
- perf_event_read_event(child_event, child);
+ if (child_event->attr.inherit_stat) {
+ struct task_struct *task = child_event->ctx->task;
+
+ if (task)
+ perf_event_read_event(child_event, task);
+ }
child_val = perf_event_count(child_event);
@@ -12395,60 +12417,53 @@ static void sync_child_event(struct perf_event *child_event,
}
static void
-perf_event_exit_event(struct perf_event *child_event,
- struct perf_event_context *child_ctx,
- struct task_struct *child)
+perf_event_exit_event(struct perf_event *event, struct perf_event_context *ctx)
{
- struct perf_event *parent_event = child_event->parent;
+ struct perf_event *parent_event = event->parent;
+ unsigned long detach_flags = 0;
- /*
- * Do not destroy the 'original' grouping; because of the context
- * switch optimization the original events could've ended up in a
- * random child task.
- *
- * If we were to destroy the original group, all group related
- * operations would cease to function properly after this random
- * child dies.
- *
- * Do destroy all inherited groups, we don't care about those
- * and being thorough is better.
- */
- raw_spin_lock_irq(&child_ctx->lock);
- WARN_ON_ONCE(child_ctx->is_active);
+ if (parent_event) {
+ /*
+ * Do not destroy the 'original' grouping; because of the
+ * context switch optimization the original events could've
+ * ended up in a random child task.
+ *
+ * If we were to destroy the original group, all group related
+ * operations would cease to function properly after this
+ * random child dies.
+ *
+ * Do destroy all inherited groups, we don't care about those
+ * and being thorough is better.
+ */
+ detach_flags = DETACH_GROUP | DETACH_CHILD;
+ mutex_lock(&parent_event->child_mutex);
+ }
- if (parent_event)
- perf_group_detach(child_event);
- list_del_event(child_event, child_ctx);
- perf_event_set_state(child_event, PERF_EVENT_STATE_EXIT); /* is_event_hup() */
- raw_spin_unlock_irq(&child_ctx->lock);
+ perf_remove_from_context(event, detach_flags);
+
+ raw_spin_lock_irq(&ctx->lock);
+ if (event->state > PERF_EVENT_STATE_EXIT)
+ perf_event_set_state(event, PERF_EVENT_STATE_EXIT);
+ raw_spin_unlock_irq(&ctx->lock);
/*
- * Parent events are governed by their filedesc, retain them.
+ * Child events can be freed.
*/
- if (!parent_event) {
- perf_event_wakeup(child_event);
+ if (parent_event) {
+ mutex_unlock(&parent_event->child_mutex);
+ /*
+ * Kick perf_poll() for is_event_hup();
+ */
+ perf_event_wakeup(parent_event);
+ free_event(event);
+ put_event(parent_event);
return;
}
- /*
- * Child events can be cleaned up.
- */
-
- sync_child_event(child_event, child);
/*
- * Remove this event from the parent's list
- */
- WARN_ON_ONCE(parent_event->ctx->parent_ctx);
- mutex_lock(&parent_event->child_mutex);
- list_del_init(&child_event->child_list);
- mutex_unlock(&parent_event->child_mutex);
-
- /*
- * Kick perf_poll() for is_event_hup().
+ * Parent events are governed by their filedesc, retain them.
*/
- perf_event_wakeup(parent_event);
- free_event(child_event);
- put_event(parent_event);
+ perf_event_wakeup(event);
}
static void perf_event_exit_task_context(struct task_struct *child, int ctxn)
@@ -12505,7 +12520,7 @@ static void perf_event_exit_task_context(struct task_struct *child, int ctxn)
perf_event_task(child, child_ctx, 0);
list_for_each_entry_safe(child_event, next, &child_ctx->event_list, event_entry)
- perf_event_exit_event(child_event, child_ctx, child);
+ perf_event_exit_event(child_event, child_ctx);
mutex_unlock(&child_ctx->mutex);
@@ -12765,6 +12780,7 @@ inherit_event(struct perf_event *parent_event,
*/
raw_spin_lock_irqsave(&child_ctx->lock, flags);
add_event_to_ctx(child_event, child_ctx);
+ child_event->attach_state |= PERF_ATTACH_CHILD;
raw_spin_unlock_irqrestore(&child_ctx->lock, flags);
/*
--
2.31.0.291.g576ba9dcdaf-goog
On Wed, Mar 24, 2021 at 12:24PM +0100, Marco Elver wrote:
> From: Peter Zijlstra <[email protected]>
>
> Make perf_event_exit_event() more robust, such that we can use it from
> other contexts. Specifically the up and coming remove_on_exec.
>
> For this to work we need to address a few issues. Remove_on_exec will
> not destroy the entire context, so we cannot rely on TASK_TOMBSTONE to
> disable event_function_call() and we thus have to use
> perf_remove_from_context().
>
> When using perf_remove_from_context(), there's two races to consider.
> The first is against close(), where we can have concurrent tear-down
> of the event. The second is against child_list iteration, which should
> not find a half baked event.
>
> To address this, teach perf_remove_from_context() to special case
> !ctx->is_active and about DETACH_CHILD.
>
> Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
> Signed-off-by: Marco Elver <[email protected]>
> ---
> v3:
> * New dependency for series:
> https://lkml.kernel.org/r/YFn/[email protected]
> ---
syzkaller found a crash with stack trace pointing at changes in this
patch. Can't tell if this is an old issue or introduced in this series.
It looks like task_pid_ptr() wants to access task_struct::signal, but
the task_struct pointer is NULL.
Any ideas?
general protection fault, probably for non-canonical address 0xdffffc0000000103: 0000 [#1] PREEMPT SMP KASAN
KASAN: null-ptr-deref in range [0x0000000000000818-0x000000000000081f]
CPU: 2 PID: 15084 Comm: syz-executor.1 Not tainted 5.12.0-rc4+ #5
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.14.0-2 04/01/2014
RIP: 0010:task_pid_ptr kernel/pid.c:325 [inline]
RIP: 0010:__task_pid_nr_ns+0x137/0x3e0 kernel/pid.c:500
Code: 8b 75 00 eb 08 e8 59 28 29 00 45 31 f6 31 ff 44 89 fe e8 5c 2c 29 00 45 85 ff 74 49 48 81 c3 20 08 00 00 48 89 d8 48 c1 e8 03 <42> 80 3c 20 00 74 08 48 89 df e8 aa 03 6d 00 48 8b 2b 44 89 fb bf
RSP: 0018:ffffc9000c76f6d0 EFLAGS: 00010007
RAX: 0000000000000103 RBX: 000000000000081f RCX: ffff8880717d8000
RDX: ffff8880717d8000 RSI: 0000000000000001 RDI: 0000000000000000
RBP: 0000000000000001 R08: ffffffff814fe814 R09: fffffbfff1f296b1
R10: fffffbfff1f296b1 R11: 0000000000000000 R12: dffffc0000000000
R13: 1ffff1100e6dfc5c R14: ffff888057fba108 R15: 0000000000000001
FS: 0000000000000000(0000) GS:ffff88802cf00000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007ffcc3b05bc0 CR3: 0000000040ac0000 CR4: 0000000000750ee0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000600
PKRU: 55555554
Call Trace:
perf_event_pid_type kernel/events/core.c:1412 [inline]
perf_event_pid kernel/events/core.c:1421 [inline]
perf_event_read_event kernel/events/core.c:7511 [inline]
sync_child_event kernel/events/core.c:12521 [inline]
perf_child_detach kernel/events/core.c:2223 [inline]
__perf_remove_from_context+0x569/0xd30 kernel/events/core.c:2359
perf_remove_from_context+0x19d/0x220 kernel/events/core.c:2395
perf_event_exit_event+0x76/0x950 kernel/events/core.c:12559
perf_event_exit_task_context kernel/events/core.c:12640 [inline]
perf_event_exit_task+0x715/0xa40 kernel/events/core.c:12673
do_exit+0x6c2/0x2290 kernel/exit.c:834
do_group_exit+0x168/0x2d0 kernel/exit.c:922
get_signal+0x1734/0x1ef0 kernel/signal.c:2779
arch_do_signal_or_restart+0x41/0x620 arch/x86/kernel/signal.c:789
handle_signal_work kernel/entry/common.c:147 [inline]
exit_to_user_mode_loop kernel/entry/common.c:171 [inline]
exit_to_user_mode_prepare+0xac/0x1e0 kernel/entry/common.c:208
irqentry_exit_to_user_mode+0x6/0x40 kernel/entry/common.c:314
exc_general_protection+0x222/0x370 arch/x86/kernel/traps.c:530
asm_exc_general_protection+0x1e/0x30 arch/x86/include/asm/idtentry.h:571
On Thu, Mar 25, 2021 at 11:17AM +0100, Marco Elver wrote:
> On Wed, Mar 24, 2021 at 12:24PM +0100, Marco Elver wrote:
> > From: Peter Zijlstra <[email protected]>
> >
> > Make perf_event_exit_event() more robust, such that we can use it from
> > other contexts. Specifically the up and coming remove_on_exec.
> >
> > For this to work we need to address a few issues. Remove_on_exec will
> > not destroy the entire context, so we cannot rely on TASK_TOMBSTONE to
> > disable event_function_call() and we thus have to use
> > perf_remove_from_context().
> >
> > When using perf_remove_from_context(), there's two races to consider.
> > The first is against close(), where we can have concurrent tear-down
> > of the event. The second is against child_list iteration, which should
> > not find a half baked event.
> >
> > To address this, teach perf_remove_from_context() to special case
> > !ctx->is_active and about DETACH_CHILD.
> >
> > Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
> > Signed-off-by: Marco Elver <[email protected]>
> > ---
> > v3:
> > * New dependency for series:
> > https://lkml.kernel.org/r/YFn/[email protected]
> > ---
>
> syzkaller found a crash with stack trace pointing at changes in this
> patch. Can't tell if this is an old issue or introduced in this series.
Yay, I found a reproducer. v5.12-rc4 is good, and sadly with this patch only we
crash. :-/
Here's a stacktrace with just this patch applied:
| BUG: kernel NULL pointer dereference, address: 00000000000007af
| #PF: supervisor read access in kernel mode
| #PF: error_code(0x0000) - not-present page
| PGD 0 P4D 0
| Oops: 0000 [#1] PREEMPT SMP PTI
| CPU: 7 PID: 465 Comm: a.out Not tainted 5.12.0-rc4+ #25
| Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.14.0-2 04/01/2014
| RIP: 0010:task_pid_ptr kernel/pid.c:324 [inline]
| RIP: 0010:__task_pid_nr_ns+0x112/0x240 kernel/pid.c:500
| Code: e8 13 55 07 00 e8 1e a6 0e 00 48 c7 c6 83 1e 0b 81 48 c7 c7 a0 2e d5 82 e8 4b 08 04 00 44 89 e0 5b 5d 41 5c c3 e8 fe a5 0e 00 <48> 8b 85 b0 07 00 00 4a 8d ac e0 98 01 00 00 e9 5a ff ff ff e8 e5
| RSP: 0000:ffffc90001b73a60 EFLAGS: 00010093
| RAX: 0000000000000000 RBX: ffffffff82c69820 RCX: ffffffff810b1eb2
| RDX: ffff888108d143c0 RSI: 0000000000000000 RDI: ffffffff8299ccc6
| RBP: ffffffffffffffff R08: 0000000000000001 R09: 0000000000000000
| R10: ffff888108d14db8 R11: 0000000000000000 R12: 0000000000000001
| R13: ffffffffffffffff R14: ffffffffffffffff R15: ffff888108e05240
| FS: 0000000000000000(0000) GS:ffff88842fdc0000(0000) knlGS:0000000000000000
| CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
| CR2: 00000000000007af CR3: 0000000002c22002 CR4: 0000000000770ee0
| DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
| DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
| PKRU: 55555554
| Call Trace:
| perf_event_pid_type kernel/events/core.c:1412 [inline]
| perf_event_pid kernel/events/core.c:1421 [inline]
| perf_event_read_event+0x78/0x1d0 kernel/events/core.c:7406
| sync_child_event kernel/events/core.c:12404 [inline]
| perf_child_detach kernel/events/core.c:2223 [inline]
| __perf_remove_from_context+0x14d/0x280 kernel/events/core.c:2359
| perf_remove_from_context+0x9f/0xf0 kernel/events/core.c:2395
| perf_event_exit_event kernel/events/core.c:12442 [inline]
| perf_event_exit_task_context kernel/events/core.c:12523 [inline]
| perf_event_exit_task+0x276/0x4c0 kernel/events/core.c:12556
| do_exit+0x4cd/0xed0 kernel/exit.c:834
| do_group_exit+0x4d/0xf0 kernel/exit.c:922
| get_signal+0x1d2/0xf30 kernel/signal.c:2777
| arch_do_signal_or_restart+0xf7/0x750 arch/x86/kernel/signal.c:789
| handle_signal_work kernel/entry/common.c:147 [inline]
| exit_to_user_mode_loop kernel/entry/common.c:171 [inline]
| exit_to_user_mode_prepare+0x113/0x190 kernel/entry/common.c:208
| irqentry_exit_to_user_mode+0x6/0x30 kernel/entry/common.c:314
| asm_exc_general_protection+0x1e/0x30 arch/x86/include/asm/idtentry.h:571
Attached is a C reproducer of the syzkaller program that crashes us.
Thanks,
-- Marco
On Thu, Mar 25, 2021 at 05:17PM +0100, Marco Elver wrote:
[...]
> > syzkaller found a crash with stack trace pointing at changes in this
> > patch. Can't tell if this is an old issue or introduced in this series.
>
> Yay, I found a reproducer. v5.12-rc4 is good, and sadly with this patch only we
> crash. :-/
>
> Here's a stacktrace with just this patch applied:
>
> | BUG: kernel NULL pointer dereference, address: 00000000000007af
[...]
> | RIP: 0010:task_pid_ptr kernel/pid.c:324 [inline]
> | RIP: 0010:__task_pid_nr_ns+0x112/0x240 kernel/pid.c:500
[...]
> | Call Trace:
> | perf_event_pid_type kernel/events/core.c:1412 [inline]
> | perf_event_pid kernel/events/core.c:1421 [inline]
> | perf_event_read_event+0x78/0x1d0 kernel/events/core.c:7406
> | sync_child_event kernel/events/core.c:12404 [inline]
> | perf_child_detach kernel/events/core.c:2223 [inline]
> | __perf_remove_from_context+0x14d/0x280 kernel/events/core.c:2359
> | perf_remove_from_context+0x9f/0xf0 kernel/events/core.c:2395
> | perf_event_exit_event kernel/events/core.c:12442 [inline]
> | perf_event_exit_task_context kernel/events/core.c:12523 [inline]
> | perf_event_exit_task+0x276/0x4c0 kernel/events/core.c:12556
> | do_exit+0x4cd/0xed0 kernel/exit.c:834
> | do_group_exit+0x4d/0xf0 kernel/exit.c:922
> | get_signal+0x1d2/0xf30 kernel/signal.c:2777
> | arch_do_signal_or_restart+0xf7/0x750 arch/x86/kernel/signal.c:789
> | handle_signal_work kernel/entry/common.c:147 [inline]
> | exit_to_user_mode_loop kernel/entry/common.c:171 [inline]
> | exit_to_user_mode_prepare+0x113/0x190 kernel/entry/common.c:208
> | irqentry_exit_to_user_mode+0x6/0x30 kernel/entry/common.c:314
> | asm_exc_general_protection+0x1e/0x30 arch/x86/include/asm/idtentry.h:571
I spun up gdb, and it showed me this:
| #0 perf_event_read_event (event=event@entry=0xffff888107cd5000, task=task@entry=0xffffffffffffffff)
| at kernel/events/core.c:7397
^^^ TASK_TOMBSTONE
| #1 0xffffffff811fc9cd in sync_child_event (child_event=0xffff888107cd5000) at kernel/events/core.c:12404
| #2 perf_child_detach (event=0xffff888107cd5000) at kernel/events/core.c:2223
| #3 __perf_remove_from_context (event=event@entry=0xffff888107cd5000, cpuctx=cpuctx@entry=0xffff88842fdf0c00,
| ctx=ctx@entry=0xffff8881073cb800, info=info@entry=0x3 <fixed_percpu_data+3>) at kernel/events/core.c:2359
| #4 0xffffffff811fcb9f in perf_remove_from_context (event=event@entry=0xffff888107cd5000, flags=flags@entry=3)
| at kernel/events/core.c:2395
| #5 0xffffffff81204526 in perf_event_exit_event (ctx=0xffff8881073cb800, event=0xffff888107cd5000)
| at kernel/events/core.c:12442
| #6 perf_event_exit_task_context (ctxn=0, child=0xffff88810531a200) at kernel/events/core.c:12523
| #7 perf_event_exit_task (child=0xffff88810531a200) at kernel/events/core.c:12556
| #8 0xffffffff8108838d in do_exit (code=code@entry=11) at kernel/exit.c:834
| #9 0xffffffff81088e4d in do_group_exit (exit_code=11) at kernel/exit.c:922
and therefore synthesized this fix on top:
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 57de8d436efd..e77294c7e654 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -12400,7 +12400,7 @@ static void sync_child_event(struct perf_event *child_event)
if (child_event->attr.inherit_stat) {
struct task_struct *task = child_event->ctx->task;
- if (task)
+ if (task && task != TASK_TOMBSTONE)
perf_event_read_event(child_event, task);
}
which fixes the problem. My guess is that the parent and child are both
racing to exit?
Does that make any sense?
Thanks,
-- Marco
On Thu, Mar 25, 2021 at 08:10:51PM +0100, Marco Elver wrote:
> and therefore synthesized this fix on top:
>
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 57de8d436efd..e77294c7e654 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -12400,7 +12400,7 @@ static void sync_child_event(struct perf_event *child_event)
> if (child_event->attr.inherit_stat) {
> struct task_struct *task = child_event->ctx->task;
>
> - if (task)
> + if (task && task != TASK_TOMBSTONE)
> perf_event_read_event(child_event, task);
> }
>
> which fixes the problem. My guess is that the parent and child are both
> racing to exit?
>
> Does that make any sense?
Yes, I think it does. ACK