Messages in this thread | | | Subject | Re: [PATCH/RFC] signal races/bugs, losing TIF_SIGPENDING and other woes | From | Benjamin Herrenschmidt <> | Date | Tue, 05 Jun 2007 12:10:13 +1000 |
| |
> > > > - 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/
| |