Re: [PATCH] de_thread: eliminate unneccessary sighand locking

[Roland McGrath <roland@redhat.com>]

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