+ signals-tracehook_notify_jctl-change.patch added to -mm tree

+ signals-tracehook_notify_jctl-change.patch added to -mm tree

[akpm]

The patch titled
     signals: tracehook_notify_jctl change
has been added to the -mm tree.  Its filename is
     signals-tracehook_notify_jctl-change.patch

Before you just go and hit "reply", please:
   a) Consider who else should be cc'ed
   b) Prefer to cc a suitable mailing list as well
   c) Ideally: find the original patch on the mailing list and do a
      reply-to-all to that, adding suitable additional cc's

*** Remember to use Documentation/SubmitChecklist when testing your code ***

See http://userweb.kernel.org/~akpm/stuff/added-to-mm.txt to find
out what to do about this

The current -mm tree may be found at http://userweb.kernel.org/~akpm/mmotm/

------------------------------------------------------
Subject: signals: tracehook_notify_jctl change
From: Roland McGrath <roland@redhat.com>

This changes tracehook_notify_jctl() so it's called with the siglock held,
and changes its argument and return value definition.  These clean-ups
make it a better fit for what new tracing hooks need to check.

Tracing needs the siglock here, held from the time TASK_STOPPED was set,
to avoid potential SIGCONT races if it wants to allow any blocking in its
tracing hooks.

This also folds the finish_stop() function into its caller
do_signal_stop().  The function is short, called only once and only
unconditionally.  It aids readability to fold it in.

Signed-off-by: Roland McGrath <roland@redhat.com>
Cc: Oleg Nesterov <oleg@redhat.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 include/linux/tracehook.h |   25 ++++++++-----
 kernel/signal.c           |   69 ++++++++++++++++++------------------
 2 files changed, 51 insertions(+), 43 deletions(-)

diff -puN include/linux/tracehook.h~signals-tracehook_notify_jctl-change include/linux/tracehook.h
--- a/include/linux/tracehook.h~signals-tracehook_notify_jctl-change
+++ a/include/linux/tracehook.h
@@ -1,7 +1,7 @@
 /*
  * Tracing hooks
  *
- * Copyright (C) 2008 Red Hat, Inc.  All rights reserved.
+ * Copyright (C) 2008-2009 Red Hat, Inc.  All rights reserved.
  *
  * This copyrighted material is made available to anyone wishing to use,
  * modify, copy, or redistribute it subject to the terms and conditions
@@ -464,22 +464,29 @@ static inline int tracehook_get_signal(s
 
 /**
  * tracehook_notify_jctl - report about job control stop/continue
- * @notify:		nonzero if this is the last thread in the group to stop
+ * @notify:		zero, %CLD_STOPPED or %CLD_CONTINUED
  * @why:		%CLD_STOPPED or %CLD_CONTINUED
  *
  * This is called when we might call do_notify_parent_cldstop().
- * It's called when about to stop for job control; we are already in
- * %TASK_STOPPED state, about to call schedule().  It's also called when
- * a delayed %CLD_STOPPED or %CLD_CONTINUED report is ready to be made.
  *
- * Return nonzero to generate a %SIGCHLD with @why, which is
- * normal if @notify is nonzero.
+ * @notify is zero if we would not ordinarily send a %SIGCHLD,
+ * or is the %CLD_STOPPED or %CLD_CONTINUED .si_code for %SIGCHLD.
  *
- * Called with no locks held.
+ * @why is %CLD_STOPPED when about to stop for job control;
+ * we are already in %TASK_STOPPED state, about to call schedule().
+ * It might also be that we have just exited (check %PF_EXITING),
+ * but need to report that a group-wide stop is complete.
+ *
+ * @why is %CLD_CONTINUED when waking up after job control stop and
+ * ready to make a delayed @notify report.
+ *
+ * Return the %CLD_* value for %SIGCHLD, or zero to generate no signal.
+ *
+ * Called with the siglock held.
  */
 static inline int tracehook_notify_jctl(int notify, int why)
 {
-	return notify || (current->ptrace & PT_PTRACED);
+	return notify ?: (current->ptrace & PT_PTRACED) ? why : 0;
 }
 
 #define DEATH_REAP			-1
