ptrace: make former thread ID available via PTRACE_GETEVENTMSG after PTRACE_EVENT_EXEC stop (v.2)

This patch allows tracer to figure out which of its potentially many tracees performed the execve.

Run-tested.
Below is the output of a test program which creates two additional threads, and one of them execs. PTRACE_O_TRACECLONE, PTRACE_O_TRACEEXIT and PTRACE_O_TRACEEXEC are in effect:

4857: thread leader
4857: status:0003057f WIFSTOPPED sig:5 (TRAP) event:CLONE eventdata:0x12fa (4858) 4858: status:0000137f WIFSTOPPED sig:19 (STOP) event:none eventdata:0x0 (0) 4857: status:0003057f WIFSTOPPED sig:5 (TRAP) event:CLONE eventdata:0x12fb (4859) 4859: status:0000137f WIFSTOPPED sig:19 (STOP) event:none eventdata:0x12fa (4858) 4858: status:0006057f WIFSTOPPED sig:5 (TRAP) event:EXIT eventdata:0x0 (0) 4857: status:0006057f WIFSTOPPED sig:5 (TRAP) event:EXIT eventdata:0x0 (0) 4858: status:00000000 WIFEXITED exitcode:0
4857: status:0004057f WIFSTOPPED sig:5 (TRAP) event:EXEC eventdata:0x12fb (4859)

