[PATCH 0/1] (Was: connector: add an event for monitoring processtracers)

From: Oleg Nesterov
Date: Thu Jul 21 2011 - 14:37:56 EST


On 07/18, Oleg Nesterov wrote:
>
> 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.

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

> 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?

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

Oleg.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/