diff -puN kernel/signal.c~signals-tracehook_notify_jctl-change kernel/signal.c
--- a/kernel/signal.c~signals-tracehook_notify_jctl-change
+++ a/kernel/signal.c
@@ -702,7 +702,7 @@ static int prepare_signal(int sig, struc
 
 		if (why) {
 			/*
-			 * The first thread which returns from finish_stop()
+			 * The first thread which returns from do_signal_stop()
 			 * will take ->siglock, notice SIGNAL_CLD_MASK, and
 			 * notify its parent. See get_signal_to_deliver().
 			 */
@@ -1665,29 +1665,6 @@ void ptrace_notify(int exit_code)
 	spin_unlock_irq(&current->sighand->siglock);
 }
 
-static void
-finish_stop(int stop_count)
-{
-	/*
-	 * If there are no other threads in the group, or if there is
-	 * a group stop in progress and we are the last to stop,
-	 * report to the parent.  When ptraced, every thread reports itself.
-	 */
-	if (tracehook_notify_jctl(stop_count == 0, CLD_STOPPED)) {
-		read_lock(&tasklist_lock);
-		do_notify_parent_cldstop(current, CLD_STOPPED);
-		read_unlock(&tasklist_lock);
-	}
-
-	do {
-		schedule();
-	} while (try_to_freeze());
-	/*
-	 * Now we don't run again until continued.
-	 */
-	current->exit_code = 0;
-}
-
 /*
  * This performs the stopping for SIGSTOP and other stop signals.
  * We have to stop all threads in the thread group.
@@ -1698,6 +1675,7 @@ static int do_signal_stop(int signr)
 {
 	struct signal_struct *sig = current->signal;
 	int stop_count;
+	int notify;
 
 	if (sig->group_stop_count > 0) {
 		/*
@@ -1737,8 +1715,30 @@ static int do_signal_stop(int signr)
 	current->exit_code = sig->group_exit_code;
 	__set_current_state(TASK_STOPPED);
 
+	/*
+	 * If there are no other threads in the group, or if there is
+	 * a group stop in progress and we are the last to stop,
+	 * report to the parent.  When ptraced, every thread reports itself.
+	 */
+	notify = tracehook_notify_jctl(stop_count == 0 ? CLD_STOPPED : 0,
+				       CLD_STOPPED);
+
 	spin_unlock_irq(&current->sighand->siglock);
-	finish_stop(stop_count);
+
+	if (notify) {
+		read_lock(&tasklist_lock);
+		do_notify_parent_cldstop(current, notify);
+		read_unlock(&tasklist_lock);
+	}
+
+	do {
+		schedule();
+	} while (try_to_freeze());
+	/*
+	 * Now we don't run again until continued.
+	 */
+	current->exit_code = 0;
+
 	return 1;
 }
 
@@ -1807,14 +1807,15 @@ relock:
 		int why = (signal->flags & SIGNAL_STOP_CONTINUED)
 				? CLD_CONTINUED : CLD_STOPPED;
 		signal->flags &= ~SIGNAL_CLD_MASK;
-		spin_unlock_irq(&sighand->siglock);
 
-		if (unlikely(!tracehook_notify_jctl(1, why)))
-			goto relock;
+		why = tracehook_notify_jctl(why, CLD_CONTINUED);
+		spin_unlock_irq(&sighand->siglock);
 
-		read_lock(&tasklist_lock);
-		do_notify_parent_cldstop(current->group_leader, why);
-		read_unlock(&tasklist_lock);
+		if (why) {
+			read_lock(&tasklist_lock);
+			do_notify_parent_cldstop(current->group_leader, why);
+			read_unlock(&tasklist_lock);
+		}
 		goto relock;
 	}
 
