* [PATCH 0/1] (Was: connector: add an event for monitoring process tracers)
[not found] ` <20110718171420.GA11470@redhat.com>
@ 2011-07-21 18:34 ` Oleg Nesterov
2011-07-21 18:35 ` [PATCH 1/1] proc_fork_connector: a lockless ->real_parent usage is not safe Oleg Nesterov
0 siblings, 1 reply; 2+ messages in thread
From: Oleg Nesterov @ 2011-07-21 18:34 UTC (permalink / raw)
To: Andrew Morton
Cc: Vladimir Zapolskiy, David S. Miller, Evgeniy Polyakov,
linux-kernel
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.
^ permalink raw reply [flat|nested] 2+ messages in thread