> >
> > - 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
