[PATCH] connector: add an event for monitoring process tracers

[PATCH] connector: add an event for monitoring process tracers

[Vladimir Zapolskiy]

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 not emitted, if a tracer process terminates
without explicit PTRACE_DETACH request. Such cases can be covered
listening to PROC_EVENT_EXIT connector events.

Signed-off-by: Vladimir Zapolskiy <vzapolskiy@gmail.com>
---
 drivers/connector/cn_proc.c |   38 ++++++++++++++++++++++++++++++++++++++
 include/linux/cn_proc.h     |   12 ++++++++++++
 kernel/ptrace.c             |    6 ++++++
 3 files changed, 56 insertions(+), 0 deletions(-)

diff --git a/drivers/connector/cn_proc.c b/drivers/connector/cn_proc.c
index 2b46a7e..44f9881 100644
--- a/drivers/connector/cn_proc.c
+++ b/drivers/connector/cn_proc.c
@@ -28,6 +28,7 @@
 #include <linux/init.h>
 #include <linux/connector.h>
 #include <linux/gfp.h>
+#include <linux/tracehook.h>
 #include <asm/atomic.h>
 #include <asm/unaligned.h>
 
@@ -166,6 +167,43 @@ 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)
+{
+	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;
+
+	rcu_read_lock();
+	tracer = tracehook_tracer_task(task);
+	if (tracer) {
+		ev->event_data.ptrace.tracer_pid  = tracer->pid;
+		ev->event_data.ptrace.tracer_tgid = tracer->tgid;
+	} else {
+		ev->event_data.ptrace.tracer_pid  = 0;
+		ev->event_data.ptrace.tracer_tgid = 0;
+	}
+	rcu_read_unlock();
+
+	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;
diff --git a/include/linux/cn_proc.h b/include/linux/cn_proc.h
index 47dac5e..03013ad 100644
--- a/include/linux/cn_proc.h
+++ b/include/linux/cn_proc.h
@@ -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);
 void proc_exit_connector(struct task_struct *task);
 #else
 static inline void proc_fork_connector(struct task_struct *task)
@@ -124,6 +133,9 @@ 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)
+{}
+
 static inline void proc_exit_connector(struct task_struct *task)
 {}
 #endif	/* CONFIG_PROC_EVENTS */
diff --git a/kernel/ptrace.c b/kernel/ptrace.c
index 2df1157..9238b5d 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -23,6 +23,7 @@
 #include <linux/uaccess.h>
 #include <linux/regset.h>
 #include <linux/hw_breakpoint.h>
+#include <linux/cn_proc.h>
 
 
 /*
@@ -260,6 +261,9 @@ out:
 	if (wait_trap)
 		wait_event(current->signal->wait_chldexit,
 			   !(task->group_stop & GROUP_STOP_TRAPPING));
+	if (!retval)
+		proc_ptrace_connector(task);
+
 	return retval;
 }
 
@@ -365,6 +369,8 @@ static int ptrace_detach(struct task_struct *child, unsigned int data)
 	}
 	write_unlock_irq(&tasklist_lock);
 
+	proc_ptrace_connector(child);
+
 	if (unlikely(dead))
 		release_task(child);
 
-- 
1.7.5.1

Re: [PATCH] connector: add an event for monitoring process tracers

[Evgeniy Polyakov]

Hi Vladimir

On Tue, Jul 12, 2011 at 11:32:37PM +0300, Vladimir Zapolskiy (vzapolskiy@gmail.com) wrote:
> 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.

Looks good to me, thank you.
Acked-by: Evgeniy Polyakov <zbr@ioremap.net>

-- 
	Evgeniy Polyakov

Re: [PATCH] connector: add an event for monitoring process tracers

[David Miller]

From: Evgeniy Polyakov <zbr@ioremap.net>
Date: Wed, 13 Jul 2011 16:48:32 +0400

> On Tue, Jul 12, 2011 at 11:32:37PM +0300, Vladimir Zapolskiy (vzapolskiy@gmail.com) wrote:
>> 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.
> 
> Looks good to me, thank you.
> Acked-by: Evgeniy Polyakov <zbr@ioremap.net>

Since this isn't really "networking" it would be really nice if this
was taken in via some other tree, you can add my ack:

Acked-by: David S. Miller <davem@davemloft.net>

Re: [PATCH] connector: add an event for monitoring process tracers

[Oleg Nesterov]

On 07/12, Vladimir Zapolskiy wrote:
>
> Note, a detach signal is not emitted, if a tracer process terminates
> without explicit PTRACE_DETACH request. Such cases can be covered
> listening to PROC_EVENT_EXIT connector events.

Hmm. More and more reasons to make the implicit detach sleepable...

But. There is another case. The (dead) tracee can be detached via
do_wait(). Perhaps this falls into "covered listening to EXIT" too,
but imho makes sense to document in the changelog. Oh, and probably
we will add the ability to detach a zombie...

I don't really understand why do you need this, but I won't argue.

As for the patch,

> +void proc_ptrace_connector(struct task_struct *task)
> +{
> +	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;
> +
> +	rcu_read_lock();
> +	tracer = tracehook_tracer_task(task);
> +	if (tracer) {
> +		ev->event_data.ptrace.tracer_pid  = tracer->pid;
> +		ev->event_data.ptrace.tracer_tgid = tracer->tgid;
> +	} else {
> +		ev->event_data.ptrace.tracer_pid  = 0;
> +		ev->event_data.ptrace.tracer_tgid = 0;
> +	}

This doesn't look right. The code uses tracehook_tracer_task() to
figure out whether this task traced or not. But this is racy.

ptrace_attach:

	...attach...

	/* WINDOW */

	proc_ptrace_connector(task);

