pids: make task_tgid_nr_ns() safe

PATCH Applied Oleg Nesterov 4 participants 8 messages Aug 21, 2017 – Aug 22, 2017 6 messages not in the archive

Subsystems (3)

Share & tools

A message that is not in the archive (sent off-list or not yet received): CAMKL9z2A8VSmLc9gz4Vtd1N5nGwR5d9Mxi1Nc8VjTQKsURoMkA@mail.gmail.com

A message that is not in the archive (sent off-list or not yet received): CA+55aFyfaBZmDVSpx3F7cQZcck8u_vg53wL1ykRE-ccgrtg14w@mail.gmail.com

A message that is not in the archive (sent off-list or not yet received): CAGXu5jKLDAnKoRg_v8Nc7s6AtQ6_QfaMtJorjFkbaX64MDHy2g@mail.gmail.com

A message that is not in the archive (sent off-list or not yet received): CA+55aFy+0cQUcPjWmnD8qFL2gHVYHYTxJmHVy6L=Bz03KaP2Rw@mail.gmail.com

A message that is not in the archive (sent off-list or not yet received): [email protected]

A message that is not in the archive (sent off-list or not yet received): CA+55aFzA+0jzTdknmf0OUB7WSPgAa56nqckqdz=iDmbR2P9eRw@mail.gmail.com

Oleg Nesterov

author

2017-08-21 15:35 UTC #6 lore raw

PATCH 1/1 pids: make task_tgid_nr_ns() safe

This was reported many times, and this was even mentioned in commit 52ee2dfdd4f5 "pids: refactor vnr/nr_ns helpers to make them safe" but somehow nobody bothered to fix the obvious problem: task_tgid_nr_ns() is not safe because task->group_leader points to nowhere after the exiting task passes exit_notify(), rcu_read_lock() can not help.

We really need to change __unhash_process() to nullify group_leader, parent, and real_parent, but this needs some cleanups. Until then we can turn task_tgid_nr_ns() into another user of __task_pid_nr_ns() and fix the problem.

Reported-by Troy Kensinger <[email protected]>

Signed-off-by Oleg Nesterov <[email protected]>

---
 include/linux/pid.h   |  4 +++-
 include/linux/sched.h | 51 +++++++++++++++++++++++++++------------------------
 kernel/pid.c          | 11 ++++-------
 3 files changed, 34 insertions(+), 32 deletions(-)

include/linux/pid.h +3 -1

index 4d17931..7195827 100644
@@ -8,7 +8,9 @@ enum pid_type
 	PIDTYPE_PID,
 	PIDTYPE_PGID,
 	PIDTYPE_SID,
-	PIDTYPE_MAX
+	PIDTYPE_MAX,
+	/* only valid to __task_pid_nr_ns() */
+	__PIDTYPE_TGID
 };
 
 /*

include/linux/sched.h +27 ~~-24~~

69 lines not on this page · raw message

kernel/pid.c +4 -7

index c69c30d..020dedb 100644
@@ -527,8 +527,11 @@ pid_t __task_pid_nr_ns(struct task_struct *task, enum pid_type type,
 	if (!ns)
 		ns = task_active_pid_ns(current);
 	if (likely(pid_alive(task))) {
-		if (type != PIDTYPE_PID)
+		if (type != PIDTYPE_PID) {
+			if (type == __PIDTYPE_TGID)
+				type = PIDTYPE_PID;
 			task = task->group_leader;
+		}
 		nr = pid_nr_ns(rcu_dereference(task->pids[type].pid), ns);
 	}
 	rcu_read_unlock();

@@ -537,12 +540,6 @@ pid_t __task_pid_nr_ns(struct task_struct *task, enum pid_type type,
 }
 EXPORT_SYMBOL(__task_pid_nr_ns);
 
-pid_t task_tgid_nr_ns(struct task_struct *tsk, struct pid_namespace *ns)
-{
-	return pid_nr_ns(task_tgid(tsk), ns);
-}
-EXPORT_SYMBOL(task_tgid_nr_ns);
-
 struct pid_namespace *task_active_pid_ns(struct task_struct *tsk)
 {
 	return ns_of_pid(task_pid(tsk));

Signature · 1 line

2.5.0

Oleg Nesterov

author

2017-08-21 15:36 UTC #7 lore raw

perf_event_pid() (Was: [PATCH] pids: make task_tgid_nr_ns() safe)

Quoted 6 lines · Oleg Nesterov

This was reported many times, and this was even mentioned in commit 52ee2dfdd4f5 "pids: refactor vnr/nr_ns helpers to make them safe" but somehow nobody bothered to fix the obvious problem: task_tgid_nr_ns() is not safe because task->group_leader points to nowhere after the exiting task passes exit_notify(), rcu_read_lock() can not help.

Peter,

we already discussed this before, but I can't recall the result...

perf_event_pid() can hit this problem too, with this patch the problem goes away but (with or without this patch) we do not want to report zero pids to avoid the confusion with idle threads, right?

So do you think the patch below makes sense or we do not care?

Oleg.
---

kernel/events/core.c +14 -9

@@ -1249,26 +1249,31 @@ unclone_ctx(struct perf_event_context *ctx)
 	return parent_ctx;
 }
 
-static u32 perf_event_pid(struct perf_event *event, struct task_struct *p)
+static u32 perf_event_xxx(struct perf_event *event, struct task_struct *p,
+				enum pid_type type)
 {
+	u32 ret;
 	/*
 	 * only top level events have the pid namespace they were created in
 	 */
 	if (event->parent)
 		event = event->parent;
 
