| Message ID | 1310751918-31579-1-git-send-email-vzapolskiy@gmail.com |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | David Miller |
| Headers | show |
Hi. On Fri, Jul 15, 2011 at 08:45:18PM +0300, Vladimir Zapolskiy (vzapolskiy@gmail.com) wrote: > 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. I still ack this one :) Acked-by: Evgeniy Polyakov <zbr@ioremap.net>
On 07/18, Evgeniy Polyakov wrote: > > Acked-by: Evgeniy Polyakov <zbr@ioremap.net> 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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/15, Vladimir Zapolskiy wrote: > > 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. 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 ;) > +void proc_ptrace_connector(struct task_struct *task, int which_id); "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. > - if (!retval) > + if (!retval) { > wait_on_bit(&task->jobctl, JOBCTL_TRAPPING_BIT, > ptrace_trapping_sleep_fn, TASK_UNINTERRUPTIBLE); > + proc_ptrace_connector(task, PTRACE_ATTACH); > + } OK, but it is a bit strange we are waiting for STOPPED/TRACED transition before we report PROC_EVENT_PTRACE. Perhaps it makes more sense to call proc_ptrace_connector() first, this also decreases the probability PTRACE_ATTACH will be reported after PROC_EVENT_EXIT. But once again, this is very minor and cosmetic. I am going to apply the patch as is unless you send v3 quickly. Oleg. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jul 18, 2011 at 8:54 PM, Oleg Nesterov <oleg@redhat.com> wrote: > On 07/15, Vladimir Zapolskiy wrote: >> >> 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. > > 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. > 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. > > Can't resist, a couple of very minor/cosmetics nits. Just because I am > blighter ;) > >> +void proc_ptrace_connector(struct task_struct *task, int which_id); > > "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. > I'd like to preserve that variant, in my opinion its just a bit more undisguised version rather than bare true/false. >> - if (!retval) >> + if (!retval) { >> wait_on_bit(&task->jobctl, JOBCTL_TRAPPING_BIT, >> ptrace_trapping_sleep_fn, TASK_UNINTERRUPTIBLE); >> + proc_ptrace_connector(task, PTRACE_ATTACH); >> + } > > OK, but it is a bit strange we are waiting for STOPPED/TRACED transition > before we report PROC_EVENT_PTRACE. Perhaps it makes more sense to > call proc_ptrace_connector() first, this also decreases the probability > PTRACE_ATTACH will be reported after PROC_EVENT_EXIT. > 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. > > But once again, this is very minor and cosmetic. I am going to apply > the patch as is unless you send v3 quickly. > 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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/18, Vladimir Zapolskiy wrote: > > On Mon, Jul 18, 2011 at 8:54 PM, Oleg Nesterov <oleg@redhat.com> wrote: > > > "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. > > >^ > I'd like to preserve that variant, in my opinion its just a bit more > undisguised version rather than bare true/false. OK. Although this "else return" in proc_ptrace_connector() looks like the "hide the potentional error" to me. > >> - if (!retval) > >> + if (!retval) { > >> wait_on_bit(&task->jobctl, JOBCTL_TRAPPING_BIT, > >> ptrace_trapping_sleep_fn, TASK_UNINTERRUPTIBLE); > >> + proc_ptrace_connector(task, PTRACE_ATTACH); > >> + } > > > > OK, but it is a bit strange we are waiting for STOPPED/TRACED transition > > before we report PROC_EVENT_PTRACE. Perhaps it makes more sense to > > call proc_ptrace_connector() first, this also decreases the probability > > PTRACE_ATTACH will be reported after PROC_EVENT_EXIT. > > > 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. 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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/connector/cn_proc.c b/drivers/connector/cn_proc.c index 2b46a7e..281902d 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/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; diff --git a/include/linux/cn_proc.h b/include/linux/cn_proc.h index 47dac5e..12c517b 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, 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 */ diff --git a/kernel/ptrace.c b/kernel/ptrace.c index d7ccc79..9de3ecf 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> 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);
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. Signed-off-by: Vladimir Zapolskiy <vzapolskiy@gmail.com> Cc: Evgeniy Polyakov <zbr@ioremap.net> Cc: David S. Miller <davem@davemloft.net> --- 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(-)