@@ -1979,14 +1980,14 @@ void exit_signals(struct task_struct *ts
 	if (unlikely(tsk->signal->group_stop_count) &&
 			!--tsk->signal->group_stop_count) {
 		tsk->signal->flags = SIGNAL_STOP_STOPPED;
-		group_stop = 1;
+		group_stop = tracehook_notify_jctl(CLD_STOPPED, CLD_STOPPED);
 	}
 out:
 	spin_unlock_irq(&tsk->sighand->siglock);
 
-	if (unlikely(group_stop) && tracehook_notify_jctl(1, CLD_STOPPED)) {
+	if (unlikely(group_stop)) {
 		read_lock(&tasklist_lock);
-		do_notify_parent_cldstop(tsk, CLD_STOPPED);
+		do_notify_parent_cldstop(tsk, group_stop);
 		read_unlock(&tasklist_lock);
 	}
 }
_

Patches currently in -mm which might be from roland@redhat.com are

origin.patch
linux-next.patch
do_wait-fix-waiting-for-the-group-stop-with-the-dead-leader.patch
signals-remove-handler-parameter-to-tracehook-functions.patch
signals-protect-init-from-unwanted-signals-more.patch
signals-add-from_ancestor_ns-parameter-to-send_signal.patch
signals-protect-cinit-from-unblocked-sig_dfl-signals.patch
signals-zap_pid_ns_process-should-use-force_sig.patch
signals-protect-cinit-from-blocked-fatal-signals.patch
signals-si_user-masquerade-si_pid-when-crossing-pid-ns-boundary.patch
ptrace-kill-__ptrace_detach-fix-exit_state-check.patch
ptrace-simplify-ptrace_exit-ignoring_children-path.patch
ptrace-simplify-ptrace_exit-ignoring_children-pathpatch-fix.patch
ptrace-reintroduce-__ptrace_detach-as-a-callee-of-ptrace_exit.patch
ptrace-reintroduce-__ptrace_detach-as-a-callee-of-ptrace_exit-fix.patch
ptrace-fix-possible-zombie-leak-on-ptrace_detach.patch
reparent_thread-dont-call-kill_orphaned_pgrp-if-task_detached.patch
reparent_thread-fix-the-is-it-traced-check.patch
reparent_thread-fix-a-zombie-leak-if-sbin-init-ignores-sigchld.patch
forget_original_parent-split-out-the-un-ptrace-part.patch
forget_original_parent-do-not-abuse-child-ptrace_entry.patch
forget_original_parent-do-not-abuse-child-ptrace_entry-fix.patch
tracehook_notify_death-use-task_detached-helper.patch
ptrace_detach-the-wrong-wakeup-breaks-the-erestartxxx-logic.patch
ptrace_untrace-fix-the-signal_stop_stopped-check.patch
pids-document-task_pgrp-task_session-is-not-safe-without-tasklist-rcu.patch
pids-document-task_pgrp-task_session-is-not-safe-without-tasklist-rcu-fix.patch
pids-improve-get_task_pid-to-fix-the-unsafe-sys_wait4-task_pgrp.patch
pids-refactor-vnr-nr_ns-helpers-to-make-them-safe.patch
pids-kill-now-unused-signal_struct-__pgrp-__session-and-friends.patch
signals-tracehook_notify_jctl-change.patch
utrace-core.patch

Re: + signals-tracehook_notify_jctl-change.patch added to -mm tree

[Ingo Molnar]

* akpm@linux-foundation.org <akpm@linux-foundation.org> wrote:

> Subject: signals: tracehook_notify_jctl change
> From: Roland McGrath <roland@redhat.com>
> 
> This changes tracehook_notify_jctl() so it's called with the siglock held,
> and changes its argument and return value definition.  These clean-ups
> make it a better fit for what new tracing hooks need to check.
> 
> Tracing needs the siglock here, held from the time TASK_STOPPED was set,
> to avoid potential SIGCONT races if it wants to allow any blocking in its
> tracing hooks.
> 
> This also folds the finish_stop() function into its caller
> do_signal_stop().  The function is short, called only once and only
> unconditionally.  It aids readability to fold it in.
> 
> Signed-off-by: Roland McGrath <roland@redhat.com>
> Cc: Oleg Nesterov <oleg@redhat.com>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>

Roland: as i have asked you before, i'd like to see explicit Ack's 
from Oleg for all ptrace patches you submit.

For example with the patch above you've brushed aside the concerns 
pointed out by Oleg - full discussion thread attached below. Does 
Oleg accept your incomplete patch? Does he ack this approach? It 
might be so but there is no trace of that in the patch and there 
should be - we use Acked-by for good reasons.

Andrew: if you start applying these patches to -mm then please make 
sure you follow and are aware of the full discussion context and 
dont queue up incomplete patches.

Thanks,

	Ingo


----- Forwarded message from Oleg Nesterov <oleg@redhat.com> -----

Date: Wed, 18 Mar 2009 19:15:12 +0100
From: Oleg Nesterov <oleg@redhat.com>
To: Roland McGrath <roland@redhat.com>
Subject: [PATCH] simplify do_signal_stop() && utrace_report_jctl()
	interaction
Cc: utrace-devel@redhat.com

On 03/18, Roland McGrath wrote:
>
> It's OK to change the tracehook definition.  I did this on the new git
> branch tracehook, then utrace branch commit 7b0be6e4 merges that and uses it.

Roland, I think it better to change tracehook definition more, please
see below.

> This drops all the JCTL bit kludgery and utrace_report_jctl just backs out
> of TASK_STOPPED before dropping the siglock in the first place.  I think
> the bookkeeping covers all the angles, but please check it in the new code.

Heh. I was thinking about the very similar change. But I have problems
with tracehook_notify_jctl().

Please find the patch below, on top of your changes. At the cost of
one additional ->group_stop_count != 0 in do_signal_stop(), we can
avoid playing with state/group_stop_count/flags twice.

But, with or without this patch we have a small problem: we can wrongly
send SIGCHLD twice. I'll write a separate email.

> Also please verify if you think all ->stopped bookkeeping is bulletproof
> now.  I fiddled utrace_get_signal() a little because I wasn't quite sure
> that all possibly paths there after TASK_STOPPED were resetting it.

Will do. I didn't study the signal part of utrace yet.

> I want to post it
> to LKML in the next day or two so it has aired before the 2.6.30 merge
> window.

Yes! I think it should be posted really soon.

BTW. exit_signals() calls tracehook_notify_jctl(why => CLD_STOPPED),
could you confirm this is right?

-------------------------------------------------------------------------
[PATCH] simplify do_signal_stop() && utrace_report_jctl() interaction

do_signal_stop() can call utrace_report_jctl() before decrementing
->group_stop_count and setting TASK_STOPPED/SIGNAL_STOP_STOPPED.
This allow to greatly simplify utrace_report_jctl() and avoid playing
with group-stop bookkeeping twice.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>

 signal.c |   29 +++++++++++------------------
 utrace.c |   20 --------------------
 2 files changed, 11 insertions(+), 38 deletions(-)

--- xxx/kernel/signal.c~JCTL_SIMPLIFY	2009-03-18 14:50:06.000000000 +0100
+++ xxx/kernel/signal.c	2009-03-18 18:20:35.000000000 +0100
@@ -1638,16 +1638,9 @@ void ptrace_notify(int exit_code)
 static int do_signal_stop(int signr)
 {
 	struct signal_struct *sig = current->signal;
-	int stop_count;
 	int notify;
 
-	if (sig->group_stop_count > 0) {
-		/*
-		 * There is a group stop in progress.  We don't need to
-		 * start another one.
-		 */
-		stop_count = --sig->group_stop_count;
-	} else {
+	if (!sig->group_stop_count) {
 		struct task_struct *t;
 
 		if (!likely(sig->flags & SIGNAL_STOP_DEQUEUED) ||
@@ -1659,7 +1652,7 @@ static int do_signal_stop(int signr)
 		 */
 		sig->group_exit_code = signr;
 
-		stop_count = 0;
+		sig->group_stop_count = 1;
 		for (t = next_thread(current); t != current; t = next_thread(t))
 			/*
 			 * Setting state to TASK_STOPPED for a group
@@ -1668,25 +1661,25 @@ static int do_signal_stop(int signr)
 			 */
 			if (!(t->flags & PF_EXITING) &&
 			    !task_is_stopped_or_traced(t)) {
-				stop_count++;
+				sig->group_stop_count++;
 				signal_wake_up(t, 0);
 			}
-		sig->group_stop_count = stop_count;
 	}
 
-	if (stop_count == 0)
-		sig->flags = SIGNAL_STOP_STOPPED;
-	current->exit_code = sig->group_exit_code;
-	__set_current_state(TASK_STOPPED);
-
 	/*
 	 * If there are no other threads in the group, or if there is
 	 * a group stop in progress and we are the last to stop,
 	 * report to the parent.  When ptraced, every thread reports itself.
 	 */
-	notify = tracehook_notify_jctl(stop_count == 0 ? CLD_STOPPED : 0,
-				       CLD_STOPPED);
+	notify = sig->group_stop_count == 1 ? CLD_STOPPED : 0;
+	notify = tracehook_notify_jctl(notify, CLD_STOPPED);
 
+	if (sig->group_stop_count) {
+		if (!--sig->group_stop_count)
+			sig->flags = SIGNAL_STOP_STOPPED;
+		current->exit_code = sig->group_exit_code;
+		__set_current_state(TASK_STOPPED);
+	}
 	spin_unlock_irq(&current->sighand->siglock);
 
 	if (notify) {
--- xxx/kernel/utrace.c~JCTL_SIMPLIFY	2009-03-18 14:50:06.000000000 +0100
+++ xxx/kernel/utrace.c	2009-03-18 18:23:01.000000000 +0100
@@ -1618,18 +1618,7 @@ void utrace_report_jctl(int notify, int 
 	struct task_struct *task = current;
 	struct utrace *utrace = task_utrace_struct(task);
 	INIT_REPORT(report);
-	bool stop = task_is_stopped(task);
 
-	/*
-	 * We have to come out of TASK_STOPPED in case the event report
-	 * hooks might block.  Since we held the siglock throughout, it's
-	 * as if we were never in TASK_STOPPED yet at all.
-	 */
-	if (stop) {
-		__set_current_state(TASK_RUNNING);
-		task->signal->flags &= ~SIGNAL_STOP_STOPPED;
-		++task->signal->group_stop_count;
-	}
 	spin_unlock_irq(&task->sighand->siglock);
 
 	/*
@@ -1654,16 +1643,7 @@ void utrace_report_jctl(int notify, int 
 	REPORT(task, utrace, &report, UTRACE_EVENT(JCTL),
 	       report_jctl, what, notify);
 
-	/*
-	 * Retake the lock, and go back into TASK_STOPPED
-	 * unless the stop was just cleared.
-	 */
 	spin_lock_irq(&task->sighand->siglock);
-	if (stop && task->signal->group_stop_count > 0) {
-		__set_current_state(TASK_STOPPED);
-		if (--task->signal->group_stop_count == 0)
-			task->signal->flags |= SIGNAL_STOP_STOPPED;
-	}
 }
 
 /*

----- End forwarded message -----

----- Forwarded message from Oleg Nesterov <oleg@redhat.com> -----

Date: Wed, 18 Mar 2009 20:49:41 +0100
From: Oleg Nesterov <oleg@redhat.com>
To: Roland McGrath <roland@redhat.com>
Subject: PATCH? tracehook_notify_jctl && SIGCONT
Cc: utrace-devel@redhat.com

On 03/18, Oleg Nesterov wrote:
>
> On 03/18, Roland McGrath wrote:
> >
> > It's OK to change the tracehook definition.  I did this on the new git
> > branch tracehook, then utrace branch commit 7b0be6e4 merges that and uses it.
>
> Roland, I think it better to change tracehook definition more, please
> see below.

The problem is that, since utrace_report_jctl() drops ->siglock,
tracehook_notify_jctl() can return false positive. This is easy
to fix, but then we have to check PT_PTRACED twice, not good.

Suppose we have 2 threads, T1 and T2, T1 has JCTL in ->utrace_flags.

T2 dequeues SIGSTOP, calls do_signal_stop(), and sleeps in TASK_STOPPED.

T1 calls do_signal_stop(). ->group_stop_count == 1, so it does
notify = tracehook_notify_jctl(notify => CLD_STOPPED), this means
that notify always becomes CLD_STOPPED.

When tracehook_notify_jctl()->utrace_notify_jctl() drops siglock,
SIGCONT comes, notices ->group_stop_count != 0, and adds SIGNAL_CLD_STOPPED
to signal flags.

Now we send SIGCHLD with si_code = CLD_STOPPED twice. By T1 from
do_signal_stop(), and by T1 or T2 from get_signal_to_deliver() which
checks SIGNAL_CLD_MASK.

I'd suggest something like the patch below. At least for now.

Oleg.

--- xxx/include/linux/tracehook.h~JCTL_NOTIFY	2009-03-18 14:50:05.000000000 +0100
+++ xxx/include/linux/tracehook.h	2009-03-18 20:18:54.000000000 +0100
@@ -520,11 +520,10 @@ static inline int tracehook_get_signal(s
  *
  * Called with the siglock held.
  */
-static inline int tracehook_notify_jctl(int notify, int why)
+static inline void tracehook_notify_jctl(int notify, int why)
 {
 	if (task_utrace_flags(current) & UTRACE_EVENT(JCTL))
 		utrace_report_jctl(notify, why);
-	return notify ?: (current->ptrace & PT_PTRACED) ? why : 0;
 }
 
 #define DEATH_REAP			-1
--- xxx/kernel/signal.c~JCTL_NOTIFY	2009-03-18 18:20:35.000000000 +0100
+++ xxx/kernel/signal.c	2009-03-18 20:28:39.000000000 +0100
@@ -1671,18 +1671,21 @@ static int do_signal_stop(int signr)
 	 * a group stop in progress and we are the last to stop,
 	 * report to the parent.  When ptraced, every thread reports itself.
 	 */
-	notify = sig->group_stop_count == 1 ? CLD_STOPPED : 0;
-	notify = tracehook_notify_jctl(notify, CLD_STOPPED);
+	tracehook_notify_jctl(sig->group_stop_count == 1 ? CLD_STOPPED : 0,
+				CLD_STOPPED);
 
+	notify = 0;
 	if (sig->group_stop_count) {
-		if (!--sig->group_stop_count)
+		if (!--sig->group_stop_count) {
 			sig->flags = SIGNAL_STOP_STOPPED;
+			notify = 1;
+		}
 		current->exit_code = sig->group_exit_code;
 		__set_current_state(TASK_STOPPED);
 	}
 	spin_unlock_irq(&current->sighand->siglock);
 
-	if (notify) {
+	if (notify || (current->ptrace & PT_PTRACED)) {
 		read_lock(&tasklist_lock);
 		do_notify_parent_cldstop(current, notify);
 		read_unlock(&tasklist_lock);
@@ -1765,14 +1768,12 @@ relock:
 				? CLD_CONTINUED : CLD_STOPPED;
 		signal->flags &= ~SIGNAL_CLD_MASK;
 
-		why = tracehook_notify_jctl(why, CLD_CONTINUED);
+		tracehook_notify_jctl(why, CLD_CONTINUED);
 		spin_unlock_irq(&sighand->siglock);
 
-		if (why) {
-			read_lock(&tasklist_lock);
-			do_notify_parent_cldstop(current->group_leader, why);
-			read_unlock(&tasklist_lock);
-		}
+		read_lock(&tasklist_lock);
+		do_notify_parent_cldstop(current->group_leader, why);
+		read_unlock(&tasklist_lock);
 		goto relock;
 	}
 
@@ -1930,7 +1931,8 @@ void exit_signals(struct task_struct *ts
 	if (unlikely(tsk->signal->group_stop_count) &&
 			!--tsk->signal->group_stop_count) {
 		tsk->signal->flags = SIGNAL_STOP_STOPPED;
-		group_stop = tracehook_notify_jctl(CLD_STOPPED, CLD_STOPPED);
+		tracehook_notify_jctl(CLD_STOPPED, CLD_STOPPED);
+		group_stop = 1;
 	}
 out:
 	spin_unlock_irq(&tsk->sighand->siglock);

----- End forwarded message -----

----- Forwarded message from Roland McGrath <roland@redhat.com> -----

Date: Thu, 19 Mar 2009 00:47:50 -0700 (PDT)
From: Roland McGrath <roland@redhat.com>
To: Oleg Nesterov <oleg@redhat.com>
Subject: Re: PATCH? tracehook_notify_jctl && SIGCONT
Cc: utrace-devel@redhat.com

> Now we send SIGCHLD with si_code = CLD_STOPPED twice. By T1 from
> do_signal_stop(), and by T1 or T2 from get_signal_to_deliver() which
> checks SIGNAL_CLD_MASK.

Yes, I considered this problem.  It's just not so big a deal as to worry
overmuch about this corner case in the first version.  What seems to me
will be the obvious and straightforward way to address it is to give
utrace_report_jctl() a return value that tracehook_notify_jctl() uses.
Then we can omit a notification that has been superceded.

Your patch does not seem very straightforward to me.  Moreover, you moved
some ptrace magic out of the tracehook function back into core signals code.
That is going in the wrong direction and we won't have any of that.


Thanks,
Roland

----- End forwarded message -----

----- Forwarded message from Roland McGrath <roland@redhat.com> -----

Date: Thu, 19 Mar 2009 00:43:16 -0700 (PDT)
From: Roland McGrath <roland@redhat.com>
To: Oleg Nesterov <oleg@redhat.com>
Subject: Re: [PATCH] simplify do_signal_stop() && utrace_report_jctl()
	interaction
Cc: utrace-devel@redhat.com

> Roland, I think it better to change tracehook definition more, please
> see below.

I don't really object to this in principle.  But this touches signal.c a
lot more in less obviously-trivial ways than my tracehook patch.  That is
more of an issue at the outset than some extra fiddling in the utrace code.
I think we should consider this for a later clean-up after merging.

> BTW. exit_signals() calls tracehook_notify_jctl(why => CLD_STOPPED),
> could you confirm this is right?

Yes, it's right.  I considered passing CLD_EXITED here to distinguish this
odd case, but that would make the vanilla tracehook_notify_jctl()
definition less simple.  Instead, we put the onus on a ->report_jctl hook
to check for PF_EXITING to tell if it's really going to stop.


Thanks,
Roland

----- End forwarded message -----

Re: + signals-tracehook_notify_jctl-change.patch added to -mm tree

[Oleg Nesterov]

On 04/01, Ingo Molnar wrote:
>
> * akpm@linux-foundation.org <akpm@linux-foundation.org> wrote:
>
> > Subject: signals: tracehook_notify_jctl change
> > From: Roland McGrath <roland@redhat.com>
> >
> > This changes tracehook_notify_jctl() so it's called with the siglock held,
> > and changes its argument and return value definition.  These clean-ups
> > make it a better fit for what new tracing hooks need to check.
> >
> > Tracing needs the siglock here, held from the time TASK_STOPPED was set,
> > to avoid potential SIGCONT races if it wants to allow any blocking in its
> > tracing hooks.
> >
> > This also folds the finish_stop() function into its caller
> > do_signal_stop().  The function is short, called only once and only
> > unconditionally.  It aids readability to fold it in.
> >
> > Signed-off-by: Roland McGrath <roland@redhat.com>
> > Cc: Oleg Nesterov <oleg@redhat.com>
> > Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
>
> Roland: as i have asked you before, i'd like to see explicit Ack's
> from Oleg for all ptrace patches you submit.

I agree with Roland's patch.

In my opinion, we can simplify utrace_report_jctl() a bit, but this
requires more changes in kernel/signal.c, we can do this later.

Acked-by: Oleg Nesterov <oleg@redhat.com>