-	return task_tgid_nr_ns(p, event->ns);
+	ret = __task_pid_nr_ns(p, type, event->ns);
+	/* avoid -1 if it is idle thread or runs in another ns */
+	if (!ret && !pid_alive(p))
+		ret = -1;
+	return ret;
 }
 
-static u32 perf_event_tid(struct perf_event *event, struct task_struct *p)
+static u32 perf_event_pid(struct perf_event *event, struct task_struct *p)
 {
-	/*
-	 * only top level events have the pid namespace they were created in
-	 */
-	if (event->parent)
-		event = event->parent;
+	return perf_event_xxx(event, p, __PIDTYPE_TGID);
+}
 
-	return task_pid_nr_ns(p, event->ns);
+static u32 perf_event_tid(struct perf_event *event, struct task_struct *p)
+{
+	return perf_event_xxx(event, p, PIDTYPE_PID);
 }
 
 /*

Peter Zijlstra

maintainer

2017-08-21 19:19 UTC #8 lore raw

Re: perf_event_pid() (Was: [PATCH] pids: make task_tgid_nr_ns() safe)

Quoted 10 lines · Oleg Nesterov

Quoted 6 lines · Oleg Nesterov
This was reported many times, and this was even mentioned in commit 52ee2dfdd4f5 "pids: refactor vnr/nr_ns helpers to make them safe" but somehow nobody bothered to fix the obvious problem: task_tgid_nr_ns() is not safe because task->group_leader points to nowhere after the exiting task passes exit_notify(), rcu_read_lock() can not help.
Peter,

Quoted 7 lines

we already discussed this before, but I can't recall the result...
perf_event_pid() can hit this problem too, with this patch the problem goes away but (with or without this patch) we do not want to report zero pids to avoid the confusion with idle threads, right?
So do you think the patch below makes sense or we do not care?

Yeah, works for me. While rare I think it makes sense to distinguish between a recently dead and the idle tasks.

Linus Torvalds

Applied

2017-08-21 19:49 UTC #9 lore raw

In reply to Oleg Nesterov

Quoted 5 lines · Oleg Nesterov

We really need to change __unhash_process() to nullify group_leader, parent, and real_parent, but this needs some cleanups. Until then we can turn task_tgid_nr_ns() into another user of __task_pid_nr_ns() and fix the problem.

Applied.

Should it perhaps have had a "Cc: stable" on it? I didn't do so, but it might be worth pointing Greg at this explicitly (dd1c1f2f2028 "pids: make task_tgid_nr_ns() safe").

I haven't pushed it out yet, it's going through my usual build first, but it's in my tree.

Linus

Oleg Nesterov

author

2017-08-22 12:20 UTC #10 lore raw

Quoted 8 lines · Linus Torvalds

Quoted 5 lines · Oleg Nesterov
We really need to change __unhash_process() to nullify group_leader, parent, and real_parent, but this needs some cleanups. Until then we can turn task_tgid_nr_ns() into another user of __task_pid_nr_ns() and fix the problem.

Quoted 5 lines

Applied.
Should it perhaps have had a "Cc: stable" on it? I didn't do so, but it might be worth pointing Greg at this explicitly (dd1c1f2f2028 "pids: make task_tgid_nr_ns() safe").

Yes, agreed, I forgot to add this tag.

Oleg.

Linus Torvalds

2017-08-22 16:57 UTC #11 lore raw

I suspect Greg saw this on the security list already, but let's just cc the stable list explicitly for

    dd1c1f2f2028  ("pids: make task_tgid_nr_ns() safe")

since the original commit didn't end up having been marked.

Linus

Quoted 10 lines · Oleg Nesterov

Quoted 4 lines · Linus Torvalds
Should it perhaps have had a "Cc: stable" on it? I didn't do so, but it might be worth pointing Greg at this explicitly (dd1c1f2f2028 "pids: make task_tgid_nr_ns() safe").
Yes, agreed, I forgot to add this tag.
Oleg.

Greg KH

2017-08-22 17:51 UTC #12 lore raw

Quoted 6 lines · Linus Torvalds

I suspect Greg saw this on the security list already, but let's just cc the stable list explicitly for
    dd1c1f2f2028  ("pids: make task_tgid_nr_ns() safe")
since the original commit didn't end up having been marked.

Thanks, I picked it up now.

greg k-h

Peter Zijlstra

maintainer Acked-by on v1 1/1

2017-08-21 20:05 UTC #13 lore raw

In reply to Oleg Nesterov

Quoted 6 lines · Oleg Nesterov

This was reported many times, and this was even mentioned in commit 52ee2dfdd4f5 "pids: refactor vnr/nr_ns helpers to make them safe" but somehow nobody bothered to fix the obvious problem: task_tgid_nr_ns() is not safe because task->group_leader points to nowhere after the exiting task passes exit_notify(), rcu_read_lock() can not help.

Quoted 7 lines

We really need to change __unhash_process() to nullify group_leader, parent, and real_parent, but this needs some cleanups. Until then we can turn task_tgid_nr_ns() into another user of __task_pid_nr_ns() and fix the problem.
Reported-by Troy Kensinger <[email protected]>
Signed-off-by Oleg Nesterov <[email protected]>

Acked-by Peter Zijlstra (Intel) <[email protected]>