Based on discussion with Jiri and Roland.
In short: currently handle_stop_signal(SIGCONT, p) sends the notification
to p->parent, with this patch p itself notifies its parent when it becomes
running.
handle_stop_signal(SIGCONT) has to drop ->siglock temporary in order to
notify the parent with do_notify_parent_cldstop(). This leads to multiple
problems:
- as Jiri Kosina pointed out, the stopped task can resume without
actually seeing SIGCONT which may have a handler.
- we race with another sig_kernel_stop() signal which may come in
that window.
- we race with sig_fatal() signals which may set SIGNAL_GROUP_EXIT
in that window.
- we can't avoid taking tasklist_lock() while sending SIGCONT.
With this patch handle_stop_signal() just sets the new SIGNAL_CLD_CONTINUED
flag in p->signal->flags and returns. The notification is sent by the first
task which returns from finish_stop() (there should be at least one) or any
other signalled thread from get_signal_to_deliver().
This is a user-visible change. Say, currently kill(SIGCONT, stopped_child)
can't return without seeing SIGCHLD, with this patch SIGCHLD can be delayed
unpredictably. Another oddity is that if the child is ptraced by another
process, CLD_CONTINUED may be delivered to ->real_parent after ptrace_detach().
Hopefully these problems are minor.
The patch asks for the futher obvious cleanups, I'll send them separately.
Signed-off-by: Oleg Nesterov <[email protected]>
include/linux/sched.h | 6 ++++++
kernel/signal.c | 29 +++++++++++++++++++----------
2 files changed, 25 insertions(+), 10 deletions(-)
--- 25/include/linux/sched.h~1_SIGCONT_IMPL 2008-03-02 15:35:23.000000000 +0300
+++ 25/include/linux/sched.h 2008-03-03 17:06:53.000000000 +0300
@@ -555,6 +555,12 @@ struct signal_struct {
#define SIGNAL_STOP_DEQUEUED 0x00000002 /* stop signal dequeued */
#define SIGNAL_STOP_CONTINUED 0x00000004 /* SIGCONT since WCONTINUED reap */
#define SIGNAL_GROUP_EXIT 0x00000008 /* group exit in progress */
+/*
+ * Pending notifications to parent.
+ */
+#define SIGNAL_CLD_STOPPED 0x00000010
+#define SIGNAL_CLD_CONTINUED 0x00000020
+#define SIGNAL_CLD_MASK (SIGNAL_CLD_STOPPED|SIGNAL_CLD_CONTINUED)
/* If true, all threads except ->group_exit_task have pending SIGKILL */
static inline int signal_group_exit(const struct signal_struct *sig)
--- 25/kernel/signal.c~1_SIGCONT_IMPL 2008-03-02 15:35:23.000000000 +0300
+++ 25/kernel/signal.c 2008-03-03 17:06:53.000000000 +0300
@@ -596,10 +596,8 @@ static void handle_stop_signal(int sig,
* the SIGCHLD was pending on entry to this kill.
*/
p->signal->group_stop_count = 0;
- p->signal->flags = SIGNAL_STOP_CONTINUED;
- spin_unlock(&p->sighand->siglock);
- do_notify_parent_cldstop(p, CLD_STOPPED);
- spin_lock(&p->sighand->siglock);
+ p->signal->flags = SIGNAL_STOP_CONTINUED |
+ SIGNAL_CLD_STOPPED;
}
rm_from_queue(SIG_KERNEL_STOP_MASK, &p->signal->shared_pending);
t = p;
@@ -636,25 +634,23 @@ static void handle_stop_signal(int sig,
* We were in fact stopped, and are now continued.
* Notify the parent with CLD_CONTINUED.
*/
- p->signal->flags = SIGNAL_STOP_CONTINUED;
+ p->signal->flags = SIGNAL_STOP_CONTINUED |
+ SIGNAL_CLD_CONTINUED;
p->signal->group_exit_code = 0;
- spin_unlock(&p->sighand->siglock);
- do_notify_parent_cldstop(p, CLD_CONTINUED);
- spin_lock(&p->sighand->siglock);
} else {
/*
* We are not stopped, but there could be a stop
* signal in the middle of being processed after
* being removed from the queue. Clear that too.
*/
- p->signal->flags = 0;
+ p->signal->flags &= ~SIGNAL_STOP_DEQUEUED;
}
} else if (sig == SIGKILL) {
/*
* Make sure that any pending stop signal already dequeued
* is undone by the wakeup for SIGKILL.
*/
- p->signal->flags = 0;
+ p->signal->flags &= ~SIGNAL_STOP_DEQUEUED;
}
}
@@ -1758,6 +1754,19 @@ int get_signal_to_deliver(siginfo_t *inf
relock:
spin_lock_irq(¤t->sighand->siglock);
+
+ if (unlikely(current->signal->flags & SIGNAL_CLD_MASK)) {
+ int why = (current->signal->flags & SIGNAL_STOP_CONTINUED)
+ ? CLD_CONTINUED : CLD_STOPPED;
+ current->signal->flags &= ~SIGNAL_CLD_MASK;
+ spin_unlock_irq(¤t->sighand->siglock);
+
+ read_lock(&tasklist_lock);
+ do_notify_parent_cldstop(current->group_leader, why);
+ read_unlock(&tasklist_lock);
+ goto relock;
+ }
+
for (;;) {
struct k_sigaction *ka;
I like this version fine, but I would like to see that comment in
get_signal_to_deliver added in a cleanup patch.
Thanks,
Roland
On 03/06, Roland McGrath wrote:
>
> I like this version fine, but I would like to see that comment in
> get_signal_to_deliver added in a cleanup patch.
Yes. Will do.
>From another message:
>
> > Perhaps, we can add the new helper which sets SIGNAL_GROUP_EXIT but preserves
> > SIGNAL_CLD_MASK, but I don't think this is really needed.
>
> Actually, I think it is. This made think of another pedantic concern.
> (Not that I'm sure we get this right now anyway.)
This "I don't think this is really needed" above meant that the patch doesn't
make things worse than we currently have, but
> Say a child was previously stopped, parent had seen CLD_STOPPED and done
> WSTOPPED wait. Now we send SIGCONT followed shortly by SIGTERM (unhandled
> fatal signal). In the interval before the SIGTERM has yet been dequeued
> (nor SIGKILLs sent to other threads by __group_complete_signal), the parent
> does waitpid(pid, WSTOPPED|WCONTINUED|WEXITED|WNOHANG). It should see one
> of: WIFSTOPPED (still); WIFSIGNALED (dead); WIFCONTINUED. Instead, waitpid
> will return 0 (it's running, not stopped, not continued). This says
> SIGNAL_STOP_CONTINUED ought to function as normal (i.e. WCONTINUED polling)
> until we're actually ready for wait to report the process as dead.
Good point, completely agreed. (except I think it doesn't matter was that
fatal signal dequeued or not).
> We already break this pedantic nit because flags = SIGNAL_GROUP_EXIT clears
> SIGNAL_STOP_CONTINUED. But if we clean up all flags usage, I think we
> should make it preserve SIGNAL_STOP_CONTINUED.
>
> But if we were to make wait key on SIGNAL_STOP_STOPPED, as is
> probably necessary to do WSTOPPED waits for processes with a zombie
> group_leader, then this too could be made consistent. (Leave the
> SIGNAL_STOP_STOPPED bit set throughout, so WSTOPPED|WNOHANG waits continues
> to report it as stopped until it actually dies.)
Yes, thanks. Will try to do a bit later.
Oleg.