The task can exit in between, and the caller's subthread can do
wait4() and release it. In this case proc_ptrace_connector() will
see tracehook_tracer_task() == NULL and report "detach".

The similar race in ptrace_detach() path. Another tracer can attach
to this task before we proc_ptrace_connector().

I think proc_ptrace_connector() needs the explicit "task_struct *tracer"
argument, NULL if ptrace_detach(). Or a simple boolean, the tracer is
current.

If you think this is fine - I won't argue.



But in any case, please rediff against

	git://git.kernel.org/pub/scm/linux/kernel/git/oleg/misc.git ptrace

tracehook_tracer_task() was removed, and

> @@ -260,6 +261,9 @@ out:
>  	if (wait_trap)
>  		wait_event(current->signal->wait_chldexit,
>  			   !(task->group_stop & GROUP_STOP_TRAPPING));
> +	if (!retval)
> +		proc_ptrace_connector(task);
> +
>  	return retval;
>  }

this chunk probably should be updated.

Oleg.

Re: [PATCH] connector: add an event for monitoring process tracers

[Vladimir Zapolskiy]

Oleg,

first of all thank you for a good review, please see my comments below.

On Wed, Jul 13, 2011 at 6:09 PM, Oleg Nesterov <oleg@redhat.com> wrote:
> On 07/12, Vladimir Zapolskiy wrote:
>>
>> Note, a detach signal is not emitted, if a tracer process terminates
>> without explicit PTRACE_DETACH request. Such cases can be covered
>> listening to PROC_EVENT_EXIT connector events.
>
> Hmm. More and more reasons to make the implicit detach sleepable...
>
> But. There is another case. The (dead) tracee can be detached via
> do_wait(). Perhaps this falls into "covered listening to EXIT" too,
> but imho makes sense to document in the changelog. Oh, and probably
> we will add the ability to detach a zombie...
>
> I don't really understand why do you need this, but I won't argue.
>
I found that implicit ptrace detach codepath is quite mutable and
vast, and I don't want to interfere in that changes without knowing
even basic pitfalls. Somehow the sending a connector signal on
explicit detach is quite sufficient at least for the most of the proc
connector usecases I can imagine, because hopefully almost(?) all
implicit detach cases are related to tracer or tracee thread
completion, and that is supposed to be reported to userspace via
do_exit()/proc_exit_connector() path.

> As for the patch,
>
>> +void proc_ptrace_connector(struct task_struct *task)
>> +{
>> +     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;
>> +
>> +     rcu_read_lock();
>> +     tracer = tracehook_tracer_task(task);
>> +     if (tracer) {
>> +             ev->event_data.ptrace.tracer_pid  = tracer->pid;
>> +             ev->event_data.ptrace.tracer_tgid = tracer->tgid;
>> +     } else {
>> +             ev->event_data.ptrace.tracer_pid  = 0;
>> +             ev->event_data.ptrace.tracer_tgid = 0;
>> +     }
>
> This doesn't look right. The code uses tracehook_tracer_task() to
> figure out whether this task traced or not. But this is racy.
>
> ptrace_attach:
>
>        ...attach...
>
>        /* WINDOW */
>
>        proc_ptrace_connector(task);
>
> The task can exit in between, and the caller's subthread can do
> wait4() and release it. In this case proc_ptrace_connector() will
> see tracehook_tracer_task() == NULL and report "detach".
>
> The similar race in ptrace_detach() path. Another tracer can attach
> to this task before we proc_ptrace_connector().
>
> I think proc_ptrace_connector() needs the explicit "task_struct *tracer"
> argument, NULL if ptrace_detach(). Or a simple boolean, the tracer is
> current.
>
> If you think this is fine - I won't argue.
>
Fixed in the second version of the change, thanks.

>
>
> But in any case, please rediff against
>
>        git://git.kernel.org/pub/scm/linux/kernel/git/oleg/misc.git ptrace
>
> tracehook_tracer_task() was removed, and
>
>> @@ -260,6 +261,9 @@ out:
>>       if (wait_trap)
>>               wait_event(current->signal->wait_chldexit,
>>                          !(task->group_stop & GROUP_STOP_TRAPPING));
>> +     if (!retval)
>> +             proc_ptrace_connector(task);
>> +
>>       return retval;
>>  }
>
> this chunk probably should be updated.
Rebased, thanks a lot.

Vladimir