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

[Oleg Nesterov <oleg@redhat.com>]

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)