LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [RFC,PATCH 1/2] signals: re-assign CLD_CONTINUED notification from the sender to reciever
@ 2008-03-04 18:57 Oleg Nesterov
  2008-03-06 10:53 ` Roland McGrath
  0 siblings, 1 reply; 3+ messages in thread
From: Oleg Nesterov @ 2008-03-04 18:57 UTC (permalink / raw)
  To: Andrew Morton, Roland McGrath
  Cc: Eric W. Biederman, Davide Libenzi, Ingo Molnar, Jiri Kosina,
	Linus Torvalds, Pavel Emelyanov, linux-kernel

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 <oleg@tv-sign.ru>

 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(&current->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(&current->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;
 


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [RFC,PATCH 1/2] signals: re-assign CLD_CONTINUED notification from the sender to reciever
  2008-03-04 18:57 [RFC,PATCH 1/2] signals: re-assign CLD_CONTINUED notification from the sender to reciever Oleg Nesterov
@ 2008-03-06 10:53 ` Roland McGrath
  2008-03-07  2:39   ` Oleg Nesterov
  0 siblings, 1 reply; 3+ messages in thread
From: Roland McGrath @ 2008-03-06 10:53 UTC (permalink / raw)
  To: Oleg Nesterov
  Cc: Andrew Morton, Eric W. Biederman, Davide Libenzi, Ingo Molnar,
	Jiri Kosina, Linus Torvalds, Pavel Emelyanov, linux-kernel

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

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [RFC,PATCH 1/2] signals: re-assign CLD_CONTINUED notification from the sender to reciever
  2008-03-06 10:53 ` Roland McGrath
@ 2008-03-07  2:39   ` Oleg Nesterov
  0 siblings, 0 replies; 3+ messages in thread
From: Oleg Nesterov @ 2008-03-07  2:39 UTC (permalink / raw)
  To: Roland McGrath
  Cc: Andrew Morton, Eric W. Biederman, Davide Libenzi, Ingo Molnar,
	Jiri Kosina, Linus Torvalds, Pavel Emelyanov, linux-kernel

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.


^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2008-03-07  2:40 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2008-03-04 18:57 [RFC,PATCH 1/2] signals: re-assign CLD_CONTINUED notification from the sender to reciever Oleg Nesterov
2008-03-06 10:53 ` Roland McGrath
2008-03-07  2:39   ` Oleg Nesterov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).