Messages in this thread Patch in this message | | | From | David Howells <> | Subject | Re: [PATCH 2/2] CRED: Fix __task_cred()'s lockdep check and banner comment | Date | Tue, 03 Aug 2010 10:34:07 +0100 |
| |
Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote:
> I got below warning. Is this related to this patch? > > [ 140.173556] =================================================== > [ 140.215379] [ INFO: suspicious rcu_dereference_check() usage. ] > [ 140.216461] --------------------------------------------------- > [ 140.217530] kernel/signal.c:660 invoked rcu_dereference_check() without protection!
Yes. The patch has uncovered a case of where we should be holding a lock, but aren't.
Can you try the attached patch?
David --- From: David Howells <dhowells@redhat.com> Subject: [PATCH] CRED: Fix RCU warning due to previous patch fixing __task_cred()'s checks
A previous patch:
commit 8f92054e7ca1d3a3ae50fb42d2253ac8730d9b2a Author: David Howells <dhowells@redhat.com> Date: Thu Jul 29 12:45:55 2010 +0100 Subject: CRED: Fix __task_cred()'s lockdep check and banner comment
fixed the lockdep checks on __task_cred(). This has shown up a place in the signalling code where a lock should be held - namely that check_kill_permission() requires its callers to hold the RCU lock.
It's may be that it would be better to add RCU read lock calls in group_send_sig_info() only, around the call to check_kill_permission(). On the other hand, some of the callers are either holding the RCU read lock already, or have disabled interrupts, in which case, it's just extra overhead to do it in g_s_s_i().
Without this patch, the following warning can occur:
[ 140.173556] =================================================== [ 140.215379] [ INFO: suspicious rcu_dereference_check() usage. ] [ 140.216461] --------------------------------------------------- [ 140.217530] kernel/signal.c:660 invoked rcu_dereference_check() without protection! [ 140.218937] [ 140.218938] other info that might help us debug this: [ 140.218939] [ 140.220508] [ 140.220509] rcu_scheduler_active = 1, debug_locks = 1 [ 140.221991] 1 lock held by init/1: [ 140.222668] #0: (tasklist_lock){.+.+..}, at: [<c104a0ac>] kill_something_info+0x7c/0x160 [ 140.224709] [ 140.224711] stack backtrace: [ 140.225661] Pid: 1, comm: init Not tainted 2.6.35 #1 [ 140.226576] Call Trace: [ 140.227111] [<c103cca8>] ? printk+0x18/0x20 [ 140.227908] [<c1069884>] lockdep_rcu_dereference+0x94/0xb0 [ 140.228931] [<c104936a>] check_kill_permission+0x15a/0x170 [ 140.229932] [<c104a0ac>] ? kill_something_info+0x7c/0x160 [ 140.230921] [<c1049cca>] group_send_sig_info+0x1a/0x50 [ 140.231866] [<c1049d36>] __kill_pgrp_info+0x36/0x60 [ 140.232780] [<c104a0d0>] kill_something_info+0xa0/0x160 [ 140.233740] [<c10831c5>] ? __call_rcu+0xa5/0x110 [ 140.234596] [<c104b7ee>] sys_kill+0x5e/0x70 [ 140.235387] [<c10d1eee>] ? mntput_no_expire+0x1e/0xa0 [ 140.236329] [<c10bbd10>] ? __fput+0x170/0x220 [ 140.257756] [<c10bbdd9>] ? fput+0x19/0x20 [ 140.258529] [<c137ad94>] ? restore_all_notrace+0x0/0x18 [ 140.259496] [<c11bfb04>] ? trace_hardirqs_on_thunk+0xc/0x10 [ 140.260531] [<c137ad61>] syscall_call+0x7/0xb [ 144.627841] nfsd: last server has exited, flushing export cache [ 154.040420] Restarting system. [ 154.041061] machine restart
Reported-by: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> Signed-off-by: David Howells <dhowells@redhat.com> ---
kernel/exit.c | 2 ++ kernel/signal.c | 8 ++++++-- 2 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/kernel/exit.c b/kernel/exit.c index ceffc67..7858ebf 100644 --- a/kernel/exit.c +++ b/kernel/exit.c @@ -773,6 +773,7 @@ static void forget_original_parent(struct task_struct *father) exit_ptrace(father); + rcu_read_lock(); write_lock_irq(&tasklist_lock); reaper = find_new_reaper(father); @@ -791,6 +792,7 @@ static void forget_original_parent(struct task_struct *father) reparent_leader(father, p, &dead_children); } write_unlock_irq(&tasklist_lock); + rcu_read_unlock(); BUG_ON(!list_empty(&father->children)); diff --git a/kernel/signal.c b/kernel/signal.c index 906ae5a..f771156 100644 --- a/kernel/signal.c +++ b/kernel/signal.c @@ -637,7 +637,7 @@ static inline bool si_fromuser(const struct siginfo *info) /* * Bad permissions for sending the signal - * - the caller must hold at least the RCU read lock + * - the caller must hold the RCU read lock */ static int check_kill_permission(int sig, struct siginfo *info, struct task_struct *t) @@ -1127,7 +1127,7 @@ struct sighand_struct *lock_task_sighand(struct task_struct *tsk, unsigned long /* * send signal info to all the members of a group - * - the caller must hold the RCU read lock at least + * - the caller must hold the RCU read lock */ int group_send_sig_info(int sig, struct siginfo *info, struct task_struct *p) { @@ -1151,11 +1151,13 @@ int __kill_pgrp_info(int sig, struct siginfo *info, struct pid *pgrp) success = 0; retval = -ESRCH; + rcu_read_lock(); do_each_pid_task(pgrp, PIDTYPE_PGID, p) { int err = group_send_sig_info(sig, info, p); success |= !err; retval = err; } while_each_pid_task(pgrp, PIDTYPE_PGID, p); + rcu_read_unlock(); return success ? 0 : retval; } @@ -1261,6 +1263,7 @@ static int kill_something_info(int sig, struct siginfo *info, pid_t pid) int retval = 0, count = 0; struct task_struct * p; + rcu_read_lock(); for_each_process(p) { if (task_pid_vnr(p) > 1 && !same_thread_group(p, current)) { @@ -1270,6 +1273,7 @@ static int kill_something_info(int sig, struct siginfo *info, pid_t pid) retval = err; } } + rcu_read_unlock(); ret = count ? retval : -ESRCH; } read_unlock(&tasklist_lock);
| |