Skip to content

(Was: connector: add an event for monitoring process tracers)

Oleg Nesterov lkml netdev 1 version New

Get series with b4
b4 am [email protected]

Versions newest first

  1. v1 Latest1 patch 0 repliesNew

Patches in v1 No applies recorded yet

#Subject and filesSizeRepliesApplied
1/1proc_fork_connector: a lockless ->real_parent usage is not safedrivers/connector/cn_proc.c+6 -20 replies

Reviews trailers per patch

No Reviewed-by, Acked-by or Tested-by trailers for v1 yet.

Read the v1 thread

Cover letter [PATCH 0/1] (Was: connector: add an event for monitoring process tracers)
Quoted 29 lines · Oleg Nesterov

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;
Quoted 2 lines

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

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

Quoted 26 lines

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);
Quoted 2 lines

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

Read the cover letter in its thread