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

Groups > linux.kernel > #1302985 > unrolled thread

[tip:perf/core] perf: Fix race in perf_event_exec()

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [tip:perf/core] perf: Fix race in perf_event_exec() tip-bot for Peter Zijlstra <tipbot@zytor.com> - 2016-01-06 19:50 +0100
    Re: [tip:perf/core] perf: Fix race in perf_event_exec() Eric Dumazet <edumazet@google.com> - 2016-01-06 20:00 +0100
      Re: [tip:perf/core] perf: Fix race in perf_event_exec() Peter Zijlstra <peterz@infradead.org> - 2016-01-07 14:50 +0100
        Re: [tip:perf/core] perf: Fix race in perf_event_exec() "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-01-07 17:30 +0100
          Re: [tip:perf/core] perf: Fix race in perf_event_exec() Eric Dumazet <edumazet@google.com> - 2016-01-07 17:40 +0100
            Re: [tip:perf/core] perf: Fix race in perf_event_exec() "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-01-07 17:50 +0100

#1302985 — [tip:perf/core] perf: Fix race in perf_event_exec()

Commit-ID:  c127449944659543e5e2423002f08f0af98dba5c
Gitweb:     http://git.kernel.org/tip/c127449944659543e5e2423002f08f0af98dba5c
Author:     Peter Zijlstra <peterz@infradead.org>
AuthorDate: Thu, 10 Dec 2015 20:57:40 +0100
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Wed, 6 Jan 2016 10:52:38 +0100

perf: Fix race in perf_event_exec()

I managed to tickle this warning:

  [ 2338.884942] ------------[ cut here ]------------
  [ 2338.890112] WARNING: CPU: 13 PID: 35162 at ../kernel/events/core.c:2702 task_ctx_sched_out+0x6b/0x80()
  [ 2338.900504] Modules linked in:
  [ 2338.903933] CPU: 13 PID: 35162 Comm: bash Not tainted 4.4.0-rc4-dirty #244
  [ 2338.911610] Hardware name: Intel Corporation S2600GZ/S2600GZ, BIOS SE5C600.86B.02.02.0002.122320131210 12/23/2013
  [ 2338.923071]  ffffffff81f1468e ffff8807c6457cb8 ffffffff815c680c 0000000000000000
  [ 2338.931382]  ffff8807c6457cf0 ffffffff810c8a56 ffffe8ffff8c1bd0 ffff8808132ed400
  [ 2338.939678]  0000000000000286 ffff880813170380 ffff8808132ed400 ffff8807c6457d00
  [ 2338.947987] Call Trace:
  [ 2338.950726]  [<ffffffff815c680c>] dump_stack+0x4e/0x82
  [ 2338.956474]  [<ffffffff810c8a56>] warn_slowpath_common+0x86/0xc0
  [ 2338.963195]  [<ffffffff810c8b4a>] warn_slowpath_null+0x1a/0x20
  [ 2338.969720]  [<ffffffff811a49cb>] task_ctx_sched_out+0x6b/0x80
  [ 2338.976244]  [<ffffffff811a62d2>] perf_event_exec+0xe2/0x180
  [ 2338.982575]  [<ffffffff8121fb6f>] setup_new_exec+0x6f/0x1b0
  [ 2338.988810]  [<ffffffff8126de83>] load_elf_binary+0x393/0x1660
  [ 2338.995339]  [<ffffffff811dc772>] ? get_user_pages+0x52/0x60
  [ 2339.001669]  [<ffffffff8121e297>] search_binary_handler+0x97/0x200
  [ 2339.008581]  [<ffffffff8121f8b3>] do_execveat_common.isra.33+0x543/0x6e0
  [ 2339.016072]  [<ffffffff8121fcea>] SyS_execve+0x3a/0x50
  [ 2339.021819]  [<ffffffff819fc165>] stub_execve+0x5/0x5
  [ 2339.027469]  [<ffffffff819fbeb2>] ? entry_SYSCALL_64_fastpath+0x12/0x71
  [ 2339.034860] ---[ end trace ee1337c59a0ddeac ]---

