[RFC PATCH 11/12 v2] ptrace: mv task_struct->ptrace_message ptrace_ctx->message

[RFC PATCH 11/12 v2] ptrace: mv task_struct->ptrace_message ptrace_ctx->message

[Oleg Nesterov]

Move task_struct->ptrace_message into ptrace_ctx->message.

ptrace_event() can use current->ptrace_ctx safely. If mask == 0 and we do
not check task_ptrace() we are called from tracehook_report_clone_complete(),
in this case trace == T means current was traced when it called do_fork().

 include/linux/sched.h  |    1 -
 include/linux/ptrace.h |    3 ++-
 kernel/ptrace.c        |    6 ++++--
 3 files changed, 6 insertions(+), 4 deletions(-)

--- PTRACE/include/linux/sched.h~10_MESSAGE	2009-05-28 10:45:59.000000000 +0200
+++ PTRACE/include/linux/sched.h	2009-05-28 12:03:26.000000000 +0200
@@ -1347,7 +1347,6 @@ struct task_struct {
 
 	struct io_context *io_context;
 
-	unsigned long ptrace_message;
 	struct task_io_accounting ioac;
 #if defined(CONFIG_TASK_XACCT)
 	u64 acct_rss_mem1;	/* accumulated rss usage */
--- PTRACE/include/linux/ptrace.h~10_MESSAGE	2009-05-28 11:59:06.000000000 +0200
+++ PTRACE/include/linux/ptrace.h	2009-05-28 12:09:30.000000000 +0200
@@ -79,6 +79,7 @@ struct ptrace_context {
 	unsigned long		flags;
 	struct task_struct	*tracer;
 	struct siginfo		*infop;
+	unsigned long		message;
 };
 
 extern int alloc_ptrace_context(struct task_struct *child);
@@ -159,7 +160,7 @@ static inline int ptrace_event(int mask,
 {
 	if (mask && likely(!(task_ptrace(current) & mask)))
 		return 0;
-	current->ptrace_message = message;
+	current->ptrace_ctx->message = message;
 	ptrace_notify((event << 8) | SIGTRAP);
 	return 1;
 }
--- PTRACE/kernel/ptrace.c~10_MESSAGE	2009-05-28 11:29:45.000000000 +0200
+++ PTRACE/kernel/ptrace.c	2009-05-28 12:11:20.000000000 +0200
@@ -568,7 +568,8 @@ int ptrace_request(struct task_struct *c
 		ret = ptrace_setoptions(child->ptrace_ctx, data);
 		break;
 	case PTRACE_GETEVENTMSG:
-		ret = put_user(child->ptrace_message, (unsigned long __user *) data);
+		ret = put_user(child->ptrace_ctx->message,
+				(unsigned long __user *) data);
 		break;
 
 	case PTRACE_GETSIGINFO:
@@ -728,7 +729,8 @@ int compat_ptrace_request(struct task_st
 		break;
 
 	case PTRACE_GETEVENTMSG:
-		ret = put_user((compat_ulong_t) child->ptrace_message, datap);
+		ret = put_user((compat_ulong_t) child->ptrace_ctx->message,
+				datap);
 		break;
 
 	case PTRACE_GETSIGINFO:

Re: [RFC PATCH 11/12 v2] ptrace: mv task_struct->ptrace_message ptrace_ctx->message

[Oleg Nesterov]

On 05/28, Oleg Nesterov wrote:
>
> ptrace_event() can use current->ptrace_ctx safely. If mask == 0 and we do
> not check task_ptrace() we are called from tracehook_report_clone_complete(),
> in this case trace == T means current was traced when it called do_fork().

Perhaps it is better to optimize out "mask != 0" check in ptrace_event().
This special !mask case is only needed for _report_clone(), we can use
mask == PT_PTRACED instead.

Oleg.

--- PTRACE/include/linux/ptrace.h~10_HOOK_COMPLETE	2009-05-28 11:19:33.000000000 +0200
+++ PTRACE/include/linux/ptrace.h	2009-05-28 11:49:29.000000000 +0200
@@ -157,7 +157,7 @@ int generic_ptrace_pokedata(struct task_
  */
 static inline int ptrace_event(int mask, int event, unsigned long message)
 {
-	if (mask && likely(!(task_ptrace(current) & mask)))
+	if (likely(!(task_ptrace(current) & mask)))
 		return 0;
 	current->ptrace_message = message;
 	ptrace_notify((event << 8) | SIGTRAP);
--- PTRACE/include/linux/tracehook.h~10_HOOK_COMPLETE	2009-05-28 08:41:11.000000000 +0200
+++ PTRACE/include/linux/tracehook.h	2009-05-28 11:50:14.000000000 +0200
@@ -344,7 +344,7 @@ static inline void tracehook_report_clon
 						   struct task_struct *child)
 {
 	if (unlikely(trace))
-		ptrace_event(0, trace, pid);
+		ptrace_event(PT_PTRACED, trace, pid);
 }
 
 /**

Re: [RFC PATCH 11/12 v2] ptrace: mv task_struct->ptrace_message ptrace_ctx->message

[Roland McGrath]

> Perhaps it is better to optimize out "mask != 0" check in ptrace_event().
> This special !mask case is only needed for _report_clone(), we can use
> mask == PT_PTRACED instead.

Sure, that is fine.  But note it should always have been compiled away
before anyway, since it's inline and constant in every caller.


Thanks,
Roland

Re: [RFC PATCH 11/12 v2] ptrace: mv task_struct->ptrace_message ptrace_ctx->message

[Oleg Nesterov]

On 05/28, Roland McGrath wrote:
>
> > Perhaps it is better to optimize out "mask != 0" check in ptrace_event().
> > This special !mask case is only needed for _report_clone(), we can use
> > mask == PT_PTRACED instead.
>
> Sure, that is fine.  But note it should always have been compiled away
> before anyway, since it's inline and constant in every caller.

Ah, I missed that.

I wonder if can kill "int trace" in do_fork/copy_process. Looks like we
can...

Oleg.

PATCH? tracehook_report_clone: fix false positives

[Oleg Nesterov]

On 05/29, Oleg Nesterov wrote:
>
> I wonder if can kill "int trace" in do_fork/copy_process. Looks like we
> can...

Hmm. In fact, I think "int trace" is not always right.

tracehook_report_clone:

	if (unlikely(trace) || unlikely(clone_flags & CLONE_PTRACE)) {
		/*
		 * The child starts up with an immediate SIGSTOP.
		 */
		sigaddset(&child->pending.signal, SIGSTOP);
		set_tsk_thread_flag(child, TIF_SIGPENDING);
	}

Firtsly, I don't understand CLONE_PTRACE check. Suppose that untraced
task does clone(CLONE_PTRACE). In that case we create the untraced
child (this is correct) but still we send SIGSTOP.

I do not really know if this bug or not, but this doesn't look right.
At least this should be commented, imho. And, looking at 2.6.26, I think
the behaviour was different before tracehooks.

So, I assume this is bug for now.

But even the "trace != 0" check is not right, it can be false positive.
Suppose that PT_TRACE_FORK task P forks, tracehook_finish_clone() does
ptrace_link(). Then, the tracer exits and untraces both P and its new
child. In that case, I don't think it is right to send SIGSTOP.

Afaics, we can fix these problems (and kill "trace" arg):

	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)) {
			/*
			 * The child starts up with an immediate SIGSTOP.
			 */
			sigaddset(&child->pending.signal, SIGSTOP);
			set_tsk_thread_flag(child, TIF_SIGPENDING);
		}
	}

Is task_ptrace() check racy? Yes. But a) this race is harmless and
b) the current code is equally racy.

