ptrace: kill most tracehooks

PATCH In review Tejun Heo 4 participants 44 messages Jun 17, 2011 – Jun 27, 2011

Subsystems (14)

Memory management VFS MM core Tracing +10 more subsystems

lore

Share & tools

lore

Tejun Heo

author

2011-06-17 14:53 UTC #0 lore raw

PATCH 1/1 ptrace: kill most tracehooks

Hello,

At this point, tracehooks aren't useful to mainline kernel and mostly just add an extra layer of obfuscation. Although they have comments, without actual in-kernel users, it is difficult to tell what are their assumptions and they're actually trying to achieve. To mainline kernel, they just aren't worth keeping around.

This patchset kills most tracehooks which aren't used from arch codes. The remaining ones will be dealt with with future changes.

0001-ptrace-kill-task_ptrace.patch 0002-ptrace-introduce-ptrace_event_enabled-and-simplify-p.patch 0003-ptrace-move-SIGTRAP-on-exec-2-logic-to-ptrace_event.patch 0004-ptrace-kill-trivial-tracehooks.patch 0005-ptrace-kill-clone-exec-tracehooks.patch 0006-ptrace-kill-detah-tracehooks.patch 0007-ptrace-s-tracehook_tracer_task-ptrace_parent.patch

Most conversions are straightforward. The only tricky one is 0006 which reimplements the decision logic.

This patch is on top of Oleg's ptrace branch[1] - 544b2c91a9 (ptrace:
implement PTRACE_LISTEN), and available in the following git branch.

git://git.kernel.org/pub/scm/linux/kernel/git/tj/misc.git review-tracehook

diffstat follows.

 arch/s390/kernel/traps.c   |    4 
 fs/exec.c                  |    9 -
 fs/proc/array.c            |    2 
 fs/proc/base.c             |    2 
 include/linux/ptrace.h     |   71 ++++++---
 include/linux/tracehook.h  |  333 ---------------------------------------------
 kernel/exit.c              |   43 +++--
 kernel/fork.c              |   43 ++++-
 kernel/signal.c            |   22 +-
 mm/nommu.c                 |    3 
 mm/oom_kill.c              |    3 
 security/apparmor/domain.c |    2 
 security/selinux/hooks.c   |    4 
 13 files changed, 136 insertions(+), 405 deletions(-)

--
tejun

[1] git://git.kernel.org/pub/scm/linux/kernel/git/oleg/misc.git ptrace

Tejun Heo

author

2011-06-17 14:53 UTC #1 lore raw

PATCH 1/7 ptrace: kill task_ptrace()

task_ptrace(task) simply dereferences task->ptrace and isn't even used consistently only adding confusion. Kill it and directly access ->ptrace instead.

This doesn't introduce any behavior change.

Signed-off-by Tejun Heo <[email protected]>

---
 include/linux/ptrace.h    |   11 -----------
 include/linux/tracehook.h |   16 ++++++++--------
 kernel/exit.c             |    8 ++++----
 kernel/signal.c           |   14 +++++++-------
 mm/oom_kill.c             |    3 +--
 5 files changed, 20 insertions(+), 32 deletions(-)

include/linux/ptrace.h +0 ~~-11~~