Which is a WARN_ON_ONCE() indicating that cpuctx->task_ctx is not
what we expected it to be.

This is because context switches can swap the task_struct::perf_event_ctxp[]
pointer around. Therefore you have to either disable preemption when looking
at current, or hold ctx->lock.

Fix perf_event_enable_on_exec(), it loads current->perf_event_ctxp[]
before disabling interrupts, therefore a preemption in the right place
can swap contexts around and we're using the wrong one.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Alexander Potapenko <glider@google.com>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Dmitry Vyukov <dvyukov@google.com>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Kostya Serebryany <kcc@google.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Sasha Levin <sasha.levin@oracle.com>
Cc: Stephane Eranian <eranian@google.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Vince Weaver <vincent.weaver@maine.edu>
Cc: syzkaller <syzkaller@googlegroups.com>
Link: http://lkml.kernel.org/r/20151210195740.GG6357@twins.programming.kicks-ass.net
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 kernel/events/core.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index 39cf4a4..fd7de04 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -3154,15 +3154,16 @@ static int event_enable_on_exec(struct perf_event *event,
  * Enable all of a task's events that have been marked enable-on-exec.
  * This expects task == current.
  */
-static void perf_event_enable_on_exec(struct perf_event_context *ctx)
+static void perf_event_enable_on_exec(int ctxn)
 {
-	struct perf_event_context *clone_ctx = NULL;
+	struct perf_event_context *ctx, *clone_ctx = NULL;
 	struct perf_event *event;
 	unsigned long flags;
 	int enabled = 0;
 	int ret;
 
 	local_irq_save(flags);
+	ctx = current->perf_event_ctxp[ctxn];
 	if (!ctx || !ctx->nr_events)
 		goto out;
 
@@ -3205,17 +3206,11 @@ out:
 
 void perf_event_exec(void)
 {
-	struct perf_event_context *ctx;
 	int ctxn;
 
 	rcu_read_lock();
-	for_each_task_context_nr(ctxn) {
-		ctx = current->perf_event_ctxp[ctxn];
-		if (!ctx)
-			continue;
-
-		perf_event_enable_on_exec(ctx);
-	}
+	for_each_task_context_nr(ctxn)
+		perf_event_enable_on_exec(ctxn);
 	rcu_read_unlock();
 }
 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1302990

On Wed, Jan 6, 2016 at 1:46 PM, tip-bot for Peter Zijlstra
<tipbot@zytor.com> wrote:

>
> This is because context switches can swap the task_struct::perf_event_ctxp[]
> pointer around. Therefore you have to either disable preemption when looking
> at current, or hold ctx->lock.
>


>
>  void perf_event_exec(void)
>  {
> -       struct perf_event_context *ctx;
>         int ctxn;
>
>         rcu_read_lock();

Do we still need this rcu_read_lock(), if perf_event_enable_on_exec()
uses  local_irq_save( ?

> -       for_each_task_context_nr(ctxn) {
> -               ctx = current->perf_event_ctxp[ctxn];
> -               if (!ctx)
> -                       continue;
> -
> -               perf_event_enable_on_exec(ctx);
> -       }
> +       for_each_task_context_nr(ctxn)
> +               perf_event_enable_on_exec(ctxn);
>         rcu_read_unlock();
>  }
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1303594

On Wed, Jan 06, 2016 at 01:56:56PM -0500, Eric Dumazet wrote:
> On Wed, Jan 6, 2016 at 1:46 PM, tip-bot for Peter Zijlstra
> <tipbot@zytor.com> wrote:
> 
> >
> > This is because context switches can swap the task_struct::perf_event_ctxp[]
> > pointer around. Therefore you have to either disable preemption when looking
> > at current, or hold ctx->lock.
> >
> 
> 
> >
> >  void perf_event_exec(void)
> >  {
> > -       struct perf_event_context *ctx;
> >         int ctxn;
> >
> >         rcu_read_lock();
> 
> Do we still need this rcu_read_lock(), if perf_event_enable_on_exec()
> uses  local_irq_save( ?

Strictly speaking we should not rely on the fact that RCU grace periods
do not progress with IRQs disabled, so yes.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1303722

On Thu, Jan 07, 2016 at 02:40:08PM +0100, Peter Zijlstra wrote:
> On Wed, Jan 06, 2016 at 01:56:56PM -0500, Eric Dumazet wrote:
> > On Wed, Jan 6, 2016 at 1:46 PM, tip-bot for Peter Zijlstra
> > <tipbot@zytor.com> wrote:
> > 
> > >
> > > This is because context switches can swap the task_struct::perf_event_ctxp[]
> > > pointer around. Therefore you have to either disable preemption when looking
> > > at current, or hold ctx->lock.
> > >
> > 
> > 
> > >
> > >  void perf_event_exec(void)
> > >  {
> > > -       struct perf_event_context *ctx;
> > >         int ctxn;
> > >
> > >         rcu_read_lock();
> > 
> > Do we still need this rcu_read_lock(), if perf_event_enable_on_exec()
> > uses  local_irq_save( ?
> 
> Strictly speaking we should not rely on the fact that RCU grace periods
> do not progress with IRQs disabled, so yes.

What Peter said!

The current implementation would let you get away with IRQs disabled
(aside from lockdep splats), but it is easy to imagine implementations
that do not, hence the restriction.

Is the extra rcu_read_lock() and rcu_read_unlock()  causing a performance
problem?  If so, one option would be use of synchronize_sched() and
call_rcu_sched(), which explicitly recognize disabled IRQs as read-side
critical sections.  Of course, this might well put you in a position
where you would like to use IRQ disabling in some cases, BH disabling
(or softirq context) in others, and rcu_read_lock() in yet other cases.

If so, the good news is that RCU actually now supports this efficiently.
There is the shiny new synchronize_rcu_mult() macro that can be used
as follows to wait for all three types of grace periods:

	synchronize_rcu_mult(call_rcu, call_rcu_bh, call_rcu_sched);

This would wait concurrently for all three grace periods to elapse.

							Thanx, Paul

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1303728

On Thu, Jan 7, 2016 at 11:26 AM, Paul E. McKenney
<paulmck@linux.vnet.ibm.com> wrote:
> On Thu, Jan 07, 2016 at 02:40:08PM +0100, Peter Zijlstra wrote:
>> On Wed, Jan 06, 2016 at 01:56:56PM -0500, Eric Dumazet wrote:
>> > On Wed, Jan 6, 2016 at 1:46 PM, tip-bot for Peter Zijlstra
>> > <tipbot@zytor.com> wrote:
>> >
>> > >
>> > > This is because context switches can swap the task_struct::perf_event_ctxp[]
>> > > pointer around. Therefore you have to either disable preemption when looking
>> > > at current, or hold ctx->lock.
>> > >
>> >
>> >
>> > >
>> > >  void perf_event_exec(void)
>> > >  {
>> > > -       struct perf_event_context *ctx;
>> > >         int ctxn;
>> > >
>> > >         rcu_read_lock();
>> >
>> > Do we still need this rcu_read_lock(), if perf_event_enable_on_exec()
>> > uses  local_irq_save( ?
>>
>> Strictly speaking we should not rely on the fact that RCU grace periods
>> do not progress with IRQs disabled, so yes.
>
> What Peter said!
>
> The current implementation would let you get away with IRQs disabled
> (aside from lockdep splats), but it is easy to imagine implementations
> that do not, hence the restriction.
>
> Is the extra rcu_read_lock() and rcu_read_unlock()  causing a performance
> problem?  If so, one option would be use of synchronize_sched() and
> call_rcu_sched(), which explicitly recognize disabled IRQs as read-side
> critical sections.  Of course, this might well put you in a position
> where you would like to use IRQ disabling in some cases, BH disabling
> (or softirq context) in others, and rcu_read_lock() in yet other cases.
>

I guess I could not see why the rcu_read_lock() was needed around this loop.

rcu_read_lock();
for ((ctxn) = 0; (ctxn) < perf_nr_task_contexts; (ctxn)++)
       perf_event_enable_on_exec(ctxn);
rcu_read_unlock();

But apparently it is needed in perf_event_enable_on_exec() and we avoid calling
rcu_read_lock() and rcu_read_unlock()  (perf_nr_task_contexts - 1 ) times

> If so, the good news is that RCU actually now supports this efficiently.
> There is the shiny new synchronize_rcu_mult() macro that can be used
> as follows to wait for all three types of grace periods:
>
>         synchronize_rcu_mult(call_rcu, call_rcu_bh, call_rcu_sched);
>
> This would wait concurrently for all three grace periods to elapse.
>

Interesting ;)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1303733

On Thu, Jan 07, 2016 at 11:36:21AM -0500, Eric Dumazet wrote:
> On Thu, Jan 7, 2016 at 11:26 AM, Paul E. McKenney
> <paulmck@linux.vnet.ibm.com> wrote:
> > On Thu, Jan 07, 2016 at 02:40:08PM +0100, Peter Zijlstra wrote:
> >> On Wed, Jan 06, 2016 at 01:56:56PM -0500, Eric Dumazet wrote:
> >> > On Wed, Jan 6, 2016 at 1:46 PM, tip-bot for Peter Zijlstra
> >> > <tipbot@zytor.com> wrote:
> >> >
> >> > >
> >> > > This is because context switches can swap the task_struct::perf_event_ctxp[]
> >> > > pointer around. Therefore you have to either disable preemption when looking
> >> > > at current, or hold ctx->lock.
> >> > >
> >> >
> >> >
> >> > >
> >> > >  void perf_event_exec(void)
> >> > >  {
> >> > > -       struct perf_event_context *ctx;
> >> > >         int ctxn;
> >> > >
> >> > >         rcu_read_lock();
> >> >
> >> > Do we still need this rcu_read_lock(), if perf_event_enable_on_exec()
> >> > uses  local_irq_save( ?
> >>
> >> Strictly speaking we should not rely on the fact that RCU grace periods
> >> do not progress with IRQs disabled, so yes.
> >
> > What Peter said!
> >
> > The current implementation would let you get away with IRQs disabled
> > (aside from lockdep splats), but it is easy to imagine implementations
> > that do not, hence the restriction.
> >
> > Is the extra rcu_read_lock() and rcu_read_unlock()  causing a performance
> > problem?  If so, one option would be use of synchronize_sched() and
> > call_rcu_sched(), which explicitly recognize disabled IRQs as read-side
> > critical sections.  Of course, this might well put you in a position
> > where you would like to use IRQ disabling in some cases, BH disabling
> > (or softirq context) in others, and rcu_read_lock() in yet other cases.
> 
> I guess I could not see why the rcu_read_lock() was needed around this loop.
> 
> rcu_read_lock();
> for ((ctxn) = 0; (ctxn) < perf_nr_task_contexts; (ctxn)++)
>        perf_event_enable_on_exec(ctxn);
> rcu_read_unlock();
> 
> But apparently it is needed in perf_event_enable_on_exec() and we avoid calling
> rcu_read_lock() and rcu_read_unlock()  (perf_nr_task_contexts - 1 ) times

If the overhead of rcu_read_lock() and rcu_read_unlock() really are
a noticeable fraction of the loop overhead, and if perf_nr_task_contexts
is large, then you might well have a problem.  I still suggest actually
measuring it, but you know me.  ;-)

> > If so, the good news is that RCU actually now supports this efficiently.
> > There is the shiny new synchronize_rcu_mult() macro that can be used
> > as follows to wait for all three types of grace periods:
> >
> >         synchronize_rcu_mult(call_rcu, call_rcu_bh, call_rcu_sched);
> >
> > This would wait concurrently for all three grace periods to elapse.
> 
> Interesting ;)

;-) ;-) ;-)

							Thanx, Paul

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web