connector: add an event for monitoring process tracers

This change adds a procfs connector event, which is emitted on every successful process tracer attach or detach.

If some process connects to other one, kernelspace connector reports process id and thread group id of both these involved processes. On disconnection null process id is returned.

Such an event allows to create a simple automated userspace mechanism to be aware about processes connecting to others, therefore predefined process policies can be applied to them if needed.

Note, a detach signal is emitted only in case, if a tracer process explicitly executes PTRACE_DETACH request. In other cases like tracee or tracer exit detach event from proc connector is not reported.

---

Originally proposed change can be found here:
* http://patchwork.ozlabs.org/patch/104434/

This version of the change extends proc_ptrace_connector() argument list, so it becomes possible to specify a ptrace request eliminating process attach/detach race.

Also tracehook_tracer_task() function was renamed to ptrace_parent(), but as far as proc_ptrace_connector(task, PTRACE_ATTACH) is called from a tracer process itself, it becomes possible to get rid of that call usage completely.

Oleg, I've rebased the change, and if you don't have objections, I'd be glad, if you can apply this change upon your ptrace branch, thank you in advance.

 drivers/connector/cn_proc.c |   35 +++++++++++++++++++++++++++++++++++
 include/linux/cn_proc.h     |   13 +++++++++++++
 kernel/ptrace.c             |    7 ++++++-
 3 files changed, 54 insertions(+), 1 deletions(-)

index 2b46a7e..281902d 100644
@@ -28,6 +28,7 @@
 #include <linux/init.h>
 #include <linux/connector.h>
 #include <linux/gfp.h>
+#include <linux/ptrace.h>
 #include <asm/atomic.h>
 #include <asm/unaligned.h>
 

@@ -166,6 +167,40 @@ void proc_sid_connector(struct task_struct *task)
 	cn_netlink_send(msg, CN_IDX_PROC, GFP_KERNEL);
 }
 
