Re: [PATCH/RFC] signal races/bugs, losing TIF_SIGPENDING and other woes

[Benjamin Herrenschmidt <benh@kernel.crashing.org>]

> > 
> >  - something calls recalc_sigpending_tsk() on thread A (for example,
> >    something try to sends it S2 which is blocked). There is no longer
> >    an active signal and thus TIF_SIGPENDING is cleared on thread A
> 
> I agree. That's unquestionably a bug. We should *never* clear sigpending 
> for somebody else.
> 
> I have to say, your patch looks pretty ugly though. It also looks like 
> it's rife to be subtly buggy (ie somebody calls "recalc_sigpending_tsk()" 
> with "current" and doesn't realize the subtle rule.

Yeah. Also, I think we have done a mistake in our analysis.

We don't actually call recalc_sigpending_tsk() when sending a signal to
some other task, we just set the flag... so I need to recheck my theory
here about recalc_sigpending_tsk being called for somebody else...
Something is doing it somewhere it seems (we are losing the
TIF_SIGPENDING bit) but I'll need to cook up a repro case to track
exactly where.

However, there is still something wrong imho with actually passing it a
task if it should really only ever be called on current... What are we
saving by passing this task around rather than using "current" ? It's
expensive to get to current on some archs ? The only somewhat legic case
I can see is the exit.c code ... though that could cause the scenario
I've explained, I suppose it's fair to say that we're going to exit so
it doesn't matter anyway.

Another case I see is force_sig_info() which is dodgy... it -firsts-
unblocks, then recalc_sigpending_and_wake() the target, -then- sends the
signal... so there might be a small window there where we could clear
TIF_SIGPENDING....

That leaves us to these:

 - There's a bug somewhere, maybe the force_sig_info thing above, maybe
some other path I haven't quite found yet, that can cause syscalls to
return -ERESTARTSYS. I need to make a reprocase to verify we get the
right fix.

 - We should still, I think, make sure that we never clear anybody but
ourselve's TIF_SIGPENDING as a general good thing to do, to make the
whole thing "robust" (and fix the potential issue with force_sig_info).

 - However, I'm not sure about your patch. The problem is, can't things
like force_sig_info() be called from an interrupt ? Thus I think using
the value of current inside recalc_sigpending_tsk might not be the best
approach... I was trying to make sure we really only do it when we are
in a known code path where current can be relied upon.

 - I still think there's something wrong with dequeue_signal() being
potentially called with a task different than current by signalfd, since
__dequeue_signal() (among others) mucks around with current regardless.
I'd love to just make signalfd's read() only do anything if current ==
ctx->tsk and remove the task argument from dequeue_signal... that would
fix it nicely too no ?

> I also wonder if we should just remove the "clear_tsk_thread_flag()" thing 
> *entirely*, and do it only in the do_sigal() path. THAT, however, is a 
> much bigger change. This one-liner seems the trivial and most obvious 
> patch.

I've been thinking about that too (in fact, doing it in
get_signal_to_deliver()), but there are some issues with kernel theads
ending up with TIF_SIGPENDING set forever.

I think the right thing is that dequeue_signal() is the one who needs to
do it, and dequeue_signal() should never be allowed on anything but
current. But that means fixing signalfd.

Cheers,
Ben.


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