We have multiple scenarious, but for example, suppose that untraced task
does clone(CLONE_PTRACE), do_fork() returns the new untraced child. This
child C is already visible to user-space, so it is possible that another
tracer has already attached to C, or ptrace_attach() is in progress.

However, afaics in this case we are fine. We do not care if we race with
another tracer. Because this tracer will send or has alredy sent SIGSTOP
anyway. And given that wake_up_new_task() was not called yet, we should
not worry about untrace. The child can be untraced (if tracer exits) in
parallel, but the pending SIGSTOP won't go away in any case.


So, I am going to send the patch below. But this leads to another question:
should not we move these sigaddset() + set_tsk_thread_flag() into
ptrace_init_task() ?

I don't thinks this makes sense for 2.6.30, we need another check. But
with ->ptrace_ctx patches we can do

	static inline void ptrace_init_task(struct task_struct *child)
	{
		INIT_LIST_HEAD(&child->ptraced);

		if (unlikely(child->ptrace_ctx) && ptrace_tracer(current)) {
			ptrace_link(child, task_ptrace(current),
					ptrace_tracer(current));
			sigaddset(&child->pending.signal, SIGSTOP);
			set_tsk_thread_flag(child, TIF_SIGPENDING);
		}
	}

This way we optimize the fork() path a little bit, and it becomes more
clear. Yes, utrace-ptrace will likely change this code further anyway
and move the code from _init() to _report_clone() back, but in this case
I guess the whole tracehook_finish_clone() will go away, so this change
looks right anyway to me.

