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