The story starts with...
We've been seeing some occasional reports of -ERESTARTSYS and friends
being returned to userland from syscalls like accept(). After some
digging, we beleive that the signal code is inherently racy vs. the
way one task (or interrupt) can cause another task TIG_SIGPENDING
to be cleared.
(And while looking at the code, found more potential issues with
signalfd, see later for that one)
The circumstances are a bit hard to trigger (and thus we haven't yet
made a proper repro-case, the field problem happens every now and then),
but basically, the idea is something around the lines of:
- thread A does accept() or select() or some other syscall that can
restart upon reception of a signal. It also has signal S1 unblocked
and signal S2 blocked.
- Something sends S1 to the process (shared signal), thread A is the
selected victim from the signal code and gets TIF_SIGPENDING set.
- thread A decides to return -ERESTARTSYS. On the way to userland, to
make the race more likely, we also end up rescheduling, thus we
call schedule() before we actually call do_signal().
- thread B has TIF_SIGPENDING set (for some other signal) and ends up
handling and dequeuing both that other signal and S1, thus there is
no more shared signal pending
- 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
- thread A gets back out of schedule(), there is no more TIF_SIGPENDING,
thus it goes back to userland returning -ERESTARTSYS and never restarts
the syscall
There might be variations on the above scenario. We beleive that the
base problem is that the only one who should ever be able to clear
TIF_SIGPENDING is the task who has it set, nobody else, since it might
have been "seen" by that task long enough to make decisions such as
returning -ERESTARTSYS, at which point it must not be cleared until
do_signal() is called.
In fact, it makes a lot of sense to only ever clear that flag on yourself
anyway.
The patch below does that by making recalc_sigpending() be the only one to
clear it, not recalc_sigpending_tsk(). I also added a WARN_ON in case
we ever call recalc_sigpending() without the siglock since that would mean
a race causing a potential loss of TIF_SIGPENDING.
Note that the code in dequeue_signal() is a bit uglier than I would have
liked. I would have much preferred making dequeue_signal() not take a task
argument anymore and just always operate on "current" but that's being
defeated by signalfd, which seems to be only one to potentially dequeue
signals off another task. So I leave it alone for now.
However, I find that very dodgy since dequeue_signal() calls
__dequeue_signal() which does things vs. a notifier which I don't quite
know what it's for, but it's definitely not going to fly if dequeue_signal()
is called for somebody else than current... and potentially cause another
spurrious clearing of TIF_SIGPENDING.
Thus, I think that signalfd probably needs fixing to only ever allow
read() to be called from the task that created the fd in the first place
thus only allowing a thread to dequeue itself.
Any better idea ? Comments ?
(Patch built & booted on a pSeries box)
Cheers,
Ben.
Signed-off-by: Benjamin Herrenschmidt <[email protected]>
Index: linux-work/kernel/signal.c
===================================================================
--- linux-work.orig/kernel/signal.c 2007-06-05 10:56:53.000000000 +1000
+++ linux-work/kernel/signal.c 2007-06-05 11:15:14.000000000 +1000
@@ -105,7 +105,6 @@ static int recalc_sigpending_tsk(struct
set_tsk_thread_flag(t, TIF_SIGPENDING);
return 1;
}
- clear_tsk_thread_flag(t, TIF_SIGPENDING);
return 0;
}
@@ -119,9 +118,22 @@ void recalc_sigpending_and_wake(struct t
signal_wake_up(t, 0);
}
+/*
+ * Only when recalculating on yourself are you allowed to clear TIF_SIGPENDING
+ * never when recalculating somebody else. If you were to allow it, then
+ * a syscall could see signal_pending(), decide to return -ERESTARTSYS, and
+ * lose TIF_SIGPENDING before actually hitting do_signal(), thus effectively
+ * returning -ERESTARTSYS to userland and failing to restart the syscall.
+ */
void recalc_sigpending(void)
{
- recalc_sigpending_tsk(current);
+ /* This must be called with the siglock or we might lose
+ * TIF_SIGPENDING
+ */
+ WARN_ON(!spin_is_locked(¤t->sighand->siglock));
+
+ if (!recalc_sigpending_tsk(current))
+ clear_thread_flag(TIF_SIGPENDING);
}
/* Given the mask, find the first available signal that should be serviced. */
@@ -385,7 +397,18 @@ int dequeue_signal(struct task_struct *t
}
}
}
- recalc_sigpending_tsk(tsk);
+ /* We need to call recalc_sigpending() when recalculating ourselves
+ * so that TIF_SIGPENDING is cleated when appropriate. The only reason
+ * we actually have to compare against current rather than
+ * unconditionally doing recalc_sigpending() is the same that prevents
+ * me from removing the task argument from dequeue_signal() and is
+ * signalfd which can dequeue signals of another task (yuck)
+ */
+ if (tsk == current)
+ recalc_sigpending();
+ else
+ recalc_sigpending_tsk(tsk);
+
if (signr && unlikely(sig_kernel_stop(signr))) {
/*
* Set a marker that we have dequeued a stop signal. Our
On Tue, 5 Jun 2007, Benjamin Herrenschmidt wrote:
>
> - 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.
So I'd rather just make it more explicit, and simple, and just have
something really simple like the appended instead..
If we want to be fancier (and avoid the unnecessary compare for the cases
where we statically know that we're calling it with "t" being "current"),
we could make it an inline function and factor things out a bit, but I'm
not sure how worthwhile that really is.
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.
Comments?
Linus
---
kernel/signal.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/kernel/signal.c b/kernel/signal.c
index acdfc05..61abd8d 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -105,7 +105,8 @@ static int recalc_sigpending_tsk(struct task_struct *t)
set_tsk_thread_flag(t, TIF_SIGPENDING);
return 1;
}
- clear_tsk_thread_flag(t, TIF_SIGPENDING);
+ if (t == current)
+ clear_tsk_thread_flag(t, TIF_SIGPENDING);
return 0;
}
> >
> > - 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.
On Tue, 5 Jun 2007, Benjamin Herrenschmidt wrote:
> - 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 ?
There's got to be a clean solution that does not limit signalfd, no? I
have no time to look at it immediately, but I can look into it in the
next few days, if someone else does not do it before...
- Davide
On Mon, 2007-06-04 at 19:38 -0700, Davide Libenzi wrote:
> > - 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 ?
>
> There's got to be a clean solution that does not limit signalfd, no? I
> have no time to look at it immediately, but I can look into it in the
> next few days, if someone else does not do it before...
Is there a real usage to dequeuing somebody else signals with signalfd ?
If yes, then we can do something around the lines of passing task down
to __dequeue_signal, though I'm not too sure waht this notifier is about
and wether it might rely on being called from within the affected task
context...
Ben.
On Tue, 2007-06-05 at 13:22 +1000, Benjamin Herrenschmidt wrote:
> On Mon, 2007-06-04 at 19:38 -0700, Davide Libenzi wrote:
> > > - 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 ?
> >
> > There's got to be a clean solution that does not limit signalfd, no? I
> > have no time to look at it immediately, but I can look into it in the
> > next few days, if someone else does not do it before...
>
> Is there a real usage to dequeuing somebody else signals with signalfd ?
> If yes, then we can do something around the lines of passing task down
> to __dequeue_signal, though I'm not too sure waht this notifier is about
> and wether it might rely on being called from within the affected task
> context...
>
> Ben.
>
signalfd() doesn't deliver thread-targeted signals to the wrong threads,
does it?
Hmm.
It looks like reading from a signalfd will give you either
process-global signals or the thread-specific signals that are targeted
towards the thread that originally created the signalfd (regardless of
which thread actually calls read()).
Which is weird, to say the least. Definitely needs to be noted in the
man page, which doesn't seem to exist yet.
Is there a reason why signalfd() doesn't behave like regular signals in
this regard?
--
Nicholas Miell <[email protected]>
On Mon, 2007-06-04 at 23:09 -0700, Nicholas Miell wrote:
> signalfd() doesn't deliver thread-targeted signals to the wrong
> threads,
> does it?
>
> Hmm.
>
> It looks like reading from a signalfd will give you either
> process-global signals or the thread-specific signals that are
> targeted
> towards the thread that originally created the signalfd (regardless of
> which thread actually calls read()).
>
> Which is weird, to say the least. Definitely needs to be noted in the
> man page, which doesn't seem to exist yet.
>
> Is there a reason why signalfd() doesn't behave like regular signals
> in
> this regard?
It's worse than that ... by being able to call dequeue_signal from the
contxt of another thread than the one dequeuing from.
Ben.
On Tue, 5 Jun 2007, Benjamin Herrenschmidt wrote:
> 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.
>>
> - 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 agree with Ben that recalc_sigpending_tsk() on tsk!=current is a bad
idea. I think, since dequeue_signal() is called from user context, we
could just have only recalc_sigpending(), that acts on current, and then
inside dequeue_signal we have:
if (tsk == current)
recalc_sigpending();
What can happen, is that a task may notice TIF_SIGPENDING and not find a
signal once it calls dequeue_signal(), but this is fine as far as signalfd
goes. This should be OK in general, no?
- Davide
On Tue, 2007-06-05 at 08:52 -0700, Davide Libenzi wrote:
> if (tsk == current)
> recalc_sigpending();
>
> What can happen, is that a task may notice TIF_SIGPENDING and not find
> a
> signal once it calls dequeue_signal(), but this is fine as far as
> signalfd
> goes. This should be OK in general, no?
What about the code in __dequeue_signal though ? That notifier thing is
used by the DRI though I'm not sure what would happen if it acts on the
wrong task.
Ben.
On Wed, 6 Jun 2007, Benjamin Herrenschmidt wrote:
> On Tue, 2007-06-05 at 08:52 -0700, Davide Libenzi wrote:
> > if (tsk == current)
> > recalc_sigpending();
> >
> > What can happen, is that a task may notice TIF_SIGPENDING and not find
> > a
> > signal once it calls dequeue_signal(), but this is fine as far as
> > signalfd
> > goes. This should be OK in general, no?
>
> What about the code in __dequeue_signal though ? That notifier thing is
> used by the DRI though I'm not sure what would happen if it acts on the
> wrong task.
Hmm, looking at the comments in block_all_signals(), it seems that they're
interested in the fact that a specific task dequeue the signal. So, at
a first sight, it seems that such code should not not be executed if
another task dequeue the message. What do you think?
- Davide
On Tue, 2007-06-05 at 15:50 -0700, Davide Libenzi wrote:
> > What about the code in __dequeue_signal though ? That notifier thing
> is
> > used by the DRI though I'm not sure what would happen if it acts on
> the
> > wrong task.
>
> Hmm, looking at the comments in block_all_signals(), it seems that
> they're
> interested in the fact that a specific task dequeue the signal. So,
> at
> a first sight, it seems that such code should not not be executed if
> another task dequeue the message. What do you think?
Yes, I think the idea is that the DRM uses that to prevent signals to be
delivered to the task that is blocking them with the notifier (I have no
idea why they can't use the normal block mecanism for that... looks like
a hack to me).
So I suppose it's fine, as long as you add a test of tsk == current to
avoid calling it.
Ben.
On Tue, 2007-06-05 at 17:27 +1000, Benjamin Herrenschmidt wrote:
> On Mon, 2007-06-04 at 23:09 -0700, Nicholas Miell wrote:
> > signalfd() doesn't deliver thread-targeted signals to the wrong
> > threads,
> > does it?
> >
> > Hmm.
> >
> > It looks like reading from a signalfd will give you either
> > process-global signals or the thread-specific signals that are
> > targeted
> > towards the thread that originally created the signalfd (regardless of
> > which thread actually calls read()).
> >
> > Which is weird, to say the least. Definitely needs to be noted in the
> > man page, which doesn't seem to exist yet.
> >
> > Is there a reason why signalfd() doesn't behave like regular signals
> > in
> > this regard?
>
> It's worse than that ... by being able to call dequeue_signal from the
> contxt of another thread than the one dequeuing from.
>
> Ben.
Yes, that's certainly wrong, but that's an implementation issue. I was
more concerned about the design of the API.
Naively, I would expect a reads on a signalfd to return either process
signals or thread signals targeted towards the thread doing the read.
What it actually does (delivering process signals or thread signals
targeted towards the thread that created the signalfd) is weird.
For one, it means you can't create a single signalfd, stick it in an
epoll set, and then wait on that set from multiple threads.
--
Nicholas Miell <[email protected]>
On Tue, 2007-06-05 at 16:51 -0700, Nicholas Miell wrote:
> Yes, that's certainly wrong, but that's an implementation issue. I was
> more concerned about the design of the API.
>
> Naively, I would expect a reads on a signalfd to return either process
> signals or thread signals targeted towards the thread doing the read.
>
> What it actually does (delivering process signals or thread signals
> targeted towards the thread that created the signalfd) is weird.
>
> For one, it means you can't create a single signalfd, stick it in an
> epoll set, and then wait on that set from multiple threads.
Heh, well, I'll let you discuss that apsect with Davide. Right now, I'm
just trying to make sure that the implementation in the kernel copes :-)
Ben.
On Tue, 5 Jun 2007, Nicholas Miell wrote:
> Yes, that's certainly wrong, but that's an implementation issue. I was
> more concerned about the design of the API.
>
> Naively, I would expect a reads on a signalfd to return either process
> signals or thread signals targeted towards the thread doing the read.
>
> What it actually does (delivering process signals or thread signals
> targeted towards the thread that created the signalfd) is weird.
>
> For one, it means you can't create a single signalfd, stick it in an
> epoll set, and then wait on that set from multiple threads.
In your box threads do share the sighand, don't they? :)
- Davide
On Wed, 6 Jun 2007, Benjamin Herrenschmidt wrote:
> On Tue, 2007-06-05 at 15:50 -0700, Davide Libenzi wrote:
> > > What about the code in __dequeue_signal though ? That notifier thing
> > is
> > > used by the DRI though I'm not sure what would happen if it acts on
> > the
> > > wrong task.
> >
> > Hmm, looking at the comments in block_all_signals(), it seems that
> > they're
> > interested in the fact that a specific task dequeue the signal. So,
> > at
> > a first sight, it seems that such code should not not be executed if
> > another task dequeue the message. What do you think?
>
> Yes, I think the idea is that the DRM uses that to prevent signals to be
> delivered to the task that is blocking them with the notifier (I have no
> idea why they can't use the normal block mecanism for that... looks like
> a hack to me).
>
> So I suppose it's fine, as long as you add a test of tsk == current to
> avoid calling it.
Are you going patchwise, or should I do it?
- Davide
On Tue, 2007-06-05 at 17:11 -0700, Davide Libenzi wrote:
> On Tue, 5 Jun 2007, Nicholas Miell wrote:
>
> > Yes, that's certainly wrong, but that's an implementation issue. I was
> > more concerned about the design of the API.
> >
> > Naively, I would expect a reads on a signalfd to return either process
> > signals or thread signals targeted towards the thread doing the read.
> >
> > What it actually does (delivering process signals or thread signals
> > targeted towards the thread that created the signalfd) is weird.
> >
> > For one, it means you can't create a single signalfd, stick it in an
> > epoll set, and then wait on that set from multiple threads.
>
> In your box threads do share the sighand, don't they? :)
>
I have no idea what you're trying to say, but it doesn't appear to
address the issue I raise.
--
Nicholas Miell <[email protected]>
On Tue, 5 Jun 2007, Nicholas Miell wrote:
> On Tue, 2007-06-05 at 17:11 -0700, Davide Libenzi wrote:
> > On Tue, 5 Jun 2007, Nicholas Miell wrote:
> >
> > > Yes, that's certainly wrong, but that's an implementation issue. I was
> > > more concerned about the design of the API.
> > >
> > > Naively, I would expect a reads on a signalfd to return either process
> > > signals or thread signals targeted towards the thread doing the read.
> > >
> > > What it actually does (delivering process signals or thread signals
> > > targeted towards the thread that created the signalfd) is weird.
> > >
> > > For one, it means you can't create a single signalfd, stick it in an
> > > epoll set, and then wait on that set from multiple threads.
> >
> > In your box threads do share the sighand, don't they? :)
> >
>
> I have no idea what you're trying to say, but it doesn't appear to
> address the issue I raise.
"For one, it means you can't create a single signalfd, stick it in an
epoll set, and then wait on that set from multiple threads."
Why not?
A signalfd, like I said, is attached to the sighand, that is shared by the
threads.
- Davide
On Tue, 2007-06-05 at 17:37 -0700, Davide Libenzi wrote:
> On Tue, 5 Jun 2007, Nicholas Miell wrote:
>
> > On Tue, 2007-06-05 at 17:11 -0700, Davide Libenzi wrote:
> > > On Tue, 5 Jun 2007, Nicholas Miell wrote:
> > >
> > > > Yes, that's certainly wrong, but that's an implementation issue. I was
> > > > more concerned about the design of the API.
> > > >
> > > > Naively, I would expect a reads on a signalfd to return either process
> > > > signals or thread signals targeted towards the thread doing the read.
> > > >
> > > > What it actually does (delivering process signals or thread signals
> > > > targeted towards the thread that created the signalfd) is weird.
> > > >
> > > > For one, it means you can't create a single signalfd, stick it in an
> > > > epoll set, and then wait on that set from multiple threads.
> > >
> > > In your box threads do share the sighand, don't they? :)
> > >
> >
> > I have no idea what you're trying to say, but it doesn't appear to
> > address the issue I raise.
>
> "For one, it means you can't create a single signalfd, stick it in an
> epoll set, and then wait on that set from multiple threads."
>
> Why not?
> A signalfd, like I said, is attached to the sighand, that is shared by the
> threads.
>
>
POSIX requires the following:
"At the time of generation, a determination shall be made whether the
signal has been generated for the process or for a specific thread
within the process. Signals which are generated by some action
attributable to a particular thread, such as a hardware fault, shall be
generated for the thread that caused the signal to be generated. Signals
that are generated in association with a process ID or process group ID
or an asynchronous event, such as terminal activity, shall be generated
for the process."
In practice, this means that signals like SIGSEGV/SIGFPE/SIGILL/etc. and
signals generated by pthread_kill() (i.e. tkill() or tgkill()) are
directed to a specific threads, while other signals are directed to the
process as a whole and serviced by any thread that isn't blocking that
specific signal.
Linux accomplishes this by having two lists of pending signals --
current->pending is the per-thread list and
current->signal->shared_pending is the process-wide list.
dequeue_signal(tsk, ...) looks for signals first in tsk->pending and
then in tsk->signal->shared_pending.
sys_signalfd() stores current in signalfd_ctx. signalfd_read() passes
that context to signalfd_dequeue, which passes that that saved
task_struct pointer to dequeue_signal.
This means that a signalfd will deliver signals targeted towards either
the original thread that created that signalfd, or signals targeted
towards the process as a whole.
This means that a single signalfd is not adequate to handle signal
delivery for all threads in a process, because signals targeted towards
threads other than the thread that originally created the signalfd will
never be queued to that signalfd.
Is my analysis wrong?
--
Nicholas Miell <[email protected]>
On Tue, 2007-06-05 at 17:58 -0700, Nicholas Miell wrote:
>
> "At the time of generation, a determination shall be made whether the
> signal has been generated for the process or for a specific thread
> within the process. Signals which are generated by some action
> attributable to a particular thread, such as a hardware fault, shall
> be generated for the thread that caused the signal to be generated.
Yeah, synchronous signals should probably never be delivered to another
process, even via signalfd. There's no point delivering a SEGV to
somebody else :-)
I'm actually thinking we shoud -also- only handle shared signals in
dequeue_signal() when called from a different task.
> dequeue_signal(tsk, ...) looks for signals first in tsk->pending and
> then in tsk->signal->shared_pending.
>
> sys_signalfd() stores current in signalfd_ctx. signalfd_read() passes
> that context to signalfd_dequeue, which passes that that saved
> task_struct pointer to dequeue_signal.
>
> This means that a signalfd will deliver signals targeted towards
> either the original thread that created that signalfd, or signals
> targeted towards the process as a whole.
>
> This means that a single signalfd is not adequate to handle signal
> delivery for all threads in a process, because signals targeted
> towards threads other than the thread that originally created the
> signalfd will never be queued to that signalfd.
Well.. you certainly need to instanciate a signalfd for every thread in
the process if you want to get shared signals for sure.
BTW. Not directly related, but that notifier thing ... it looks really
really dodgy. It's also only ever used by the DRM. Somebody around knows
why that's in and why the DRM cannot just use normal signal blocking
techniques ?
Cheers,
Ben.
On Wed, 6 Jun 2007, Benjamin Herrenschmidt wrote:
> On Tue, 2007-06-05 at 17:58 -0700, Nicholas Miell wrote:
> >
> > "At the time of generation, a determination shall be made whether the
> > signal has been generated for the process or for a specific thread
> > within the process. Signals which are generated by some action
> > attributable to a particular thread, such as a hardware fault, shall
> > be generated for the thread that caused the signal to be generated.
>
> Yeah, synchronous signals should probably never be delivered to another
> process, even via signalfd. There's no point delivering a SEGV to
> somebody else :-)
That'd be a limitation. Like you can choose to not handle SEGV, you can
choose to have a signalfd listening to it. Of course, not with the
intention to *handle* the signal, but with a notification intent.
> I'm actually thinking we shoud -also- only handle shared signals in
> dequeue_signal() when called from a different task.
Why do you want to impose this? signalfd is a "sniffer", and the user
controls what it can dequeue/sniff or what not. I don't see a reason of
imposing such limits, unless there're clear technical issues.
> > dequeue_signal(tsk, ...) looks for signals first in tsk->pending and
> > then in tsk->signal->shared_pending.
> >
> > sys_signalfd() stores current in signalfd_ctx. signalfd_read() passes
> > that context to signalfd_dequeue, which passes that that saved
> > task_struct pointer to dequeue_signal.
> >
> > This means that a signalfd will deliver signals targeted towards
> > either the original thread that created that signalfd, or signals
> > targeted towards the process as a whole.
> >
> > This means that a single signalfd is not adequate to handle signal
> > delivery for all threads in a process, because signals targeted
> > towards threads other than the thread that originally created the
> > signalfd will never be queued to that signalfd.
>
> Well.. you certainly need to instanciate a signalfd for every thread in
> the process if you want to get shared signals for sure.
Why? Or better, what do you mean for "instanciate"?
- Davide
On Tue, 5 Jun 2007, Davide Libenzi wrote:
> On Wed, 6 Jun 2007, Benjamin Herrenschmidt wrote:
> >
> > Yeah, synchronous signals should probably never be delivered to another
> > process, even via signalfd. There's no point delivering a SEGV to
> > somebody else :-)
>
> That'd be a limitation. Like you can choose to not handle SEGV, you can
> choose to have a signalfd listening to it. Of course, not with the
> intention to *handle* the signal, but with a notification intent.
I agree that it would be a limitation, but it would be a sane one.
How about we try to live with that limitation, if only to avoid the issue
of having the private signals being stolen by anybody else. If we actually
find a real-live use-case where that is bad in the future, we can re-visit
the issue - it's always easier to _expand_ semantics later than it is to
restrict them, so I think this thread is a good argument for starting it
out in a more restricted form before people start depending on semantics
that can be nasty..
Linus
> That'd be a limitation. Like you can choose to not handle SEGV, you can
> choose to have a signalfd listening to it. Of course, not with the
> intention to *handle* the signal, but with a notification intent.
Hrm.. either you handle it or you are dead ... I fail to see how
signalfd can sneak in to catch it just at the right time...
> > I'm actually thinking we shoud -also- only handle shared signals in
> > dequeue_signal() when called from a different task.
>
> Why do you want to impose this? signalfd is a "sniffer", and the user
> controls what it can dequeue/sniff or what not. I don't see a reason of
> imposing such limits, unless there're clear technical issues.
Well, a synchronous signal such a SIGSEGV, SIGILL or SIGFPE generally
means that execution cannot continue unless the signal handler does
something about it...
I think you are opening a whole can of worms here.
> > Well.. you certainly need to instanciate a signalfd for every thread in
> > the process if you want to get shared signals for sure.
>
> Why? Or better, what do you mean for "instanciate"?
Well, because the kernel makes the decision of which thread to target
the signal for a shared signal at emission time (though it -can- be
caught by another thread).
/me reads more code to be sure..
Oh well, a read from signalfd created on one thread -will- catch any
shared signal that was already pending, whatever thread the kernel
decided to target it at, it seems (that is, whatever thread actually got
TIF_SIGPENDING set), but will only catch private signals for _that_
thread (and I still think catching private signals is a wrong thing).
However, I'm not sure about the wakeup condition. signalfd_deliver will
wakeup anybody in the signalfd_list, which is -not- whoever is blocked
in signalfd_read() unless I'm missing something.
For your scheme to work, signalfd_read() you probably need to keep a
separate list of people to be nofified of signals and add current to it
from signalfd_list().
Ben.
On Tue, 2007-06-05 at 20:37 -0700, Linus Torvalds wrote:
>
> On Tue, 5 Jun 2007, Davide Libenzi wrote:
> > On Wed, 6 Jun 2007, Benjamin Herrenschmidt wrote:
> > >
> > > Yeah, synchronous signals should probably never be delivered to another
> > > process, even via signalfd. There's no point delivering a SEGV to
> > > somebody else :-)
> >
> > That'd be a limitation. Like you can choose to not handle SEGV, you can
> > choose to have a signalfd listening to it. Of course, not with the
> > intention to *handle* the signal, but with a notification intent.
>
> I agree that it would be a limitation, but it would be a sane one.
>
> How about we try to live with that limitation, if only to avoid the issue
> of having the private signals being stolen by anybody else. If we actually
> find a real-live use-case where that is bad in the future, we can re-visit
> the issue - it's always easier to _expand_ semantics later than it is to
> restrict them, so I think this thread is a good argument for starting it
> out in a more restricted form before people start depending on semantics
> that can be nasty..
>
> Linus
Proposed semantics:
a) Process-global signals can be read by any thread (inside or outside
of the process receiving the signal).
Rationale:
This should always work, so there's no reason to limit it.
b) Thread-specific signals can only be read by their target thread.
Rationale:
This behavior is required by POSIX, and if an application is using
pthread_kill()/tkill()/tgkill()/etc. to specifically direct a signal, it
damn well better get to where the app wants it to go.
c) Synchronous signals ("Naturally" generated SIGILL, SIGFPE, SIGSEGV,
SIGBUS, and SIGTRAP. Did I miss any?) are not delivered via signalfd()
at all. (And by "naturally" generated, I mean signals that would have
the SI_KERNEL flag set.)
Rationale:
These are a subset of thread-specific signals, so they can only be read
from a signalfd by their target thread.
However, there's no way for the target thread to get the signal because
it is either:
a) not blocked in a syscall waiting for signal delivery and thus further
execution beyond the instruction causing the signal is impossible
OR
b) it is blocked in a syscall waiting for signal delivery and the error
is caused by the signal delivery mechanism itself (i.e. a bad pointer
passed to read/select/poll/epoll_wait/etc.) and thus the signal can't be
delivered
--
Nicholas Miell <[email protected]>
> a) Process-global signals can be read by any thread (inside or outside
> of the process receiving the signal).
>
> Rationale:
> This should always work, so there's no reason to limit it.
I agree, with an appropriate fix to recalc_sigpending_tsk() to only
clear TIF_SIGPENDING if tsk == current (the patch Linus posted
basically) _along_ with a fix to avoid the notifier thingy if stealing
from another task, that would work.
> b) Thread-specific signals can only be read by their target thread.
>
> Rationale:
> This behavior is required by POSIX, and if an application is using
> pthread_kill()/tkill()/tgkill()/etc. to specifically direct a signal, it
> damn well better get to where the app wants it to go.
I agree there too. I don't see the point of the 'feature' of allowing
those to be stolen and can only lead into all sort of new headaches
nobody needs.
> c) Synchronous signals ("Naturally" generated SIGILL, SIGFPE, SIGSEGV,
> SIGBUS, and SIGTRAP. Did I miss any?) are not delivered via signalfd()
> at all. (And by "naturally" generated, I mean signals that would have
> the SI_KERNEL flag set.)
Heh, well, as you say later, it can't be delivered anyway... I don't
think we need to do anything explicit to prevent them from being read()
in signalfd, it will just not happen.
> Rationale:
> These are a subset of thread-specific signals, so they can only be read
> from a signalfd by their target thread.
>
> However, there's no way for the target thread to get the signal because
> it is either:
>
> a) not blocked in a syscall waiting for signal delivery and thus further
> execution beyond the instruction causing the signal is impossible
> OR
> b) it is blocked in a syscall waiting for signal delivery and the error
> is caused by the signal delivery mechanism itself (i.e. a bad pointer
> passed to read/select/poll/epoll_wait/etc.) and thus the signal can't be
> delivered
Ben.
On Tue, 5 Jun 2007, Linus Torvalds wrote:
> On Tue, 5 Jun 2007, Davide Libenzi wrote:
> > On Wed, 6 Jun 2007, Benjamin Herrenschmidt wrote:
> > >
> > > Yeah, synchronous signals should probably never be delivered to another
> > > process, even via signalfd. There's no point delivering a SEGV to
> > > somebody else :-)
> >
> > That'd be a limitation. Like you can choose to not handle SEGV, you can
> > choose to have a signalfd listening to it. Of course, not with the
> > intention to *handle* the signal, but with a notification intent.
>
> I agree that it would be a limitation, but it would be a sane one.
>
> How about we try to live with that limitation, if only to avoid the issue
> of having the private signals being stolen by anybody else. If we actually
> find a real-live use-case where that is bad in the future, we can re-visit
> the issue - it's always easier to _expand_ semantics later than it is to
> restrict them, so I think this thread is a good argument for starting it
> out in a more restricted form before people start depending on semantics
> that can be nasty..
Yeah, that's easy. We can exclude them at signalfd creation time.
- Davide
On Tue, 2007-06-05 at 20:37 -0700, Linus Torvalds wrote:
> I agree that it would be a limitation, but it would be a sane one.
>
> How about we try to live with that limitation, if only to avoid the issue
> of having the private signals being stolen by anybody else. If we actually
> find a real-live use-case where that is bad in the future, we can re-visit
> the issue - it's always easier to _expand_ semantics later than it is to
> restrict them, so I think this thread is a good argument for starting it
> out in a more restricted form before people start depending on semantics
> that can be nasty..
Here's a patch. Let me know if I missed something.
Fix races with signalfd and TIF_SIGPENDING
We must never clear TIF_SIGPENDING for another task. This patch
ensures that by preventing recalc_sigpending_tsk() from clearing
that bit if the target task is not current.
In addition we also prevent __dequeue_signal() from calling the
DRM notifier thingy when stealing signals from another task via
signalfd.
Finally, we only dequeue shared signals when called from another
task (via signalfd), we leave private signals alone.
Signed-off-by: Benjamin Herrenschmidt <[email protected]>
---
Index: linux-work/kernel/signal.c
===================================================================
--- linux-work.orig/kernel/signal.c 2007-06-06 16:38:05.000000000 +1000
+++ linux-work/kernel/signal.c 2007-06-06 16:42:43.000000000 +1000
@@ -105,7 +105,12 @@ static int recalc_sigpending_tsk(struct
set_tsk_thread_flag(t, TIF_SIGPENDING);
return 1;
}
- clear_tsk_thread_flag(t, TIF_SIGPENDING);
+ /* Only clear the flag when this is issued by the target task to
+ * clearing TIF_SIGPENDING after the target task decided to return
+ * -ERESTARTSYS from a syscall
+ */
+ if (t == current)
+ clear_tsk_thread_flag(t, TIF_SIGPENDING);
return 0;
}
@@ -328,12 +333,12 @@ static int collect_signal(int sig, struc
}
static int __dequeue_signal(struct sigpending *pending, sigset_t *mask,
- siginfo_t *info)
+ siginfo_t *info, int stealing)
{
int sig = next_signal(pending, mask);
if (sig) {
- if (current->notifier) {
+ if (current->notifier && !stealing) {
if (sigismember(current->notifier_mask, sig)) {
if (!(current->notifier)(current->notifier_data)) {
clear_thread_flag(TIF_SIGPENDING);
@@ -357,10 +362,19 @@ static int __dequeue_signal(struct sigpe
*/
int dequeue_signal(struct task_struct *tsk, sigset_t *mask, siginfo_t *info)
{
- int signr = __dequeue_signal(&tsk->pending, mask, info);
+ int stealing = tsk != current;
+ int signr = 0;
+
+ /* Only dequeue private signals if we are the owner, not when signals
+ * are being stolen by another task via signalfd
+ */
+ if (!stealing)
+ signr = __dequeue_signal(&tsk->pending, mask, info, 0);
+
+ /* No private signal, look for shared ones */
if (!signr) {
signr = __dequeue_signal(&tsk->signal->shared_pending,
- mask, info);
+ mask, info, stealing);
/*
* itimer signal ?
*
On Wed, Jun 06, 2007 at 12:50:04PM +1000, Benjamin Herrenschmidt wrote:
> Yeah, synchronous signals should probably never be delivered to another
> process, even via signalfd. There's no point delivering a SEGV to
> somebody else :-)
Sure there is. UML does exactly that - intercepting child signals
(including SEGV) with wait.
Jeff
--
Work email - jdike at linux dot intel dot com
On Wed, 6 Jun 2007, Benjamin Herrenschmidt wrote:
> On Tue, 2007-06-05 at 20:37 -0700, Linus Torvalds wrote:
>
> > I agree that it would be a limitation, but it would be a sane one.
> >
> > How about we try to live with that limitation, if only to avoid the issue
> > of having the private signals being stolen by anybody else. If we actually
> > find a real-live use-case where that is bad in the future, we can re-visit
> > the issue - it's always easier to _expand_ semantics later than it is to
> > restrict them, so I think this thread is a good argument for starting it
> > out in a more restricted form before people start depending on semantics
> > that can be nasty..
>
> Here's a patch. Let me know if I missed something.
>
> Fix races with signalfd and TIF_SIGPENDING
>
> We must never clear TIF_SIGPENDING for another task. This patch
> ensures that by preventing recalc_sigpending_tsk() from clearing
> that bit if the target task is not current.
>
> In addition we also prevent __dequeue_signal() from calling the
> DRM notifier thingy when stealing signals from another task via
> signalfd.
>
> Finally, we only dequeue shared signals when called from another
> task (via signalfd), we leave private signals alone.
Makes sense...
> Signed-off-by: Benjamin Herrenschmidt <[email protected]>
Acked-by: Davide Libenzi <[email protected]>
- Davide
Jeff Dike writes:
> On Wed, Jun 06, 2007 at 12:50:04PM +1000, Benjamin Herrenschmidt wrote:
> > Yeah, synchronous signals should probably never be delivered to another
> > process, even via signalfd. There's no point delivering a SEGV to
> > somebody else :-)
>
> Sure there is. UML does exactly that - intercepting child signals
> (including SEGV) with wait.
What Ben was talking about was stealing a synchronous SEGV from a task
without stopping it, and as Ben says that makes no sense.
Intercepting a signal and stopping the task is reasonable, and that is
what ptrace does, and I assume also UML.
Paul.
On Thu, Jun 07, 2007 at 08:43:42AM +1000, Paul Mackerras wrote:
> What Ben was talking about was stealing a synchronous SEGV from a task
> without stopping it, and as Ben says that makes no sense.
> Intercepting a signal and stopping the task is reasonable, and that is
> what ptrace does, and I assume also UML.
It is, but I can also see UML stealing the SEGV from the child. The
UML skas does this - a ptrace extension, PTRACE_FAULTINFO, is used to
extract page fault information from the child, and other pieces of the
patch are used to fix the fault without the child continuing until
it's fixed. So, in this case, the child never sees the SEGV.
Jeff
--
Work email - jdike at linux dot intel dot com
On Wed, 2007-06-06 at 08:52 -0400, Jeff Dike wrote:
> On Wed, Jun 06, 2007 at 12:50:04PM +1000, Benjamin Herrenschmidt wrote:
> > Yeah, synchronous signals should probably never be delivered to another
> > process, even via signalfd. There's no point delivering a SEGV to
> > somebody else :-)
>
> Sure there is. UML does exactly that - intercepting child signals
> (including SEGV) with wait.
UML is definitely what I call a special case :-) Now the question is how
do you get them ? Are you via some code path I haven't figured out
calling dequeue_signal() from another context ?
Ben.
On Wed, 2007-06-06 at 22:20 -0400, Jeff Dike wrote:
> On Thu, Jun 07, 2007 at 08:43:42AM +1000, Paul Mackerras wrote:
> > What Ben was talking about was stealing a synchronous SEGV from a task
> > without stopping it, and as Ben says that makes no sense.
> > Intercepting a signal and stopping the task is reasonable, and that is
> > what ptrace does, and I assume also UML.
>
> It is, but I can also see UML stealing the SEGV from the child. The
> UML skas does this - a ptrace extension, PTRACE_FAULTINFO, is used to
> extract page fault information from the child, and other pieces of the
> patch are used to fix the fault without the child continuing until
> it's fixed. So, in this case, the child never sees the SEGV.
But you use ptrace and don't steal signals with dequeue_signal() on a
live other task, which is ok.
Ben.
On Thu, Jun 07, 2007 at 01:29:23PM +1000, Benjamin Herrenschmidt wrote:
> But you use ptrace and don't steal signals with dequeue_signal() on a
> live other task, which is ok.
True. However, with an SMP UML running a threaded app (which is also
threads on the host), if the thread on one CPU gets a signal which
will be nullified (SIGTRAP, SIG*ALRM, later SIGSEGV) and the other
thread continues running, does that count as stealing a signal from
another live task?
Jeff
--
Work email - jdike at linux dot intel dot com