Oleg.

--- a/include/linux/tracehook.h
+++ b/include/linux/tracehook.h
@@ -259,14 +259,12 @@ static inline void tracehook_finish_clon
 
 /**
  * tracehook_report_clone - in parent, new child is about to start running
- * @trace:		return value from tracehook_prepare_clone()
  * @regs:		parent's user register state
  * @clone_flags:	flags from parent's system call
  * @pid:		new child's PID in the parent's namespace
  * @child:		new child task
  *
- * Called after a child is set up, but before it has been started
- * running.  @trace is the value returned by tracehook_prepare_clone().
+ * Called after a child is set up, but before it has been started running.
  * This is not a good place to block, because the child has not started
  * yet.  Suspend the child here if desired, and then block in
  * tracehook_report_clone_complete().  This must prevent the child from
@@ -276,13 +274,14 @@ static inline void tracehook_finish_clon
  *
  * Called with no locks held, but the child cannot run until this returns.
  */
-static inline void tracehook_report_clone(int trace, struct pt_regs *regs,
+static inline void tracehook_report_clone(struct pt_regs *regs,
 					  unsigned long clone_flags,
 					  pid_t pid, struct task_struct *child)
 {
-	if (unlikely(trace) || unlikely(clone_flags & CLONE_PTRACE)) {
+	if (unlikely(task_ptrace(child))) {
 		/*
-		 * The child starts up with an immediate SIGSTOP.
+		 * It doesn't matter who attached/attaching to this
+		 * task, the pending SIGSTOP is right in any case.
 		 */
 		sigaddset(&child->pending.signal, SIGSTOP);
 		set_tsk_thread_flag(child, TIF_SIGPENDING);
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1409,7 +1409,7 @@ long do_fork(unsigned long clone_flags,
 		}
 
 		audit_finish_fork(p);
-		tracehook_report_clone(trace, regs, clone_flags, nr, p);
+		tracehook_report_clone(regs, clone_flags, nr, p);
 
 		/*
 		 * We set PF_STARTING at creation in case tracing wants to

Re: PATCH? tracehook_report_clone: fix false positives

[Roland McGrath]

> Firtsly, I don't understand CLONE_PTRACE check. Suppose that untraced
> task does clone(CLONE_PTRACE). In that case we create the untraced
> child (this is correct) but still we send SIGSTOP.
> 
> I do not really know if this bug or not, but this doesn't look right.
> At least this should be commented, imho. And, looking at 2.6.26, I think
> the behaviour was different before tracehooks.
> 
> So, I assume this is bug for now.

You're right.  CLONE_PTRACE when not traced will misbehave (not that anyone
ever uses it).  The old code just checked child->ptrace, and that is fine
to do again now.  I probably changed that thinking it had a race--which it
does--with asynchronous PTRACE_ATTACH after an untraced fork.  But that is
a harmless race as you explained.

ACK on the 2.6.30 patch attached.

> So, I am going to send the patch below. But this leads to another question:
> should not we move these sigaddset() + set_tsk_thread_flag() into
> ptrace_init_task() ?

It might make sense to consolidate them.  But note that ptrace_attach()
uses send_sig_info().  With SEND_SIG_FORCED, this does almost nothing more
than sigaddset() (i.e. no queue entry).  But it does do prepare_signal(),
which will clear any pending SIGCONTs.  It's possible that something in
userland manages to rely on that behavior for the asynchronous attach case
(unrelated to startup-time races).  It wouldn't hurt for the creation-time
case to use send_sig_info() too, though it would go through a bunch more
code to do nothing effectual but sigaddset() in the end.


Thanks,
Roland

Re: PATCH? tracehook_report_clone: fix false positives

[Oleg Nesterov]

On 05/31, Roland McGrath wrote:
>
> ACK on the 2.6.30 patch attached.

Thanks, I am adding your Acked-by to tha patch.

> > So, I am going to send the patch below. But this leads to another question:
> > should not we move these sigaddset() + set_tsk_thread_flag() into
> > ptrace_init_task() ?
>
> It might make sense to consolidate them.  But note that ptrace_attach()
> uses send_sig_info().  With SEND_SIG_FORCED, this does almost nothing more
> than sigaddset() (i.e. no queue entry).  But it does do prepare_signal(),
> which will clear any pending SIGCONTs.  It's possible that something in
> userland manages to rely on that behavior for the asynchronous attach case
> (unrelated to startup-time races).  It wouldn't hurt for the creation-time
> case to use send_sig_info() too, though it would go through a bunch more
> code to do nothing effectual but sigaddset() in the end.

Oh, I never thought about attach && SIGCONT interaction...

But, tracehook_report_clone() has the same problems?

And if we move sigaddset to ptrace_task_init(), we should not worry about
SIGCONT? Without CLONE_THREAD the new task is not visible to user-space yet.
Even if we clone a sub-thread, ptrace_init_task() runs under ->siglock.
If SIGCONT is already pending, copy_process() won't succeed.

Or do you mean something else?

Oleg.

Re: PATCH? tracehook_report_clone: fix false positives

[Roland McGrath]

> Oh, I never thought about attach && SIGCONT interaction...
> 
> But, tracehook_report_clone() has the same problems?

I don't follow.

> And if we move sigaddset to ptrace_task_init(), we should not worry about
> SIGCONT? Without CLONE_THREAD the new task is not visible to user-space yet.
> Even if we clone a sub-thread, ptrace_init_task() runs under ->siglock.
> If SIGCONT is already pending, copy_process() won't succeed.

It could be pending and blocked.

> Or do you mean something else?

Sorry, I don't think I understood what your question was.
I just pointed out that the element of PTRACE_ATTACH semantics
that would be changed unintentionally if you just replaced its
send_sig_info() call with ptrace_init_task() using sigaddset().


Thanks,
Roland

Re: PATCH? tracehook_report_clone: fix false positives

[Oleg Nesterov]

On 06/01, Roland McGrath wrote:
>
> > Oh, I never thought about attach && SIGCONT interaction...
> >
> > But, tracehook_report_clone() has the same problems?
>
> I don't follow.
>
> > And if we move sigaddset to ptrace_task_init(), we should not worry about
> > SIGCONT? Without CLONE_THREAD the new task is not visible to user-space yet.
> > Even if we clone a sub-thread, ptrace_init_task() runs under ->siglock.
> > If SIGCONT is already pending, copy_process() won't succeed.
>
> It could be pending and blocked.

Yes, I missed that, thanks.

> > Or do you mean something else?
>
> Sorry, I don't think I understood what your question was.
> I just pointed out that the element of PTRACE_ATTACH semantics
> that would be changed unintentionally if you just replaced its
> send_sig_info() call with ptrace_init_task() using sigaddset().

I suspect you misread my previous question.

I didn't mean PTRACE_ATTACH should use ptrace_init_task). I just meant that
perhaps it makes sense to move sigaddset() from tracehook_finish_clone()
to tracehook_finish_clone()->ptrace_init_task().

As you correctly pointed out, this sigaddset() is not the same as
send_sig_info(), but the same is true for tracehook_finish_clone() too.

Oleg.

Re: PATCH? tracehook_report_clone: fix false positives

[Roland McGrath]

> I suspect you misread my previous question.

Apparently so.

> I didn't mean PTRACE_ATTACH should use ptrace_init_task). I just meant that
> perhaps it makes sense to move sigaddset() from tracehook_finish_clone()
> to tracehook_finish_clone()->ptrace_init_task().

You mean from tracehook_report_clone to ptrace_init_task.  Perhaps.
tracehook_finish_clone->ptrace_init_task is inside write_lock_irq,
so it should really be kept to the minimum of what has to be inside there.

But the real reason is just that tracehook_report_clone() is called at the
place in do_fork() where the ptrace SIGSTOP code was originally before the
introduction of tracehook.h.  

This is where the utrace attachment point has to be (i.e. outside all the
locking).  So I don't see any benefit to changing the ptrace status quo now
for its own sake.


Thanks,
Roland

Re: PATCH? tracehook_report_clone: fix false positives

[Oleg Nesterov]

On 06/01, Roland McGrath wrote:
>
> This is where the utrace attachment point has to be (i.e. outside all the
> locking).

Yes I see. Quoting myself:

	Yes, utrace-ptrace will likely change this code further anyway
	and move the code from _init() to _report_clone() back, but in this case
	I guess the whole tracehook_finish_clone() will go away, so this change
	looks right anyway to me.

> So I don't see any benefit to changing the ptrace status quo now
> for its own sake.

OK, agreed.

Oleg.