index 6075a1e..edf9ed2 100644
@@ -1366,13 +1366,22 @@ int search_binary_handler(struct linux_binprm *bprm,struct pt_regs *regs)
 	for (try=0; try<2; try++) {
 		read_lock(&binfmt_lock);
 		list_for_each_entry(fmt, &formats, lh) {
-			int (*fn)(struct linux_binprm *, struct pt_regs *) = fmt->load_binary;
-			if (!fn)
+			int (*load_binary)(struct linux_binprm *, struct pt_regs *);
+			pid_t old_pid = old_pid; /* for compiler */
+
+			load_binary = fmt->load_binary;
+			if (!load_binary)
 				continue;
 			if (!try_module_get(fmt->module))
 				continue;
 			read_unlock(&binfmt_lock);
-			retval = fn(bprm, regs);
+			if (task_ptrace(current) & PT_PTRACED) {
+				/* Need to fetch pid before load_binary changes it */
+				rcu_read_lock();
+				old_pid = task_pid_nr_ns(current, task_active_pid_ns(current->parent));
+				rcu_read_unlock();
+			}
+			retval = load_binary(bprm, regs);
 			/*
 			 * Restore the depth counter to its starting value
 			 * in this call, so we don't have to rely on every

@@ -1381,7 +1390,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);
+					tracehook_report_exec(fmt, bprm, regs, old_pid);
 				put_binfmt(fmt);
 				allow_write_access(bprm->file);
 				if (bprm->file)

index e95f523..c87866d 100644
@@ -199,9 +199,10 @@ static inline struct task_struct *tracehook_tracer_task(struct task_struct *tsk)
  */
 static inline void tracehook_report_exec(struct linux_binfmt *fmt,
 					 struct linux_binprm *bprm,
-					 struct pt_regs *regs)
+					 struct pt_regs *regs,
+					 pid_t old_pid)
 {
-	if (!ptrace_event(PT_TRACE_EXEC, PTRACE_EVENT_EXEC, 0) &&
+	if (!ptrace_event(PT_TRACE_EXEC, PTRACE_EVENT_EXEC, old_pid) &&
 	    unlikely(task_ptrace(current) & PT_PTRACED))
 		send_sig(SIGTRAP, current, 0);
 }

Hello, Oleg, Denys.

Do we want to enable this silently? Wouldn't it be better to make it dependent on PT_SEIZED? Otherwise, it's something which simply might or might not be there.

tejun

Hmm. Not sure I understand. Why can't PTRACE_SEIZE add PT_TRACE_EXEC (and PT_TRACESYSGOOD) along with PT_SEIZED during attach?

I think this makes more sense, this way the tracer can disable this later via PTRACE_SETOPTIONS if it wants. Not that I think this is really useful but still. Otherwise we are going to silently disable PTRACE_O_TRACEEXEC, this may be confusing.

But. If we want the PT_TRACE_EXEC behaviour for PT_SEIZED task (personally I think we do), then we should probably record the old pid unconditionally, the tracer can attach later.

Even in this case ptrace_event(PTRACE_EVENT_EXEC) can race with detach + attach in theory, but I think in this case we do not care.

Oleg.

Hello, Oleg.

I'm worrying about !PT_SEIZED case. If we make it solely depend on PT_TRACE_EXEC, newer kernels report the old pid while olders ones don't and the only way to discover would be either comparing kernel version or actually trying it - both aren't too nice.

Thanks.

tejun

Ah, understood. So you think that the old pid should be only reported if PT_SEIZED.

May be... Denys, what do you think?

OTOH, it looks simpler if PT_TRACE_EXEC always reports the old pid, this can't break the applications which do not know about this new feature.

Fortunately, currently tracehook_report_exec() zeroes ->ptrace_message. At least this means that anything != 0 means it works.

Oleg.

Hello,

Yeah, but that's a pretty silly way to do it. If we make it depend on PT_SEIZED, we can simply say "if seized, EXEC reports..." but as it currently stands, it would go like "If the message is non-zero on EXEC, it indicates... This behavior is valid since kernel version x.x.x". Maybe adding a guarantee that PTRACE_SEIZE capable kernel always reports the old pid on EXEC but that would still seem unnecessarily complicated. It isn't a bug fix. I don't see much point in introducing new behavior separately.

Thanks.

tejun

This is true for any new addition to API.
It starts from some kernel version.

This new feature looks orthogonal to PTRACE_SEIZE to me.

vda

Hello,

Hmmm... but as I wrote above, we have a choice to make here and the two options are clearly different?

Yeah, sure, but we're bundling a number of behavior changes with PT_SEIZED because it's impractical to introduce each with its own compatibility / feature test bits, so it makes sense to do the same with this one, I think. It's not like there's a lot to gain by supporting this outside of SEIZE anyway.

Thanks.

tejun

I do not really understand your concerns. Yes, yes, yes, if we do not bind this feauture with PT_SEIZED, then the application has to take care _if_ it wants to use it without PT_SEIZED. So what? This is the problem of that application. This change can't break the application which do not want or do not know about this feature.

And how can we bind it to PT_SEIZED? We can't do something like

	old_pid = 0;
	if (PT_SEIZED) {
		old_pid = task_pid_nr_ns(...);
	}

	...

	ptrace_event(PTRACE_EVENT_EXEC, old_pid);

in search_binary_handler(), this is racy, the tracer can attach in the window and we want to avoid SIGTRAP.

So, we should pass the valid old_pid to ptrace_event() unconditionally (btw, Denys, I think we should do this anyway, but perhaps we can do this later) and then uglify ptrace_event() somehow.

Say,

	static inline void ptrace_event(int event, unsigned long message)
	{

+ if (event == PTRACE_EVENT_EXEC && !PT_SEIZED)
+ message = 0;

		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);
		}
	}

looks a bit ugly. Or, perhaps,

	static inline void ptrace_event(int event, unsigned long message)
	{
		bool enabled = ptrace_event_enabled(current, event);

		if (event == PTRACE_EVENT_EXEC) {
			if (PT_SEIZED) {
				enabled = true;
			} else {
				if (!enabled) {
					send_sig(SIGTRAP, current, 0);
					return;
				}
				message = 0;
			}
		}

		if (enabled) {
			current->ptrace_message = message;
			ptrace_notify((event << 8) | SIGTRAP);
		}
	}

a bit better, bit still not very nice. For what? I tend to agree with Denys, and I think we should listen to the user-space developer who actually uses ptrace.

That said, I leave it to you and Denys, personally I am fine either way.

FYI, I need to disappear till Thursday.

Oleg.

Hello,

It's not really a technical concern, more about documentation simplicity. It's just simpler to be able to say "blah blah changes when seized".

It's minor either way. Let's go with the current patch then.

Thanks.

tejun

I just sent rediffed version against this branch as a separate mail.

I think I addressed all your concerns in it.

vda