[PATCH] de_thread: eliminate unneccessary sighand locking

[PATCH] de_thread: eliminate unneccessary sighand locking

[Oleg Nesterov]

while switching current->sighand de_thread does:

	write_lock_irq(&tasklist_lock);
	spin_lock(&oldsighand->siglock);
	spin_lock(&newsighand->siglock);

	current->sighand = newsighand;
	recalc_sigpending();

Is these 2 sighand locks are really needed?

At this moment we already zapped other threads, so nobody
can access newsighand via current->. And we are holding
tasklist_lock, so other processes can't send signals to us
or use our ->sighand in any way.

oldsighand can be seen from CLONE_SIGHAND processes, but
we are not using oldsighand in any way, so this lock seems
to be unneeded too.

The only possibility that I can imagine is that some process
does:
	read_lock(tasklist_lock);
	task = find_task();
	spin_lock(task->sighand->siglock);
	read_unlock(tasklist_lock);
	play with task->signal

Is this possible/allowed?

And why do we need recalc_sigpending() ? We are not changing
->pending or ->blocked, just ->sighand.

Signed-off-by: Oleg Nesterov <oleg@tv-sign.ru>

--- 2.6.12/fs/exec.c~	2005-05-09 16:37:16.000000000 +0400
+++ 2.6.12/fs/exec.c	2005-06-20 00:03:24.000000000 +0400
@@ -758,14 +758,7 @@ no_thread_group:
 		       sizeof(newsighand->action));
 
 		write_lock_irq(&tasklist_lock);
-		spin_lock(&oldsighand->siglock);
-		spin_lock(&newsighand->siglock);
-
 		current->sighand = newsighand;
-		recalc_sigpending();
-
-		spin_unlock(&newsighand->siglock);
-		spin_unlock(&oldsighand->siglock);
 		write_unlock_irq(&tasklist_lock);
 
 		if (atomic_dec_and_test(&oldsighand->count))

Re: [PATCH] de_thread: eliminate unneccessary sighand locking

[Roland McGrath]

> while switching current->sighand de_thread does:
> 
> 	write_lock_irq(&tasklist_lock);
> 	spin_lock(&oldsighand->siglock);
> 	spin_lock(&newsighand->siglock);
> 
> 	current->sighand = newsighand;
> 	recalc_sigpending();
> 
> Is these 2 sighand locks are really needed?

Yes.  Other processes can do spin_lock_irq(&ourtask->sighand->siglock);
without holding tasklist_lock.  If someone just did that, they hold
oldsighand->siglock but no newsighand->siglock, and may then be about to
look at ourtask->sighand.  By holding oldsighand->siglock, we ensure that
we can't be colliding with anything like that.  

> The only possibility that I can imagine is that some process
> does:
> 	read_lock(tasklist_lock);
> 	task = find_task();
> 	spin_lock(task->sighand->siglock);
> 	read_unlock(tasklist_lock);
> 	play with task->signal
> 
> Is this possible/allowed?

Yes.

> Signed-off-by: Oleg Nesterov <oleg@tv-sign.ru>

NAK from me.


Thanks,
Roland

Re: [PATCH] de_thread: eliminate unneccessary sighand locking

[Oleg Nesterov]

Roland McGrath wrote:
>
> > while switching current->sighand de_thread does:
> >
> > 	write_lock_irq(&tasklist_lock);
> > 	spin_lock(&oldsighand->siglock);
> > 	spin_lock(&newsighand->siglock);
> >
> > 	current->sighand = newsighand;
> > 	recalc_sigpending();
> >
> > Is these 2 sighand locks are really needed?
>
> Yes.  Other processes can do spin_lock_irq(&ourtask->sighand->siglock);
> without holding tasklist_lock.  If someone just did that, they hold
> oldsighand->siglock but no newsighand->siglock, and may then be about to
> look at ourtask->sighand.  By holding oldsighand->siglock, we ensure that
> we can't be colliding with anything like that.

I think this would be a bug. If some another process can spin for
ourtask->sighand->siglock without holding tasklist_lock it can
read ourtask->sighand == oldsighand and spin for oldsighand->siglock.

Then de_thread frees oldsighand:
	if (atomic_dec_and_test(&oldsighand->count))
		kmem_cache_free(sighand_cachep, oldsighand);

And we have use after free.

So I strongly believe we do not need to lock newsighand->siglock
at least.

And what about recalc_sigpending() ? Do you think it is needed?

> > The only possibility that I can imagine is that some process
> > does:
> > 	read_lock(tasklist_lock);
> > 	task = find_task();
> > 	spin_lock(task->sighand->siglock);
> > 	read_unlock(tasklist_lock);
> > 	play with task->signal
> >
> > Is this possible/allowed?
>
> Yes.

Just for my education, could you please point me to the existed
example?

Oleg.

Re: [PATCH] de_thread: eliminate unneccessary sighand locking

[Roland McGrath]

> I think this would be a bug. If some another process can spin for
> ourtask->sighand->siglock without holding tasklist_lock it can
> read ourtask->sighand == oldsighand and spin for oldsighand->siglock.
> 
> Then de_thread frees oldsighand:
> 	if (atomic_dec_and_test(&oldsighand->count))
> 		kmem_cache_free(sighand_cachep, oldsighand);
> 
> And we have use after free.

That is not the scenario.  Something else must hold tasklist_lock while
acquiring ourtask->sighand->siglock, but need not hold tasklist_lock
throughout.  Someone can be holding oldsighand->lock but not not
tasklist_lock, at the time we lock tasklist_lock.  So like I said, we need
to hold oldsighand->siglock until the switch has been done.

