Re: [GIT PULL rcu/next] RCU commits for 3.1

[Paul E. McKenney]

On Fri, Oct 28, 2011 at 10:34:09AM +0800, Li Zefan wrote:
> >>>>
> >>>> And I cannot reproduce after merging into 3.1.  :-(
> >>>
> >>> Here's another one i just got with latest -tip:
> >>>
> >>> PM: Adding info for No Bus:vcsa2
> >>>
> >>> ===============================
> >>> [ INFO: suspicious RCU usage. ]
> >>> -------------------------------
> >>> include/linux/cgroup.h:548 suspicious rcu_dereference_check() usage!
> >>>
> >>> other info that might help us debug this:
> >>>
> >>>
> >>> rcu_scheduler_active = 1, debug_locks = 0
> >>> 1 lock held by true/655:
> >>>  #0:  (&sig->cred_guard_mutex){+.+.+.}, at: [<810d1bd7>] prepare_bprm_creds+0x27/0x70
> >>>
> >>> stack backtrace:
> >>> Pid: 655, comm: true Not tainted 3.1.0-tip-01868-g1271bd2-dirty #161079
> >>> Call Trace:
> >>>  [<81abe239>] ? printk+0x18/0x1a
> >>>  [<81064920>] lockdep_rcu_suspicious+0xc0/0xd0
> >>>  [<8108aa02>] perf_event_enable_on_exec+0x1d2/0x1e0
> >>>  [<81063764>] ? __lock_release+0x54/0xb0
> >>>  [<8108cca8>] perf_event_comm+0x18/0x60
> >>>  [<810d1abd>] ? set_task_comm+0x5d/0x80
> 
> void set_task_comm(struct task_struct *tsk, char *buf)
> {
>         task_lock(tsk);
> 	...
>         task_unlock(tsk);
>         perf_event_comm(tsk);
> }
> 
> see, perf_event_comm() is called after releasing task_lock.
> 
> perf_event_comm()
>   perf_event_enable_on_exec()
>     perf_cgroup_sched_out()
>       perf_cgroup_from_task()
>         task_subsys_state()
> 
> No proper lock is held, hence the warning.

Thank you for the analysis.  Does the following patch fix this problem?

							Thanx, Paul

------------------------------------------------------------------------

fs: Add RCU protection in set_task_comm()

Running "perf stat true" results in the following RCU-lockdep splat:

===============================
[ INFO: suspicious RCU usage. ]
-------------------------------
include/linux/cgroup.h:548 suspicious rcu_dereference_check() usage!

other info that might help us debug this:

rcu_scheduler_active = 1, debug_locks = 0
1 lock held by true/655:
#0:  (&sig->cred_guard_mutex){+.+.+.}, at: [<810d1bd7>] prepare_bprm_creds+0x27/0x70

stack backtrace:
Pid: 655, comm: true Not tainted 3.1.0-tip-01868-g1271bd2-dirty #161079
Call Trace:
[<81abe239>] ? printk+0x18/0x1a
[<81064920>] lockdep_rcu_suspicious+0xc0/0xd0
[<8108aa02>] perf_event_enable_on_exec+0x1d2/0x1e0
[<81063764>] ? __lock_release+0x54/0xb0
[<8108cca8>] perf_event_comm+0x18/0x60
[<810d1abd>] ? set_task_comm+0x5d/0x80
[<81af622d>] ? _raw_spin_unlock+0x1d/0x40
[<810d1ac4>] set_task_comm+0x64/0x80
[<810d25fd>] setup_new_exec+0xbd/0x1d0
[<810d1b61>] ? flush_old_exec+0x81/0xa0
[<8110753e>] load_elf_binary+0x28e/0xa00
[<810d2101>] ? search_binary_handler+0xd1/0x1d0
[<81063764>] ? __lock_release+0x54/0xb0
[<811072b0>] ? load_elf_library+0x260/0x260
[<810d2108>] search_binary_handler+0xd8/0x1d0
[<810d2060>] ? search_binary_handler+0x30/0x1d0
[<810d242f>] do_execve_common+0x22f/0x2a0
[<810d24b2>] do_execve+0x12/0x20
[<81009592>] sys_execve+0x32/0x70
[<81af7752>] ptregs_execve+0x12/0x20
[<81af76d4>] ? sysenter_do_call+0x12/0x36

Li Zefan noted that this is due to set_task_comm() dropping the task
lock before invoking perf_event_comm(), which could in fact result in
the task being freed up before perf_event_comm() completed tracing in
the case where one task invokes set_task_comm() on another task -- which
actually does occur via comm_write(), which can be invoked via /proc.

This commit fixes this problem by entering an RCU read-side critical
section before acquiring the task lock and exiting this critical section
after perf_event_comm() returns.

Reported-by: Ingo Molnar <mingo@xxxxxxx>
Signed-off-by: Paul E. McKenney <paulmck@xxxxxxxxxxxxxxxxxx>

diff --git a/fs/exec.c b/fs/exec.c
index 25dcbe5..fb928d3 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1056,6 +1056,7 @@ EXPORT_SYMBOL_GPL(get_task_comm);
 
 void set_task_comm(struct task_struct *tsk, char *buf)
 {
+	rcu_read_lock(); /* protect task pointer through tracing. */
 	task_lock(tsk);
 
 	/*
@@ -1069,6 +1070,7 @@ void set_task_comm(struct task_struct *tsk, char *buf)
 	strlcpy(tsk->comm, buf, sizeof(tsk->comm));
 	task_unlock(tsk);
 	perf_event_comm(tsk);
+	rcu_read_unlock();
 }
 
 int flush_old_exec(struct linux_binprm * bprm)

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