signals: kill(-1) should only signal processes in the same namespace
PATCH 1/2 signals: kill(-1) should only signal processes in the same namespace
While moving Linux-VServer to using pid namespaces, I noticed that kill(-1) from inside a pid namespace is currently signalling every process in the entire system, including processes that are otherwise unreachable from the current process.
This patch fixes it by making sure that only processes which are in the same pid namespace as current get signalled.
include/linux/pid_namespace.h +0 -0
@@ -40,6 +40,8 @@ static inline struct pid_namespace *get_pid_ns(struct
pid_namespace *ns)
extern struct pid_namespace *copy_pid_ns(unsigned long flags, structpid_namespace *ns);
extern void free_pid_ns(struct kref *kref);
extern void zap_pid_ns_processes(struct pid_namespace *pid_ns);
+extern int task_in_pid_ns(struct task_struct *tsk,
+ struct pid_namespace *pid_ns);
static inline void put_pid_ns(struct pid_namespace *ns) {
@@ -72,6 +74,12 @@ static inline void zap_pid_ns_processes(struct pid_namespace *ns)
{
BUG();
}
+
+static inline int task_in_pid_ns(struct task_struct *tsk,
+ struct pid_namespace *ns)
+{
+ return 1;
+}
#endif /* CONFIG_PID_NS */
static inline struct pid_namespace *task_active_pid_ns(structtask_struct *tsk)
kernel/pid_namespace.c +20 -0
@@ -188,6 +188,26 @@ void zap_pid_ns_processes(struct pid_namespace *pid_ns)return ; } +/* +* Checks whether tsk has a pid in the pid namespace ns. +* Must be called with tasklist_lock read-locked or under rcu_read_lock() +*/ +int task_in_pid_ns (struct task_struct *tsk,struct pid_namespace *ns) +{ +struct pid *pid =task_pid (tsk); + +if (!pid) +return 0 ; + +if (pid->level < ns->level) +return 0 ; + +if (pid->numbers[ns->level].ns != ns) +return 0 ; + +return 1 ; +} +static __init int pid_namespaces_init (void ) { pid_ns_cachep =KMEM_CACHE (pid_namespace,SLAB_PANIC );
kernel/signal.c +0 -0
@@ -1145,7 +1145,8 @@ static int kill_something_info(int sig, struct
siginfo *info, int pid)
struct task_struct * p;
for_each_process(p) {
- if (p->pid > 1 && !same_thread_group(p, current)) {
+ if (p->pid > 1 && !same_thread_group(p, current) &&
+ task_in_pid_ns(p, current->nsproxy->pid_ns)) {
int err = group_send_sig_info(sig, info, p); ++count;
if (err != -EPERM)
Signature · 1 line
1.5.5.1
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 4 lines · Daniel Hokka Zakrisson
While moving Linux-VServer to using pid namespaces, I noticed that kill(-1) from inside a pid namespace is currently signalling every process in the entire system, including processes that are otherwise unreachable from the current process.
This is not a "news" actually, buy anyway - thanks :)
Quoted 2 lines
This patch fixes it by making sure that only processes which are in the same pid namespace as current get signalled.
This is to be done, indeed, but I do not like the proposed implementation, since you have to walk all the tasks in the system (under tasklist_lock, by the way) to search for a couple of interesting ones. Better look at how zap_pid_ns_processes works (by the way - I saw some patch doing so some time ago).
Quoted 78 lines
Signed-off-by Daniel Hokka Zakrisson <[email protected]>include/linux/pid_namespace.h +2
-0@@ -40,6 +40,8 @@ static inline struct pid_namespace *get_pid_ns(struct pid_namespace *ns)extern struct pid_namespace *copy_pid_ns (unsigned long flags,struct pid_namespace *ns);extern void free_pid_ns (struct kref *kref);extern void zap_pid_ns_processes (struct pid_namespace *pid_ns); +extern int task_in_pid_ns (struct task_struct *tsk, +struct pid_namespace *pid_ns);static inline void put_pid_ns(struct pid_namespace *ns) {include/linux/pid_namespace.h +6
-0@@ -72,6 +74,12 @@ static inline void zap_pid_ns_processes(struct pid_namespace *ns) {BUG (); } + +static inline int task_in_pid_ns (struct task_struct *tsk, +struct pid_namespace *ns) +{ +return 1 ; +}#endif /* CONFIG_PID_NS */ static inline struct
pid_namespace*task_active_pid_ns(structtask_struct*tsk)kernel/pid_namespace.c +20
-0@@ -188,6 +188,26 @@ void zap_pid_ns_processes(struct pid_namespace *pid_ns)return ; } +/* +* Checks whether tsk has a pid in the pid namespace ns. +* Must be called with tasklist_lock read-locked or under rcu_read_lock() +*/ +int task_in_pid_ns (struct task_struct *tsk,struct pid_namespace *ns) +{ +struct pid *pid =task_pid (tsk); + +if (!pid) +return 0 ; + +if (pid->level < ns->level) +return 0 ; + +if (pid->numbers[ns->level].ns != ns) +return 0 ; + +return 1 ; +} +static __init int pid_namespaces_init (void ) { pid_ns_cachep =KMEM_CACHE (pid_namespace,SLAB_PANIC );kernel/signal.c +2
-1@@ -1145,7 +1145,8 @@ static int kill_something_info(int sig, struct siginfo *info, int pid)struct task_struct * p;for_each_process (p) { -if (p->pid >1 && !same_thread_group (p, current)) { +if (p->pid >1 && !same_thread_group (p, current) && +task_in_pid_ns (p, current->nsproxy->pid_ns)) {int err =group_send_sig_info (sig, info, p); ++count;if (err !=
-EPERM)
Daniel Hokka Zakrisson [[email protected]] wrote:
| While moving Linux-VServer to using pid namespaces, I noticed that | kill(-1) from inside a pid namespace is currently signalling every | process in the entire system, including processes that are otherwise | unreachable from the current process. | | This patch fixes it by making sure that only processes which are in | the same pid namespace as current get signalled. | | Signed-off-by: Daniel Hokka Zakrisson <[email protected]> | | diff --git a/include/linux/pid_namespace.h b/include/linux/pid_namespace.h | index caff528..4cf41bd 100644 | --- a/include/linux/pid_namespace.h | +++ b/include/linux/pid_namespace.h | @@ -40,6 +40,8 @@ static inline struct pid_namespace *get_pid_ns(struct | pid_namespace *ns) | extern struct pid_namespace *copy_pid_ns(unsigned long flags, struct | pid_namespace *ns); | extern void free_pid_ns(struct kref *kref); | extern void zap_pid_ns_processes(struct pid_namespace *pid_ns); | +extern int task_in_pid_ns(struct task_struct *tsk, | + struct pid_namespace *pid_ns); | | static inline void put_pid_ns(struct pid_namespace *ns) | { | @@ -72,6 +74,12 @@ static inline void zap_pid_ns_processes(struct | pid_namespace *ns) | { | BUG(); | } | + | +static inline int task_in_pid_ns(struct task_struct *tsk, | + struct pid_namespace *ns) | +{ | + return 1; | +} | #endif /* CONFIG_PID_NS */ | | static inline struct pid_namespace *task_active_pid_ns(struct | task_struct *tsk) | diff --git a/kernel/pid_namespace.c b/kernel/pid_namespace.c | index 98702b4..3e71011 100644 | --- a/kernel/pid_namespace.c | +++ b/kernel/pid_namespace.c | @@ -188,6 +188,26 @@ void zap_pid_ns_processes(struct pid_namespace *pid_ns) | return; | } | | +/* | + * Checks whether tsk has a pid in the pid namespace ns. | + * Must be called with tasklist_lock read-locked or under rcu_read_lock() | + */ | +int task_in_pid_ns(struct task_struct *tsk, struct pid_namespace *ns) | +{ | + struct pid *pid = task_pid(tsk); | + | + if (!pid) | + return 0; | + | + if (pid->level < ns->level) | + return 0;
ns can be NULL if tsk is exiting.
Like Pavel said, we had couple of attempts to fix the larger problem of signal semantics in containers but did not have a consensus on handling blocked/unhandled signals to container-init.
It would still be good to fix this "kill -1" problem.
Eric had a slightly optimized interface, 'pid_in_pid_ns()' in following patchset. Maybe we could use that ?
https://lists.linux-foundation.org/pipermail/containers/2007-December/009174.html
| +
| + if (pid->numbers[ns->level].ns != ns)
| + return 0;
| +
| + return 1;
| +}
| +
| static __init int pid_namespaces_init(void)
| {
| pid_ns_cachep = KMEM_CACHE(pid_namespace, SLAB_PANIC);
| diff --git a/kernel/signal.c b/kernel/signal.c
| index 6c0958e..93713a5 100644
| --- a/kernel/signal.c
| +++ b/kernel/signal.c
| @@ -1145,7 +1145,8 @@ static int kill_something_info(int sig, struct
| siginfo *info, int pid)
| struct task_struct * p;
|
| for_each_process(p) {
| - if (p->pid > 1 && !same_thread_group(p, current)) {
| + if (p->pid > 1 && !same_thread_group(p, current) &&
| + task_in_pid_ns(p, current->nsproxy->pid_ns)) {
| int err = group_send_sig_info(sig, info, p);
| ++count;
| if (err != -EPERM)
| --
| 1.5.5.1
| _______________________________________________
| Containers mailing list
| Containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
| https://lists.linux-foundation.org/mailman/listinfo/containersQuoted 34 lines · Daniel Hokka Zakrisson
kernel/pid_namespace.c +16
-0+int task_in_pid_ns (struct task_struct *tsk,struct pid_namespace *ns) +{ +struct pid *pid =task_pid (tsk); + +if (!pid) +return 0 ; + +if (pid->level < ns->level) +return 0 ; + +if (pid->numbers[ns->level].ns != ns) +return 0 ; + +return 1 ; +} +static __init int pid_namespaces_init (void ) { pid_ns_cachep =KMEM_CACHE (pid_namespace,SLAB_PANIC );kernel/signal.c +2
-1@@ -1145,7 +1145,8 @@ static int kill_something_info(int sig, struct siginfo *info, int pid)struct task_struct * p;for_each_process (p) { -if (p->pid >1 && !same_thread_group (p, current)) { +if (p->pid >1 && !same_thread_group (p, current) && +task_in_pid_ns (p, current->nsproxy->pid_ns)) {int err =group_send_sig_info (sig, info, p); ++count;
Quoted 1 line
if (err !=
-EPERM)
Do we really need all these complications? Afaics, we can make a simpler patch,
--- kernel/signal.c
+++ kernel/signal.c
@@ -1136,7 +1136,7 @@ static int kill_something_info(int sig,
struct task_struct * p;
for_each_process(p) {
- if (p->pid > 1 && !same_thread_group(p, current)) {
+ if (task_pid_vnr(p) > 1 && !same_thread_group(p, current)) {
int err = group_send_sig_info(sig, info, p);
++count;
if (err != -EPERM)task_pid_vnr(p) returns 0 if "p" is not visible from the current's namespace. "> 1" ensures we don't kill the child reaper as well.
No?
Oleg.
Quoted 4 lines · Daniel Hokka Zakrisson
While moving Linux-VServer to using pid namespaces, I noticed that kill(-1) from inside a pid namespace is currently signalling every process in the entire system, including processes that are otherwise unreachable from the current process.
This is not a "news" actually, buy anyway - thanks :)
Quoted 2 lines
This patch fixes it by making sure that only processes which are in the same pid namespace as current get signalled.
This is to be done, indeed, but I do not like the proposed implementation, since you have to walk all the tasks in the system (under tasklist_lock, by the way) to search for a couple of interesting ones. Better look at how zap_pid_ns_processes works (by the way - I saw some patch doing so some time ago).
Quoted 78 lines
Signed-off-by Daniel Hokka Zakrisson <[email protected]>include/linux/pid_namespace.h +2
-0@@ -40,6 +40,8 @@ static inline struct pid_namespace *get_pid_ns(struct pid_namespace *ns)extern struct pid_namespace *copy_pid_ns (unsigned long flags,struct pid_namespace *ns);extern void free_pid_ns (struct kref *kref);extern void zap_pid_ns_processes (struct pid_namespace *pid_ns); +extern int task_in_pid_ns (struct task_struct *tsk, +struct pid_namespace *pid_ns);static inline void put_pid_ns(struct pid_namespace *ns) {include/linux/pid_namespace.h +6
-0@@ -72,6 +74,12 @@ static inline void zap_pid_ns_processes(struct pid_namespace *ns) {BUG (); } + +static inline int task_in_pid_ns (struct task_struct *tsk, +struct pid_namespace *ns) +{ +return 1 ; +}#endif /* CONFIG_PID_NS */ static inline struct
pid_namespace*task_active_pid_ns(structtask_struct*tsk)kernel/pid_namespace.c +20
-0@@ -188,6 +188,26 @@ void zap_pid_ns_processes(struct pid_namespace *pid_ns)return ; } +/* +* Checks whether tsk has a pid in the pid namespace ns. +* Must be called with tasklist_lock read-locked or under rcu_read_lock() +*/ +int task_in_pid_ns (struct task_struct *tsk,struct pid_namespace *ns) +{ +struct pid *pid =task_pid (tsk); + +if (!pid) +return 0 ; + +if (pid->level < ns->level) +return 0 ; + +if (pid->numbers[ns->level].ns != ns) +return 0 ; + +return 1 ; +} +static __init int pid_namespaces_init (void ) { pid_ns_cachep =KMEM_CACHE (pid_namespace,SLAB_PANIC );kernel/signal.c +2
-1@@ -1145,7 +1145,8 @@ static int kill_something_info(int sig, struct siginfo *info, int pid)struct task_struct * p;for_each_process (p) { -if (p->pid >1 && !same_thread_group (p, current)) { +if (p->pid >1 && !same_thread_group (p, current) && +task_in_pid_ns (p, current->nsproxy->pid_ns)) {int err =group_send_sig_info (sig, info, p); ++count;if (err !=
-EPERM)
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 7 lines · Pavel Emelyanov
Quoted 4 lines · Daniel Hokka Zakrisson
While moving Linux-VServer to using pid namespaces, I noticed that kill(-1) from inside a pid namespace is currently signalling every process in the entire system, including processes that are otherwise unreachable from the current process.
This is not a "news" actually, buy anyway - thanks :)
And yet nobody's fixed it... Kind of a critical thing, if you actually want to use them, since most distribution's rc-scripts do a kill(-1, SIGTERM), followed by kill(-1, SIGKILL) when halting (which, needless to say, would be very bad).
Quoted 8 lines
Quoted 2 lines
This patch fixes it by making sure that only processes which are in the same pid namespace as current get signalled.
This is to be done, indeed, but I do not like the proposed implementation, since you have to walk all the tasks in the system (under
tasklist_lock, by the way) to search for a couple of interesting ones. Better look at howzap_pid_ns_processesworks (by the way - I saw some patch doing so some time ago).
The way zap_pid_ns_processes does it is worse, since it signals every thread in the namespace rather than every thread group. So either we walk the global tasklist, or we create a per-namespace one. Is that what we want?
Quoted 81 lines
Quoted 81 lines
Signed-off-by Daniel Hokka Zakrisson <[email protected]>include/linux/pid_namespace.h +2
-0@@ -40,6 +40,8 @@ static inline struct pid_namespace *get_pid_ns(struct pid_namespace *ns)extern struct pid_namespace *copy_pid_ns (unsigned long flags,struct pid_namespace *ns);extern void free_pid_ns (struct kref *kref);extern void zap_pid_ns_processes (struct pid_namespace *pid_ns); +extern int task_in_pid_ns (struct task_struct *tsk, +struct pid_namespace *pid_ns);static inline void put_pid_ns(struct pid_namespace *ns) {include/linux/pid_namespace.h +6
-0@@ -72,6 +74,12 @@ static inline void zap_pid_ns_processes(struct pid_namespace *ns) {BUG (); } + +static inline int task_in_pid_ns (struct task_struct *tsk, +struct pid_namespace *ns) +{ +return 1 ; +}#endif /* CONFIG_PID_NS */ static inline struct
pid_namespace*task_active_pid_ns(structtask_struct*tsk)kernel/pid_namespace.c +20
-0@@ -188,6 +188,26 @@ void zap_pid_ns_processes(struct pid_namespace *pid_ns)return ; } +/* +* Checks whether tsk has a pid in the pid namespace ns. +* Must be called with tasklist_lock read-locked or under rcu_read_lock() +*/ +int task_in_pid_ns (struct task_struct *tsk,struct pid_namespace *ns) +{ +struct pid *pid =task_pid (tsk); + +if (!pid) +return 0 ; + +if (pid->level < ns->level) +return 0 ; + +if (pid->numbers[ns->level].ns != ns) +return 0 ; + +return 1 ; +} +static __init int pid_namespaces_init (void ){ pid_ns_cachep = KMEM_CACHE(pid_namespace, SLAB_PANIC);kernel/signal.c +2
-1@@ -1145,7 +1145,8 @@ static int kill_something_info(int sig, struct siginfo *info, int pid)struct task_struct * p;for_each_process (p) { -if (p->pid >1 && !same_thread_group (p, current)) { +if (p->pid >1 && !same_thread_group (p, current) && +task_in_pid_ns (p, current->nsproxy->pid_ns)) {int err =group_send_sig_info (sig, info, p); ++count;if (err !=
-EPERM)
Signature · 1 line
Daniel Hokka Zakrisson
Quoted 7 lines · Pavel Emelyanov
Quoted 4 lines · Daniel Hokka Zakrisson
While moving Linux-VServer to using pid namespaces, I noticed that kill(-1) from inside a pid namespace is currently signalling every process in the entire system, including processes that are otherwise unreachable from the current process.
This is not a "news" actually, buy anyway - thanks :)
And yet nobody's fixed it... Kind of a critical thing, if you actually want to use them, since most distribution's rc-scripts do a kill(-1, SIGTERM), followed by kill(-1, SIGKILL) when halting (which, needless to say, would be very bad).
Quoted 8 lines
Quoted 2 lines
This patch fixes it by making sure that only processes which are in the same pid namespace as current get signalled.
This is to be done, indeed, but I do not like the proposed implementation, since you have to walk all the tasks in the system (under
tasklist_lock, by the way) to search for a couple of interesting ones. Better look at howzap_pid_ns_processesworks (by the way - I saw some patch doing so some time ago).
The way zap_pid_ns_processes does it is worse, since it signals every thread in the namespace rather than every thread group. So either we walk the global tasklist, or we create a per-namespace one. Is that what we want?
Quoted 81 lines
Quoted 81 lines
Signed-off-by Daniel Hokka Zakrisson <[email protected]>include/linux/pid_namespace.h +2
-0@@ -40,6 +40,8 @@ static inline struct pid_namespace *get_pid_ns(struct pid_namespace *ns)extern struct pid_namespace *copy_pid_ns (unsigned long flags,struct pid_namespace *ns);extern void free_pid_ns (struct kref *kref);extern void zap_pid_ns_processes (struct pid_namespace *pid_ns); +extern int task_in_pid_ns (struct task_struct *tsk, +struct pid_namespace *pid_ns);static inline void put_pid_ns(struct pid_namespace *ns) {include/linux/pid_namespace.h +6
-0@@ -72,6 +74,12 @@ static inline void zap_pid_ns_processes(struct pid_namespace *ns) {BUG (); } + +static inline int task_in_pid_ns (struct task_struct *tsk, +struct pid_namespace *ns) +{ +return 1 ; +}#endif /* CONFIG_PID_NS */ static inline struct
pid_namespace*task_active_pid_ns(structtask_struct*tsk)kernel/pid_namespace.c +20
-0@@ -188,6 +188,26 @@ void zap_pid_ns_processes(struct pid_namespace *pid_ns)return ; } +/* +* Checks whether tsk has a pid in the pid namespace ns. +* Must be called with tasklist_lock read-locked or under rcu_read_lock() +*/ +int task_in_pid_ns (struct task_struct *tsk,struct pid_namespace *ns) +{ +struct pid *pid =task_pid (tsk); + +if (!pid) +return 0 ; + +if (pid->level < ns->level) +return 0 ; + +if (pid->numbers[ns->level].ns != ns) +return 0 ; + +return 1 ; +} +static __init int pid_namespaces_init (void ){ pid_ns_cachep = KMEM_CACHE(pid_namespace, SLAB_PANIC);kernel/signal.c +2
-1@@ -1145,7 +1145,8 @@ static int kill_something_info(int sig, struct siginfo *info, int pid)struct task_struct * p;for_each_process (p) { -if (p->pid >1 && !same_thread_group (p, current)) { +if (p->pid >1 && !same_thread_group (p, current) && +task_in_pid_ns (p, current->nsproxy->pid_ns)) {int err =group_send_sig_info (sig, info, p); ++count;if (err !=
-EPERM)
Signature · 1 line
Daniel Hokka Zakrisson
Quoted 15 lines · Daniel Hokka Zakrisson
Quoted 6 lines · Pavel Emelyanov
Quoted 4 lines · Daniel Hokka Zakrisson
While moving Linux-VServer to using pid namespaces, I noticed that kill(-1) from inside a pid namespace is currently signalling every process in the entire system, including processes that are otherwise unreachable from the current process.
This is not a "news" actually, buy anyway - thanks :)
And yet nobody's fixed it... Kind of a critical thing, if you actually want to use them, since most distribution's rc-scripts do a
kill(-1, SIGTERM), followed bykill(-1, SIGKILL)when halting (which, needless to say, would be very bad).Quoted 2 lines
Quoted 2 lines
This patch fixes it by making sure that only processes which are in the same pid namespace as current get signalled.
Quoted 8 lines
Quoted 5 lines
This is to be done, indeed, but I do not like the proposed implementation, since you have to walk all the tasks in the system (under
tasklist_lock, by the way) to search for a couple of interesting ones. Better look at howzap_pid_ns_processesworks (by the way - I saw some patch doing so some time ago).The way
zap_pid_ns_processesdoes it is worse, since it signals every thread in the namespace rather than every thread group. So either we walk
It's questionable whether there are more "threads in a pid namespace" than "processes in a system".
E.g. on my notebook there are ~110 processes and ~150 threads. So having this setup launched in 10 containers you'll have to walk 1100 tasks, while zap_pid_ns_processes only 150 ;)
Some real-life example with containers: on one of our servers with 10 containers serving as git repo, bulding system and some other stuff there are ~200 process totally and ~20 threads in each container. See?
I tend to believe that walking threads in a container is cheaper then walking processes in a system...
Quoted 2 lines
the global tasklist, or we create a per-namespace one. Is that what we want?
We want to kill all tasks in current pid namespace. There are variants of how to do this. You particular implementation of handling this case seems poor to me for the reasons described above.
Quoted 82 lines
Quoted 81 lines
Quoted 81 lines
Signed-off-by Daniel Hokka Zakrisson <[email protected]>include/linux/pid_namespace.h +2
-0@@ -40,6 +40,8 @@ static inline struct pid_namespace *get_pid_ns(struct pid_namespace *ns)extern struct pid_namespace *copy_pid_ns (unsigned long flags,struct pid_namespace *ns);extern void free_pid_ns (struct kref *kref);extern void zap_pid_ns_processes (struct pid_namespace *pid_ns); +extern int task_in_pid_ns (struct task_struct *tsk, +struct pid_namespace *pid_ns);static inline void put_pid_ns(struct pid_namespace *ns) {include/linux/pid_namespace.h +6
-0@@ -72,6 +74,12 @@ static inline void zap_pid_ns_processes(struct pid_namespace *ns) {BUG (); } + +static inline int task_in_pid_ns (struct task_struct *tsk, +struct pid_namespace *ns) +{ +return 1 ; +}#endif /* CONFIG_PID_NS */ static inline struct
pid_namespace*task_active_pid_ns(structtask_struct*tsk)kernel/pid_namespace.c +20
-0@@ -188,6 +188,26 @@ void zap_pid_ns_processes(struct pid_namespace *pid_ns)return ; } +/* +* Checks whether tsk has a pid in the pid namespace ns. +* Must be called with tasklist_lock read-locked or under rcu_read_lock() +*/ +int task_in_pid_ns (struct task_struct *tsk,struct pid_namespace *ns) +{ +struct pid *pid =task_pid (tsk); + +if (!pid) +return 0 ; + +if (pid->level < ns->level) +return 0 ; + +if (pid->numbers[ns->level].ns != ns) +return 0 ; + +return 1 ; +} +static __init int pid_namespaces_init (void ){ pid_ns_cachep = KMEM_CACHE(pid_namespace, SLAB_PANIC);kernel/signal.c +2
-1@@ -1145,7 +1145,8 @@ static int kill_something_info(int sig, struct siginfo *info, int pid)struct task_struct * p;for_each_process (p) { -if (p->pid >1 && !same_thread_group (p, current)) { +if (p->pid >1 && !same_thread_group (p, current) && +task_in_pid_ns (p, current->nsproxy->pid_ns)) {int err =group_send_sig_info (sig, info, p); ++count;if (err !=
-EPERM)
Quoted 15 lines · Daniel Hokka Zakrisson
Quoted 6 lines · Pavel Emelyanov
Quoted 4 lines · Daniel Hokka Zakrisson
While moving Linux-VServer to using pid namespaces, I noticed that kill(-1) from inside a pid namespace is currently signalling every process in the entire system, including processes that are otherwise unreachable from the current process.
This is not a "news" actually, buy anyway - thanks :)
And yet nobody's fixed it... Kind of a critical thing, if you actually want to use them, since most distribution's rc-scripts do a
kill(-1, SIGTERM), followed bykill(-1, SIGKILL)when halting (which, needless to say, would be very bad).Quoted 2 lines
Quoted 2 lines
This patch fixes it by making sure that only processes which are in the same pid namespace as current get signalled.
Quoted 8 lines
Quoted 5 lines
This is to be done, indeed, but I do not like the proposed implementation, since you have to walk all the tasks in the system (under
tasklist_lock, by the way) to search for a couple of interesting ones. Better look at howzap_pid_ns_processesworks (by the way - I saw some patch doing so some time ago).The way
zap_pid_ns_processesdoes it is worse, since it signals every thread in the namespace rather than every thread group. So either we walk
It's questionable whether there are more "threads in a pid namespace" than "processes in a system".
E.g. on my notebook there are ~110 processes and ~150 threads. So having this setup launched in 10 containers you'll have to walk 1100 tasks, while zap_pid_ns_processes only 150 ;)
Some real-life example with containers: on one of our servers with 10 containers serving as git repo, bulding system and some other stuff there are ~200 process totally and ~20 threads in each container. See?
I tend to believe that walking threads in a container is cheaper then walking processes in a system...
Quoted 2 lines
the global tasklist, or we create a per-namespace one. Is that what we want?
We want to kill all tasks in current pid namespace. There are variants of how to do this. You particular implementation of handling this case seems poor to me for the reasons described above.
Quoted 82 lines
Quoted 81 lines
Quoted 81 lines
Signed-off-by Daniel Hokka Zakrisson <[email protected]>include/linux/pid_namespace.h +2
-0@@ -40,6 +40,8 @@ static inline struct pid_namespace *get_pid_ns(struct pid_namespace *ns)extern struct pid_namespace *copy_pid_ns (unsigned long flags,struct pid_namespace *ns);extern void free_pid_ns (struct kref *kref);extern void zap_pid_ns_processes (struct pid_namespace *pid_ns); +extern int task_in_pid_ns (struct task_struct *tsk, +struct pid_namespace *pid_ns);static inline void put_pid_ns(struct pid_namespace *ns) {include/linux/pid_namespace.h +6
-0@@ -72,6 +74,12 @@ static inline void zap_pid_ns_processes(struct pid_namespace *ns) {BUG (); } + +static inline int task_in_pid_ns (struct task_struct *tsk, +struct pid_namespace *ns) +{ +return 1 ; +}#endif /* CONFIG_PID_NS */ static inline struct
pid_namespace*task_active_pid_ns(structtask_struct*tsk)kernel/pid_namespace.c +20
-0@@ -188,6 +188,26 @@ void zap_pid_ns_processes(struct pid_namespace *pid_ns)return ; } +/* +* Checks whether tsk has a pid in the pid namespace ns. +* Must be called with tasklist_lock read-locked or under rcu_read_lock() +*/ +int task_in_pid_ns (struct task_struct *tsk,struct pid_namespace *ns) +{ +struct pid *pid =task_pid (tsk); + +if (!pid) +return 0 ; + +if (pid->level < ns->level) +return 0 ; + +if (pid->numbers[ns->level].ns != ns) +return 0 ; + +return 1 ; +} +static __init int pid_namespaces_init (void ){ pid_ns_cachep = KMEM_CACHE(pid_namespace, SLAB_PANIC);kernel/signal.c +2
-1@@ -1145,7 +1145,8 @@ static int kill_something_info(int sig, struct siginfo *info, int pid)struct task_struct * p;for_each_process (p) { -if (p->pid >1 && !same_thread_group (p, current)) { +if (p->pid >1 && !same_thread_group (p, current) && +task_in_pid_ns (p, current->nsproxy->pid_ns)) {int err =group_send_sig_info (sig, info, p); ++count;if (err !=
-EPERM)
Quoted 13 lines · Pavel Emelyanov
Quoted 3 lines · Daniel Hokka Zakrisson
The way
zap_pid_ns_processesdoes it is worse, since it signals every thread in the namespace rather than every thread group. So either we walkIt's questionable whether there are more "threads in a pid namespace" than "processes in a system".
E.g. on my notebook there are ~110 processes and ~150 threads. So having this setup launched in 10 containers you'll have to walk 1100 tasks, while
zap_pid_ns_processesonly 150 ;)
Quoted 6 lines
Some real-life example with containers: on one of our servers with 10 containers serving as git repo, bulding system and some other stuff there are ~200 process totally and ~20 threads in each container. See?
I tend to believe that walking threads in a container is cheaper then walking processes in a system...
kill_something_info() can't walk threads, think about the realtime signals.
Anyway, I think we should change kill_something_info(-1) to use rcu_read_lock() instead of tasklist.
Oleg.
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 1 line · Oleg Nesterov
kill_something_info()can't walk threads, think about the realtime signals.
walking threads is fine delivering signals to non thread group leaders is a problem.
Quoted 2 lines
Anyway, I think we should change
kill_something_info(-1)to usercu_read_lock()instead of tasklist.
Being dense I think the locking implications of a correct implementation are more then we are ready to deal with to fix this bug. Although I remember discussing it and seeing something reasonable.
Eric
Quoted 1 line · Oleg Nesterov
kill_something_info()can't walk threads, think about the realtime signals.
walking threads is fine delivering signals to non thread group leaders is a problem.
Quoted 2 lines
Anyway, I think we should change
kill_something_info(-1)to usercu_read_lock()instead of tasklist.
Being dense I think the locking implications of a correct implementation are more then we are ready to deal with to fix this bug. Although I remember discussing it and seeing something reasonable.
Eric
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 13 lines · Pavel Emelyanov
Quoted 3 lines · Daniel Hokka Zakrisson
The way
zap_pid_ns_processesdoes it is worse, since it signals every thread in the namespace rather than every thread group. So either we walkIt's questionable whether there are more "threads in a pid namespace" than "processes in a system".
E.g. on my notebook there are ~110 processes and ~150 threads. So having this setup launched in 10 containers you'll have to walk 1100 tasks, while
zap_pid_ns_processesonly 150 ;)
Quoted 6 lines
Some real-life example with containers: on one of our servers with 10 containers serving as git repo, bulding system and some other stuff there are ~200 process totally and ~20 threads in each container. See?
I tend to believe that walking threads in a container is cheaper then walking processes in a system...
kill_something_info() can't walk threads, think about the realtime signals.
Anyway, I think we should change kill_something_info(-1) to use rcu_read_lock() instead of tasklist.
Oleg.
Quoted 23 lines · Daniel Hokka Zakrisson
Quoted 7 lines · Pavel Emelyanov
Quoted 4 lines · Daniel Hokka Zakrisson
While moving Linux-VServer to using pid namespaces, I noticed that kill(-1) from inside a pid namespace is currently signalling every process in the entire system, including processes that are otherwise unreachable from the current process.
This is not a "news" actually, buy anyway - thanks :)
And yet nobody's fixed it... Kind of a critical thing, if you actually want to use them, since most distribution's rc-scripts do a
kill(-1, SIGTERM), followed bykill(-1, SIGKILL)when halting (which, needless to say, would be very bad).Quoted 8 lines
Quoted 2 lines
This patch fixes it by making sure that only processes which are in the same pid namespace as current get signalled.
This is to be done, indeed, but I do not like the proposed implementation, since you have to walk all the tasks in the system (under
tasklist_lock, by the way) to search for a couple of interesting ones. Better look at howzap_pid_ns_processesworks (by the way - I saw some patch doing so some time ago).
Quoted 4 lines
The way
zap_pid_ns_processesdoes it is worse, since it signals every thread in the namespace rather than every thread group. So either we walk the global tasklist, or we create a per-namespace one. Is that what we want?
Can you please introduce kill_pidns_info and have both kill_something_info and zap_pid_ns_processes call this common function?
We want to walk the set of all pids in a pid namespace. /proc does this and it is the recommended idiom. If walking all of the pids in a pid namespace is not fast enough we can accelerate that.
You are correct signalling every thread in a namespace is worse, in fact it is semantically incorrect. zap_pid_ns_processes gets away
with it because it is sending SIGKILL. Therefore kill_pidns_info
should skip sending a signal to every task that is not the thread_group_leader.
We need to hold the tasklist_lock to prevent new processes from joining the list of all processes. Otherwise we could run the code under the rcu_read_lock.
Eric
Quoted 23 lines · Daniel Hokka Zakrisson
Quoted 7 lines · Pavel Emelyanov
Quoted 4 lines · Daniel Hokka Zakrisson
While moving Linux-VServer to using pid namespaces, I noticed that kill(-1) from inside a pid namespace is currently signalling every process in the entire system, including processes that are otherwise unreachable from the current process.
This is not a "news" actually, buy anyway - thanks :)
And yet nobody's fixed it... Kind of a critical thing, if you actually want to use them, since most distribution's rc-scripts do a
kill(-1, SIGTERM), followed bykill(-1, SIGKILL)when halting (which, needless to say, would be very bad).Quoted 8 lines
Quoted 2 lines
This patch fixes it by making sure that only processes which are in the same pid namespace as current get signalled.
This is to be done, indeed, but I do not like the proposed implementation, since you have to walk all the tasks in the system (under
tasklist_lock, by the way) to search for a couple of interesting ones. Better look at howzap_pid_ns_processesworks (by the way - I saw some patch doing so some time ago).
Quoted 4 lines
The way
zap_pid_ns_processesdoes it is worse, since it signals every thread in the namespace rather than every thread group. So either we walk the global tasklist, or we create a per-namespace one. Is that what we want?
Can you please introduce kill_pidns_info and have both kill_something_info and zap_pid_ns_processes call this common function?
We want to walk the set of all pids in a pid namespace. /proc does this and it is the recommended idiom. If walking all of the pids in a pid namespace is not fast enough we can accelerate that.
You are correct signalling every thread in a namespace is worse, in fact it is semantically incorrect. zap_pid_ns_processes gets away
with it because it is sending SIGKILL. Therefore kill_pidns_info
should skip sending a signal to every task that is not the thread_group_leader.
We need to hold the tasklist_lock to prevent new processes from joining the list of all processes. Otherwise we could run the code under the rcu_read_lock.
Eric
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 34 lines · Eric W. Biederman
Quoted 26 lines · Daniel Hokka Zakrisson
Quoted 7 lines · Pavel Emelyanov
Quoted 4 lines · Daniel Hokka Zakrisson
While moving Linux-VServer to using pid namespaces, I noticed that kill(-1) from inside a pid namespace is currently signalling every process in the entire system, including processes that are otherwise unreachable from the current process.
This is not a "news" actually, buy anyway - thanks :)
And yet nobody's fixed it... Kind of a critical thing, if you actually want to use them, since most distribution's rc-scripts do a
kill(-1, SIGTERM), followed bykill(-1, SIGKILL)when halting (which, needless to say, would be very bad).Quoted 11 lines
Quoted 2 lines
This patch fixes it by making sure that only processes which are in the same pid namespace as current get signalled.
This is to be done, indeed, but I do not like the proposed implementation,
since you have to walk all the tasks in the system (undertasklist_lock,
by the way) to search for a couple of interesting ones. Better look at howzap_pid_ns_processesworks (by the way - I saw some patch doing so some time ago).Quoted 5 lines
The way
zap_pid_ns_processesdoes it is worse, since it signals every thread in the namespace rather than every thread group. So either we walk
the global tasklist, or we create a per-namespace one. Is that what we want?
Quoted 3 lines
Can you please introduce
kill_pidns_infoand have bothkill_something_infoandzap_pid_ns_processescall this common function?
Looks like you've already done that. :-) (Referring to Sukadev's email.) Is there any reason we don't just merge that patch?
Quoted 15 lines
We want to walk the set of all pids in a pid namespace. /proc does this and it is the recommended idiom. If walking all of the pids in a pid namespace is not fast enough we can accelerate that.
You are correct signalling every thread in a namespace is worse, in fact it is semantically incorrect.
zap_pid_ns_processesgets away
with it because it is sending SIGKILL. Thereforekill_pidns_info
should skip sending a signal to every task that is not thethread_group_leader.We need to hold the
tasklist_lockto prevent new processes from joining the list of all processes. Otherwise we could run the code under thercu_read_lock.Eric
Signature · 1 line
Daniel Hokka Zakrisson
Quoted 34 lines · Eric W. Biederman
Quoted 26 lines · Daniel Hokka Zakrisson
Quoted 7 lines · Pavel Emelyanov
Quoted 4 lines · Daniel Hokka Zakrisson
While moving Linux-VServer to using pid namespaces, I noticed that kill(-1) from inside a pid namespace is currently signalling every process in the entire system, including processes that are otherwise unreachable from the current process.
This is not a "news" actually, buy anyway - thanks :)
And yet nobody's fixed it... Kind of a critical thing, if you actually want to use them, since most distribution's rc-scripts do a
kill(-1, SIGTERM), followed bykill(-1, SIGKILL)when halting (which, needless to say, would be very bad).Quoted 11 lines
Quoted 2 lines
This patch fixes it by making sure that only processes which are in the same pid namespace as current get signalled.
This is to be done, indeed, but I do not like the proposed implementation,
since you have to walk all the tasks in the system (undertasklist_lock,
by the way) to search for a couple of interesting ones. Better look at howzap_pid_ns_processesworks (by the way - I saw some patch doing so some time ago).Quoted 5 lines
The way
zap_pid_ns_processesdoes it is worse, since it signals every thread in the namespace rather than every thread group. So either we walk
the global tasklist, or we create a per-namespace one. Is that what we want?
Quoted 3 lines
Can you please introduce
kill_pidns_infoand have bothkill_something_infoandzap_pid_ns_processescall this common function?
Looks like you've already done that. :-) (Referring to Sukadev's email.) Is there any reason we don't just merge that patch?
Quoted 15 lines
We want to walk the set of all pids in a pid namespace. /proc does this and it is the recommended idiom. If walking all of the pids in a pid namespace is not fast enough we can accelerate that.
You are correct signalling every thread in a namespace is worse, in fact it is semantically incorrect.
zap_pid_ns_processesgets away
with it because it is sending SIGKILL. Thereforekill_pidns_info
should skip sending a signal to every task that is not thethread_group_leader.We need to hold the
tasklist_lockto prevent new processes from joining the list of all processes. Otherwise we could run the code under thercu_read_lock.Eric
Signature · 1 line
Daniel Hokka Zakrisson
Quoted 2 lines · Daniel Hokka Zakrisson
Looks like you've already done that. :-) (Referring to Sukadev's email.) Is there any reason we don't just merge that patch?
I knew I had done something like that. Sure let's revive the patch and send
it. I don't know why it got lost the first time.
Eric
Quoted 2 lines · Daniel Hokka Zakrisson
Looks like you've already done that. :-) (Referring to Sukadev's email.) Is there any reason we don't just merge that patch?
I knew I had done something like that. Sure let's revive the patch and send
it. I don't know why it got lost the first time.
Eric
Daniel Hokka Zakrisson [[email protected]] wrote:
| While moving Linux-VServer to using pid namespaces, I noticed that | kill(-1) from inside a pid namespace is currently signalling every | process in the entire system, including processes that are otherwise | unreachable from the current process. | | This patch fixes it by making sure that only processes which are in | the same pid namespace as current get signalled. | | Signed-off-by: Daniel Hokka Zakrisson <[email protected]> | | diff --git a/include/linux/pid_namespace.h b/include/linux/pid_namespace.h | index caff528..4cf41bd 100644 | --- a/include/linux/pid_namespace.h | +++ b/include/linux/pid_namespace.h | @@ -40,6 +40,8 @@ static inline struct pid_namespace *get_pid_ns(struct | pid_namespace *ns) | extern struct pid_namespace *copy_pid_ns(unsigned long flags, struct | pid_namespace *ns); | extern void free_pid_ns(struct kref *kref); | extern void zap_pid_ns_processes(struct pid_namespace *pid_ns); | +extern int task_in_pid_ns(struct task_struct *tsk, | + struct pid_namespace *pid_ns); | | static inline void put_pid_ns(struct pid_namespace *ns) | { | @@ -72,6 +74,12 @@ static inline void zap_pid_ns_processes(struct | pid_namespace *ns) | { | BUG(); | } | + | +static inline int task_in_pid_ns(struct task_struct *tsk, | + struct pid_namespace *ns) | +{ | + return 1; | +} | #endif /* CONFIG_PID_NS */ | | static inline struct pid_namespace *task_active_pid_ns(struct | task_struct *tsk) | diff --git a/kernel/pid_namespace.c b/kernel/pid_namespace.c | index 98702b4..3e71011 100644 | --- a/kernel/pid_namespace.c | +++ b/kernel/pid_namespace.c | @@ -188,6 +188,26 @@ void zap_pid_ns_processes(struct pid_namespace *pid_ns) | return; | } | | +/* | + * Checks whether tsk has a pid in the pid namespace ns. | + * Must be called with tasklist_lock read-locked or under rcu_read_lock() | + */ | +int task_in_pid_ns(struct task_struct *tsk, struct pid_namespace *ns) | +{ | + struct pid *pid = task_pid(tsk); | + | + if (!pid) | + return 0; | + | + if (pid->level < ns->level) | + return 0;
ns can be NULL if tsk is exiting.
Like Pavel said, we had couple of attempts to fix the larger problem of signal semantics in containers but did not have a consensus on handling blocked/unhandled signals to container-init.
It would still be good to fix this "kill -1" problem.
Eric had a slightly optimized interface, 'pid_in_pid_ns()' in following patchset. Maybe we could use that ?
https://lists.linux-foundation.org/pipermail/containers/2007-December/009174.html
| +
| + if (pid->numbers[ns->level].ns != ns)
| + return 0;
| +
| + return 1;
| +}
| +
| static __init int pid_namespaces_init(void)
| {
| pid_ns_cachep = KMEM_CACHE(pid_namespace, SLAB_PANIC);
| diff --git a/kernel/signal.c b/kernel/signal.c
| index 6c0958e..93713a5 100644
| --- a/kernel/signal.c
| +++ b/kernel/signal.c
| @@ -1145,7 +1145,8 @@ static int kill_something_info(int sig, struct
| siginfo *info, int pid)
| struct task_struct * p;
|
| for_each_process(p) {
| - if (p->pid > 1 && !same_thread_group(p, current)) {
| + if (p->pid > 1 && !same_thread_group(p, current) &&
| + task_in_pid_ns(p, current->nsproxy->pid_ns)) {
| int err = group_send_sig_info(sig, info, p);
| ++count;
| if (err != -EPERM)
| --
| 1.5.5.1
| _______________________________________________
| Containers mailing list
| [email protected]
| https://lists.linux-foundation.org/mailman/listinfo/containersA message that is not in the archive (sent off-list or not yet received): 20080717181313.GA2289-r/[email protected]
Quoted 66 lines · sukadev-r/[email protected] wrote:
Daniel Hokka Zakrisson [[email protected]] wrote:
| While moving Linux-VServer to using pid namespaces, I noticed that | kill(-1) from inside a pid namespace is currently signalling every | process in the entire system, including processes that are otherwise | unreachable from the current process. | | This patch fixes it by making sure that only processes which are in | the same pid namespace as current get signalled. | | Signed-off-by: Daniel Hokka Zakrisson <[email protected]> | | diff --git a/include/linux/pid_namespace.h b/include/linux/pid_namespace.h | index caff528..4cf41bd 100644 | --- a/include/linux/pid_namespace.h | +++ b/include/linux/pid_namespace.h | @@ -40,6 +40,8 @@ static inline struct pid_namespace *get_pid_ns(struct | pid_namespace *ns) | extern struct pid_namespace *copy_pid_ns(unsigned long flags, struct | pid_namespace *ns); | extern void free_pid_ns(struct kref *kref); | extern void zap_pid_ns_processes(struct pid_namespace *pid_ns); | +extern int task_in_pid_ns(struct task_struct *tsk, | + struct pid_namespace *pid_ns); | | static inline void put_pid_ns(struct pid_namespace *ns) | { | @@ -72,6 +74,12 @@ static inline void zap_pid_ns_processes(struct | pid_namespace *ns) | { | BUG(); | } | + | +static inline int task_in_pid_ns(struct task_struct *tsk, | + struct pid_namespace *ns) | +{ | + return 1; | +} | #endif /* CONFIG_PID_NS */ | | static inline struct pid_namespace *task_active_pid_ns(struct | task_struct *tsk) | diff --git a/kernel/pid_namespace.c b/kernel/pid_namespace.c | index 98702b4..3e71011 100644 | --- a/kernel/pid_namespace.c | +++ b/kernel/pid_namespace.c | @@ -188,6 +188,26 @@ void zap_pid_ns_processes(struct pid_namespace *pid_ns) | return; | } | | +/* | + * Checks whether tsk has a pid in the pid namespace ns. | + * Must be called with tasklist_lock read-locked or under rcu_read_lock() | + */ | +int task_in_pid_ns(struct task_struct *tsk, struct pid_namespace *ns) | +{ | + struct pid *pid = task_pid(tsk); | + | + if (!pid) | + return 0; | + | + if (pid->level < ns->level) | + return 0;
Quoted 1 line
ns can be NULL if tsk is exiting.
ns is from current, and this is currently only called from kill_something_info, so it should not be exiting in this path.
Quoted 5 lines
Like Pavel said, we had couple of attempts to fix the larger problem of signal semantics in containers but did not have a consensus on handling blocked/unhandled signals to container-init.
It would still be good to fix this "kill -1" problem.
It is a separate issue, so, yeah.
Quoted 4 lines
Eric had a slightly optimized interface, '
pid_in_pid_ns()' in following patchset. Maybe we could use that ?https://lists.linux-foundation.org/pipermail/containers/2007-December/009174.html
See my response to Eric. I think that patch looks good... (Well, nr could be set to 2 initially, to avoid the nr <= 1 check.)
Quoted 27 lines
| +
| + if (pid->numbers[ns->level].ns != ns)
| + return 0;
| +
| + return 1;
| +}
| +
| static __init int pid_namespaces_init(void)
| {
| pid_ns_cachep = KMEM_CACHE(pid_namespace, SLAB_PANIC);
| diff --git a/kernel/signal.c b/kernel/signal.c
| index 6c0958e..93713a5 100644
| --- a/kernel/signal.c
| +++ b/kernel/signal.c
| @@ -1145,7 +1145,8 @@ static int kill_something_info(int sig, struct
| siginfo *info, int pid)
| struct task_struct * p;
|
| for_each_process(p) {
| - if (p->pid > 1 && !same_thread_group(p, current)) {
| + if (p->pid > 1 && !same_thread_group(p, current) &&
| + task_in_pid_ns(p, current->nsproxy->pid_ns)) {
| int err = group_send_sig_info(sig, info, p);
| ++count;
| if (err != -EPERM)
| --
| 1.5.5.1Signature · 1 line
Daniel Hokka Zakrisson
Quoted 5 lines · sukadev-r/[email protected] writes:
Like Pavel said, we had couple of attempts to fix the larger problem of signal semantics in containers but did not have a consensus on handling blocked/unhandled signals to container-init.
Oh. I thought we were pretty close then I or somebody ran out of steam.
Quoted 6 lines
It would still be good to fix this "kill -1" problem.
Eric had a slightly optimized interface, '
pid_in_pid_ns()' in following patchset. Maybe we could use that ?https://lists.linux-foundation.org/pipermail/containers/2007-December/009174.html
Eric
Quoted 66 lines · [email protected] wrote:
Daniel Hokka Zakrisson [[email protected]] wrote:
| While moving Linux-VServer to using pid namespaces, I noticed that | kill(-1) from inside a pid namespace is currently signalling every | process in the entire system, including processes that are otherwise | unreachable from the current process. | | This patch fixes it by making sure that only processes which are in | the same pid namespace as current get signalled. | | Signed-off-by: Daniel Hokka Zakrisson <[email protected]> | | diff --git a/include/linux/pid_namespace.h b/include/linux/pid_namespace.h | index caff528..4cf41bd 100644 | --- a/include/linux/pid_namespace.h | +++ b/include/linux/pid_namespace.h | @@ -40,6 +40,8 @@ static inline struct pid_namespace *get_pid_ns(struct | pid_namespace *ns) | extern struct pid_namespace *copy_pid_ns(unsigned long flags, struct | pid_namespace *ns); | extern void free_pid_ns(struct kref *kref); | extern void zap_pid_ns_processes(struct pid_namespace *pid_ns); | +extern int task_in_pid_ns(struct task_struct *tsk, | + struct pid_namespace *pid_ns); | | static inline void put_pid_ns(struct pid_namespace *ns) | { | @@ -72,6 +74,12 @@ static inline void zap_pid_ns_processes(struct | pid_namespace *ns) | { | BUG(); | } | + | +static inline int task_in_pid_ns(struct task_struct *tsk, | + struct pid_namespace *ns) | +{ | + return 1; | +} | #endif /* CONFIG_PID_NS */ | | static inline struct pid_namespace *task_active_pid_ns(struct | task_struct *tsk) | diff --git a/kernel/pid_namespace.c b/kernel/pid_namespace.c | index 98702b4..3e71011 100644 | --- a/kernel/pid_namespace.c | +++ b/kernel/pid_namespace.c | @@ -188,6 +188,26 @@ void zap_pid_ns_processes(struct pid_namespace *pid_ns) | return; | } | | +/* | + * Checks whether tsk has a pid in the pid namespace ns. | + * Must be called with tasklist_lock read-locked or under rcu_read_lock() | + */ | +int task_in_pid_ns(struct task_struct *tsk, struct pid_namespace *ns) | +{ | + struct pid *pid = task_pid(tsk); | + | + if (!pid) | + return 0; | + | + if (pid->level < ns->level) | + return 0;
Quoted 1 line
ns can be NULL if tsk is exiting.
ns is from current, and this is currently only called from kill_something_info, so it should not be exiting in this path.
Quoted 5 lines
Like Pavel said, we had couple of attempts to fix the larger problem of signal semantics in containers but did not have a consensus on handling blocked/unhandled signals to container-init.
It would still be good to fix this "kill -1" problem.
It is a separate issue, so, yeah.
Quoted 4 lines
Eric had a slightly optimized interface, '
pid_in_pid_ns()' in following patchset. Maybe we could use that ?https://lists.linux-foundation.org/pipermail/containers/2007-December/009174.html
See my response to Eric. I think that patch looks good... (Well, nr could be set to 2 initially, to avoid the nr <= 1 check.)
Quoted 27 lines
| +
| + if (pid->numbers[ns->level].ns != ns)
| + return 0;
| +
| + return 1;
| +}
| +
| static __init int pid_namespaces_init(void)
| {
| pid_ns_cachep = KMEM_CACHE(pid_namespace, SLAB_PANIC);
| diff --git a/kernel/signal.c b/kernel/signal.c
| index 6c0958e..93713a5 100644
| --- a/kernel/signal.c
| +++ b/kernel/signal.c
| @@ -1145,7 +1145,8 @@ static int kill_something_info(int sig, struct
| siginfo *info, int pid)
| struct task_struct * p;
|
| for_each_process(p) {
| - if (p->pid > 1 && !same_thread_group(p, current)) {
| + if (p->pid > 1 && !same_thread_group(p, current) &&
| + task_in_pid_ns(p, current->nsproxy->pid_ns)) {
| int err = group_send_sig_info(sig, info, p);
| ++count;
| if (err != -EPERM)
| --
| 1.5.5.1Signature · 1 line
Daniel Hokka Zakrisson
Quoted 5 lines · [email protected] writes:
Like Pavel said, we had couple of attempts to fix the larger problem of signal semantics in containers but did not have a consensus on handling blocked/unhandled signals to container-init.
Oh. I thought we were pretty close then I or somebody ran out of steam.
Quoted 6 lines
It would still be good to fix this "kill -1" problem.
Eric had a slightly optimized interface, '
pid_in_pid_ns()' in following patchset. Maybe we could use that ?https://lists.linux-foundation.org/pipermail/containers/2007-December/009174.html
Eric
Quoted 34 lines · Daniel Hokka Zakrisson
kernel/pid_namespace.c +16
-0+int task_in_pid_ns (struct task_struct *tsk,struct pid_namespace *ns) +{ +struct pid *pid =task_pid (tsk); + +if (!pid) +return 0 ; + +if (pid->level < ns->level) +return 0 ; + +if (pid->numbers[ns->level].ns != ns) +return 0 ; + +return 1 ; +} +static __init int pid_namespaces_init (void ) { pid_ns_cachep =KMEM_CACHE (pid_namespace,SLAB_PANIC );kernel/signal.c +2
-1@@ -1145,7 +1145,8 @@ static int kill_something_info(int sig, struct siginfo *info, int pid)struct task_struct * p;for_each_process (p) { -if (p->pid >1 && !same_thread_group (p, current)) { +if (p->pid >1 && !same_thread_group (p, current) && +task_in_pid_ns (p, current->nsproxy->pid_ns)) {int err =group_send_sig_info (sig, info, p); ++count;
Quoted 1 line
if (err !=
-EPERM)
Do we really need all these complications? Afaics, we can make a simpler patch,
--- kernel/signal.c
+++ kernel/signal.c
@@ -1136,7 +1136,7 @@ static int kill_something_info(int sig,
struct task_struct * p;
for_each_process(p) {
- if (p->pid > 1 && !same_thread_group(p, current)) {
+ if (task_pid_vnr(p) > 1 && !same_thread_group(p, current)) {
int err = group_send_sig_info(sig, info, p);
++count;
if (err != -EPERM)task_pid_vnr(p) returns 0 if "p" is not visible from the current's namespace. "> 1" ensures we don't kill the child reaper as well.
No?
Oleg.
Quoted 53 lines · Oleg Nesterov
Quoted 34 lines · Daniel Hokka Zakrisson
kernel/pid_namespace.c +16
-0+int task_in_pid_ns (struct task_struct *tsk,struct pid_namespace *ns) +{ +struct pid *pid =task_pid (tsk); + +if (!pid) +return 0 ; + +if (pid->level < ns->level) +return 0 ; + +if (pid->numbers[ns->level].ns != ns) +return 0 ; + +return 1 ; +} +static __init int pid_namespaces_init (void ) { pid_ns_cachep =KMEM_CACHE (pid_namespace,SLAB_PANIC );kernel/signal.c +2
-1@@ -1145,7 +1145,8 @@ static int kill_something_info(int sig, struct siginfo *info, int pid)struct task_struct * p;for_each_process (p) { -if (p->pid >1 && !same_thread_group (p, current)) { +if (p->pid >1 && !same_thread_group (p, current) && +task_in_pid_ns (p, current->nsproxy->pid_ns)) {int err =group_send_sig_info (sig, info, p); ++count;Quoted 1 line
if (err !=
-EPERM)Do we really need all these complications? Afaics, we can make a simpler patch,
--- kernel/signal.c +++ kernel/signal.c @@ -1136,7 +1136,7 @@ static int kill_something_info(int sig, struct task_struct * p; for_each_process(p) { - if (p->pid > 1 && !same_thread_group(p, current)) { + if (task_pid_vnr(p) > 1 && !same_thread_group(p, current)) { int err = group_send_sig_info(sig, info, p); ++count; if (err != -EPERM)
Quoted 6 lines
task_pid_vnr(p)returns 0 if "p" is not visible from the current's namespace. "> 1" ensures we don't kill the child reaper as well.No?
Oleg.
You are absolutely right, that is sufficient and much cleaner.
Signature · 1 line
Daniel Hokka Zakrisson