index 4f224f1..3ff20b3 100644
@@ -146,17 +146,6 @@ int generic_ptrace_pokedata(struct task_struct *tsk, unsigned long addr,
 			    unsigned long data);
 
 /**
- * task_ptrace - return %PT_* flags that apply to a task
- * @task:	pointer to &task_struct in question
- *
- * Returns the %PT_* flags that apply to @task.
- */
-static inline int task_ptrace(struct task_struct *task)
-{
-	return task->ptrace;
-}
-
-/**
  * ptrace_event - possibly stop for a ptrace event notification
  * @mask:	%PT_* bit to check in @current->ptrace
  * @event:	%PTRACE_EVENT_* value to report if @mask is set

include/linux/tracehook.h +8 -8

index 15745cd..a3e8387 100644
@@ -63,7 +63,7 @@ struct linux_binprm;
  */
 static inline int tracehook_expect_breakpoints(struct task_struct *task)
 {
-	return (task_ptrace(task) & PT_PTRACED) != 0;
+	return (task->ptrace & PT_PTRACED) != 0;
 }
 
 /*

@@ -71,7 +71,7 @@ static inline int tracehook_expect_breakpoints(struct task_struct *task)
  */
 static inline void ptrace_report_syscall(struct pt_regs *regs)
 {
-	int ptrace = task_ptrace(current);
+	int ptrace = current->ptrace;
 
 	if (!(ptrace & PT_PTRACED))
 		return;

@@ -155,7 +155,7 @@ static inline void tracehook_report_syscall_exit(struct pt_regs *regs, int step)
 static inline int tracehook_unsafe_exec(struct task_struct *task)
 {
 	int unsafe = 0;
-	int ptrace = task_ptrace(task);
+	int ptrace = task->ptrace;
 	if (ptrace & PT_PTRACED) {
 		if (ptrace & PT_PTRACE_CAP)
 			unsafe |= LSM_UNSAFE_PTRACE_CAP;

@@ -178,7 +178,7 @@ static inline int tracehook_unsafe_exec(struct task_struct *task)
  */
 static inline struct task_struct *tracehook_tracer_task(struct task_struct *tsk)
 {
-	if (task_ptrace(tsk) & PT_PTRACED)
+	if (tsk->ptrace & PT_PTRACED)
 		return rcu_dereference(tsk->parent);
 	return NULL;
 }

@@ -202,7 +202,7 @@ static inline void tracehook_report_exec(struct linux_binfmt *fmt,
 					 struct pt_regs *regs)
 {
 	if (!ptrace_event(PT_TRACE_EXEC, PTRACE_EVENT_EXEC, 0) &&
-	    unlikely(task_ptrace(current) & PT_PTRACED))
+	    unlikely(current->ptrace & PT_PTRACED))
 		send_sig(SIGTRAP, current, 0);
 }

@@ -285,7 +285,7 @@ static inline void tracehook_report_clone(struct pt_regs *regs,
 					  unsigned long clone_flags,
 					  pid_t pid, struct task_struct *child)
 {
-	if (unlikely(task_ptrace(child))) {
+	if (unlikely(child->ptrace)) {
 		/*
 		 * It doesn't matter who attached/attaching to this
 		 * task, the pending SIGSTOP is right in any case.

@@ -403,7 +403,7 @@ static inline void tracehook_signal_handler(int sig, siginfo_t *info,
 static inline int tracehook_consider_ignored_signal(struct task_struct *task,
 						    int sig)
 {
-	return (task_ptrace(task) & PT_PTRACED) != 0;
+	return (task->ptrace & PT_PTRACED) != 0;
 }
 
 /**

@@ -422,7 +422,7 @@ static inline int tracehook_consider_ignored_signal(struct task_struct *task,
 static inline int tracehook_consider_fatal_signal(struct task_struct *task,
 						  int sig)
 {
-	return (task_ptrace(task) & PT_PTRACED) != 0;
+	return (task->ptrace & PT_PTRACED) != 0;
 }
 
 #define DEATH_REAP			-1

kernel/exit.c +4 -4

index 289f59d..e5cc056 100644
@@ -765,7 +765,7 @@ static void reparent_leader(struct task_struct *father, struct task_struct *p,
 	p->exit_signal = SIGCHLD;
 
 	/* If it has exited notify the new parent about this child's death. */
-	if (!task_ptrace(p) &&
+	if (!p->ptrace &&
 	    p->exit_state == EXIT_ZOMBIE && thread_group_empty(p)) {
 		do_notify_parent(p, p->exit_signal);
 		if (task_detached(p)) {

@@ -795,7 +795,7 @@ static void forget_original_parent(struct task_struct *father)
 		do {
 			t->real_parent = reaper;
 			if (t->parent == father) {
-				BUG_ON(task_ptrace(t));
+				BUG_ON(t->ptrace);
 				t->parent = t->real_parent;
 			}
 			if (t->pdeath_signal)

@@ -1565,7 +1565,7 @@ static int wait_consider_task(struct wait_opts *wo, int ptrace,
 		 * Notification and reaping will be cascaded to the real
 		 * parent when the ptracer detaches.
 		 */
-		if (likely(!ptrace) && unlikely(task_ptrace(p))) {
+		if (likely(!ptrace) && unlikely(p->ptrace)) {
 			/* it will become visible, clear notask_error */
 			wo->notask_error = 0;
 			return 0;

@@ -1608,7 +1608,7 @@ static int wait_consider_task(struct wait_opts *wo, int ptrace,
 		 * own children, it should create a separate process which
 		 * takes the role of real parent.
 		 */
-		if (likely(!ptrace) && task_ptrace(p) &&
+		if (likely(!ptrace) && p->ptrace &&
 		    same_thread_group(p->parent, p->real_parent))
 			return 0;

kernel/signal.c +7 -7

index 97e575a..0f33708 100644
@@ -1592,7 +1592,7 @@ int do_notify_parent(struct task_struct *tsk, int sig)
  	/* do_notify_parent_cldstop should have been called instead.  */
  	BUG_ON(task_is_stopped_or_traced(tsk));
 
-	BUG_ON(!task_ptrace(tsk) &&
+	BUG_ON(!tsk->ptrace &&
 	       (tsk->group_leader != tsk || !thread_group_empty(tsk)));
 
 	info.si_signo = sig;

@@ -1631,7 +1631,7 @@ int do_notify_parent(struct task_struct *tsk, int sig)
 
 	psig = tsk->parent->sighand;
 	spin_lock_irqsave(&psig->siglock, flags);
-	if (!task_ptrace(tsk) && sig == SIGCHLD &&
+	if (!tsk->ptrace && sig == SIGCHLD &&
 	    (psig->action[SIGCHLD-1].sa.sa_handler == SIG_IGN ||
 	     (psig->action[SIGCHLD-1].sa.sa_flags & SA_NOCLDWAIT))) {
 		/*

@@ -1731,7 +1731,7 @@ static void do_notify_parent_cldstop(struct task_struct *tsk,
 
 static inline int may_ptrace_stop(void)
 {
-	if (!likely(task_ptrace(current)))
+	if (!likely(current->ptrace))
 		return 0;
 	/*
 	 * Are we in the middle of do_coredump?

@@ -1989,7 +1989,7 @@ static bool do_signal_stop(int signr)
 		if (!(sig->flags & SIGNAL_STOP_STOPPED))
 			sig->group_exit_code = signr;
 		else
-			WARN_ON_ONCE(!task_ptrace(current));
+			WARN_ON_ONCE(!current->ptrace);
 
 		sig->group_stop_count = 0;

@@ -2014,7 +2014,7 @@ static bool do_signal_stop(int signr)
 		}
 	}
 
-	if (likely(!task_ptrace(current))) {
+	if (likely(!current->ptrace)) {
 		int notify = 0;
 
 		/*

@@ -2093,7 +2093,7 @@ static void do_jobctl_trap(void)
 static int ptrace_signal(int signr, siginfo_t *info,
 			 struct pt_regs *regs, void *cookie)
 {
-	if (!task_ptrace(current))
+	if (!current->ptrace)
 		return signr;
 
 	ptrace_signal_deliver(regs, cookie);

@@ -2179,7 +2179,7 @@ relock:
 		do_notify_parent_cldstop(current, false, why);
 
 		leader = current->group_leader;
-		if (task_ptrace(leader) && !real_parent_is_ptracer(leader))
+		if (leader->ptrace && !real_parent_is_ptracer(leader))
 			do_notify_parent_cldstop(leader, true, why);
 
 		read_unlock(&tasklist_lock);

mm/oom_kill.c +1 -2

index e4b0991..b0be989 100644
@@ -339,8 +339,7 @@ static struct task_struct *select_bad_process(unsigned int *ppoints,
 				 * then wait for it to finish before killing
 				 * some other task unnecessarily.
 				 */
-				if (!(task_ptrace(p->group_leader) &
-							PT_TRACE_EXIT))
+				if (!(p->group_leader->ptrace & PT_TRACE_EXIT))
 					return ERR_PTR(-1UL);
 			}
 		}

Signature · 1 line

1.7.5.4

Tejun Heo

author

2011-06-17 14:51 UTC #2 lore raw

PATCH 2/7 ptrace: introduce ptrace_event_enabled() and simplify ptrace_event() and tracehook_prepare_clone()

In reply to Tejun Heo

This patch implements ptrace_event_enabled() which tests whether a given PTRACE_EVENT_* is enabled and use it to simplify ptrace_event() and tracehook_prepare_clone().

PT_EVENT_FLAG() macro is added which calculates PT_TRACE_* flag from PTRACE_EVENT_*. This is used to define PT_TRACE_* flags and by ptrace_event_enabled() to find the matching flag.

This is used to make ptrace_event() and tracehook_prepare_clone() simpler.

* ptrace_event() callers were responsible for providing mask to test whether the event was enabled. This patch implements ptrace_event_enabled() and make ptrace_event() drop @mask and determine whether the event is enabled from @event. Note that @event is constant and this conversion doesn't add runtime overhead.

All conversions except tracehook_report_clone_complete() are trivial. tracehook_report_clone_complete() used to use 0 for @mask (always enabled) but now tests whether the specified event is enabled. This doesn't cause any behavior difference as it's guaranteed that the event specified by @trace is enabled.

* tracehook_prepare_clone() now only determines which event is applicable and use ptrace_event_enabled() for enable test.

This doesn't introduce any behavior change.

Signed-off-by Tejun Heo <[email protected]>

---
 include/linux/ptrace.h    |   46 +++++++++++++++++++++++++++++++-------------
 include/linux/tracehook.h |   26 ++++++++++++------------
 2 files changed, 45 insertions(+), 27 deletions(-)

include/linux/ptrace.h +32 ~~-14~~

index 3ff20b3..18feac6 100644
@@ -90,12 +90,17 @@
 #define PT_DTRACE	0x00000002	/* delayed trace (used on m68k, i386) */
 #define PT_TRACESYSGOOD	0x00000004
 #define PT_PTRACE_CAP	0x00000008	/* ptracer can follow suid-exec */
-#define PT_TRACE_FORK	0x00000010
-#define PT_TRACE_VFORK	0x00000020
-#define PT_TRACE_CLONE	0x00000040
-#define PT_TRACE_EXEC	0x00000080
-#define PT_TRACE_VFORK_DONE	0x00000100
-#define PT_TRACE_EXIT	0x00000200
+
+/* PT_TRACE_* event enable flags */
+#define PT_EVENT_FLAG_SHIFT	4
+#define PT_EVENT_FLAG(event)	(1 << (PT_EVENT_FLAG_SHIFT + (event) - 1))
+
+#define PT_TRACE_FORK		PT_EVENT_FLAG(PTRACE_EVENT_FORK)
+#define PT_TRACE_VFORK		PT_EVENT_FLAG(PTRACE_EVENT_VFORK)
+#define PT_TRACE_CLONE		PT_EVENT_FLAG(PTRACE_EVENT_CLONE)
+#define PT_TRACE_EXEC		PT_EVENT_FLAG(PTRACE_EVENT_EXEC)
+#define PT_TRACE_VFORK_DONE	PT_EVENT_FLAG(PTRACE_EVENT_VFORK_DONE)
+#define PT_TRACE_EXIT		PT_EVENT_FLAG(PTRACE_EVENT_EXIT)
 
 #define PT_TRACE_MASK	0x000003f4

@@ -146,25 +151,38 @@ int generic_ptrace_pokedata(struct task_struct *tsk, unsigned long addr,
 			    unsigned long data);
 
 /**
+ * ptrace_event_enabled - test whether a ptrace event is enabled
+ * @task: ptracee of interest
+ * @event: %PTRACE_EVENT_* to test
+ *
+ * Test whether @event is enabled for ptracee @task.
+ *
+ * Returns %true if @event is enabled, %false otherwise.
+ */
+static inline bool ptrace_event_enabled(struct task_struct *task, int event)
+{
+	return task->ptrace & PT_EVENT_FLAG(event);
+}
+
+/**
  * ptrace_event - possibly stop for a ptrace event notification
- * @mask:	%PT_* bit to check in @current->ptrace
- * @event:	%PTRACE_EVENT_* value to report if @mask is set
+ * @event:	%PTRACE_EVENT_* value to report
  * @message:	value for %PTRACE_GETEVENTMSG to return
  *
- * This checks the @mask bit to see if ptrace wants stops for this event.
- * If so we stop, reporting @event and @message to the ptrace parent.
+ * Check whether @event is enabled and, if so, report @event and @message
+ * to the ptrace parent.
  *
  * Returns nonzero if we did a ptrace notification, zero if not.
  *
  * Called without locks.
  */
-static inline int ptrace_event(int mask, int event, unsigned long message)
+static inline int ptrace_event(int event, unsigned long message)
 {
-	if (mask && likely(!(current->ptrace & mask)))
-		return 0;
+	if (likely(!ptrace_event_enabled(current, event)))
+		return false;
 	current->ptrace_message = message;
 	ptrace_notify((event << 8) | SIGTRAP);
-	return 1;
+	return true;
 }
 
 /**

include/linux/tracehook.h +13 ~~-13~~

index a3e8387..7d38571 100644
@@ -201,7 +201,7 @@ static inline void tracehook_report_exec(struct linux_binfmt *fmt,
 					 struct linux_binprm *bprm,
 					 struct pt_regs *regs)
 {
-	if (!ptrace_event(PT_TRACE_EXEC, PTRACE_EVENT_EXEC, 0) &&
+	if (!ptrace_event(PTRACE_EVENT_EXEC, 0) &&
 	    unlikely(current->ptrace & PT_PTRACED))
 		send_sig(SIGTRAP, current, 0);
 }

@@ -218,7 +218,7 @@ static inline void tracehook_report_exec(struct linux_binfmt *fmt,
  */
 static inline void tracehook_report_exit(long *exit_code)
 {
-	ptrace_event(PT_TRACE_EXIT, PTRACE_EVENT_EXIT, *exit_code);
+	ptrace_event(PTRACE_EVENT_EXIT, *exit_code);
 }
 
 /**

@@ -232,19 +232,19 @@ static inline void tracehook_report_exit(long *exit_code)
  */
 static inline int tracehook_prepare_clone(unsigned clone_flags)
 {
+	int event = 0;
+
 	if (clone_flags & CLONE_UNTRACED)
 		return 0;
 
-	if (clone_flags & CLONE_VFORK) {
-		if (current->ptrace & PT_TRACE_VFORK)
-			return PTRACE_EVENT_VFORK;
-	} else if ((clone_flags & CSIGNAL) != SIGCHLD) {
-		if (current->ptrace & PT_TRACE_CLONE)
-			return PTRACE_EVENT_CLONE;
-	} else if (current->ptrace & PT_TRACE_FORK)
-		return PTRACE_EVENT_FORK;
+	if (clone_flags & CLONE_VFORK)
+		event = PTRACE_EVENT_VFORK;
+	else if ((clone_flags & CSIGNAL) != SIGCHLD)
+		event = PTRACE_EVENT_CLONE;
+	else
+		event = PTRACE_EVENT_FORK;
 
-	return 0;
+	return ptrace_event_enabled(current, event) ? event : 0;
 }
 
 /**

@@ -318,7 +318,7 @@ static inline void tracehook_report_clone_complete(int trace,
 						   struct task_struct *child)
 {
 	if (unlikely(trace))
-		ptrace_event(0, trace, pid);
+		ptrace_event(trace, pid);
 }
 
 /**

@@ -336,7 +336,7 @@ static inline void tracehook_report_clone_complete(int trace,
 static inline void tracehook_report_vfork_done(struct task_struct *child,
 					       pid_t pid)
 {
-	ptrace_event(PT_TRACE_VFORK_DONE, PTRACE_EVENT_VFORK_DONE, pid);
+	ptrace_event(PTRACE_EVENT_VFORK_DONE, pid);
 }
 
 /**

Signature · 1 line

1.7.5.4

Tejun Heo

author

2011-06-17 14:51 UTC #3 lore raw

PATCH 3/7 ptrace: move SIGTRAP on exec(2) logic to ptrace_event()

In reply to Tejun Heo

Move SIGTRAP on exec(2) logic from tracehook_report_exec() to ptrace_event(). This is part of changes to make ptrace_event() smarter and handle ptrace event related details in one place.

This doesn't introduce any behavior change.

Signed-off-by Tejun Heo <[email protected]>

---
 include/linux/ptrace.h    |   16 ++++++++--------
 include/linux/tracehook.h |    4 +---
 2 files changed, 9 insertions(+), 11 deletions(-)

include/linux/ptrace.h +8 -8

index 18feac6..b546fd6 100644
@@ -172,17 +172,17 @@ static inline bool ptrace_event_enabled(struct task_struct *task, int event)
  * Check whether @event is enabled and, if so, report @event and @message
  * to the ptrace parent.
  *
- * Returns nonzero if we did a ptrace notification, zero if not.
- *
  * Called without locks.
  */
-static inline int ptrace_event(int event, unsigned long message)
+static inline void ptrace_event(int event, unsigned long message)
 {
-	if (likely(!ptrace_event_enabled(current, event)))
-		return false;
-	current->ptrace_message = message;
-	ptrace_notify((event << 8) | SIGTRAP);
-	return true;
+	if (unlikely(ptrace_event_enabled(current, event))) {
+		current->ptrace_message = message;
+		ptrace_notify((event << 8) | SIGTRAP);
+	} else if (event == PTRACE_EVENT_EXEC && unlikely(current->ptrace)) {
+		/* legacy EXEC report via SIGTRAP */
+		send_sig(SIGTRAP, current, 0);
+	}
 }
 
 /**

include/linux/tracehook.h +1 -3

index 7d38571..3b68aa8 100644
@@ -201,9 +201,7 @@ static inline void tracehook_report_exec(struct linux_binfmt *fmt,
 					 struct linux_binprm *bprm,
 					 struct pt_regs *regs)
 {
-	if (!ptrace_event(PTRACE_EVENT_EXEC, 0) &&
-	    unlikely(current->ptrace & PT_PTRACED))
-		send_sig(SIGTRAP, current, 0);
+	ptrace_event(PTRACE_EVENT_EXEC, 0);
 }
 
 /**

Signature · 1 line

1.7.5.4

Oleg Nesterov

maintainer

2011-06-20 20:26 UTC #4 lore raw

Re: [PATCH 3/7] ptrace: move SIGTRAP on exec(2) logic to ptrace_event()

Quoted 4 lines · Tejun Heo

Move SIGTRAP on exec(2) logic from tracehook_report_exec() to ptrace_event(). This is part of changes to make ptrace_event() smarter and handle ptrace event related details in one place.

I am going to apply all patches except 6/7, but to be honest this one looks a bit ugly to me.

Quoted 5 lines

include/linux/ptrace.h +1 -3

-static inline int ptrace_event(int event, unsigned long message)
+static inline void ptrace_event(int event, unsigned long message)
 {
-	if (likely(!ptrace_event_enabled(current, event)))
-		return false;

include/linux/ptrace.h static inline bool ptrace_event_enabled(struct task_struct *task, int event) · line 185

Quoted 10 lines

include/linux/ptrace.h +7 -3

-	current->ptrace_message = message;
-	ptrace_notify((event << 8) | SIGTRAP);
-	return true;
+	if (unlikely(ptrace_event_enabled(current, event))) {
+		current->ptrace_message = message;
+		ptrace_notify((event << 8) | SIGTRAP);
+	} else if (event == PTRACE_EVENT_EXEC && unlikely(current->ptrace)) {
+		/* legacy EXEC report via SIGTRAP */
+		send_sig(SIGTRAP, current, 0);
+	}

Why does this make ptrace_event() smarter?

OK, tracehooks should die. But why should we move this special case into ptrace_event? Say, a simple

	static inline void ptrace_exec_event(...)
	{
		if (!ptrace_event_enabled(PTRACE_EVENT_EXEC))
			send_sig(SIGTRAP, current, 0);
		else
			ptrace_event(PTRACE_EVENT_EXEC);
	}

in fs/exec.c looks a bit better to me.

Oleg.

Tejun Heo

author

2011-06-21 07:21 UTC #5 lore raw

Re: [PATCH 3/7] ptrace: move SIGTRAP on exec(2) logic to ptrace_event()

Hello,

Quoted 13 lines · Oleg Nesterov

Why does this make ptrace_event() smarter?
OK, tracehooks should die. But why should we move this special case into ptrace_event? Say, a simple
static inline void ptrace_exec_event(...)
{
if (!ptrace_event_enabled(PTRACE_EVENT_EXEC)) send_sig(SIGTRAP, current, 0); else ptrace_event(PTRACE_EVENT_EXEC); }

Quoted 1 line

in fs/exec.c looks a bit better to me.

The intention is to concentrate ptrace specific logic in ptrace_event(). We'll have more of them, mostly dependent on PT_SEIZED and I don't think it's a good idea to scatter them across the kernel. They're of no interest outside of ptrace after all. I think it's better to have them collected in one place than scattered around. The PT_SEIZED ones would probably have some commonalities among them too.

Thanks.

Signature · 1 line

tejun

Oleg Nesterov

maintainer

2011-06-21 20:42 UTC #6 lore raw

Re: [PATCH 3/7] ptrace: move SIGTRAP on exec(2) logic to ptrace_event()

Quoted 17 lines · Tejun Heo

Hello,
Quoted 13 lines · Oleg Nesterov
Why does this make ptrace_event() smarter?
OK, tracehooks should die. But why should we move this special case into ptrace_event? Say, a simple
static inline void ptrace_exec_event(...)
{
if (!ptrace_event_enabled(PTRACE_EVENT_EXEC)) send_sig(SIGTRAP, current, 0); else ptrace_event(PTRACE_EVENT_EXEC); }

Quoted 8 lines

Quoted 1 line
in fs/exec.c looks a bit better to me.
The intention is to concentrate ptrace specific logic in ptrace_event(). We'll have more of them, mostly dependent on PT_SEIZED and I don't think it's a good idea to scatter them across the kernel. They're of no interest outside of ptrace after all. I think it's better to have them collected in one place than scattered around.

This was one of the reasons for tracehooks ;)

OK, we can move this helper to ptrace.h although I do not think this makes sense. As for "scattered around", imho the code which calculates trace in do_fork() falls into the same category.

I still can't understand why ptrace_event() should check EVENT_EXEC. This is the special case, it should be handled specially. And while I think this is not that important, this is not friendly to do_fork, compiler has to generate the code to check event.

But OK, I applied 1-5 and 7. This is minor, and we can reconsider this later. I mean, right now I think I'll send the cleanup later, and you will have to explain your nack ;)

Oleg.

Tejun Heo

author

2011-06-23 08:58 UTC #7 lore raw

Re: [PATCH 3/7] ptrace: move SIGTRAP on exec(2) logic to ptrace_event()

Hello,

Quoted 8 lines · Oleg Nesterov

Quoted 6 lines
The intention is to concentrate ptrace specific logic in ptrace_event(). We'll have more of them, mostly dependent on PT_SEIZED and I don't think it's a good idea to scatter them across the kernel. They're of no interest outside of ptrace after all. I think it's better to have them collected in one place than scattered around.
This was one of the reasons for tracehooks ;)

Yeah, sure. The problem with tracehook is not that it's a fundamentally wrong thing to do but it's way over done than actually necessary and full of requirements which are of no interest to inside the upstream kernel.

Quoted 8 lines

OK, we can move this helper to ptrace.h although I do not think this makes sense. As for "scattered around", imho the code which calculates trace in do_fork() falls into the same category.
I still can't understand why ptrace_event() should check EVENT_EXEC. This is the special case, it should be handled specially. And while I think this is not that important, this is not friendly to do_fork, compiler has to generate the code to check event.

The generated code should be the same. I didn't check all the cases but the few I check didn't really change.

Quoted 3 lines

But OK, I applied 1-5 and 7. This is minor, and we can reconsider this later. I mean, right now I think I'll send the cleanup later, and you will have to explain your nack ;)

Yeah, yeah, it's a minor point one way or the other. I was (and still is) thinking about folding most of ptrace-specific event logics into ptrace_event() and this was just a first step as I think adding more condition checks in fs/exec.c doesn't make much sense - ie. I might end up adding PT_SEZIED there too.

Thanks.

Signature · 1 line

tejun

Tejun Heo

author

2011-06-17 14:53 UTC #8 lore raw

PATCH 4/7 ptrace: kill trivial tracehooks

In reply to Tejun Heo

This patch kills the following trivial tracehooks.

* Ones testing whether task is ptraced. Replace with ->ptrace test.

tracehook_expect_breakpoints()
tracehook_consider_ignored_signal()
tracehook_consider_fatal_signal()

* ptrace_event() wrappers. Call directly.

tracehook_report_exec()
tracehook_report_exit()
tracehook_report_vfork_done()

* ptrace_release_task() wrapper. Call directly.

tracehook_finish_release_task()

* noop

tracehook_prepare_release_task()
tracehook_report_death()

This doesn't introduce any behavior change.

Signed-off-by Tejun Heo <[email protected]>

Cc Christoph Hellwig <[email protected]>

Cc Martin Schwidefsky <[email protected]>

---
 arch/s390/kernel/traps.c  |    4 +-
 fs/exec.c                 |    2 +-
 include/linux/tracehook.h |  156 ---------------------------------------------
 kernel/exit.c             |    7 +--
 kernel/fork.c             |    2 +-
 kernel/signal.c           |    8 +-
 mm/nommu.c                |    3 +-
 7 files changed, 11 insertions(+), 171 deletions(-)

arch/s390/kernel/traps.c +2 -2

index a65d2e8..a63d34c 100644
@@ -331,7 +331,7 @@ void __kprobes do_per_trap(struct pt_regs *regs)
 {
 	if (notify_die(DIE_SSTEP, "sstep", regs, 0, 0, SIGTRAP) == NOTIFY_STOP)
 		return;
-	if (tracehook_consider_fatal_signal(current, SIGTRAP))
+	if (current->ptrace)
 		force_sig(SIGTRAP, current);
 }

@@ -425,7 +425,7 @@ static void __kprobes illegal_op(struct pt_regs *regs, long pgm_int_code,
 		if (get_user(*((__u16 *) opcode), (__u16 __user *) location))
 			return;
 		if (*((__u16 *) opcode) == S390_BREAKPOINT_U16) {
-			if (tracehook_consider_fatal_signal(current, SIGTRAP))
+			if (current->ptrace)
 				force_sig(SIGTRAP, current);
 			else
 				signal = SIGILL;

fs/exec.c +1 -1

index a9f2b36..b37030d 100644
@@ -1384,7 +1384,7 @@ int search_binary_handler(struct linux_binprm *bprm,struct pt_regs *regs)
 			bprm->recursion_depth = depth;
 			if (retval >= 0) {
 				if (depth == 0)
-					tracehook_report_exec(fmt, bprm, regs);
+					ptrace_event(PTRACE_EVENT_EXEC, 0);
 				put_binfmt(fmt);
 				allow_write_access(bprm->file);
 				if (bprm->file)

include/linux/tracehook.h +0 ~~-156~~

186 lines not on this page · raw message

kernel/exit.c +2 -5

index e5cc056..d49134a 100644
@@ -169,7 +169,6 @@ void release_task(struct task_struct * p)
 	struct task_struct *leader;
 	int zap_leader;
 repeat:
-	tracehook_prepare_release_task(p);
 	/* don't need to get the RCU readlock here - the process is dead and
 	 * can't be modifying its own credentials. But shut RCU-lockdep up */
 	rcu_read_lock();

@@ -179,7 +178,7 @@ repeat:
 	proc_flush_task(p);
 
 	write_lock_irq(&tasklist_lock);
-	tracehook_finish_release_task(p);
+	ptrace_release_task(p);
 	__exit_signal(p);
 
 	/*

@@ -868,8 +867,6 @@ static void exit_notify(struct task_struct *tsk, int group_dead)
 		wake_up_process(tsk->signal->group_exit_task);
 	write_unlock_irq(&tasklist_lock);
 
-	tracehook_report_death(tsk, signal, cookie, group_dead);
-
 	/* If the process is dead, release it - nobody will wait for it */
 	if (signal == DEATH_REAP)
 		release_task(tsk);

@@ -924,7 +921,7 @@ NORET_TYPE void do_exit(long code)
 	 */
 	set_fs(USER_DS);
 
-	tracehook_report_exit(&code);
+	ptrace_event(PTRACE_EVENT_EXIT, code);
 
 	validate_creds_for_do_exit(tsk);

kernel/fork.c +1 -1

index 0276c30..d4f0dff 100644
@@ -1527,7 +1527,7 @@ long do_fork(unsigned long clone_flags,
 			freezer_do_not_count();
 			wait_for_completion(&vfork);
 			freezer_count();
-			tracehook_report_vfork_done(p, nr);
+			ptrace_event(PTRACE_EVENT_VFORK_DONE, nr);
 		}
 	} else {
 		nr = PTR_ERR(p);

kernel/signal.c +4 -4

index 0f33708..1550aee 100644
@@ -87,7 +87,7 @@ static int sig_ignored(struct task_struct *t, int sig, int from_ancestor_ns)
 	/*
 	 * Tracers may want to know about even ignored signals.
 	 */
-	return !tracehook_consider_ignored_signal(t, sig);
+	return !t->ptrace;
 }
 
 /*

@@ -493,7 +493,8 @@ int unhandled_signal(struct task_struct *tsk, int sig)
 		return 1;
 	if (handler != SIG_IGN && handler != SIG_DFL)
 		return 0;
-	return !tracehook_consider_fatal_signal(tsk, sig);
+	/* if ptraced, let the tracer determine */
+	return !tsk->ptrace;
 }
 
 /*

@@ -981,8 +982,7 @@ static void complete_signal(int sig, struct task_struct *p, int group)
 	if (sig_fatal(p, sig) &&
 	    !(signal->flags & (SIGNAL_UNKILLABLE | SIGNAL_GROUP_EXIT)) &&
 	    !sigismember(&t->real_blocked, sig) &&
-	    (sig == SIGKILL ||
-	     !tracehook_consider_fatal_signal(t, sig))) {
+	    (sig == SIGKILL || !t->ptrace)) {
 		/*
 		 * This signal will be fatal to the whole group.
 		 */

mm/nommu.c +1 -2

index 1fd0c51..54ae707 100644
@@ -22,7 +22,6 @@
 #include <linux/pagemap.h>
 #include <linux/slab.h>
 #include <linux/vmalloc.h>
-#include <linux/tracehook.h>
 #include <linux/blkdev.h>
 #include <linux/backing-dev.h>
 #include <linux/mount.h>

@@ -1087,7 +1086,7 @@ static unsigned long determine_vm_flags(struct file *file,
 	 * it's being traced - otherwise breakpoints set in it may interfere
 	 * with another untraced process
 	 */
-	if ((flags & MAP_PRIVATE) && tracehook_expect_breakpoints(current))
+	if ((flags & MAP_PRIVATE) && current->ptrace)
 		vm_flags &= ~VM_MAYSHARE;
 
 	return vm_flags;

Signature · 1 line

1.7.5.4

Tejun Heo

author

2011-06-17 14:52 UTC #9 lore raw

PATCH 5/7 ptrace: kill clone/exec tracehooks

In reply to Tejun Heo

This patch kills the following clone and exec related tracehooks.

tracehook_prepare_clone()
tracehook_finish_clone()
tracehook_report_clone()
tracehook_report_clone_complete()
tracehook_unsafe_exec()

The changes are mostly trivial - logic is moved to the caller and comments are merged and adjusted appropriately.

The only exception is in check_unsafe_exec() where LSM_UNSAFE_PTRACE* are OR'd to bprm->unsafe instead of setting it, which produces the same result as the field is always zero on entry. It also tests p->ptrace instead of (p->ptrace & PT_PTRACED) for consistency, which also gives the same result.

This doesn't introduce any behavior change.

Signed-off-by Tejun Heo <[email protected]>

Cc Christoph Hellwig <[email protected]>

---
 fs/exec.c                 |    7 ++-
 include/linux/tracehook.h |  121 ---------------------------------------------
 kernel/fork.c             |   41 ++++++++++++---
 3 files changed, 38 insertions(+), 131 deletions(-)

fs/exec.c +6 -1

index b37030d..8dca45b 100644
@@ -1224,7 +1224,12 @@ int check_unsafe_exec(struct linux_binprm *bprm)
 	unsigned n_fs;
 	int res = 0;
 
-	bprm->unsafe = tracehook_unsafe_exec(p);
+	if (p->ptrace) {
+		if (p->ptrace & PT_PTRACE_CAP)
+			bprm->unsafe |= LSM_UNSAFE_PTRACE_CAP;
+		else
+			bprm->unsafe |= LSM_UNSAFE_PTRACE;
+	}
 
 	n_fs = 1;
 	spin_lock(&p->fs->lock);

include/linux/tracehook.h +0 ~~-121~~

133 lines not on this page · raw message

kernel/fork.c +32 -9

69 lines not on this page · raw message

Signature · 1 line

1.7.5.4

Oleg Nesterov

maintainer

2011-06-20 20:35 UTC #10 lore raw

Re: [PATCH 5/7] ptrace: kill clone/exec tracehooks

Quoted 3 lines · Tejun Heo

At this point, tracehooks aren't useful to mainline kernel and mostly just add an extra layer of obfuscation.

Yes, this is true.

kernel/fork.c long do_fork(unsigned long clone_flags, · line 1489

Quoted 13 lines

kernel/fork.c +5 -3

@@ -1481,10 +1481,22 @@ long do_fork(unsigned long clone_flags,
 	}
 
 	/*
-	 * When called from kernel_thread, don't do user tracing stuff.
+	 * Determine whether and which event to report to ptracer.  When
+	 * called from kernel_thread or CLONE_UNTRACED is explicitly
+	 * requested, no event is reported; otherwise, report if the event
+	 * for the type of forking is enabled.
 	 */
-	if (likely(user_mode(regs)))
-		trace = tracehook_prepare_clone(clone_flags);
+	if (likely(user_mode(regs)) && !(clone_flags & CLONE_UNTRACED)) {

Off-topic, but I never understood this user_mode() check... OK, a traced task can call kernel_thread() in kernel, but this used CLONE_UNTRACED.

kernel/fork.c long do_fork(unsigned long clone_flags, · line 1499

Quoted 10 lines

kernel/fork.c +10 -0

+		if (clone_flags & CLONE_VFORK)
+			trace = PTRACE_EVENT_VFORK;
+		else if ((clone_flags & CSIGNAL) != SIGCHLD)
+			trace = PTRACE_EVENT_CLONE;
+		else
+			trace = PTRACE_EVENT_FORK;
+
+		if (likely(!ptrace_event_enabled(current, trace)))
+			trace = 0;
+	}

As I said am going to apply this all except 6/7, but imho this deserves a trivial helper anyway.

Oleg.

Tejun Heo

author

2011-06-21 07:24 UTC #11 lore raw

Re: [PATCH 5/7] ptrace: kill clone/exec tracehooks

Hello,

Quoted 5 lines · Oleg Nesterov

Quoted 1 line
+ if (likely(user_mode(regs)) && !(clone_flags & CLONE_UNTRACED)) {
Off-topic, but I never understood this user_mode() check... OK, a traced task can call kernel_thread() in kernel, but this used CLONE_UNTRACED.

Me neither. I suppose it's added as a safety net. Maybe we can turn it into WARN_ON(!user_mode) && !CLONE_UNTRACED) but just leaving it alone doesn't sound like a bad idea.

Quoted 11 lines

Quoted 10 lines
+ if (clone_flags & CLONE_VFORK)
+ trace = PTRACE_EVENT_VFORK;
+ else if ((clone_flags & CSIGNAL) != SIGCHLD)
+ trace = PTRACE_EVENT_CLONE;
+ else
+ trace = PTRACE_EVENT_FORK;
+
+ if (likely(!ptrace_event_enabled(current, trace)))
+ trace = 0;
+ }

Quoted 2 lines

As I said am going to apply this all except 6/7, but imho this deserves a trivial helper anyway.

Yeah, maybe. Preference, I guess.

Thanks.

Signature · 1 line

tejun

Tejun Heo

author

2011-06-17 14:52 UTC #12 lore raw

PATCH 6/7 ptrace: kill detah tracehooks

In reply to Tejun Heo

This patch kills tracehook_notify_death() by reimplementing the logic in its caller - exit_notify(). I opted for re-implementation as the existing code was a bit difficult to wrap one's head around.

The reimplemented logic yields the same result in all combinations of task_detached(), thread_group_empty(), ptrace_reparented() and ->ptrace.

Signed-off-by Tejun Heo <[email protected]>

Cc Christoph Hellwig <[email protected]>

---
 include/linux/tracehook.h |   34 ----------------------------------
 kernel/exit.c             |   28 +++++++++++++++++++++-------
 2 files changed, 21 insertions(+), 41 deletions(-)

include/linux/tracehook.h +0 ~~-34~~

index bcc4ca7..fea0d4b 100644
@@ -170,40 +170,6 @@ static inline void tracehook_signal_handler(int sig, siginfo_t *info,
 		ptrace_notify(SIGTRAP);
 }
 
-#define DEATH_REAP			-1
-#define DEATH_DELAYED_GROUP_LEADER	-2
-
-/**
- * tracehook_notify_death - task is dead, ready to notify parent
- * @task:		@current task now exiting
- * @death_cookie:	value to pass to tracehook_report_death()
- * @group_dead:		nonzero if this was the last thread in the group to die
- *
- * A return value >= 0 means call do_notify_parent() with that signal
- * number.  Negative return value can be %DEATH_REAP to self-reap right
- * now, or %DEATH_DELAYED_GROUP_LEADER to a zombie without notifying our
- * parent.  Note that a return value of 0 means a do_notify_parent() call
- * that sends no signal, but still wakes up a parent blocked in wait*().
- *
- * Called with write_lock_irq(&tasklist_lock) held.
- */
-static inline int tracehook_notify_death(struct task_struct *task,
-					 void **death_cookie, int group_dead)
-{
-	if (task_detached(task))
-		return task->ptrace ? SIGCHLD : DEATH_REAP;
-
-	/*
-	 * If something other than our normal parent is ptracing us, then
-	 * send it a SIGCHLD instead of honoring exit_signal.  exit_signal
-	 * only has special meaning to our real parent.
-	 */
-	if (thread_group_empty(task) && !ptrace_reparented(task))
-		return task->exit_signal;
-
-	return task->ptrace ? SIGCHLD : DEATH_DELAYED_GROUP_LEADER;
-}
-
 #ifdef TIF_NOTIFY_RESUME
 /**
  * set_notify_resume - cause tracehook_notify_resume() to be called

kernel/exit.c +21 -7

index d49134a..f3c49ca 100644
@@ -819,8 +819,7 @@ static void forget_original_parent(struct task_struct *father)
  */
 static void exit_notify(struct task_struct *tsk, int group_dead)
 {
-	int signal;
-	void *cookie;
+	bool dead = false;
 
 	/*
 	 * This does two things:

@@ -856,11 +855,26 @@ static void exit_notify(struct task_struct *tsk, int group_dead)
 	     tsk->self_exec_id != tsk->parent_exec_id))
 		tsk->exit_signal = SIGCHLD;
 
-	signal = tracehook_notify_death(tsk, &cookie, group_dead);
-	if (signal >= 0)
-		signal = do_notify_parent(tsk, signal);
+	/*
+	 * Notify interested parent if the whole group is dead.  Ptracer
+	 * should always be notified.  If something other than our normal
+	 * parent is ptracing us, then send it a SIGCHLD instead of
+	 * honoring exit_signal.  exit_signal only has special meaning to
+	 * our real parent.
+	 */
+	if (!task_detached(tsk) && thread_group_empty(tsk)) {
+		if (!ptrace_reparented(tsk))
+			do_notify_parent(tsk, tsk->exit_signal);
+		else
+			do_notify_parent(tsk, SIGCHLD);
+	} else {
+		if (unlikely(tsk->ptrace))
+			do_notify_parent(tsk, SIGCHLD);
+		else if (task_detached(tsk))
+			dead = true;
+	}
 
-	tsk->exit_state = signal == DEATH_REAP ? EXIT_DEAD : EXIT_ZOMBIE;
+	tsk->exit_state = dead ? EXIT_DEAD : EXIT_ZOMBIE;
 
 	/* mt-exec, de_thread() is waiting for group leader */
 	if (unlikely(tsk->signal->notify_count < 0))

@@ -868,7 +882,7 @@ static void exit_notify(struct task_struct *tsk, int group_dead)
 	write_unlock_irq(&tasklist_lock);
 
 	/* If the process is dead, release it - nobody will wait for it */
-	if (signal == DEATH_REAP)
+	if (dead)
 		release_task(tsk);
 }

Signature · 1 line

1.7.5.4

Oleg Nesterov

maintainer

2011-06-20 19:41 UTC #13 lore raw

Re: [PATCH 6/7] ptrace: kill detah tracehooks

Quoted 4 lines · Tejun Heo

This patch kills tracehook_notify_death() by reimplementing the logic in its caller - exit_notify(). I opted for re-implementation as the existing code was a bit difficult to wrap one's head around.

I always hated this particular tracehook much more than others ;)

But the patch doesn't look right, please see below.

Quoted 5 lines

kernel/exit.c +2 -3

-	signal = tracehook_notify_death(tsk, &cookie, group_dead);
-	if (signal >= 0)
-		signal = do_notify_parent(tsk, signal);
+	/*
+	 * Notify interested parent if the whole group is dead.  Ptracer

kernel/exit.c static void exit_notify(struct task_struct *tsk, int group_dead) · line 869

Quoted 10 lines

kernel/exit.c +10 -0

+	 * should always be notified.  If something other than our normal
+	 * parent is ptracing us, then send it a SIGCHLD instead of
+	 * honoring exit_signal.  exit_signal only has special meaning to
+	 * our real parent.
+	 */
+	if (!task_detached(tsk) && thread_group_empty(tsk)) {
+		if (!ptrace_reparented(tsk))
+			do_notify_parent(tsk, tsk->exit_signal);
+		else
+			do_notify_parent(tsk, SIGCHLD);

Forget about ptrace, suppose a single threaded child exits and its parent ignores SIGCHLD. In this case do_notify_parent() sets exit_signal = -1 to mark it dead, we should set exit_state = EXIT_DEAD and release it.

Note! This looks very ugly, I wanted to cleanup this a long ago. I think we should never change ->exit_signal, and do_notify_parent() should return a boolean. I'll try to make the patches tomorrow. Then _perhaps_ this patch will become simpler.

Oleg.

Oleg Nesterov

maintainer

2011-06-21 20:26 UTC #14 lore raw

Re: [PATCH 6/7] ptrace: kill detah tracehooks

Quoted 5 lines · Oleg Nesterov

Note! This looks very ugly, I wanted to cleanup this a long ago. I think we should never change ->exit_signal, and do_notify_parent() should return a boolean. I'll try to make the patches tomorrow. Then _perhaps_ this patch will become simpler.

Damn. Tomorrow (I promise ;) I'll send the full series. IMHO, we really need to cleanup the do_notify_parent/task_detached logic, and exit_signal should be "const"

But, to remove this tracehook, we only need the patch below and then exit_notify() should do:

	if (unlikely(tsk->ptrace)) {
		int sig = ptrace_reparented(tsk) || task_detached(tsk) ?
					SIGCHLD : tsk->exit_signal;
		autoreap = do_notify_parent(tsk, sig);
		WARN_ON(autoreap);
	} else if (thread_group_leader(tsk)) {
		autoreap = thread_group_empty(tsk) &&
				do_notify_parent(tsk, tsk->exit_signal);
	} else {
		autoreap = true;
	}

	tsk->exit_state = autoreap ? EXIT_DEAD : EXIT_ZOMBIE;

Note the WARN_ON() above, perhaps instead we should autoreap if the tracer is the real parent and it ignores SIGCHLD.

And. ptrace_reparented() should be changed to check same_thread_group(), I think. This also looks fine for wait_task_zombie().

Oleg.

-----------------------------------------------------------------------------
[PATCH 3/XXX] make do_notify_parent() return bool

- change do_notify_parent() to return a boolean, true if the task should be reaped because its parent ignores SIGCHLD.

- update the only caller which checks the returned value, exit_notify(), to rely DEATH_REAP only if we do not call do_notify_parent().

This temporary uglifies exit_notify() even more, will be cleanuped by the next change.

Signed-off-by Oleg Nesterov <[email protected]>

---

include/linux/sched.h renamed from include/linux/sched.h~2_do_notify_parent_bool +2 -2

@@ -2146,7 +2146,7 @@ static inline int dequeue_signal_lock(st
 	spin_unlock_irqrestore(&tsk->sighand->siglock, flags);
 
 	return ret;
-}	
+}
 
 extern void block_all_signals(int (*notifier)(void *priv), void *priv,
 			      sigset_t *mask);

@@ -2161,7 +2161,7 @@ extern int kill_pid_info_as_uid(int, str
 extern int kill_pgrp(struct pid *pid, int sig, int priv);
 extern int kill_pid(struct pid *pid, int sig, int priv);
 extern int kill_proc_info(int, struct siginfo *, pid_t);
-extern int do_notify_parent(struct task_struct *, int);
+extern bool do_notify_parent(struct task_struct *, int);
 extern void __wake_up_parent(struct task_struct *p, struct task_struct *parent);
 extern void force_sig(int, struct task_struct *);
 extern int send_sig(int, struct task_struct *, int);

kernel/signal.c renamed from kernel/signal.c~2_do_notify_parent_bool +9 -8

@@ -1577,15 +1577,15 @@ ret:
  * Let a parent know about the death of a child.
  * For a stopped/continued status change, use do_notify_parent_cldstop instead.
  *
- * Returns -1 if our parent ignored us and so we've switched to
- * self-reaping, or else @sig.
+ * Returns true if our parent ignored us and so we've switched to
+ * self-reaping.
  */
-int do_notify_parent(struct task_struct *tsk, int sig)
+bool do_notify_parent(struct task_struct *tsk, int sig)
 {
 	struct siginfo info;
 	unsigned long flags;
 	struct sighand_struct *psig;
-	int ret = sig;
+	bool autoreap = false;
 
 	BUG_ON(sig == -1);

@@ -1649,16 +1649,17 @@ int do_notify_parent(struct task_struct 
 		 * is implementation-defined: we do (if you don't want
 		 * it, just use SIG_IGN instead).
 		 */
-		ret = tsk->exit_signal = -1;
+		autoreap = true;
+		tsk->exit_signal = -1;
 		if (psig->action[SIGCHLD-1].sa.sa_handler == SIG_IGN)
-			sig = -1;
+			sig = 0;
 	}
-	if (valid_signal(sig) && sig > 0)
+	if (valid_signal(sig) && sig)
 		__group_send_sig_info(sig, &info, tsk->parent);
 	__wake_up_parent(tsk, tsk->parent);
 	spin_unlock_irqrestore(&psig->siglock, flags);
 
-	return ret;
+	return autoreap;
 }
 
 /**

kernel/exit.c renamed from kernel/exit.c~2_do_notify_parent_bool +6 -3

@@ -821,6 +821,7 @@ static void forget_original_parent(struc
 static void exit_notify(struct task_struct *tsk, int group_dead)
 {
 	int signal;
+	bool autoreap;
 	void *cookie;
 
 	/*

@@ -859,9 +860,11 @@ static void exit_notify(struct task_stru
 
 	signal = tracehook_notify_death(tsk, &cookie, group_dead);
 	if (signal >= 0)
-		signal = do_notify_parent(tsk, signal);
+		autoreap = do_notify_parent(tsk, signal);
+	else
+		autoreap = (signal == DEATH_REAP);
 
-	tsk->exit_state = signal == DEATH_REAP ? EXIT_DEAD : EXIT_ZOMBIE;
+	tsk->exit_state = autoreap ? EXIT_DEAD : EXIT_ZOMBIE;
 
 	/* mt-exec, de_thread() is waiting for group leader */
 	if (unlikely(tsk->signal->notify_count < 0))

@@ -869,7 +872,7 @@ static void exit_notify(struct task_stru
 	write_unlock_irq(&tasklist_lock);
 
 	/* If the process is dead, release it - nobody will wait for it */
-	if (signal == DEATH_REAP)
+	if (autoreap)
 		release_task(tsk);
 }

Tejun Heo

author

2011-06-23 09:24 UTC #15 lore raw

Re: [PATCH 6/7] ptrace: kill detah tracehooks

In reply to Oleg Nesterov

Hey, Oleg.

Quoted 12 lines · Oleg Nesterov

Quoted 12 lines

kernel/exit.c +9 -3

-	signal = tracehook_notify_death(tsk, &cookie, group_dead);
-	if (signal >= 0)
-		signal = do_notify_parent(tsk, signal);
+	/*
+	 * Notify interested parent if the whole group is dead.  Ptracer
+	 * should always be notified.  If something other than our normal
+	 * parent is ptracing us, then send it a SIGCHLD instead of
+	 * honoring exit_signal.  exit_signal only has special meaning to
+	 * our real parent.
+	 */
+	if (!task_detached(tsk) && thread_group_empty(tsk)) {
+		if (!ptrace_reparented(tsk))

Quoted 8 lines

kernel/exit.c static void exit_notify(struct task_struct *tsk, int group_dead) · line 869
Quoted 3 lines
kernel/exit.c +3 -0
+			do_notify_parent(tsk, tsk->exit_signal);
+		else
+			do_notify_parent(tsk, SIGCHLD);
Forget about ptrace, suppose a single threaded child exits and its parent ignores SIGCHLD. In this case do_notify_parent() sets exit_signal = -1 to mark it dead, we should set exit_state = EXIT_DEAD and release it.

Oops, right. Missed do_notify_parent() may modify exit_signal. Grumble. It seems you already have posted patches to address this one. Let's continue there.

Thanks.

Signature · 1 line

tejun

Oleg Nesterov

maintainer

2011-06-22 21:10 UTC #16 lore raw

cover kill task_detached() (Was: ptrace: kill detah tracehooks)

In reply to Tejun Heo

Remove tracehook_notify_death(), and kill task_detached() logic.

IMHO, task_detached() is very strange and confusing. Sometimes it used instead of !thread_group_leader() when the task is live. Sometimes it is used to detect the EXIT_DEAD tasks (reparent_leader). Sometimes it used after do_notify_parent() which sets ->exit_signal = -1 to signal the caller it should set EXIT_DEAD.

Basically, this series does:

- s/task_detached/thread_group_leader/ when the caller need to know if it is the group_leader or not

- s/task_detached/EXIT_DEAD/ when the caller needs to filter out the EXIT_DEAD tasks

- changes do_notify_parent() to return a boolean and converts the callers from

do_notify_parent(p);
if (task_detached(p))
// aha, do_notify_parent() changed ->exit_signal release_task(p)

to
if (do_notify_parent(p))
release_task(p)

- kills the now unused task_detached()

- makes task->exit_signal "const" and redefines thread_group_leader() for micro-optimization.

 fs/exec.c                 |    1 +
 include/linux/sched.h     |   15 ++++-------
 include/linux/tracehook.h |   34 --------------------------
 kernel/exit.c             |   58 +++++++++++++++++++-------------------------
 kernel/ptrace.c           |   32 +++++++++++++-----------
 kernel/signal.c           |   16 ++++++------
 6 files changed, 57 insertions(+), 99 deletions(-)

Oleg.

Oleg Nesterov

maintainer

2011-06-22 21:10 UTC #17 lore raw

PATCH 1/8 make do_notify_parent() return bool

- change do_notify_parent() to return a boolean, true if the task should be reaped because its parent ignores SIGCHLD.

- update the only caller which checks the returned value, exit_notify().

This temporary uglifies exit_notify() even more, will be cleanuped by the next change.

Signed-off-by Oleg Nesterov <[email protected]>

---

 include/linux/sched.h |    4 ++--
 kernel/signal.c       |   17 +++++++++--------
 kernel/exit.c         |    9 ++++++---
 3 files changed, 17 insertions(+), 13 deletions(-)

include/linux/sched.h renamed from include/linux/sched.h~1_do_notify_parent_bool +2 -2

@@ -2146,7 +2146,7 @@ static inline int dequeue_signal_lock(st
 	spin_unlock_irqrestore(&tsk->sighand->siglock, flags);
 
 	return ret;
-}	
+}
 
 extern void block_all_signals(int (*notifier)(void *priv), void *priv,
 			      sigset_t *mask);

@@ -2161,7 +2161,7 @@ extern int kill_pid_info_as_uid(int, str
 extern int kill_pgrp(struct pid *pid, int sig, int priv);
 extern int kill_pid(struct pid *pid, int sig, int priv);
 extern int kill_proc_info(int, struct siginfo *, pid_t);
-extern int do_notify_parent(struct task_struct *, int);
+extern bool do_notify_parent(struct task_struct *, int);
 extern void __wake_up_parent(struct task_struct *p, struct task_struct *parent);
 extern void force_sig(int, struct task_struct *);
 extern int send_sig(int, struct task_struct *, int);

kernel/signal.c renamed from kernel/signal.c~1_do_notify_parent_bool +9 -8

@@ -1577,15 +1577,15 @@ ret:
  * Let a parent know about the death of a child.
  * For a stopped/continued status change, use do_notify_parent_cldstop instead.
  *
- * Returns -1 if our parent ignored us and so we've switched to
- * self-reaping, or else @sig.
+ * Returns true if our parent ignored us and so we've switched to
+ * self-reaping.
  */
-int do_notify_parent(struct task_struct *tsk, int sig)
+bool do_notify_parent(struct task_struct *tsk, int sig)
 {
 	struct siginfo info;
 	unsigned long flags;
 	struct sighand_struct *psig;
-	int ret = sig;
+	bool autoreap = false;
 
 	BUG_ON(sig == -1);

@@ -1649,16 +1649,17 @@ int do_notify_parent(struct task_struct 
 		 * is implementation-defined: we do (if you don't want
 		 * it, just use SIG_IGN instead).
 		 */
-		ret = tsk->exit_signal = -1;
+		autoreap = true;
+		tsk->exit_signal = -1;
 		if (psig->action[SIGCHLD-1].sa.sa_handler == SIG_IGN)
-			sig = -1;
+			sig = 0;
 	}
-	if (valid_signal(sig) && sig > 0)
+	if (valid_signal(sig) && sig)
 		__group_send_sig_info(sig, &info, tsk->parent);
 	__wake_up_parent(tsk, tsk->parent);
 	spin_unlock_irqrestore(&psig->siglock, flags);
 
-	return ret;
+	return autoreap;
 }
 
 /**

kernel/exit.c renamed from kernel/exit.c~1_do_notify_parent_bool +6 -3

@@ -819,6 +819,7 @@ static void forget_original_parent(struc
 static void exit_notify(struct task_struct *tsk, int group_dead)
 {
 	int signal;
+	bool autoreap;
 	void *cookie;
 
 	/*

@@ -857,9 +858,11 @@ static void exit_notify(struct task_stru
 
 	signal = tracehook_notify_death(tsk, &cookie, group_dead);
 	if (signal >= 0)
-		signal = do_notify_parent(tsk, signal);
+		autoreap = do_notify_parent(tsk, signal);
+	else
+		autoreap = (signal == DEATH_REAP);
 
-	tsk->exit_state = signal == DEATH_REAP ? EXIT_DEAD : EXIT_ZOMBIE;
+	tsk->exit_state = autoreap ? EXIT_DEAD : EXIT_ZOMBIE;
 
 	/* mt-exec, de_thread() is waiting for group leader */
 	if (unlikely(tsk->signal->notify_count < 0))

@@ -867,7 +870,7 @@ static void exit_notify(struct task_stru
 	write_unlock_irq(&tasklist_lock);
 
 	/* If the process is dead, release it - nobody will wait for it */
-	if (signal == DEATH_REAP)
+	if (autoreap)
 		release_task(tsk);
 }

Tejun Heo

author Acked-by on v1 1/8

2011-06-23 09:52 UTC #18 lore raw

Re: [PATCH 1/8] make do_notify_parent() return bool

Hello,

Quoted 11 lines · Oleg Nesterov

- change do_notify_parent() to return a boolean, true if the task should be reaped because its parent ignores SIGCHLD.
- update the only caller which checks the returned value, exit_notify().
This temporary uglifies exit_notify() even more, will be cleanuped by the next change.
Signed-off-by Oleg Nesterov <[email protected]>
---

include/linux/sched.h extern int kill_pid_info_as_uid(int, str · line 2167

Quoted 8 lines

include/linux/sched.h renamed from include/linux/sched.h~1_do_notify_parent_bool +1 -1

@@ -2161,7 +2161,7 @@ extern int kill_pid_info_as_uid(int, str
 extern int kill_pgrp(struct pid *pid, int sig, int priv);
 extern int kill_pid(struct pid *pid, int sig, int priv);
 extern int kill_proc_info(int, struct siginfo *, pid_t);
-extern int do_notify_parent(struct task_struct *, int);
+extern bool do_notify_parent(struct task_struct *, int);
 extern void __wake_up_parent(struct task_struct *p, struct task_struct *parent);
 extern void force_sig(int, struct task_struct *);
 extern int send_sig(int, struct task_struct *, int);

While at it, would you mind adding the name of the parameters to the prototype? I find prototypes w/o parameter names a bit irksome.

Other than that,

Acked-by Tejun Heo <[email protected]>

Thanks.

Signature · 1 line

tejun

Oleg Nesterov

maintainer

2011-06-22 21:11 UTC #19 lore raw

PATCH 2/8 kill tracehook_notify_death()

In reply to Oleg Nesterov

Kill tracehook_notify_death(), reimplement the logic in its caller, exit_notify().

This also fixes a minor bug, if the exiting task is the group_leader and it is traced by its real_parent, tracehook_notify_death() returns task->exit_signal or SIGCHLD depending on thread_group_empty(), this looks strange.

Also, change the exec_id check to use thread_group_leader() instead of task_detached(), this looks more clear.

Signed-off-by Oleg Nesterov <[email protected]>

---

 include/linux/tracehook.h |   34 ----------------------------------
 kernel/exit.c             |   19 +++++++++++--------
 2 files changed, 11 insertions(+), 42 deletions(-)

include/linux/tracehook.h renamed from include/linux/tracehook.h~2_kill_notify_death +0 ~~-34~~

@@ -152,40 +152,6 @@ static inline void tracehook_signal_hand
 		ptrace_notify(SIGTRAP);
 }
 
-#define DEATH_REAP			-1
-#define DEATH_DELAYED_GROUP_LEADER	-2
-
-/**
- * tracehook_notify_death - task is dead, ready to notify parent
- * @task:		@current task now exiting
- * @death_cookie:	value to pass to tracehook_report_death()
- * @group_dead:		nonzero if this was the last thread in the group to die
- *
- * A return value >= 0 means call do_notify_parent() with that signal
- * number.  Negative return value can be %DEATH_REAP to self-reap right
- * now, or %DEATH_DELAYED_GROUP_LEADER to a zombie without notifying our
- * parent.  Note that a return value of 0 means a do_notify_parent() call
- * that sends no signal, but still wakes up a parent blocked in wait*().
- *
- * Called with write_lock_irq(&tasklist_lock) held.
- */
-static inline int tracehook_notify_death(struct task_struct *task,
-					 void **death_cookie, int group_dead)
-{
-	if (task_detached(task))
-		return task->ptrace ? SIGCHLD : DEATH_REAP;
-
-	/*
-	 * If something other than our normal parent is ptracing us, then
-	 * send it a SIGCHLD instead of honoring exit_signal.  exit_signal
-	 * only has special meaning to our real parent.
-	 */
-	if (thread_group_empty(task) && !ptrace_reparented(task))
-		return task->exit_signal;
-
-	return task->ptrace ? SIGCHLD : DEATH_DELAYED_GROUP_LEADER;
-}
-
 #ifdef TIF_NOTIFY_RESUME
 /**
  * set_notify_resume - cause tracehook_notify_resume() to be called

kernel/exit.c renamed from kernel/exit.c~2_kill_notify_death +11 -8

@@ -818,9 +818,7 @@ static void forget_original_parent(struc
  */
 static void exit_notify(struct task_struct *tsk, int group_dead)
 {
-	int signal;
 	bool autoreap;
-	void *cookie;
 
 	/*
 	 * This does two things:

@@ -851,16 +849,21 @@ static void exit_notify(struct task_stru
 	 * we have changed execution domain as these two values started
 	 * the same after a fork.
 	 */
-	if (tsk->exit_signal != SIGCHLD && !task_detached(tsk) &&
+	if (thread_group_leader(tsk) && tsk->exit_signal != SIGCHLD &&
 	    (tsk->parent_exec_id != tsk->real_parent->self_exec_id ||
 	     tsk->self_exec_id != tsk->parent_exec_id))
 		tsk->exit_signal = SIGCHLD;
 
-	signal = tracehook_notify_death(tsk, &cookie, group_dead);
-	if (signal >= 0)
-		autoreap = do_notify_parent(tsk, signal);
-	else
-		autoreap = (signal == DEATH_REAP);
+	if (unlikely(tsk->ptrace)) {
+		int sig = thread_group_leader(tsk) && !ptrace_reparented(tsk)
+			? tsk->exit_signal : SIGCHLD;
+		autoreap = do_notify_parent(tsk, sig);
+	} else if (thread_group_leader(tsk)) {
+		autoreap = thread_group_empty(tsk) &&
+			do_notify_parent(tsk, tsk->exit_signal);
+	} else {
+		autoreap = true;
+	}
 
 	tsk->exit_state = autoreap ? EXIT_DEAD : EXIT_ZOMBIE;

Tejun Heo

author

2011-06-23 12:22 UTC #20 lore raw

Re: [PATCH 2/8] kill tracehook_notify_death()

Hello, Oleg.

Quoted 7 lines · Oleg Nesterov

Kill tracehook_notify_death(), reimplement the logic in its caller, exit_notify().
This also fixes a minor bug, if the exiting task is the group_leader and it is traced by its real_parent, tracehook_notify_death() returns task->exit_signal or SIGCHLD depending on thread_group_empty(), this looks strange.

Maybe we should do the above in a separate patch?

kernel/exit.c static void exit_notify(struct task_stru · line 852

Quoted 2 lines

kernel/exit.c renamed from kernel/exit.c~2_kill_notify_death +1 -1
-	if (tsk->exit_signal != SIGCHLD && !task_detached(tsk) &&
+	if (thread_group_leader(tsk) && tsk->exit_signal != SIGCHLD &&

Hmmm... it probably depends on POV but wouldn't (exit_signal != -1 && exit_signal != SIGCHLD) be easier? The logic here is about demoting specials sigs to SIGCHLD under certain circumstances and the check is there to prevent promoting -1 to SIGCHLD. I agree the detached() is more distracting than helping but thread_group_leader() seems unnecessarily indirect.

Thanks.

Signature · 1 line

tejun

Oleg Nesterov

maintainer

2011-06-23 13:23 UTC #21 lore raw

Re: [PATCH 2/8] kill tracehook_notify_death()

Hi Tejun,

Quoted 7 lines · Tejun Heo

Hello, Oleg.
Quoted 3 lines · Oleg Nesterov
Kill tracehook_notify_death(), reimplement the logic in its caller, exit_notify().

Quoted 6 lines

Quoted 4 lines
This also fixes a minor bug, if the exiting task is the group_leader and it is traced by its real_parent, tracehook_notify_death() returns task->exit_signal or SIGCHLD depending on thread_group_empty(), this looks strange.
Maybe we should do the above in a separate patch?

Do you think this makes sense? OK, I can do this...

Quoted 6 lines

kernel/exit.c static void exit_notify(struct task_stru · line 852
Quoted 3 lines
kernel/exit.c renamed from kernel/exit.c~2_kill_notify_death +1 -1
-	if (tsk->exit_signal != SIGCHLD && !task_detached(tsk) &&
+	if (thread_group_leader(tsk) && tsk->exit_signal != SIGCHLD &&
Hmmm... it probably depends on POV but wouldn't (exit_signal != -1 && exit_signal != SIGCHLD) be easier?

I disagree.

Quoted 2 lines

The logic here is about demoting
specials sigs to SIGCHLD under certain circumstances

Yes. And what is ->exit_signal? It is in fact per-process, lives in the group_leader's task_struct. We could move it to signal_struct.

Quoted 2 lines

and the check is
there to prevent promoting -1 to SIGCHLD.

Yes, because we should never change ->exit_signal for sub-threads.

And it doesn't make sense to check exec ids (this is again per-process, should be cleanuped) unless the task is the group leader.

Quoted 2 lines

thread_group_leader() seems
unnecessarily indirect.

This is what I disagree with. Contrary, I think thread_group_leader() exactly explains what do we want to check. (but once again, exec_id logic should be cleanuped, not only in this function).

Oleg.

Tejun Heo

author

2011-06-23 13:28 UTC #22 lore raw

Re: [PATCH 2/8] kill tracehook_notify_death()

Hey,

Quoted 8 lines · Oleg Nesterov

Quoted 6 lines
Quoted 4 lines
This also fixes a minor bug, if the exiting task is the group_leader and it is traced by its real_parent, tracehook_notify_death() returns task->exit_signal or SIGCHLD depending on thread_group_empty(), this looks strange.
Maybe we should do the above in a separate patch?
Do you think this makes sense? OK, I can do this...

Having subtle behavior change mixed with reorganization isn't too nice, so I think separating is better.

Quoted 5 lines

Quoted 1 line
thread_group_leader() seems unnecessarily indirect.
This is what I disagree with. Contrary, I think thread_group_leader() exactly explains what do we want to check. (but once again, exec_id logic should be cleanuped, not only in this function).

Hmmm... well, this was minor to begin with and thread_group_leader() matches later patches better, so using thread_group_leader() seems fine.

Thanks.

Signature · 1 line

tejun

Oleg Nesterov

maintainer

2011-06-23 13:30 UTC #23 lore raw

Re: [PATCH 2/8] kill tracehook_notify_death()

Quoted 9 lines · Tejun Heo

Hey,
Quoted 5 lines · Oleg Nesterov
Quoted 5 lines
Quoted 4 lines
This also fixes a minor bug, if the exiting task is the group_leader and it is traced by its real_parent, tracehook_notify_death() returns task->exit_signal or SIGCHLD depending on thread_group_empty(), this looks strange.

Quoted 6 lines

Quoted 3 lines
Quoted 1 line
Maybe we should do the above in a separate patch?
Do you think this makes sense? OK, I can do this...
Having subtle behavior change mixed with reorganization isn't too nice, so I think separating is better.

Hmm. OK, you are right. I'll redo.

Oleg.

Oleg Nesterov

maintainer

2011-06-23 17:09 UTC #24 lore raw

Re: [PATCH 2/8] kill tracehook_notify_death()

Quoted 7 lines · Oleg Nesterov

Quoted 3 lines · Tejun Heo
Having subtle behavior change mixed with reorganization isn't too nice, so I think separating is better.
Hmm. OK, you are right. I'll redo.

------------------------------------------------------------------------------
[PATCH v2 2/8] kill tracehook_notify_death()

Kill tracehook_notify_death(), reimplement the logic in its caller, exit_notify().

Also, change the exec_id's check to use thread_group_leader() instead of task_detached(), this is more clear. This logic only applies to the exiting leader, a sub-thread must never change its exit_signal.

Note: when the traced group leader exits the exit_signal-or-SIGCHLD logic looks really strange:

- we notify the tracer even if !thread_group_empty() but do_wait(WEXITED) can't work until all threads exit

- if the tracer is real_parent, it is not clear why can't we use ->exit_signal event if !thread_group_empty()

-v2: do not try to fix the 2nd oddity to avoid the subtle behavior
change mixed with reorganization, suggested by Tejun.

Signed-off-by Oleg Nesterov <[email protected]>

---

 include/linux/tracehook.h |   34 ----------------------------------
 kernel/exit.c             |   21 +++++++++++++--------
 2 files changed, 13 insertions(+), 42 deletions(-)

include/linux/tracehook.h renamed from include/linux/tracehook.h~2_kill_notify_death +0 ~~-34~~

@@ -152,40 +152,6 @@ static inline void tracehook_signal_hand
 		ptrace_notify(SIGTRAP);
 }
 
-#define DEATH_REAP			-1
-#define DEATH_DELAYED_GROUP_LEADER	-2
-
-/**
- * tracehook_notify_death - task is dead, ready to notify parent
- * @task:		@current task now exiting
- * @death_cookie:	value to pass to tracehook_report_death()
- * @group_dead:		nonzero if this was the last thread in the group to die
- *
- * A return value >= 0 means call do_notify_parent() with that signal
- * number.  Negative return value can be %DEATH_REAP to self-reap right
- * now, or %DEATH_DELAYED_GROUP_LEADER to a zombie without notifying our
- * parent.  Note that a return value of 0 means a do_notify_parent() call
- * that sends no signal, but still wakes up a parent blocked in wait*().
- *
- * Called with write_lock_irq(&tasklist_lock) held.
- */
-static inline int tracehook_notify_death(struct task_struct *task,
-					 void **death_cookie, int group_dead)
-{
-	if (task_detached(task))
-		return task->ptrace ? SIGCHLD : DEATH_REAP;
-
-	/*
-	 * If something other than our normal parent is ptracing us, then
-	 * send it a SIGCHLD instead of honoring exit_signal.  exit_signal
-	 * only has special meaning to our real parent.
-	 */
-	if (thread_group_empty(task) && !ptrace_reparented(task))
-		return task->exit_signal;
-
-	return task->ptrace ? SIGCHLD : DEATH_DELAYED_GROUP_LEADER;
-}
-
 #ifdef TIF_NOTIFY_RESUME
 /**
  * set_notify_resume - cause tracehook_notify_resume() to be called

kernel/exit.c renamed from kernel/exit.c~2_kill_notify_death +13 -8

@@ -818,9 +818,7 @@ static void forget_original_parent(struc
  */
 static void exit_notify(struct task_struct *tsk, int group_dead)
 {
-	int signal;
 	bool autoreap;
-	void *cookie;
 
 	/*
 	 * This does two things:

@@ -851,16 +849,23 @@ static void exit_notify(struct task_stru
 	 * we have changed execution domain as these two values started
 	 * the same after a fork.
 	 */
-	if (tsk->exit_signal != SIGCHLD && !task_detached(tsk) &&
+	if (thread_group_leader(tsk) && tsk->exit_signal != SIGCHLD &&
 	    (tsk->parent_exec_id != tsk->real_parent->self_exec_id ||
 	     tsk->self_exec_id != tsk->parent_exec_id))
 		tsk->exit_signal = SIGCHLD;
 
-	signal = tracehook_notify_death(tsk, &cookie, group_dead);
-	if (signal >= 0)
-		autoreap = do_notify_parent(tsk, signal);
-	else
-		autoreap = (signal == DEATH_REAP);
+	if (unlikely(tsk->ptrace)) {
+		int sig = thread_group_leader(tsk) &&
+				thread_group_empty(tsk) &&
+				!ptrace_reparented(tsk) ?
+			tsk->exit_signal : SIGCHLD;
+		autoreap = do_notify_parent(tsk, sig);
+	} else if (thread_group_leader(tsk)) {
+		autoreap = thread_group_empty(tsk) &&
+			do_notify_parent(tsk, tsk->exit_signal);
+	} else {
+		autoreap = true;
+	}
 
 	tsk->exit_state = autoreap ? EXIT_DEAD : EXIT_ZOMBIE;

Tejun Heo

author

2011-06-25 14:16 UTC #25 lore raw

Re: [PATCH 2/8] kill tracehook_notify_death()

Hello,

Quoted 9 lines · Oleg Nesterov

[PATCH v2 2/8] kill tracehook_notify_death()
Kill tracehook_notify_death(), reimplement the logic in its caller, exit_notify().
Also, change the exec_id's check to use thread_group_leader() instead of task_detached(), this is more clear. This logic only applies to the exiting leader, a sub-thread must never change its exit_signal.

Quoted 5 lines

Note: when the traced group leader exits the exit_signal-or-SIGCHLD logic looks really strange:
- we notify the tracer even if !thread_group_empty() but do_wait(WEXITED) can't work until all threads exit

Yeap, we've discussed this before and this indeed is odd. However, is there something ptracer can't do with PTRACE_EVENT_EXIT instead? If PTRACE_EVENT_EXIT can be used instead for most practical purposes, I think we're better off just documenting the fact and advise use of PTRACE_EVENT_EXIT rather than trying to change the behavior.

Quoted 2 lines

	- if the tracer is real_parent, it is not clear why can't
	  we use ->exit_signal event if !thread_group_empty()

I've been thinking a bit more about this and it doesn't seem that changing this is necessarily a good idea. The current behavior does make certain sense (overridden exit_signal is used only for the real parent when the process is being reaped) and doesn't cause any actual problem, so I don't think we need to change this behavior.

kernel/exit.c static void exit_notify(struct task_stru · line 861

Quoted 5 lines

kernel/exit.c renamed from kernel/exit.c~2_kill_notify_death +5 -0

+	if (unlikely(tsk->ptrace)) {
+		int sig = thread_group_leader(tsk) &&
+				thread_group_empty(tsk) &&
+				!ptrace_reparented(tsk) ?
+			tsk->exit_signal : SIGCHLD;

Heh, I think this needs to be prettier even at the cost of an inline function.

Thanks.

Signature · 1 line

tejun

Oleg Nesterov

maintainer

2011-06-26 20:55 UTC #26 lore raw

Re: [PATCH 2/8] kill tracehook_notify_death()

Quoted 13 lines · Tejun Heo

Hello,
Quoted 9 lines · Oleg Nesterov
[PATCH v2 2/8] kill tracehook_notify_death()
Kill tracehook_notify_death(), reimplement the logic in its caller, exit_notify().
Also, change the exec_id's check to use thread_group_leader() instead of task_detached(), this is more clear. This logic only applies to the exiting leader, a sub-thread must never change its exit_signal.

Quoted 8 lines

Quoted 5 lines
Note: when the traced group leader exits the exit_signal-or-SIGCHLD logic looks really strange:
- we notify the tracer even if !thread_group_empty() but do_wait(WEXITED) can't work until all threads exit
Yeap, we've discussed this before and this indeed is odd. However, is there something ptracer can't do with PTRACE_EVENT_EXIT instead?

Firstly, I think PTRACE_EVENT_EXIT should not stop the tracee if it was SIGKILL'ed. Even if the tracee stops, it can be killed later. The tracer can't detach after that, it can't even wait() to detecte a zombie leader.

Quoted 1 line

rather than trying to change the behavior.

Yes, perhaps we shouldn't (or can't) change this behaviour, I am not sure. We will see.

Quoted 5 lines

Quoted 2 lines
	- if the tracer is real_parent, it is not clear why can't
	  we use ->exit_signal event if !thread_group_empty()
I've been thinking a bit more about this and it doesn't seem that changing this is necessarily a good idea.

Yes, agreed. This doesn't buy us something really useful.

Quoted 3 lines

The current behavior does
make certain sense (overridden exit_signal is used only for the real parent when the process is being reaped)

Oh, but this is the traced task. I do not think this behaviour was really designed, I can be wrong of course. For example, what "being reaped" actually means? Say, the group leader can exit after all other sub-threads have already exited, but thread_group_empty() == F exactly because a sub-thread is traced and wasn't reaped yet.

To me, it would be more clean to do

	if (tsk->ptrace) {
		int sig = ptrace_reparented(tsk) ?
			SIGCHLD : tsk->group_leader->exit_signal;
		
	}

Quoted 2 lines

and doesn't cause any actual
problem, so I don't think we need to change this behavior.

Agreed.

Quoted 8 lines

kernel/exit.c static void exit_notify(struct task_stru · line 861
Quoted 5 lines
kernel/exit.c renamed from kernel/exit.c~2_kill_notify_death +5 -0
+	if (unlikely(tsk->ptrace)) {
+		int sig = thread_group_leader(tsk) &&
+				thread_group_empty(tsk) &&
+				!ptrace_reparented(tsk) ?
+			tsk->exit_signal : SIGCHLD;
Heh, I think this needs to be prettier even at the cost of an inline function.

May be, but the code is sooooo simple. In fact I thought about the helper, but can't find a good name.

Anyway, this is so minor, unless you strongly object I am going to push this patch as is. We can add a helper later although I don't think it is needed.

The same logic could be written as

	if (thread_group_empty(tsk)) {
		int sig = ptrace_reparented(tsk) ?
			SIGCHLD : tsk->exit_signal;
		autoreap = do_notify_parent(tsk, sig);
	} else if (task->ptrace) {
		autoreap = do_notify_parent(tsk, SIGCHLD);
	} else {
		autoreap = !thread_group_leader();
	}

note that it certainly looks "prettier". However, personaly I strongly prefer the non-pretty code above, imho it is more straighforward and understandable. It is hardly possible to misread/misunderstand it.

Oleg.

Tejun Heo

author

2011-06-27 08:25 UTC #27 lore raw

Re: [PATCH 2/8] kill tracehook_notify_death()

Hello, Oleg.

Quoted 7 lines · Oleg Nesterov

Quoted 2 lines
Yeap, we've discussed this before and this indeed is odd. However, is there something ptracer can't do with PTRACE_EVENT_EXIT instead?
Firstly, I think PTRACE_EVENT_EXIT should not stop the tracee if it was SIGKILL'ed. Even if the tracee stops, it can be killed later. The tracer can't detach after that, it can't even wait() to detecte a zombie leader.

For SIGKILL, yes, it is different, but if PTRACE_EVENT_EXIT is enough for all other cases, I think we're mostly set. BTW, it seems like we would actually stop at PTRACE_EVENT_EXIT even after SIGKILL. This is wrong & racy. may_ptrace_stop() should be checking for sigkill_pending(), right?

Quoted 22 lines

Quoted 2 lines
Heh, I think this needs to be prettier even at the cost of an inline function.
May be, but the code is sooooo simple. In fact I thought about the helper, but can't find a good name.
Anyway, this is so minor, unless you strongly object I am going to push this patch as is. We can add a helper later although I don't think it is needed.
The same logic could be written as
	if (thread_group_empty(tsk)) {
		int sig = ptrace_reparented(tsk) ?
			SIGCHLD : tsk->exit_signal;
		autoreap = do_notify_parent(tsk, sig);
	} else if (task->ptrace) {
		autoreap = do_notify_parent(tsk, SIGCHLD);
	} else {
		autoreap = !thread_group_leader();
	}

Quoted 3 lines

note that it certainly looks "prettier". However, personaly I strongly prefer the non-pretty code above, imho it is more straighforward and understandable. It is hardly possible to misread/misunderstand it.

Well, it's cosmetic after all and you're the maintainer. I don't have any major problem with the original. Please go ahead.

Thanks.

Signature · 1 line

tejun

Oleg Nesterov

maintainer

2011-06-27 14:23 UTC #28 lore raw

Re: [PATCH 2/8] kill tracehook_notify_death()

Hi,

Quoted 5 lines · Tejun Heo

Quoted 3 lines · Oleg Nesterov
Quoted 2 lines
Yeap, we've discussed this before and this indeed is odd. However, is there something ptracer can't do with PTRACE_EVENT_EXIT instead?

Quoted 7 lines

Quoted 4 lines
Firstly, I think PTRACE_EVENT_EXIT should not stop the tracee if it was SIGKILL'ed. Even if the tracee stops, it can be killed later. The tracer can't detach after that, it can't even wait() to detecte a zombie leader.
For SIGKILL, yes, it is different, but if PTRACE_EVENT_EXIT is enough for all other cases, I think we're mostly set.

I think this is not that simple. I already mentioned this before, I think we need a separate discussion. I'll try to return to this in a few days.

Firstly, we should decide when PTRACE_EVENT_EXIT should stop, and when it shouldn't. In this discussion I'll assume sys_exit_group() should respect PTRACE_EVENT_EXIT.

Quoted 3 lines

BTW, it seems like we
would actually stop at PTRACE_EVENT_EXIT even after SIGKILL. This is wrong & racy.

Yes! because the tracee can call ptrace_stop() after the pending SIGKILL was already dequeued from task->pending, this fools schedule()->signal_pending_state().

So, __fatal_signal_pending() is too "weak",

Quoted 2 lines

may_ptrace_stop() should be checking for
sigkill_pending(), right?

Yes, but at the same time even __fatal_signal_pending() is too strong! What if the tracee exits on its own, and its sys_exit() races with exit_group() from another thread? In this case I think it should stop, but __fatal_signal_pending() is true.

And worse. What if the tracee stops in PTRACE_EVENT_EXIT, and _then_ another thread does sys_exit_group()? The tracee will be "killed". I do not think this is right. I think the "implicit" SIGKILL in this case should _not_ wake up the tracee. Only the real SIGKILL (or any fatal signal which mutates to SIGKILL). Otherwise we simply can't guarantee PTRACE_EVENT_EXIT works "reliably" in this case.

We have signal_group_exit()/SIGNAL_GROUP_EXIT. I think we also need SIGNAL_THE_REAL_SIGKILL_WAS_SENT flag. Note also we have the similar problems with the coredump. SIGKILL should abort it. Also, we should define what TIF_SIGPENDIND and interruptible wait mean after exit_signals() and/or exit_notify(). Some drivers (tty? I do not remember) expect that the exiting task can do wait_event_interruptible() and react to ^C.

Quoted 2 lines

I don't have
any major problem with the original. Please go ahead.

Thanks. Can I add your reviewed-by/acked-by ?

Oleg.

Tejun Heo

author

2011-06-27 14:36 UTC #29 lore raw

Re: [PATCH 2/8] kill tracehook_notify_death()

Hello,

Quoted 11 lines · Oleg Nesterov

Yes, but at the same time even __fatal_signal_pending() is too strong! What if the tracee exits on its own, and its sys_exit() races with exit_group() from another thread? In this case I think it should stop, but __fatal_signal_pending() is true.
And worse. What if the tracee stops in PTRACE_EVENT_EXIT, and _then_ another thread does sys_exit_group()? The tracee will be "killed". I do not think this is right. I think the "implicit" SIGKILL in this case should _not_ wake up the tracee. Only the real SIGKILL (or any fatal signal which mutates to SIGKILL). Otherwise we simply can't guarantee PTRACE_EVENT_EXIT works "reliably" in this case.

Indeed, for PTRACE_EVENT_EXIT to work properly, we would need to discern between actual KILLs and the implicit ones.

Quoted 6 lines

We have signal_group_exit()/SIGNAL_GROUP_EXIT. I think we also need SIGNAL_THE_REAL_SIGKILL_WAS_SENT flag. Note also we have the similar problems with the coredump. SIGKILL should abort it. Also, we should define what TIF_SIGPENDIND and interruptible wait mean after exit_signals() and/or exit_notify(). Some drivers (tty? I do not remember) expect that the exiting task can do wait_event_interruptible() and react to ^C.

Eh... that sounds very shady. Maybe we should try to remove that weird requirement instead?

Quoted 4 lines

Quoted 2 lines
I don't have
any major problem with the original. Please go ahead.
Thanks. Can I add your reviewed-by/acked-by ?

Sure, please feel free to add reviewed-by. Thanks.

Signature · 1 line

tejun

Oleg Nesterov

maintainer

2011-06-22 21:11 UTC #30 lore raw

PATCH 3/8 __ptrace_detach: avoid task_detached(), check do_notify_parent()

In reply to Oleg Nesterov

__ptrace_detach() relies on the current obscure behaviour of do_notify_parent(tsk) which changes tsk->exit_signal if this child should be silently reaped. That is why we check task_detached(), it is true if the task is sub-thread, or it is the group_leader but its exit_signal was changed by do_notify_parent().

This is confusing, change the code to rely on !thread_group_leader() or the value returned by do_notify_parent().

Signed-off-by Oleg Nesterov <[email protected]>

---

 kernel/ptrace.c |   33 ++++++++++++++++++---------------
 1 file changed, 18 insertions(+), 15 deletions(-)

kernel/ptrace.c renamed from kernel/ptrace.c~3_detach_ck_notify +18 ~~-15~~

@@ -370,25 +370,28 @@ static int ignoring_children(struct sigh
  */
 static bool __ptrace_detach(struct task_struct *tracer, struct task_struct *p)
 {
+	bool dead;
+
 	__ptrace_unlink(p);
 
-	if (p->exit_state == EXIT_ZOMBIE) {
-		if (!task_detached(p) && thread_group_empty(p)) {
-			if (!same_thread_group(p->real_parent, tracer))
-				do_notify_parent(p, p->exit_signal);
-			else if (ignoring_children(tracer->sighand)) {
-				__wake_up_parent(p, tracer);
-				p->exit_signal = -1;
-			}
-		}
-		if (task_detached(p)) {
-			/* Mark it as in the process of being reaped. */
-			p->exit_state = EXIT_DEAD;
-			return true;
+	if (p->exit_state != EXIT_ZOMBIE)
+		return false;
+
+	dead = !thread_group_leader(p);
+
+	if (!dead && thread_group_empty(p)) {
+		if (!same_thread_group(p->real_parent, tracer))
+			dead = do_notify_parent(p, p->exit_signal);
+		else if (ignoring_children(tracer->sighand)) {
+			__wake_up_parent(p, tracer);
+			p->exit_signal = -1;
+			dead = true;
 		}
 	}
-
-	return false;
+	/* Mark it as in the process of being reaped. */
+	if (dead)
+		p->exit_state = EXIT_DEAD;
+	return dead;
 }
 
 static int ptrace_detach(struct task_struct *child, unsigned int data)

Tejun Heo

author Acked-by on v1 3/8

2011-06-23 13:25 UTC #31 lore raw

Re: [PATCH 3/8] __ptrace_detach: avoid task_detached(), check do_notify_parent()

Quoted 10 lines · Oleg Nesterov

__ptrace_detach() relies on the current obscure behaviour of do_notify_parent(tsk) which changes tsk->exit_signal if this child should be silently reaped. That is why we check task_detached(), it is true if the task is sub-thread, or it is the group_leader but its exit_signal was changed by do_notify_parent().
This is confusing, change the code to rely on !thread_group_leader() or the value returned by do_notify_parent().
Signed-off-by Oleg Nesterov <[email protected]>

This looks good to me.

Acked-by Tejun Heo <[email protected]>

Thanks.

Signature · 1 line

tejun

Oleg Nesterov

maintainer

2011-06-22 21:11 UTC #32 lore raw

PATCH 4/8 make do_notify_parent() __must_check, update the callers

In reply to Oleg Nesterov

Change other callers of do_notify_parent() to check the value it returns, this makes the subsequent task_detached() unnecessary. Mark do_notify_parent() as __must_check.

Use thread_group_leader() instead of !task_detached() to check if we need to notify the real parent in wait_task_zombie().

Remove the stale comment in release_task(). "just for sanity" is no longer true, we have to set EXIT_DEAD to avoid the races with do_wait().

Signed-off-by Oleg Nesterov <[email protected]>

---

 include/linux/sched.h |    2 +-
 kernel/exit.c         |   29 ++++++++---------------------
 2 files changed, 9 insertions(+), 22 deletions(-)

include/linux/sched.h renamed from include/linux/sched.h~4_all_notify_callers +1 -1

@@ -2161,7 +2161,7 @@ extern int kill_pid_info_as_uid(int, str
 extern int kill_pgrp(struct pid *pid, int sig, int priv);
 extern int kill_pid(struct pid *pid, int sig, int priv);
 extern int kill_proc_info(int, struct siginfo *, pid_t);
-extern bool do_notify_parent(struct task_struct *, int);
+extern __must_check bool do_notify_parent(struct task_struct *, int);
 extern void __wake_up_parent(struct task_struct *p, struct task_struct *parent);
 extern void force_sig(int, struct task_struct *);
 extern int send_sig(int, struct task_struct *, int);

kernel/exit.c renamed from kernel/exit.c~4_all_notify_callers +8 ~~-21~~

@@ -190,21 +190,12 @@ repeat:
 	leader = p->group_leader;
 	if (leader != p && thread_group_empty(leader) && leader->exit_state == EXIT_ZOMBIE) {
 		BUG_ON(task_detached(leader));
-		do_notify_parent(leader, leader->exit_signal);
 		/*
 		 * If we were the last child thread and the leader has
 		 * exited already, and the leader's parent ignores SIGCHLD,
 		 * then we are the one who should release the leader.
-		 *
-		 * do_notify_parent() will have marked it self-reaping in
-		 * that case.
-		 */
-		zap_leader = task_detached(leader);
-
-		/*
-		 * This maintains the invariant that release_task()
-		 * only runs on a task in EXIT_DEAD, just for sanity.
 		 */
+		zap_leader = do_notify_parent(leader, leader->exit_signal);
 		if (zap_leader)
 			leader->exit_state = EXIT_DEAD;
 	}

@@ -765,8 +756,7 @@ static void reparent_leader(struct task_
 	/* If it has exited notify the new parent about this child's death. */
 	if (!p->ptrace &&
 	    p->exit_state == EXIT_ZOMBIE && thread_group_empty(p)) {
-		do_notify_parent(p, p->exit_signal);
-		if (task_detached(p)) {
+		if (do_notify_parent(p, p->exit_signal)) {
 			p->exit_state = EXIT_DEAD;
 			list_move_tail(&p->sibling, dead);
 		}

@@ -1348,16 +1338,13 @@ static int wait_task_zombie(struct wait_
 		/* We dropped tasklist, ptracer could die and untrace */
 		ptrace_unlink(p);
 		/*
-		 * If this is not a detached task, notify the parent.
-		 * If it's still not detached after that, don't release
-		 * it now.
+		 * If this is not a sub-thread, notify the parent.
+		 * If parent wants a zombie, don't release it now.
 		 */
-		if (!task_detached(p)) {
-			do_notify_parent(p, p->exit_signal);
-			if (!task_detached(p)) {
-				p->exit_state = EXIT_ZOMBIE;
-				p = NULL;
-			}
+		if (thread_group_leader(p) &&
+		    !do_notify_parent(p, p->exit_signal)) {
+			p->exit_state = EXIT_ZOMBIE;
+			p = NULL;
 		}
 		write_unlock_irq(&tasklist_lock);
 	}

Tejun Heo

author Acked-by on v1 4/8

2011-06-23 13:38 UTC #33 lore raw

Re: [PATCH 4/8] make do_notify_parent() __must_check, update the callers

Quoted 4 lines · Oleg Nesterov

Change other callers of do_notify_parent() to check the value it returns, this makes the subsequent task_detached() unnecessary. Mark do_notify_parent() as __must_check.

Quoted 8 lines

Use thread_group_leader() instead of !task_detached() to check if we need to notify the real parent in wait_task_zombie().
Remove the stale comment in release_task(). "just for sanity" is no longer true, we have to set EXIT_DEAD to avoid the races with do_wait().
Signed-off-by Oleg Nesterov <[email protected]>

Acked-by Tejun Heo <[email protected]>

Thanks.

Signature · 1 line

tejun

Oleg Nesterov

maintainer

2011-06-22 21:12 UTC #34 lore raw

PATCH 5/8 reparent_leader: check EXIT_DEAD instead of task_detached()

In reply to Oleg Nesterov

Change reparent_leader() to check ->exit_state instead of ->exit_signal, this matches the similar EXIT_DEAD check in wait_consider_task() and allows us to cleanup the do_notify_parent/task_detached logic.

task_detached() was really needed during reparenting before 9cd80bbb "do_wait() optimization: do not place sub-threads on ->children list" to filter out the sub-threads. After this change task_detached(p) can only be true if p is the dead group_leader and its parent ignores SIGCHLD, in this case the caller of do_notify_parent() is going to reap this task and it should set EXIT_DEAD.

Signed-off-by Oleg Nesterov <[email protected]>

---

 kernel/exit.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

kernel/exit.c renamed from kernel/exit.c~5_reparent_leader_ck_exit_dead +1 -1

@@ -741,7 +741,7 @@ static void reparent_leader(struct task_
 {
 	list_move_tail(&p->sibling, &p->real_parent->children);
 
-	if (task_detached(p))
+	if (p->exit_state == EXIT_DEAD)
 		return;
 	/*
 	 * If this is a threaded reparent there is no need to

Oleg Nesterov

maintainer

2011-06-22 21:11 UTC #35 lore raw

PATCH 6/8 kill task_detached()

In reply to Oleg Nesterov

Upadate the last user of task_detached(), wait_task_zombie(), to use thread_group_leader() and kill task_detached().

Signed-off-by Oleg Nesterov <[email protected]>

---

 include/linux/sched.h |    5 -----
 kernel/exit.c         |    5 ++---
 2 files changed, 2 insertions(+), 8 deletions(-)

include/linux/sched.h renamed from include/linux/sched.h~6_kill_task_detached +0 -5

@@ -2319,11 +2319,6 @@ static inline int thread_group_empty(str
 #define delay_group_leader(p) \
 		(thread_group_leader(p) && !thread_group_empty(p))
 
-static inline int task_detached(struct task_struct *p)
-{
-	return p->exit_signal == -1;
-}
-
 /*
  * Protects ->fs, ->files, ->mm, ->group_info, ->comm, keyring
  * subscriptions and synchronises with wait4().  Also used in procfs.  Also

kernel/exit.c renamed from kernel/exit.c~6_kill_task_detached +2 -3

@@ -189,7 +189,6 @@ repeat:
 	zap_leader = 0;
 	leader = p->group_leader;
 	if (leader != p && thread_group_empty(leader) && leader->exit_state == EXIT_ZOMBIE) {
-		BUG_ON(task_detached(leader));
 		/*
 		 * If we were the last child thread and the leader has
 		 * exited already, and the leader's parent ignores SIGCHLD,

@@ -1228,9 +1227,9 @@ static int wait_task_zombie(struct wait_
 	traced = ptrace_reparented(p);
 	/*
 	 * It can be ptraced but not reparented, check
-	 * !task_detached() to filter out sub-threads.
+	 * thread_group_leader() to filter out sub-threads.
 	 */
-	if (likely(!traced) && likely(!task_detached(p))) {
+	if (likely(!traced) && thread_group_leader(p)) {
 		struct signal_struct *psig;
 		struct signal_struct *sig;
 		unsigned long maxrss;

Oleg Nesterov

maintainer

2011-06-22 21:12 UTC #36 lore raw

PATCH 7/8 do not change dead_task->exit_signal

In reply to Oleg Nesterov

__ptrace_detach() and do_notify_parent() set task->exit_signal = -1 to mark the task dead. This is no longer needed, nobody checks exit_signal to detect the EXIT_DEAD task.

Signed-off-by Oleg Nesterov <[email protected]>

---

 kernel/ptrace.c |    1 -
 kernel/signal.c |    1 -
 2 files changed, 2 deletions(-)

kernel/ptrace.c renamed from kernel/ptrace.c~7_dont_change_exit_signal +0 -1

@@ -384,7 +384,6 @@ static bool __ptrace_detach(struct task_
 			dead = do_notify_parent(p, p->exit_signal);
 		else if (ignoring_children(tracer->sighand)) {
 			__wake_up_parent(p, tracer);
-			p->exit_signal = -1;
 			dead = true;
 		}
 	}

kernel/signal.c renamed from kernel/signal.c~7_dont_change_exit_signal +0 -1

@@ -1650,7 +1650,6 @@ bool do_notify_parent(struct task_struct
 		 * it, just use SIG_IGN instead).
 		 */
 		autoreap = true;
-		tsk->exit_signal = -1;
 		if (psig->action[SIGCHLD-1].sa.sa_handler == SIG_IGN)
 			sig = 0;
 	}

Oleg Nesterov

maintainer

2011-06-22 21:12 UTC #37 lore raw

PATCH 8/8 redefine thread_group_leader() as exit_signal >= 0

In reply to Oleg Nesterov

Change de_thread() to set old_leader->exit_signal = -1. This is good for the consistency, it is no longer the leader and all sub-threads have exit_signal = -1 set by copy_process(CLONE_THREAD).

And this allows us to micro-optimize thread_group_leader(), it can simply check exit_signal >= 0. This also makes sense because we should move ->group_leader from task_struct to signal_struct.

Signed-off-by Oleg Nesterov <[email protected]>

---

 include/linux/sched.h |    6 ++++--
 fs/exec.c             |    1 +
 2 files changed, 5 insertions(+), 2 deletions(-)

include/linux/sched.h renamed from include/linux/sched.h~8_thread_group_leader +4 -2

@@ -2285,8 +2285,10 @@ static inline int get_nr_threads(struct 
 	return tsk->signal->nr_threads;
 }
 
-/* de_thread depends on thread_group_leader not being a pid based check */
-#define thread_group_leader(p)	(p == p->group_leader)
+static inline bool thread_group_leader(struct task_struct *p)
+{
+	return p->exit_signal >= 0;
+}
 
 /* Do to the insanities of de_thread it is possible for a process
  * to have the pid of the thread group leader without actually being

fs/exec.c renamed from fs/exec.c~8_thread_group_leader +1 -0

@@ -963,6 +963,7 @@ static int de_thread(struct task_struct 
 		leader->group_leader = tsk;
 
 		tsk->exit_signal = SIGCHLD;
+		leader->exit_signal = -1;
 
 		BUG_ON(leader->exit_state != EXIT_ZOMBIE);
 		leader->exit_state = EXIT_DEAD;

Tejun Heo

author

2011-06-23 13:56 UTC #38 lore raw

Re: [PATCH 8/8] redefine thread_group_leader() as exit_signal >= 0

Hello,

Quoted 9 lines · Oleg Nesterov

Change de_thread() to set old_leader->exit_signal = -1. This is good for the consistency, it is no longer the leader and all sub-threads have exit_signal = -1 set by copy_process(CLONE_THREAD).
And this allows us to micro-optimize thread_group_leader(), it can simply check exit_signal >= 0. This also makes sense because we should move ->group_leader from task_struct to signal_struct.
Signed-off-by Oleg Nesterov <[email protected]>

Patches 5-8 look correct to me but I frankly don't know the area enough to assert acked-by's. Please feel free to add reviewed-by's tho.

Thanks.

Signature · 1 line

tejun

Tejun Heo

author

2011-06-17 14:52 UTC #39 lore raw

PATCH 7/7 ptrace: s/tracehook_tracer_task()/ptrace_parent()/

In reply to Tejun Heo

tracehook.h is on the way out. Rename tracehook_tracer_task() to ptrace_parent() and move it from tracehook.h to ptrace.h.

Signed-off-by Tejun Heo <[email protected]>

Cc Christoph Hellwig <[email protected]>

Cc John Johansen <[email protected]>

Cc Stephen Smalley <[email protected]>

---
 fs/proc/array.c            |    2 +-
 fs/proc/base.c             |    2 +-
 include/linux/ptrace.h     |   18 ++++++++++++++++++
 include/linux/tracehook.h  |   18 ------------------
 security/apparmor/domain.c |    2 +-
 security/selinux/hooks.c   |    4 ++--
 6 files changed, 23 insertions(+), 23 deletions(-)

fs/proc/array.c +1 -1

index 9b45ee8..3a1dafd 100644
@@ -172,7 +172,7 @@ static inline void task_state(struct seq_file *m, struct pid_namespace *ns,
 		task_tgid_nr_ns(rcu_dereference(p->real_parent), ns) : 0;
 	tpid = 0;
 	if (pid_alive(p)) {
-		struct task_struct *tracer = tracehook_tracer_task(p);
+		struct task_struct *tracer = ptrace_parent(p);
 		if (tracer)
 			tpid = task_pid_nr_ns(tracer, ns);
 	}

fs/proc/base.c +1 -1

index 14def99..c883dad 100644
@@ -216,7 +216,7 @@ static struct mm_struct *__check_mem_permission(struct task_struct *task)
 	if (task_is_stopped_or_traced(task)) {
 		int match;
 		rcu_read_lock();
-		match = (tracehook_tracer_task(task) == current);
+		match = (ptrace_parent(task) == current);
 		rcu_read_unlock();
 		if (match && ptrace_may_access(task, PTRACE_MODE_ATTACH))
 			return mm;

include/linux/ptrace.h +18 -0

index b546fd6..bb157bd 100644
@@ -151,6 +151,24 @@ int generic_ptrace_pokedata(struct task_struct *tsk, unsigned long addr,
 			    unsigned long data);
 
 /**
+ * ptrace_parent - return the task that is tracing the given task
+ * @task: task to consider
+ *
+ * Returns %NULL if no one is tracing @task, or the &struct task_struct
+ * pointer to its tracer.
+ *
+ * Must called under rcu_read_lock().  The pointer returned might be kept
+ * live only by RCU.  During exec, this may be called with task_lock() held
+ * on @task, still held from when check_unsafe_exec() was called.
+ */
+static inline struct task_struct *ptrace_parent(struct task_struct *task)
+{
+	if (unlikely(task->ptrace))
+		return rcu_dereference(task->parent);
+	return NULL;
+}
+
+/**
  * ptrace_event_enabled - test whether a ptrace event is enabled
  * @task: ptracee of interest
  * @event: %PTRACE_EVENT_* to test

include/linux/tracehook.h +0 ~~-18~~

index fea0d4b..a71a292 100644
@@ -130,24 +130,6 @@ static inline void tracehook_report_syscall_exit(struct pt_regs *regs, int step)
 }
 
 /**
- * tracehook_tracer_task - return the task that is tracing the given task
- * @tsk:		task to consider
- *
- * Returns NULL if no one is tracing @task, or the &struct task_struct
- * pointer to its tracer.
- *
- * Must called under rcu_read_lock().  The pointer returned might be kept
- * live only by RCU.  During exec, this may be called with task_lock()
- * held on @task, still held from when tracehook_unsafe_exec() was called.
- */
-static inline struct task_struct *tracehook_tracer_task(struct task_struct *tsk)
-{
-	if (tsk->ptrace & PT_PTRACED)
-		return rcu_dereference(tsk->parent);
-	return NULL;
-}
-
-/**
  * tracehook_signal_handler - signal handler setup is complete
  * @sig:		number of signal being delivered
  * @info:		siginfo_t of signal being delivered

security/apparmor/domain.c +1 -1

index c825c6e..7312bf9 100644
@@ -67,7 +67,7 @@ static int may_change_ptraced_domain(struct task_struct *task,
 	int error = 0;
 
 	rcu_read_lock();
-	tracer = tracehook_tracer_task(task);
+	tracer = ptrace_parent(task);
 	if (tracer) {
 		/* released below */
 		cred = get_task_cred(tracer);

security/selinux/hooks.c +2 -2

index a0d3845..fc07d18 100644
@@ -2048,7 +2048,7 @@ static int selinux_bprm_set_creds(struct linux_binprm *bprm)
 			u32 ptsid = 0;
 
 			rcu_read_lock();
-			tracer = tracehook_tracer_task(current);
+			tracer = ptrace_parent(current);
 			if (likely(tracer != NULL)) {
 				sec = __task_cred(tracer)->security;
 				ptsid = sec->sid;

@@ -5314,7 +5314,7 @@ static int selinux_setprocattr(struct task_struct *p,
 		   Otherwise, leave SID unchanged and fail. */
 		ptsid = 0;
 		task_lock(p);
-		tracer = tracehook_tracer_task(p);
+		tracer = ptrace_parent(p);
 		if (tracer)
 			ptsid = task_sid(tracer);
 		task_unlock(p);

Signature · 1 line

1.7.5.4

Oleg Nesterov

maintainer

2011-06-20 20:18 UTC #40 lore raw

Re: [PATCH 7/7] ptrace: s/tracehook_tracer_task()/ptrace_parent()/

Quoted 3 lines · Tejun Heo

tracehook.h is on the way out. Rename tracehook_tracer_task() to ptrace_parent() and move it from tracehook.h to ptrace.h.

I am a bit surpised you decided to keep this helper. Can't we simply kill it?

OK, we will see. I guess this change is mostly needed to remove yet another function from tracehook.h.

fs/proc/base.c static struct mm_struct *__check_mem_permission(struct task_struct *task) · line 221

Quoted 8 lines

fs/proc/base.c +1 -1

@@ -216,7 +216,7 @@ static struct mm_struct *__check_mem_permission(struct task_struct *task)
 	if (task_is_stopped_or_traced(task)) {
 		int match;
 		rcu_read_lock();
-		match = (tracehook_tracer_task(task) == current);
+		match = (ptrace_parent(task) == current);
 		rcu_read_unlock();
 		if (match && ptrace_may_access(task, PTRACE_MODE_ATTACH))

All we need

	if (task_is_traced(task) && task->parent == current) {
		if (ptrace_may_access()
			return mm;
	}

Of course I do not blame this patch, my only point is that this helper only adds more confusion imho.

security/apparmor/domain.c static int may_change_ptraced_domain(struct task_struct *task, · line 73

Quoted 9 lines

security/apparmor/domain.c +1 -1

@@ -67,7 +67,7 @@ static int may_change_ptraced_domain(struct task_struct *task,
 	int error = 0;
 
 	rcu_read_lock();
-	tracer = tracehook_tracer_task(task);
+	tracer = ptrace_parent(task);
 	if (tracer) {
 		/* released below */
 		cred = get_task_cred(tracer);

Hmm. And then this task_struct is used after we dropped rcu_read_lock().

John, is this correct?

Oleg.

John Johansen

maintainer

2011-06-21 11:44 UTC #41 lore raw

Re: [PATCH 7/7] ptrace: s/tracehook_tracer_task()/ptrace_parent()/

Quoted 45 lines · Oleg Nesterov

Quoted 3 lines · Tejun Heo
tracehook.h is on the way out. Rename tracehook_tracer_task() to ptrace_parent() and move it from tracehook.h to ptrace.h.
I am a bit surpised you decided to keep this helper. Can't we simply kill it?
OK, we will see. I guess this change is mostly needed to remove yet another function from tracehook.h.
Review static struct mm_struct *__check_mem_permission(struct task_struct *task) · line 221
Quoted 8 lines
(quoted diff) +1 -1
@@ -216,7 +216,7 @@ static struct mm_struct *__check_mem_permission(struct task_struct *task)
 	if (task_is_stopped_or_traced(task)) {
 		int match;
 		rcu_read_lock();
-		match = (tracehook_tracer_task(task) == current);
+		match = (ptrace_parent(task) == current);
 		rcu_read_unlock();
 		if (match && ptrace_may_access(task, PTRACE_MODE_ATTACH))
All we need
	if (task_is_traced(task) && task->parent == current) {
		if (ptrace_may_access()
			return mm;
	}
Of course I do not blame this patch, my only point is that this helper only adds more confusion imho.
Review static int may_change_ptraced_domain(struct task_struct *task, · line 73
Quoted 9 lines
(quoted diff) +1 -1
@@ -67,7 +67,7 @@ static int may_change_ptraced_domain(struct task_struct *task,
 	int error = 0;
 
 	rcu_read_lock();
-	tracer = tracehook_tracer_task(task);
+	tracer = ptrace_parent(task);
 	if (tracer) {
 		/* released below */
 		cred = get_task_cred(tracer);
Hmm. And then this task_struct is used after we dropped rcu_read_lock().

Quoted 2 lines

John, is this correct?

nope this use is wrong. The following patch should fix this

===

AppArmor: Fix reference to rcu protected pointer outside of rcu_read_lock

The pointer returned from tracehook_tracer_task() is only valid inside the rcu_read_lock. However the tracer pointer obtained is being passed to aa_may_ptrace outside of the rcu_read_lock critical section.

Mover the aa_may_ptrace test into the rcu_read_lock critical section, to fix this.

Signed-off-by John Johansen <[email protected]>

---
 security/apparmor/domain.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

security/apparmor/domain.c +1 -1

index c825c6e..78adc43 100644
@@ -73,7 +73,6 @@ static int may_change_ptraced_domain(struct task_struct *task,
 		cred = get_task_cred(tracer);
 		tracerp = aa_cred_profile(cred);
 	}
-	rcu_read_unlock();
 
 	/* not ptraced */
 	if (!tracer || unconfined(tracerp))

@@ -82,6 +81,7 @@ static int may_change_ptraced_domain(struct task_struct *task,
 	error = aa_may_ptrace(tracer, tracerp, to_profile, PTRACE_MODE_ATTACH);
 
 out:
+	rcu_read_unlock();
 	if (cred)
 		put_cred(cred);

Signature · 1 line

1.7.4.1

Tejun Heo

author

2011-06-23 09:14 UTC #42 lore raw

Re: [PATCH 7/7] ptrace: s/tracehook_tracer_task()/ptrace_parent()/

In reply to Oleg Nesterov

Hello,

Quoted 7 lines · Oleg Nesterov

Quoted 3 lines · Tejun Heo
tracehook.h is on the way out. Rename tracehook_tracer_task() to ptrace_parent() and move it from tracehook.h to ptrace.h.
I am a bit surpised you decided to keep this helper. Can't we simply kill it?

I was being a bit lazy. If it can be killed, it would be great.

Quoted 14 lines

OK, we will see. I guess this change is mostly needed to remove yet another function from tracehook.h.
fs/proc/base.c static struct mm_struct *__check_mem_permission(struct task_struct *task) · line 221
Quoted 8 lines
fs/proc/base.c +1 -1
@@ -216,7 +216,7 @@ static struct mm_struct *__check_mem_permission(struct task_struct *task)
 	if (task_is_stopped_or_traced(task)) {
 		int match;
 		rcu_read_lock();
-		match = (tracehook_tracer_task(task) == current);
+		match = (ptrace_parent(task) == current);
 		rcu_read_unlock();
 		if (match && ptrace_may_access(task, PTRACE_MODE_ATTACH))
All we need

Quoted 7 lines

	if (task_is_traced(task) && task->parent == current) {
		if (ptrace_may_access()
			return mm;
	}
Of course I do not blame this patch, my only point is that this helper only adds more confusion imho.

Yeah, it didn't seem like all the users actually required rcu dereferencing and both the tracehook and ptrace function names are confusing and not consistent in their use. We can convert the ones which don't require rcu dereferencing to use ->parent directly and if the left ones are few enough, maybe kill the helper altogether.

Thanks.

Signature · 1 line

tejun

Christoph Hellwig

2011-06-20 11:17 UTC #43 lore raw

In reply to Tejun Heo

Quoted 10 lines · Tejun Heo

Hello,
At this point, tracehooks aren't useful to mainline kernel and mostly just add an extra layer of obfuscation. Although they have comments, without actual in-kernel users, it is difficult to tell what are their assumptions and they're actually trying to achieve. To mainline kernel, they just aren't worth keeping around.
This patchset kills most tracehooks which aren't used from arch codes. The remaining ones will be dealt with with future changes.

Thanks, this makes the whole ptrace interaction a lot more sensible and actually possible to follow.