Re: [PATCH] de_thread: eliminate unneccessary sighand locking

[Roland McGrath]

> i have checked it when acking the patch and it does not seem we do that 
> anywhere in the kernel. It would also be dangerous as per Oleg's 
> observations, unless something like this is done:
> 
> 	read_lock(&tasklist_lock);
> 	p = find_task_by_pid(pid);
> 	task_lock(p);
> 	spin_lock(&p->sighand->siglock);
> 	read_unlock(&tasklist_lock);
> 
> 	...
> 
> do you know any such code?

I was thinking it would look more like:

 	read_lock(&tasklist_lock);
 	p = find_task_by_pid(pid);
 	get_task_struct(p);
 	read_unlock(&tasklist_lock);
	...
	spin_lock_irq(&p->sighand->siglock);
	...

I am not sure whether such code exists or not.  It won't look quite like
that, in that the siglock use may be far away from the extraction.  There
are things that store pointers like that (like real_timer.data, and
posix_timers stuff).  But it may well be that all those places take
tasklist_lock before using the saved task_struct, in which case it's fine.
(Anything doing signals stuff usually needs tasklist_lock anyway in case it
has to traverse the thread group.)

> i have manually reviewed every .[ch] file that uses tasklist_lock, 
> siglock and lock_task:
> 
>   fs/proc/array.c
>   fs/exec.c
>   kernel/ptrace.c
>   kernel/fork.c
>   kernel/exit.c
>   kernel/sys.c
>   include/linux/sched.h
>   security/selinux/hooks.c
> 
> can other valid locking variations exist, other than the one i outlined 
> above?

You mean those are the files that use all three of those, or what?  That's
clearly not the complete list of siglock uses.  Any code using siglock
needs to be grokked adequately to see if tasklist_lock is always held
around looking at ->sighand.


Thanks,
Roland
-
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/