[PATCH] ptrace: do not use task->ptrace directly in core kernel

[PATCH] ptrace: do not use task->ptrace directly in core kernel

[Oleg Nesterov]

No functional changes.

- Nobody except ptrace.c & co should use ptrace flags directly, we have
  task_ptrace() for that.

- No need to specially check PT_PTRACED, we must not have other PT_ bits
  set without PT_PTRACED. And no need to know this flag exists.

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

 kernel/exit.c   |    6 +++---
 kernel/signal.c |   10 +++++-----
 kernel/fork.c   |    2 +-
 3 files changed, 9 insertions(+), 9 deletions(-)

--- PTRACE/kernel/exit.c~CORE_FLAGS	2009-04-23 23:11:33.000000000 +0200
+++ PTRACE/kernel/exit.c	2009-04-29 12:46:15.000000000 +0200
@@ -757,7 +757,7 @@ static void reparent_thread(struct task_
 	p->exit_signal = SIGCHLD;
 
 	/* If it has exited notify the new parent about this child's death. */
-	if (!p->ptrace &&
+	if (!task_ptrace(p) &&
 	    p->exit_state == EXIT_ZOMBIE && thread_group_empty(p)) {
 		do_notify_parent(p, p->exit_signal);
 		if (task_detached(p)) {
@@ -782,7 +782,7 @@ static void forget_original_parent(struc
 	list_for_each_entry_safe(p, n, &father->children, sibling) {
 		p->real_parent = reaper;
 		if (p->parent == father) {
-			BUG_ON(p->ptrace);
+			BUG_ON(task_ptrace(p));
 			p->parent = p->real_parent;
 		}
 		reparent_thread(father, p, &dead_children);
@@ -1478,7 +1478,7 @@ static int wait_consider_task(struct tas
 			*notask_error = ret;
 	}
 
-	if (likely(!ptrace) && unlikely(p->ptrace)) {
+	if (likely(!ptrace) && unlikely(task_ptrace(p))) {
 		/*
 		 * This child is hidden by ptrace.
 		 * We aren't allowed to see it now, but eventually we will.
--- PTRACE/kernel/signal.c~CORE_FLAGS	2009-04-06 00:03:42.000000000 +0200
+++ PTRACE/kernel/signal.c	2009-04-29 13:21:47.000000000 +0200
@@ -1402,7 +1402,7 @@ int do_notify_parent(struct task_struct 
  	/* do_notify_parent_cldstop should have been called instead.  */
  	BUG_ON(task_is_stopped_or_traced(tsk));
 
-	BUG_ON(!tsk->ptrace &&
+	BUG_ON(!task_ptrace(tsk) &&
 	       (tsk->group_leader != tsk || !thread_group_empty(tsk)));
 
 	info.si_signo = sig;
@@ -1441,7 +1441,7 @@ int do_notify_parent(struct task_struct 
 
 	psig = tsk->parent->sighand;
 	spin_lock_irqsave(&psig->siglock, flags);
-	if (!tsk->ptrace && sig == SIGCHLD &&
+	if (!task_ptrace(tsk) && sig == SIGCHLD &&
 	    (psig->action[SIGCHLD-1].sa.sa_handler == SIG_IGN ||
 	     (psig->action[SIGCHLD-1].sa.sa_flags & SA_NOCLDWAIT))) {
 		/*
@@ -1478,7 +1478,7 @@ static void do_notify_parent_cldstop(str
 	struct task_struct *parent;
 	struct sighand_struct *sighand;
 
-	if (tsk->ptrace & PT_PTRACED)
+	if (task_ptrace(tsk))
 		parent = tsk->parent;
 	else {
 		tsk = tsk->group_leader;
@@ -1527,7 +1527,7 @@ static void do_notify_parent_cldstop(str
 
 static inline int may_ptrace_stop(void)
 {
-	if (!likely(current->ptrace & PT_PTRACED))
+	if (!likely(task_ptrace(current)))
 		return 0;
 	/*
 	 * Are we in the middle of do_coredump?
@@ -1745,7 +1745,7 @@ static int do_signal_stop(int signr)
 static int ptrace_signal(int signr, siginfo_t *info,
 			 struct pt_regs *regs, void *cookie)
 {
-	if (!(current->ptrace & PT_PTRACED))
+	if (!task_ptrace(current))
 		return signr;
 
 	ptrace_signal_deliver(regs, cookie);
--- PTRACE/kernel/fork.c~CORE_FLAGS	2009-04-27 21:38:57.000000000 +0200
+++ PTRACE/kernel/fork.c	2009-04-29 13:25:29.000000000 +0200
@@ -1088,7 +1088,7 @@ static struct task_struct *copy_process(
 #ifdef CONFIG_DEBUG_MUTEXES
 	p->blocked_on = NULL; /* not blocked yet */
 #endif
-	if (unlikely(current->ptrace))
+	if (unlikely(task_ptrace(current)))
 		ptrace_fork(p, clone_flags);
 
 	/* Perform scheduler related setup. Assign this task to a CPU. */

Re: [PATCH] ptrace: do not use task->ptrace directly in core kernel

[Roland McGrath]

That is fine, but doesn't buy much.  i.e., we will be changing these again
before too long anyway I imagine.

I added task_ptrace() just for tracehook.h use, really.  There it drives
the event hooks.  Those uses are directly obsoleted by using another event
hooking mechanism such as utrace.  That applies to ptrace_signal() too.
But the other uses will be replaced by something different later, not just
go away.

The BUG_ON cases might as well just go away, probably.

The exit.c cases might be clearer if we give them a (trivial) local helper
with a more topical name like task_wait_inhibited().

ptrace_fork() is a wrapper that just calls arch_ptrace_fork(), which itself
is an empty macro on most configurations.  I think we might as well just
make ptrace_fork() an inline in linux/ptrace.h and put the test inside it.
(Thus any future changes touch ptrace.h and not fork.c.)


Thanks,
Roland

Re: [PATCH] ptrace: do not use task->ptrace directly in core kernel

[Oleg Nesterov]

On 04/30, Roland McGrath wrote:
>
> That is fine, but doesn't buy much.

Yes this patch is only cosmetic. But it allows us to change the internals
in ptrace.c without thinking about other kernel/*.c. The only goal is to
make the further changes smaller and more reviewable.

Honestly, I'd also like to temporary change ptrace.c too, see the patch
below. But this is minor.

> i.e., we will be changing these again
> before too long anyway I imagine.

Yes, agreed.

> The BUG_ON cases might as well just go away, probably.

Agreed.

> ptrace_fork() is a wrapper that just calls arch_ptrace_fork(), which itself
> is an empty macro on most configurations.  I think we might as well just
> make ptrace_fork() an inline in linux/ptrace.h and put the test inside it.
> (Thus any future changes touch ptrace.h and not fork.c.)

ptrace_fork() will die soon, so the change if fork.c will go away. Markus
already has patches, hopefully they will be merged soon.

Oleg.

--- a/kernel/ptrace.c	2009-05-03 19:57:11.000000000 +0200
+++ b/kernel/ptrace.c	2009-05-03 21:24:36.000000000 +0200
@@ -79,8 +79,6 @@ static void ptrace_untrace(struct task_s
  */
 void __ptrace_unlink(struct task_struct *child)
 {
-	BUG_ON(!child->ptrace);
-
 	child->ptrace = 0;
 	child->parent = child->real_parent;
 	list_del_init(&child->ptrace_entry);
@@ -105,7 +103,7 @@ int ptrace_check_attach(struct task_stru
 	 * be changed by us so it's not changing right after this.
 	 */
 	read_lock(&tasklist_lock);
-	if ((child->ptrace & PT_PTRACED) && child->parent == current) {
+	if (task_ptrace(child) && child->parent == current) {
 		ret = 0;
 		/*
 		 * child->sighand can't be NULL, release_task()
@@ -203,7 +201,7 @@ int ptrace_attach(struct task_struct *ta
 	retval = -EPERM;
 	if (unlikely(task->exit_state))
 		goto unlock_tasklist;
-	if (task->ptrace)
+	if (task_ptrace(task))
 		goto unlock_tasklist;
 
 	task->ptrace = PT_PTRACED;
@@ -233,7 +231,7 @@ int ptrace_traceme(void)
 
 	write_lock_irq(&tasklist_lock);
 	/* Are we already being traced? */
-	if (!current->ptrace) {
+	if (!task_ptrace(current)) {
 		ret = security_ptrace_traceme(current->parent);
 		/*
 		 * Check PF_EXITING to ensure ->real_parent has not passed
@@ -314,7 +312,7 @@ int ptrace_detach(struct task_struct *ch
 	 * This child can be already killed. Make sure de_thread() or
 	 * our sub-thread doing do_wait() didn't do release_task() yet.
 	 */
-	if (child->ptrace) {
+	if (task_ptrace(child)) {
 		child->exit_code = data;
 		dead = __ptrace_detach(current, child);
 	}
@@ -401,30 +399,35 @@ int ptrace_writedata(struct task_struct 
 
 static int ptrace_setoptions(struct task_struct *child, long data)
 {
-	child->ptrace &= ~PT_TRACE_MASK;
+	unsigned int new_flags = 0;
+
+	if (data & ~PTRACE_O_MASK)
+		return -EINVAL;
 
 	if (data & PTRACE_O_TRACESYSGOOD)
-		child->ptrace |= PT_TRACESYSGOOD;
+		new_flags |= PT_TRACESYSGOOD;
 
 	if (data & PTRACE_O_TRACEFORK)
-		child->ptrace |= PT_TRACE_FORK;
+		new_flags |= PT_TRACE_FORK;
 
 	if (data & PTRACE_O_TRACEVFORK)
-		child->ptrace |= PT_TRACE_VFORK;
+		new_flags |= PT_TRACE_VFORK;
 
 	if (data & PTRACE_O_TRACECLONE)
-		child->ptrace |= PT_TRACE_CLONE;
+		new_flags |= PT_TRACE_CLONE;
 
 	if (data & PTRACE_O_TRACEEXEC)
-		child->ptrace |= PT_TRACE_EXEC;
+		new_flags |= PT_TRACE_EXEC;
 
 	if (data & PTRACE_O_TRACEVFORKDONE)
-		child->ptrace |= PT_TRACE_VFORK_DONE;
+		new_flags |= PT_TRACE_VFORK_DONE;
 
 	if (data & PTRACE_O_TRACEEXIT)
-		child->ptrace |= PT_TRACE_EXIT;
+		new_flags |= PT_TRACE_EXIT;
 
-	return (data & ~PTRACE_O_MASK) ? -EINVAL : 0;
+	child->ptrace &= ~PT_TRACE_MASK;
+	child->ptrace |= new_flags;
+	return 0;
 }
 
 static int ptrace_getsiginfo(struct task_struct *child, siginfo_t *info)

Re: [PATCH] ptrace: do not use task->ptrace directly in core kernel

[Roland McGrath]

> Yes this patch is only cosmetic. But it allows us to change the internals
> in ptrace.c without thinking about other kernel/*.c. The only goal is to
> make the further changes smaller and more reviewable.
> 
> Honestly, I'd also like to temporary change ptrace.c too, see the patch
> below. But this is minor.

All fine by me.


Thanks,
Roland