It's possible that no such scenarios exist, but I'd really have to check on
that.  My recollection is that there might be some.

> So I strongly believe we do not need to lock newsighand->siglock
> at least.

The only reason to hold newsighand->siglock there is for the call to
recalc_sigpending.  

> And what about recalc_sigpending() ? Do you think it is needed?

I don't think the need for it has anything to do with switching
current->sighand.  That is, either it's needed in both cases or not at all.
I suspect it lingers from past revisions of this code and related signals
code that previously needed it.  I believe the redistribution of pending
signals done at the top of exit_notify covers all the need, and that will
have run in all the thread dying before we get to the end of de_thread.

So I would tentatively conclude that recalc_sigpending is not needed here.
(Of course, that call is always harmless.  So there isn't a pressing need
to take it out.)

> Just for my education, could you please point me to the existed example?

It would require some auditing to hunt down whether they exist or don't.
To make the change you suggest would require complete confidence none exist.


Thanks,
Roland

Re: [PATCH] de_thread: eliminate unneccessary sighand locking

[Ingo Molnar]

* Roland McGrath <roland@redhat.com> wrote:

> That is not the scenario.  Something else must hold tasklist_lock 
> while acquiring ourtask->sighand->siglock, but need not hold 
> tasklist_lock throughout.  Someone can be holding oldsighand->lock but 
> not not tasklist_lock, at the time we lock tasklist_lock.  So like I 
> said, we need to hold oldsighand->siglock until the switch has been 
> done.
> 
> It's possible that no such scenarios exist, but I'd really have to 
> check on that.  My recollection is that there might be some.

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?

> > Just for my education, could you please point me to the existed example?
> 
> It would require some auditing to hunt down whether they exist or 
> don't. To make the change you suggest would require complete 
> confidence none exist.

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?

	Ingo

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

Re: [PATCH] de_thread: eliminate unneccessary sighand locking

[Ingo Molnar]

* Roland McGrath <roland@redhat.com> wrote:

> > 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);
> 	...

the amount of potentially affected code (assuming all the locking is 
done in a single .[ch] file) is even smaller:

  kernel/posix-cpu-timers.c
  kernel/exit.c
  kernel/posix-timers.c
  include/linux/sched.h

checked these, we dont seem to do that.

> 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.)

this reminds me about the patch below: it gets rid of tasklist_lock use 
in the POSIX timer signal delivery critical path.

> 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.

yeah.

	Ingo

---

dont use the tasklist_lock in the POSIX timer-delivery critical path.

Signed-off-by: Ingo Molnar <mingo@elte.hu>

--- linux/kernel/signal.c.orig
+++ linux/kernel/signal.c
@@ -1384,7 +1384,7 @@ send_sigqueue(int sig, struct sigqueue *
 	 * going away or changing from under us.
 	 */
 	BUG_ON(!(q->flags & SIGQUEUE_PREALLOC));
-	read_lock(&tasklist_lock);  
+	task_lock(p);  
 	spin_lock_irqsave(&p->sighand->siglock, flags);
 	
 	if (unlikely(!list_empty(&q->list))) {
@@ -1411,7 +1411,7 @@ send_sigqueue(int sig, struct sigqueue *
 
 out:
 	spin_unlock_irqrestore(&p->sighand->siglock, flags);
-	read_unlock(&tasklist_lock);
+	task_unlock(p);
 	return(ret);
 }
 

Re: [PATCH] de_thread: eliminate unneccessary sighand locking

[Roland McGrath]

> the amount of potentially affected code (assuming all the locking is 
> done in a single .[ch] file)

I'm not sure what that means.  I'm not confident that all relevant locking
code is always in one file.  If you mean that you did as I said, checked
every use of siglock and confirmed that tasklist_lock is held before
examining ->sighand, then we are good.

> this reminds me about the patch below: it gets rid of tasklist_lock use 
> in the POSIX timer signal delivery critical path.

I don't see how that works at all.  The thought that it would seems to
contradict what we've just been discussing.  Holding tasklist_lock is what
protects against ->sighand and ->signal changing and the old pointers
becoming stale, not task_lock.  What am I missing here?



Thanks,
Roland

Re: [PATCH] de_thread: eliminate unneccessary sighand locking

[Ingo Molnar]

* Roland McGrath <roland@redhat.com> wrote:

> > the amount of potentially affected code (assuming all the locking is 
> > done in a single .[ch] file)
> 
> I'm not sure what that means.  I'm not confident that all relevant 
> locking code is always in one file.  If you mean that you did as I 
> said, checked every use of siglock and confirmed that tasklist_lock is 
> held before examining ->sighand, then we are good.

no, i didnt check all of that. I only checked the obvious places where 
all 3 methods are used in a single module.

> > this reminds me about the patch below: it gets rid of tasklist_lock use 
> > in the POSIX timer signal delivery critical path.
> 
> I don't see how that works at all.  The thought that it would seems to 
> contradict what we've just been discussing.  Holding tasklist_lock is 
> what protects against ->sighand and ->signal changing and the old 
> pointers becoming stale, not task_lock.  What am I missing here?

yeah, it's a bad patch. Offtopic, but it's a real problem: the signal 
code is inevitably holding the tasklist_lock for long times when sending 
group signals, which is interfering with the signal-sending ability of 
upcoming features like high-resolution timers (which uses 
send_sigqueue()).

Could we get rid of tasklist_lock and do a careful lock-step walking of 
the thread hierarchy (locking the next thread, unlocking the previous 
thread in every step), or are there signal semantics reasons for doing 
it all in one atomic step?

	Ingo