+void proc_ptrace_connector(struct task_struct *task, int ptrace_id)
+{
+	struct cn_msg *msg;
+	struct proc_event *ev;
+	struct timespec ts;
+	__u8 buffer[CN_PROC_MSG_SIZE];
+	struct task_struct *tracer;
+
+	if (atomic_read(&proc_event_num_listeners) < 1)
+		return;
+
+	msg = (struct cn_msg *)buffer;
+	ev = (struct proc_event *)msg->data;
+	get_seq(&msg->seq, &ev->cpu);
+	ktime_get_ts(&ts); /* get high res monotonic timestamp */
+	put_unaligned(timespec_to_ns(&ts), (__u64 *)&ev->timestamp_ns);
+	ev->what = PROC_EVENT_PTRACE;
+	ev->event_data.ptrace.process_pid  = task->pid;
+	ev->event_data.ptrace.process_tgid = task->tgid;
+	if (ptrace_id == PTRACE_ATTACH) {
+		ev->event_data.ptrace.tracer_pid  = current->pid;
+		ev->event_data.ptrace.tracer_tgid = current->tgid;
+	} else if (ptrace_id == PTRACE_DETACH) {
+		ev->event_data.ptrace.tracer_pid  = 0;
+		ev->event_data.ptrace.tracer_tgid = 0;
+	} else
+		return;
+
+	memcpy(&msg->id, &cn_proc_event_id, sizeof(msg->id));
+	msg->ack = 0; /* not used */
+	msg->len = sizeof(*ev);
+	cn_netlink_send(msg, CN_IDX_PROC, GFP_KERNEL);
+}
+
 void proc_exit_connector(struct task_struct *task)
 {
 	struct cn_msg *msg;

index 47dac5e..12c517b 100644
@@ -53,6 +53,7 @@ struct proc_event {
 		PROC_EVENT_UID  = 0x00000004,
 		PROC_EVENT_GID  = 0x00000040,
 		PROC_EVENT_SID  = 0x00000080,
+		PROC_EVENT_PTRACE = 0x00000100,
 		/* "next" should be 0x00000400 */
 		/* "last" is the last process event: exit */
 		PROC_EVENT_EXIT = 0x80000000

@@ -95,6 +96,13 @@ struct proc_event {
 			__kernel_pid_t process_tgid;
 		} sid;
 
+		struct ptrace_proc_event {
+			__kernel_pid_t process_pid;
+			__kernel_pid_t process_tgid;
+			__kernel_pid_t tracer_pid;
+			__kernel_pid_t tracer_tgid;
+		} ptrace;
+
 		struct exit_proc_event {
 			__kernel_pid_t process_pid;
 			__kernel_pid_t process_tgid;

@@ -109,6 +117,7 @@ void proc_fork_connector(struct task_struct *task);
 void proc_exec_connector(struct task_struct *task);
 void proc_id_connector(struct task_struct *task, int which_id);
 void proc_sid_connector(struct task_struct *task);
+void proc_ptrace_connector(struct task_struct *task, int which_id);
 void proc_exit_connector(struct task_struct *task);
 #else
 static inline void proc_fork_connector(struct task_struct *task)

@@ -124,6 +133,10 @@ static inline void proc_id_connector(struct task_struct *task,
 static inline void proc_sid_connector(struct task_struct *task)
 {}
 
+static inline void proc_ptrace_connector(struct task_struct *task,
+					 int ptrace_id)
+{}
+
 static inline void proc_exit_connector(struct task_struct *task)
 {}
 #endif	/* CONFIG_PROC_EVENTS */

index d7ccc79..9de3ecf 100644
@@ -23,6 +23,7 @@
 #include <linux/uaccess.h>
 #include <linux/regset.h>
 #include <linux/hw_breakpoint.h>
+#include <linux/cn_proc.h>
 
 
 static int ptrace_trapping_sleep_fn(void *flags)

@@ -305,9 +306,12 @@ unlock_tasklist:
 unlock_creds:
 	mutex_unlock(&task->signal->cred_guard_mutex);
 out:
-	if (!retval)
+	if (!retval) {
 		wait_on_bit(&task->jobctl, JOBCTL_TRAPPING_BIT,
 			    ptrace_trapping_sleep_fn, TASK_UNINTERRUPTIBLE);
+		proc_ptrace_connector(task, PTRACE_ATTACH);
+	}
+
 	return retval;
 }
 

@@ -415,6 +419,7 @@ static int ptrace_detach(struct task_struct *child, unsigned int data)
 	}
 	write_unlock_irq(&tasklist_lock);
 
+	proc_ptrace_connector(child, PTRACE_DETACH);
 	if (unlikely(dead))
 		release_task(child);
 

1.7.5.1

Hi.

I still ack this one :)

	Evgeniy Polyakov

OK, thanks, I am going to apply it then...

While we are here, a couple of questions. I've looked at connector briefly, and some things do not look exactly right to me.

proc_fork_connector() reads task->real_parent lockless. In theory this is not safe with CLONE_PTHREAD or CLONE_PARENT. Yes, this is only theoretical, but afaics we need something like

	--- x/drivers/connector/cn_proc.c
	+++ x/drivers/connector/cn_proc.c
	@@ -55,6 +55,7 @@ void proc_fork_connector(struct task_str
		struct proc_event *ev;
		__u8 buffer[CN_PROC_MSG_SIZE];
		struct timespec ts;
	+	struct task_struct *parent;
	 
		if (atomic_read(&proc_event_num_listeners) < 1)
			return;
	@@ -65,8 +66,11 @@ void proc_fork_connector(struct task_str
		ktime_get_ts(&ts); /* get high res monotonic timestamp */
		put_unaligned(timespec_to_ns(&ts), (__u64 *)&ev->timestamp_ns);
		ev->what = PROC_EVENT_FORK;
	-	ev->event_data.fork.parent_pid = task->real_parent->pid;
	-	ev->event_data.fork.parent_tgid = task->real_parent->tgid;
	+	rcu_read_lock();
	+	parent = rcu_dereference(task->real_parent);
	+	ev->event_data.fork.parent_pid = parent->pid;
	+	ev->event_data.fork.parent_tgid = parent->tgid;
	+	rcu_read_unlock();
		ev->event_data.fork.child_pid = task->pid;
		ev->event_data.fork.child_tgid = task->tgid;

Otherwise ->real_parent can point to the freed/reused and may be unmapped memory.

But the actual question is, the usage of proc_exec_connector() looks "obviously wrong", no? Don't we need

	--- x/fs/exec.c
	+++ x/fs/exec.c
	@@ -1380,15 +1380,16 @@ int search_binary_handler(struct linux_b
				 */
				bprm->recursion_depth = depth;
				if (retval >= 0) {
	-				if (depth == 0)
	+				if (depth == 0) {
						tracehook_report_exec(fmt, bprm, regs);
	+					proc_exec_connector(current);
	+				}
					put_binfmt(fmt);
					allow_write_access(bprm->file);
					if (bprm->file)
						fput(bprm->file);
					bprm->file = NULL;
					current->did_exec = 1;
	-				proc_exec_connector(current);
					return retval;
				}
				read_lock(&binfmt_lock);

? Or do we really want to call proc_exec_connector() twice or more in "#!whatever" case?

Oleg.

Looks like, nobody cares ;) I am sending the patch.

I think this should be fixed too, I'll send the patch later.

Oleg.

proc_fork_connector() uses ->real_parent lockless. This is not safe if copy_process() was called with CLONE_THREAD or CLONE_PARENT, in this case the parent != current can go away at any moment.

---

 drivers/connector/cn_proc.c |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

@@ -55,6 +55,7 @@ void proc_fork_connector(struct task_str
 	struct proc_event *ev;
 	__u8 buffer[CN_PROC_MSG_SIZE];
 	struct timespec ts;
+	struct task_struct *parent;
 
 	if (atomic_read(&proc_event_num_listeners) < 1)
 		return;

@@ -65,8 +66,11 @@ void proc_fork_connector(struct task_str
 	ktime_get_ts(&ts); /* get high res monotonic timestamp */
 	put_unaligned(timespec_to_ns(&ts), (__u64 *)&ev->timestamp_ns);
 	ev->what = PROC_EVENT_FORK;
-	ev->event_data.fork.parent_pid = task->real_parent->pid;
-	ev->event_data.fork.parent_tgid = task->real_parent->tgid;
+	rcu_read_lock();
+	parent = rcu_dereference(task->real_parent);
+	ev->event_data.fork.parent_pid = parent->pid;
+	ev->event_data.fork.parent_tgid = parent->tgid;
+	rcu_read_unlock();
 	ev->event_data.fork.child_pid = task->pid;
 	ev->event_data.fork.child_tgid = task->tgid;
 

I'd wish I could understand this ;) IOW, I still do not understand why this is useful, but this doesn't matter. Since Evgeniy acked this patch, I'll apply it to ptrace tree.

Can't resist, a couple of very minor/cosmetics nits. Just because I am blighter ;)

"which_id" doesn't match "ptrace_id" used elsewhere. And PTRACE_ATTACH instead of simple boolean looks as if you are going to add more ptrace events, but I guess this won't happen.

Originally the idea comes from a desire to classify development tools like strace or gdb in the same way as a tracee process, for instance put them to a tracee's current cgroup partition, which might be done automatically in user-space by some process "track and manage" daemon.

I'd like to preserve that variant, in my opinion its just a bit more undisguised version rather than bare true/false.

Yes, there is a difference. But as far as there is no guaranteed serialization in proc connector event reports, user-space process trackers should be designed to operate correctly having in mind possible event reordering.

Thank you a lot, especially for comments, but I think this v2 is good enough to be applied :) And if there are more users of that proc connector extension in future, it might be needed to cut some sharp corners later, and may be even add implicit detach reporting :)

--
With best wishes,
Vladimir

OK. Although this "else return" in proc_ptrace_connector() looks like the "hide the potentional error" to me.

Yes, but I didn't really mean the correctness. I meant, this looks confusing, as if this wait_on_bit() has something to do with attach. Likewise I do not understand why proc_exec_connector() is called after ptrace_event() which can sleep unpredictably long.

Nevermind, applied.

Oleg.