* [PATCH] perf: Fix RCU dereference check in perf_event_comm
@ 2012-03-21 23:43 Ari Savolainen
2012-03-22 9:53 ` Peter Zijlstra
0 siblings, 1 reply; 7+ messages in thread
From: Ari Savolainen @ 2012-03-21 23:43 UTC (permalink / raw)
To: Peter Zijlstra, Paul Mackerras, Ingo Molnar, Arnaldo Carvalho de Melo
Cc: linux-kernel
The warning below is printed when executing a command like
sudo perf record su - user -c "echo hello"
It's fixed by moving the call of perf_event_comm to be protected
by the task lock.
===============================
[ INFO: suspicious RCU usage. ]
3.3.0 #49 Not tainted
-------------------------------
include/linux/cgroup.h:567 suspicious rcu_dereference_check() usage!
other info that might help us debug this:
rcu_scheduler_active = 1, debug_locks = 0
1 lock held by su/3304:
#0: (&sig->cred_guard_mutex){+.+.+.}, at: [<ffffffff8117af66>]
prepare_bprm_creds+0x36/0x80
stack backtrace:
Pid: 3304, comm: su Not tainted 3.3.0 #49
Call Trace:
[<ffffffff8109be55>] lockdep_rcu_suspicious+0xe5/0x100
[<ffffffff811131fa>] perf_event_comm+0x37a/0x4d0
[<ffffffff81144525>] ? remove_vma+0x65/0x80
[<ffffffff810722e1>] ? get_parent_ip+0x11/0x50
[<ffffffff814e6fad>] ? sub_preempt_count+0x9d/0xd0
[<ffffffff8117ae05>] set_task_comm+0x75/0x1a0
[<ffffffff8102b945>] ? mmap_rnd+0x45/0x50
[<ffffffff8117b6d2>] setup_new_exec+0xa2/0x340
[<ffffffff811c98f7>] load_elf_binary+0x3c7/0x19d0
[<ffffffff814e3275>] ? _raw_read_unlock+0x35/0x60
[<ffffffff8117a313>] ? search_binary_handler+0x1c3/0x530
[<ffffffff810722e1>] ? get_parent_ip+0x11/0x50
[<ffffffff814e6fad>] ? sub_preempt_count+0x9d/0xd0
[<ffffffff814e6fad>] ? sub_preempt_count+0x9d/0xd0
[<ffffffff811c9530>] ? do_mmap+0x40/0x40
[<ffffffff8117a2f0>] search_binary_handler+0x1a0/0x530
[<ffffffff8117a1a7>] ? search_binary_handler+0x57/0x530
[<ffffffff8117974d>] ? copy_strings.isra.32+0x1fd/0x230
[<ffffffff8117b44f>] do_execve_common.isra.36+0x43f/0x570
[<ffffffff8117b166>] ? do_execve_common.isra.36+0x156/0x570
[<ffffffff81296837>] ? __strncpy_from_user+0x27/0x60
[<ffffffff8117b59b>] do_execve+0x1b/0x20
[<ffffffff8100c317>] sys_execve+0x47/0x70
[<ffffffff814eb39c>] stub_execve+0x6c/0xc0
Signed-off-by: Ari Savolainen <ari.m.savolainen@gmail.com>
---
fs/exec.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/fs/exec.c b/fs/exec.c
index 153dee1..373ff93 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1067,8 +1067,8 @@ void set_task_comm(struct task_struct *tsk, char *buf)
memset(tsk->comm, 0, TASK_COMM_LEN);
wmb();
strlcpy(tsk->comm, buf, sizeof(tsk->comm));
- task_unlock(tsk);
perf_event_comm(tsk);
+ task_unlock(tsk);
}
static void filename_to_taskname(char *tcomm, const char *fn, unsigned int len)
--
1.7.9.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] perf: Fix RCU dereference check in perf_event_comm
2012-03-21 23:43 [PATCH] perf: Fix RCU dereference check in perf_event_comm Ari Savolainen
@ 2012-03-22 9:53 ` Peter Zijlstra
2012-03-22 11:36 ` Ari Savolainen
0 siblings, 1 reply; 7+ messages in thread
From: Peter Zijlstra @ 2012-03-22 9:53 UTC (permalink / raw)
To: Ari Savolainen
Cc: Paul Mackerras, Ingo Molnar, Arnaldo Carvalho de Melo, linux-kernel
On Thu, 2012-03-22 at 01:43 +0200, Ari Savolainen wrote:
> The warning below is printed when executing a command like
> sudo perf record su - user -c "echo hello"
>
> It's fixed by moving the call of perf_event_comm to be protected
> by the task lock.
That seems like a rather poor solution since it increases the lock hold
time for no explained reason.
> include/linux/cgroup.h:567 suspicious rcu_dereference_check() usage!
> [<ffffffff8109be55>] lockdep_rcu_suspicious+0xe5/0x100
> [<ffffffff811131fa>] perf_event_comm+0x37a/0x4d0
So where exactly is this, perf_event_comm_event() takes rcu_read_lock()
so I presume its before that.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] perf: Fix RCU dereference check in perf_event_comm
2012-03-22 9:53 ` Peter Zijlstra
@ 2012-03-22 11:36 ` Ari Savolainen
2012-03-26 12:41 ` Peter Zijlstra
0 siblings, 1 reply; 7+ messages in thread
From: Ari Savolainen @ 2012-03-22 11:36 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Paul Mackerras, Ingo Molnar, Arnaldo Carvalho de Melo, linux-kernel
22. maaliskuuta 2012 11.53 Peter Zijlstra <a.p.zijlstra@chello.nl> kirjoitti:
> On Thu, 2012-03-22 at 01:43 +0200, Ari Savolainen wrote:
>> The warning below is printed when executing a command like
>> sudo perf record su - user -c "echo hello"
>>
>> It's fixed by moving the call of perf_event_comm to be protected
>> by the task lock.
>
> That seems like a rather poor solution since it increases the lock hold
> time for no explained reason.
>
>> include/linux/cgroup.h:567 suspicious rcu_dereference_check() usage!
>
>> [<ffffffff8109be55>] lockdep_rcu_suspicious+0xe5/0x100
>> [<ffffffff811131fa>] perf_event_comm+0x37a/0x4d0
>
> So where exactly is this, perf_event_comm_event() takes rcu_read_lock()
> so I presume its before that.
I think the warning comes from this source-level call path:
perf_event_comm ->
perf_event_enable_on_exec ->
perf_cgroup_sched_out ->
perf_cgroup_from_task ->
task_subsys_state ->
task_subsys_state_check
It seems there that path does not take rcu_read_lock(). Where should
rcu_read_lock/unlock be added? In perf_group_sched_out around the
calls of perf_cgroup_from_task? Like this:
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 1b5c081..bbb4abc 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -409,6 +409,8 @@ static inline void perf_cgroup_sched_out(struct
task_struct *task,
struct perf_cgroup *cgrp1;
struct perf_cgroup *cgrp2 = NULL;
+ rcu_read_lock();
+
/*
* we come here when we know perf_cgroup_events > 0
*/
@@ -421,6 +423,8 @@ static inline void perf_cgroup_sched_out(struct
task_struct *task,
if (next)
cgrp2 = perf_cgroup_from_task(next);
+ rcu_read_unlock();
+
/*
* only schedule out current cgroup events if we know
* that we are switching to a different cgroup. Otherwise,
--
Ari
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] perf: Fix RCU dereference check in perf_event_comm
2012-03-22 11:36 ` Ari Savolainen
@ 2012-03-26 12:41 ` Peter Zijlstra
2012-04-26 15:06 ` Stephane Eranian
2012-05-18 16:38 ` Stephane Eranian
0 siblings, 2 replies; 7+ messages in thread
From: Peter Zijlstra @ 2012-03-26 12:41 UTC (permalink / raw)
To: Ari Savolainen
Cc: Paul Mackerras, Ingo Molnar, Arnaldo Carvalho de Melo,
linux-kernel, Stephane Eranian, Paul E. McKenney
On Thu, 2012-03-22 at 13:36 +0200, Ari Savolainen wrote:
> 22. maaliskuuta 2012 11.53 Peter Zijlstra <a.p.zijlstra@chello.nl> kirjoitti:
> > On Thu, 2012-03-22 at 01:43 +0200, Ari Savolainen wrote:
> >> The warning below is printed when executing a command like
> >> sudo perf record su - user -c "echo hello"
> >>
> >> It's fixed by moving the call of perf_event_comm to be protected
> >> by the task lock.
> >
> > That seems like a rather poor solution since it increases the lock hold
> > time for no explained reason.
> >
> >> include/linux/cgroup.h:567 suspicious rcu_dereference_check() usage!
> >
> >> [<ffffffff8109be55>] lockdep_rcu_suspicious+0xe5/0x100
> >> [<ffffffff811131fa>] perf_event_comm+0x37a/0x4d0
> >
> > So where exactly is this, perf_event_comm_event() takes rcu_read_lock()
> > so I presume its before that.
>
> I think the warning comes from this source-level call path:
>
> perf_event_comm ->
> perf_event_enable_on_exec ->
> perf_cgroup_sched_out ->
> perf_cgroup_from_task ->
> task_subsys_state ->
> task_subsys_state_check
>
> It seems there that path does not take rcu_read_lock(). Where should
> rcu_read_lock/unlock be added? In perf_group_sched_out around the
> calls of perf_cgroup_from_task? Like this:
Ah, ok. So IIRC this too is not needed. As the comment near
perf_cgroup_from_task() says, we hold explicit references to the cgroup.
Ideally we'd come up with a better validation condition but all variants
I could come up with make the code ugly and might actually generate
worse code, the current true simply shuts it up.
Stephane any thoughts?
---
kernel/events/core.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/kernel/events/core.c b/kernel/events/core.c
index a6a9ec4..e423261 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -240,7 +240,7 @@ static void perf_ctx_unlock(struct perf_cpu_context *cpuctx,
static inline struct perf_cgroup *
perf_cgroup_from_task(struct task_struct *task)
{
- return container_of(task_subsys_state(task, perf_subsys_id),
+ return container_of(task_subsys_state_check(task, perf_subsys_id, true),
struct perf_cgroup, css);
}
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] perf: Fix RCU dereference check in perf_event_comm
2012-03-26 12:41 ` Peter Zijlstra
@ 2012-04-26 15:06 ` Stephane Eranian
2012-04-29 20:07 ` Ari Savolainen
2012-05-18 16:38 ` Stephane Eranian
1 sibling, 1 reply; 7+ messages in thread
From: Stephane Eranian @ 2012-04-26 15:06 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Ari Savolainen, Paul Mackerras, Ingo Molnar,
Arnaldo Carvalho de Melo, linux-kernel, Paul E. McKenney
On Mon, Mar 26, 2012 at 2:41 PM, Peter Zijlstra <a.p.zijlstra@chello.nl> wrote:
> On Thu, 2012-03-22 at 13:36 +0200, Ari Savolainen wrote:
>> 22. maaliskuuta 2012 11.53 Peter Zijlstra <a.p.zijlstra@chello.nl> kirjoitti:
>> > On Thu, 2012-03-22 at 01:43 +0200, Ari Savolainen wrote:
>> >> The warning below is printed when executing a command like
>> >> sudo perf record su - user -c "echo hello"
>> >>
>> >> It's fixed by moving the call of perf_event_comm to be protected
>> >> by the task lock.
>> >
>> > That seems like a rather poor solution since it increases the lock hold
>> > time for no explained reason.
>> >
>> >> include/linux/cgroup.h:567 suspicious rcu_dereference_check() usage!
>> >
>> >> [<ffffffff8109be55>] lockdep_rcu_suspicious+0xe5/0x100
>> >> [<ffffffff811131fa>] perf_event_comm+0x37a/0x4d0
>> >
>> > So where exactly is this, perf_event_comm_event() takes rcu_read_lock()
>> > so I presume its before that.
>>
Sorry for late reply. Forgot about this issue.
Did that warning occur while you were NOT measuring cgroup events?
>> I think the warning comes from this source-level call path:
>>
>> perf_event_comm ->
>> perf_event_enable_on_exec ->
>> perf_cgroup_sched_out ->
>> perf_cgroup_from_task ->
>> task_subsys_state ->
>> task_subsys_state_check
>>
>> It seems there that path does not take rcu_read_lock(). Where should
>> rcu_read_lock/unlock be added? In perf_group_sched_out around the
>> calls of perf_cgroup_from_task? Like this:
>
> Ah, ok. So IIRC this too is not needed. As the comment near
> perf_cgroup_from_task() says, we hold explicit references to the cgroup.
>
> Ideally we'd come up with a better validation condition but all variants
> I could come up with make the code ugly and might actually generate
> worse code, the current true simply shuts it up.
>
> Stephane any thoughts?
>
> ---
> kernel/events/core.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index a6a9ec4..e423261 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -240,7 +240,7 @@ static void perf_ctx_unlock(struct perf_cpu_context *cpuctx,
> static inline struct perf_cgroup *
> perf_cgroup_from_task(struct task_struct *task)
> {
> - return container_of(task_subsys_state(task, perf_subsys_id),
> + return container_of(task_subsys_state_check(task, perf_subsys_id, true),
> struct perf_cgroup, css);
> }
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] perf: Fix RCU dereference check in perf_event_comm
2012-04-26 15:06 ` Stephane Eranian
@ 2012-04-29 20:07 ` Ari Savolainen
0 siblings, 0 replies; 7+ messages in thread
From: Ari Savolainen @ 2012-04-29 20:07 UTC (permalink / raw)
To: Stephane Eranian
Cc: Peter Zijlstra, Paul Mackerras, Ingo Molnar,
Arnaldo Carvalho de Melo, linux-kernel, Paul E. McKenney
> Did that warning occur while you were NOT measuring cgroup events?
>
The warning occurs every time I execute perf record for the first time
after rebooting (e.g. perf record sleep 1).
Ari
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] perf: Fix RCU dereference check in perf_event_comm
2012-03-26 12:41 ` Peter Zijlstra
2012-04-26 15:06 ` Stephane Eranian
@ 2012-05-18 16:38 ` Stephane Eranian
1 sibling, 0 replies; 7+ messages in thread
From: Stephane Eranian @ 2012-05-18 16:38 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Ari Savolainen, Paul Mackerras, Ingo Molnar,
Arnaldo Carvalho de Melo, linux-kernel, Paul E. McKenney
On Mon, Mar 26, 2012 at 2:41 PM, Peter Zijlstra <a.p.zijlstra@chello.nl> wrote:
> On Thu, 2012-03-22 at 13:36 +0200, Ari Savolainen wrote:
>> 22. maaliskuuta 2012 11.53 Peter Zijlstra <a.p.zijlstra@chello.nl> kirjoitti:
>> > On Thu, 2012-03-22 at 01:43 +0200, Ari Savolainen wrote:
>> >> The warning below is printed when executing a command like
>> >> sudo perf record su - user -c "echo hello"
>> >>
>> >> It's fixed by moving the call of perf_event_comm to be protected
>> >> by the task lock.
>> >
>> > That seems like a rather poor solution since it increases the lock hold
>> > time for no explained reason.
>> >
>> >> include/linux/cgroup.h:567 suspicious rcu_dereference_check() usage!
>> >
>> >> [<ffffffff8109be55>] lockdep_rcu_suspicious+0xe5/0x100
>> >> [<ffffffff811131fa>] perf_event_comm+0x37a/0x4d0
>> >
>> > So where exactly is this, perf_event_comm_event() takes rcu_read_lock()
>> > so I presume its before that.
>>
>> I think the warning comes from this source-level call path:
>>
>> perf_event_comm ->
>> perf_event_enable_on_exec ->
>> perf_cgroup_sched_out ->
>> perf_cgroup_from_task ->
>> task_subsys_state ->
>> task_subsys_state_check
>>
>> It seems there that path does not take rcu_read_lock(). Where should
>> rcu_read_lock/unlock be added? In perf_group_sched_out around the
>> calls of perf_cgroup_from_task? Like this:
>
> Ah, ok. So IIRC this too is not needed. As the comment near
> perf_cgroup_from_task() says, we hold explicit references to the cgroup.
>
> Ideally we'd come up with a better validation condition but all variants
> I could come up with make the code ugly and might actually generate
> worse code, the current true simply shuts it up.
>
> Stephane any thoughts?
>
I think it is okay to skip the check because we only actually dereference
the point once we know we have ctx.nr_cgroup > 0 or the event is a cgroup
event. And in both cases, that means we have a refcnt on the cgroup, thus
it cannot disappear behind our back.
As you said, the alternatives would be to only call perf_cgroup_from_task()
only AFTER we've made the expensive checks (which we will do again later
in the call chain). Or we would have to grab task->alloc_lock() or cgroup_lock
none of which are cheap.
Acked-by: Stephane Eranian <eranian@google.com>
> ---
> kernel/events/core.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index a6a9ec4..e423261 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -240,7 +240,7 @@ static void perf_ctx_unlock(struct perf_cpu_context *cpuctx,
> static inline struct perf_cgroup *
> perf_cgroup_from_task(struct task_struct *task)
> {
> - return container_of(task_subsys_state(task, perf_subsys_id),
> + return container_of(task_subsys_state_check(task, perf_subsys_id, true),
> struct perf_cgroup, css);
> }
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2012-05-18 16:38 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-03-21 23:43 [PATCH] perf: Fix RCU dereference check in perf_event_comm Ari Savolainen
2012-03-22 9:53 ` Peter Zijlstra
2012-03-22 11:36 ` Ari Savolainen
2012-03-26 12:41 ` Peter Zijlstra
2012-04-26 15:06 ` Stephane Eranian
2012-04-29 20:07 ` Ari Savolainen
2012-05-18 16:38 ` Stephane Eranian
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.