OpenVZ Forum - RDF feed
type: channel
items: [RFC][PATCH 0/15] Pid namespaces
items: [PATCH 1/15] Move exit_task_namespaces()
items: [PATCH 2/15] Introduce MS_KERNMOUNT flag
items: [PATCH 3/15] kern_siginfo helper
items: [PATCH 4/15] Make proc_flust_task() flush entries from multiple proc trees
items: [PATCH 5/15] Introduce struct upid
items: [PATCH 6/15] Make alloc_pid(), free_pid() and put_pid() work with struct upid
items: [PATCH 7/15] Helpers to obtain pid numbers
items: [PATCH 8/15] Helpers to find the task by its numerical ids
items: [PATCH 9/15] Move alloc_pid() after the namespace is cloned
items: [PATCH 10/15] Make each namespace has its own proc tree
items: [PATCH 11/15] Signal semantics
items: [PATCH 12/15] Miscelaneous stuff for pid namespaces
items: [PATCH 13/15] Clone the pid namespace
items: [PATCH 14/15] Destroy pid namespace on init's death
items: [PATCH 15/15] Hooks over the code to show correct values to user
items: Re: [PATCH 1/15] Move exit_task_namespaces()
items: Re: [PATCH 7/15] Helpers to obtain pid numbers
items: Re: [PATCH 8/15] Helpers to find the task by its numerical ids
items: Re: [RFC][PATCH 0/15] Pid namespaces
items: Re: [PATCH 15/15] Hooks over the code to show correct values to user
items: Re: [PATCH 12/15] Miscelaneous stuff for pid namespaces
items: Re: [PATCH 11/15] Signal semantics
items: Re: [PATCH 9/15] Move alloc_pid() after the namespace is cloned
items: Re: [PATCH 5/15] Introduce struct upid
items: Re: [PATCH 6/15] Make alloc_pid(), free_pid() and put_pid() work with struct upid
items: Re: [PATCH 14/15] Destroy pid namespace on init's death
items: Re: [PATCH 3/15] kern_siginfo helper
items: Re: [PATCH 10/15] Make each namespace has its own proc tree

[RFC][PATCH 0/15] Pid namespaces
type: item
description: (Comment is taken from Sukadev's patchset-v3) A pid namespace is a "view" of a particular set of tasks on the system. They work in a similar way to filesystem namespaces. A file (or a process) can be accessed in multiple namespaces, but it may have a different name in each. In a filesystem, this name might be /etc/passwd in one namespace, but /chroot/etc/passwd in another. For processes, a process may have pid 1234 in one namespace, but be pid 1 in another. This allows new pid namespaces to have basically arbitrary pids, and not have to worry about what pids exist in other namespaces. This is essential for checkpoint/restart where a restarted process's pid might collide with an existing process on the system's pid. In this particular implementation, pid namespaces have a parent-child relationship, just like processes. A process in a pid namespace may see all of the processes in the same namespace, as well as all of the processes in all of the namespaces which are children of its namespace. Processes may not, however, see others which are in their parent's namespace, but not in their own. The same goes for sibling namespaces. This set is based on my patches, I sent before, but it includes some comments and patches that I received from Sukadev. Sukadev, please, add your Acked-by, Signed-off-by or From, to patches you want (everybody is also welcome :) ). The set is based on 2.6.23-rc1-mm1, which already has some preparation patches for pid namespaces. After the review and fixing all the comments, this set will be benchmarked and sent to Andrew for inclusion in -mm tree. Signed-off-by: Pavel Emelyanov <xemul@openvz.org> Signed-off-by: Sukadev Bhattiprolu <sukadev@us.ibm.com>
creator: Pavel Emelianov
date: 2007-07-26T14:45:15-00:00

[PATCH 1/15] Move exit_task_namespaces()
type: item
description: Make task release its namespaces after it has reparented all his children to child_reaper, but before it notifies its parent about its death. The reason to release namespaces after reparenting is that when task exits it may send a signal to its parent (SIGCHLD), but if the parent has already exited its namespaces there will be no way to decide what pid to dever to him - parent can be from different namespace. The reason to release namespace before notifying the parent it that when task sends a SIGCHLD to parent it can call wait() on this taks and release it. But releasing the mnt namespace implies dropping of all the mounts in the mnt namespace and NFS expects the task to have valid sighand pointer. Signed-off-by: Pavel Emelyanov <xemul@openvz.org> --- exit.c | 5 ++++- 1 files changed, 4 insertions(+), 1 deletion(-) diff -upr linux-2.6.23-rc1-mm1.orig/kernel/exit.c linux-2.6.23-rc1-mm1-7/kernel/exit.c --- linux-2.6.23-rc1-mm1.orig/kernel/exit.c 2007-07-26 16:34:45.000000000 +0400 +++ linux-2.6.23-rc1-mm1-7/kernel/exit.c 2007-07-26 16:36:37.000000000 +0400 @@ -788,6 +804,10 @@ static void exit_notify(struct task_stru BUG_ON(!list_empty(&tsk->children)); BUG_ON(!list_empty(&tsk->ptrace_children)); + write_unlock_irq(&tasklist_lock); + exit_task_namespaces(tsk); + write_lock_irq(&tasklist_lock); + /* * Check to see if any process groups have become orphaned * as a result of our exiting, and if they have any stopped @@ -999,7 +1021,6 @@ fastcall NORET_TYPE void do_exit(long co tsk->exit_code = code; proc_exit_connector(tsk); - exit_task_namespaces(tsk); exit_notify(tsk); #ifdef CONFIG_NUMA mpol_free(tsk->mempolicy);
creator: Pavel Emelianov
date: 2007-07-26T14:46:28-00:00

[PATCH 2/15] Introduce MS_KERNMOUNT flag
type: item
description: This flag tells the .get_sb callback that this is a kern_mount() call so that it can trust *data pointer to be valid in-kernel one. Running a few steps forward - this will be needed for proc to create the superblock and store a valid pid namespace on it during the namespace creation. The reason, why the namespace cannot live without proc mount is described in the appropriate patch. Signed-off-by: Pavel Emelyanov <xemul@openvz.org> --- fs/namespace.c | 3 ++- fs/super.c | 6 +++--- include/linux/fs.h | 4 +++- 3 files changed, 8 insertions(+), 5 deletions(-) diff -upr linux-2.6.23-rc1-mm1.orig/fs/namespace.c linux-2.6.23-rc1-mm1-7/fs/namespace.c --- linux-2.6.23-rc1-mm1.orig/fs/namespace.c 2007-07-26 16:34:45.000000000 +0400 +++ linux-2.6.23-rc1-mm1-7/fs/namespace.c 2007-07-26 16:36:36.000000000 +0400 @@ -1579,7 +1579,8 @@ long do_mount(char *dev_name, char *dir_ mnt_flags |= MNT_NOMNT; flags &= ~(MS_NOSUID | MS_NOEXEC | MS_NODEV | MS_ACTIVE | - MS_NOATIME | MS_NODIRATIME | MS_RELATIME | MS_NOMNT); + MS_NOATIME | MS_NODIRATIME | MS_RELATIME | + MS_NOMNT | MS_KERNMOUNT); /* ... and get the mountpoint */ retval = path_lookup(dir_name, LOOKUP_FOLLOW, &nd); diff -upr linux-2.6.23-rc1-mm1.orig/fs/super.c linux-2.6.23-rc1-mm1-7/fs/super.c --- linux-2.6.23-rc1-mm1.orig/fs/super.c 2007-07-26 16:34:45.000000000 +0400 +++ linux-2.6.23-rc1-mm1-7/fs/super.c 2007-07-26 16:36:36.000000000 +0400 @@ -944,9 +944,9 @@ do_kern_mount(const char *fstype, int fl return mnt; } -struct vfsmount *kern_mount(struct file_system_type *type) +struct vfsmount *kern_mount_data(struct file_system_type *type, void *data) { - return vfs_kern_mount(type, 0, type->name, NULL); + return vfs_kern_mount(type, MS_KERNMOUNT, type->name, data); } -EXPORT_SYMBOL(kern_mount); +EXPORT_SYMBOL_GPL(kern_mount_data); diff -upr linux-2.6.23-rc1-mm1.orig/include/linux/fs.h linux-2.6.23-rc1-mm1-7/include/linux/fs.h --- linux-2.6.23-rc1-mm1.orig/include/linux/fs.h 2007-07-26 16:34:45.000000000 +0400 +++ linux-2.6.23-rc1-m…
creator: Pavel Emelianov
date: 2007-07-26T14:47:23-00:00

[PATCH 3/15] kern_siginfo helper
type: item
description: From: Sukadev Bhattiprolu <sukadev@us.ibm.com> get_signal_to_deliver() checks to ensure that /sbin/init does not receive any unwanted signals. With implementation of multiple pid namespaces, we need to extend this check for all "container-init" processes (processes with pid == 1 in the pid namespace) IOW, A container-init process, must not receive unwanted signals from within the container. However, the container-init must receive and honor any signals it receives from an ancestor pid namespace (i.e it must appear like a normal process in its parent namespace). i.e for correct processing of the signal, either: - the recepient of the signal (in get_signal_to_deliver()) must have some information (such as pid namespace) of the sender - or the sender of the signal (in send_signal()) should know whether the signal is an "unwanted" signal from the recepient's POV. This patch provides a simple mechanism, to pass additional information from the sender to the recepient (from send_signal() to get_signal_to_deliver()). This patch is just a helper and a follow-on patch will use this infrastructure to actually pass in information about the sender. Implementation note: Most signal interfaces in the kernel operate on 'siginfo_t' data structure which is user-visible and cannot be easily modified/ extended. So this patch defines a wrapper, 'struct kern_siginfo', around the 'siginfo_t' and allows extending this wrapper for future needs. TODO: This is more an exploratory patch and modifies only interfaces necessary to implement correct signal semantics in pid namespaces. If the approach is feasible, we could consistently use 'kern_siginfo' in other signal interfaces and possibly in 'struct sigqueue'. We could modify dequeue_signal() to directly work with 'kern_siginfo' and remove dequeue_signal_kern_info(). Signed-off-by: Sukadev Bhattiprolu <sukadev@us.ibm.com> --- include/linux/signal.h | 7 +++++++ kernel/signal.c | 38 +++++++++++++++++++++++++++++++------- 2 files changed, 38 inse…
creator: Pavel Emelianov
date: 2007-07-26T14:48:15-00:00

[PATCH 4/15] Make proc_flust_task() flush entries from multiple proc trees
type: item
description: Since a task will appear in more than one proc tree we need to shrink many trees. For this case we pass the struct pid to proc_flush_task() and shrink the mounts of all the namespaces this pid belongs to. The NULL passed to it means that only global mount is to be flushed. This is a preparation for a real flushing patch. Signed-off-by: Pavel Emelyanov <xemul@openvz.org> --- fs/proc/base.c | 18 +++++++++++++++--- include/linux/proc_fs.h | 6 ++++-- kernel/exit.c | 2 +- 3 files changed, 20 insertions(+), 6 deletions(-) diff -upr linux-2.6.23-rc1-mm1.orig/fs/proc/base.c linux-2.6.23-rc1-mm1-7/fs/proc/base.c --- linux-2.6.23-rc1-mm1.orig/fs/proc/base.c 2007-07-26 16:34:45.000000000 +0400 +++ linux-2.6.23-rc1-mm1-7/fs/proc/base.c 2007-07-26 16:36:37.000000000 +0400 @@ -74,6 +74,7 @@ #include <linux/nsproxy.h> #include <linux/oom.h> #include <linux/elf.h> +#include <linux/pid_namespace.h> #include "internal.h" /* NOTE: @@ -2115,7 +2116,7 @@ static const struct inode_operations pro * that no dcache entries will exist at process exit time it * just makes it very unlikely that any will persist. */ -void proc_flush_task(struct task_struct *task) +static void proc_flush_task_mnt(struct task_struct *task, struct vfsmount *mnt) { struct dentry *dentry, *leader, *dir; char buf[PROC_NUMBUF]; @@ -2123,7 +2124,7 @@ void proc_flush_task(struct task_struct name.name = buf; name.len = snprintf(buf, sizeof(buf), "%d", task->pid); - dentry = d_hash_and_lookup(proc_mnt->mnt_root, &name); + dentry = d_hash_and_lookup(mnt->mnt_root, &name); if (dentry) { shrink_dcache_parent(dentry); d_drop(dentry); @@ -2135,7 +2136,7 @@ void proc_flush_task(struct task_struct name.name = buf; name.len = snprintf(buf, sizeof(buf), "%d", task->tgid); - leader = d_hash_and_lookup(proc_mnt->mnt_root, &name); + leader = d_hash_and_lookup(mnt->mnt_root, &name); if (!leader) goto out; @@ -2161,6 +2162,17 @@ out: return; } +/* + * when flushing dentries from proc one need to flush them from global + * proc (proc_mn…
creator: Pavel Emelianov
date: 2007-07-26T14:48:59-00:00

[PATCH 5/15] Introduce struct upid
type: item
description: From: Sukadev Bhattiprolu <sukadev@us.ibm.com> Since task will be visible from different pid namespaces each of them have to be addressed by multiple pids. struct upid is to store the information about which id refers to which namespace. The constuciton looks like this. Each struct pid carried the reference counter and the list of tasks attached to this pid. At its end it has a variable length array of struct upid-s. Each struct upid has a numerical id (pid itself), pointer to the namespace, this ID is valid in and is hashed into a pid_hash for searching the pids. Signed-off-by: Sukadev Bhattiprolu <sukadev@us.ibm.com> Signed-off-by: Pavel Emelyanov <xemul@openvz.org> --- init_task.h | 9 ++++++--- pid.h | 12 +++++++++--- 2 files changed, 15 insertions(+), 6 deletions(-) diff -upr linux-2.6.23-rc1-mm1.orig/include/linux/init_task.h linux-2.6.23-rc1-mm1-7/include/linux/init_task.h --- linux-2.6.23-rc1-mm1.orig/include/linux/init_task.h 2007-07-26 16:34:45.000000000 +0400 +++ linux-2.6.23-rc1-mm1-7/include/linux/init_task.h 2007-07-26 16:36:36.000000000 +0400 @@ -93,15 +93,18 @@ extern struct group_info init_groups; #define INIT_STRUCT_PID { \ .count = ATOMIC_INIT(1), \ - .nr = 0, \ - /* Don't put this struct pid in pid_hash */ \ - .pid_chain = { .next = NULL, .pprev = NULL }, \ .tasks = { \ { .first = &init_task.pids[PIDTYPE_PID].node }, \ { .first = &init_task.pids[PIDTYPE_PGID].node }, \ { .first = &init_task.pids[PIDTYPE_SID].node }, \ }, \ .rcu = RCU_HEAD_INIT, \ + .level = 0, \ + .numbers = { { \ + .nr = 0, \ + .ns = &init_pid_ns, \ + .pid_chain = { .next = NULL, .pprev = NULL }, \ + }, } \ } #define INIT_PID_LINK(type) \ diff -upr linux-2.6.23-rc1-mm1.orig/include/linux/pid.h linux-2.6.23-rc1-mm1-7/include/linux/pid.h --- linux-2.6.23-rc1-mm1.orig/include/linux/pid.h 2007-07-26 16:34:45.000000000 +0400 +++ linux-2.6.23-rc1-mm1-7/include/linux/pid.h 2007-07-26 16:36:37.000000000 +0400 @@ -40,15 +40,21 @@ enum pid_type * processes. */ -struct pid -{ - atomic_t cou…
creator: Pavel Emelianov
date: 2007-07-26T14:49:42-00:00

[PATCH 6/15] Make alloc_pid(), free_pid() and put_pid() work with struct upid
type: item
description: Each struct upid element of struct pid has to be initialized properly, i.e. its nr mst be allocated from appropriate pidmap and it must be inserted into the hash. Signed-off-by: Pavel Emelyanov <xemul@openvz.org> --- include/linux/pid.h | 2 +- kernel/pid.c | 52 +++++++++++++++++++++++++++++++++++++--------------- 2 files changed, 38 insertions(+), 16 deletions(-) diff -upr linux-2.6.23-rc1-mm1.orig/include/linux/pid.h linux-2.6.23-rc1-mm1-7/include/linux/pid.h --- linux-2.6.23-rc1-mm1.orig/include/linux/pid.h 2007-07-26 16:34:45.000000000 +0400 +++ linux-2.6.23-rc1-mm1-7/include/linux/pid.h 2007-07-26 16:36:37.000000000 +0400 @@ -83,7 +92,7 @@ extern void FASTCALL(detach_pid(struct t extern struct pid *find_get_pid(int nr); extern struct pid *find_ge_pid(int nr); -extern struct pid *alloc_pid(void); +extern struct pid *alloc_pid(struct pid_namespace *ns); extern void FASTCALL(free_pid(struct pid *pid)); static inline pid_t pid_nr(struct pid *pid) diff -upr linux-2.6.23-rc1-mm1.orig/kernel/pid.c linux-2.6.23-rc1-mm1-7/kernel/pid.c --- linux-2.6.23-rc1-mm1.orig/kernel/pid.c 2007-07-26 16:34:45.000000000 +0400 +++ linux-2.6.23-rc1-mm1-7/kernel/pid.c 2007-07-26 16:36:37.000000000 +0400 @@ -28,7 +28,8 @@ #include <linux/pid_namespace.h> #include <linux/init_task.h> -#define pid_hashfn(nr) hash_long((unsigned long)nr, pidhash_shift) +#define pid_hashfn(nr, ns) \ + hash_long((unsigned long)nr + (unsigned long)ns, pidhash_shift) static struct hlist_head *pid_hash; static int pidhash_shift; struct pid init_struct_pid = INIT_STRUCT_PID; @@ -187,11 +202,13 @@ fastcall void put_pid(struct pid *pid) if (!pid) return; - /* FIXME - this must be the namespace this pid lives in */ - ns = &init_pid_ns; + ns = pid->numbers[pid->level].ns; if ((atomic_read(&pid->count) == 1) || - atomic_dec_and_test(&pid->count)) + atomic_dec_and_test(&pid->count)) { kmem_cache_free(ns->pid_cachep, pid); + if (ns != &init_pid_ns) + put_pid_ns(ns); + } } EXPORT_SYMBOL_GPL(put_pid); @@ -204,45 +221,64 @@…
creator: Pavel Emelianov
date: 2007-07-26T14:50:22-00:00

[PATCH 7/15] Helpers to obtain pid numbers
type: item
description: When showing pid to user or getting the pid numerical id for in-kernel use the value of this id may differ depending on the namespace. This set of helpers is used to get the global pid nr, the virtual (i.e. seen by task in its namespace) nr and the nr as it is seen from the specified namespace. Signed-off-by: Pavel Emelyanov <xemul@openvz.org> --- include/linux/pid.h | 24 ++++++++++- include/linux/sched.h | 108 +++++++++++++++++++++++++++++++++++++++++++++----- kernel/pid.c | 8 +++ 3 files changed, 129 insertions(+), 11 deletions(-) diff -upr linux-2.6.23-rc1-mm1.orig/include/linux/pid.h linux-2.6.23-rc1-mm1-7/include/linux/pid.h --- linux-2.6.23-rc1-mm1.orig/include/linux/pid.h 2007-07-26 16:34:45.000000000 +0400 +++ linux-2.6.23-rc1-mm1-7/include/linux/pid.h 2007-07-26 16:36:37.000000000 +0400 @@ -83,12 +92,34 @@ extern void FASTCALL(detach_pid(struct t extern struct pid *alloc_pid(struct pid_namespace *ns); extern void FASTCALL(free_pid(struct pid *pid)); + +/* + * the helpers to get the pid's id seen from different namespaces + * + * pid_nr() : global id, i.e. the id seen from the init namespace; + * pid_vnr() : virtual id, i.e. the id seen from the namespace this pid + * belongs to. this only makes sence when called in the + * context of the task that belongs to the same namespace; + * pid_nr_ns() : id seen from the ns specified. + * + * see also task_xid_nr() etc in include/linux/sched.h + */ static inline pid_t pid_nr(struct pid *pid) { pid_t nr = 0; if (pid) - nr = pid->nr; + nr = pid->numbers[0].nr; + return nr; +} + +pid_t pid_nr_ns(struct pid *pid, struct pid_namespace *ns); + +static inline pid_t pid_vnr(struct pid *pid) +{ + pid_t nr = 0; + if (pid) + nr = pid->numbers[pid->level].nr; return nr; } diff -upr linux-2.6.23-rc1-mm1.orig/include/linux/sched.h linux-2.6.23-rc1-mm1-7/include/linux/sched.h --- linux-2.6.23-rc1-mm1.orig/include/linux/sched.h 2007-07-26 16:34:45.000000000 +0400 +++ linux-2.6.23-rc1-mm1-7/include/linux/sched.h 2007-07-26 16:36:37.…
creator: Pavel Emelianov
date: 2007-07-26T14:51:02-00:00

[PATCH 8/15] Helpers to find the task by its numerical ids
type: item
description: When searching the task by numerical id on may need to find it using global pid (as it is done now in kernel) or by its virtual id, e.g. when sending a signal to a task from one namespace the sender will specify the task's virtual id. Signed-off-by: Pavel Emelyanov <xemul@openvz.org> --- include/linux/pid.h | 16 ++++++++++++++-- include/linux/sched.h | 31 +++++++++++++++++++++++++++++-- kernel/pid.c | 32 +++++++++++++++++--------------- 3 files changed, 60 insertions(+), 19 deletions(-) diff -upr linux-2.6.23-rc1-mm1.orig/include/linux/pid.h linux-2.6.23-rc1-mm1-7/include/linux/pid.h --- linux-2.6.23-rc1-mm1.orig/include/linux/pid.h 2007-07-26 16:34:45.000000000 +0400 +++ linux-2.6.23-rc1-mm1-7/include/linux/pid.h 2007-07-26 16:36:37.000000000 +0400 @@ -83,17 +92,29 @@ extern void FASTCALL(detach_pid(struct t extern void FASTCALL(transfer_pid(struct task_struct *old, struct task_struct *new, enum pid_type)); +struct pid_namespace; +extern struct pid_namespace init_pid_ns; + /* * look up a PID in the hash table. Must be called with the tasklist_lock * or rcu_read_lock() held. + * + * find_pid_ns() finds the pid in the namespace specified + * find_pid() find the pid by its global id, i.e. in the init namespace + * find_vpid() finr the pid by its virtual id, i.e. in the current namespace + * + * see also find_task_by_pid() set in include/linux/sched.h */ -extern struct pid *FASTCALL(find_pid(int nr)); +extern struct pid *FASTCALL(find_pid_ns(int nr, struct pid_namespace *ns)); + +#define find_vpid(pid) find_pid_ns(pid, current->nsproxy->pid_ns) +#define find_pid(pid) find_pid_ns(pid, &init_pid_ns) /* * Lookup a PID in the hash table, and return with it's count elevated. */ extern struct pid *find_get_pid(int nr); -extern struct pid *find_ge_pid(int nr); +extern struct pid *find_ge_pid(int nr, struct pid_namespace *); extern struct pid *alloc_pid(struct pid_namespace *ns); extern void FASTCALL(free_pid(struct pid *pid)); diff -upr linux-2.6.23-rc1-mm1.orig/include/linux…
creator: Pavel Emelianov
date: 2007-07-26T14:51:46-00:00

[PATCH 9/15] Move alloc_pid() after the namespace is cloned
type: item
description: When we create new namespace we will need to allocate the struct pid, that will have one extra struct upid in array, comparing to the parent. Thus we need to know the new namespace (if any) in alloc_pid() to init this struct upid properly, so move the alloc_pid() call lower in copy_process(). This is a fix for Sukadev's patch that moved the alloc_pid() call from do_fork() into copy_process(). Signed-off-by: Pavel Emelyanov <xemul@openvz.org> --- fork.c | 59 ++++++++++++++++++++++++++++++++++++++--------------------- 1 files changed, 38 insertions(+), 21 deletions(-) diff -upr linux-2.6.23-rc1-mm1.orig/kernel/fork.c linux-2.6.23-rc1-mm1-7/kernel/fork.c --- linux-2.6.23-rc1-mm1.orig/kernel/fork.c 2007-07-26 16:34:45.000000000 +0400 +++ linux-2.6.23-rc1-mm1-7/kernel/fork.c 2007-07-26 16:36:37.000000000 +0400 @@ -1028,16 +1029,9 @@ static struct task_struct *copy_process( if (p->binfmt && !try_module_get(p->binfmt->module)) goto bad_fork_cleanup_put_domain; - if (pid != &init_struct_pid) { - pid = alloc_pid(); - if (!pid) - goto bad_fork_put_binfmt_module; - } - p->did_exec = 0; delayacct_tsk_init(p); /* Must remain after dup_task_struct() */ copy_flags(clone_flags, p); - p->pid = pid_nr(pid); INIT_LIST_HEAD(&p->children); INIT_LIST_HEAD(&p->sibling); p->vfork_done = NULL; @@ -1112,10 +1106,6 @@ static struct task_struct *copy_process( p->blocked_on = NULL; /* not blocked yet */ #endif - p->tgid = p->pid; - if (clone_flags & CLONE_THREAD) - p->tgid = current->tgid; - if ((retval = security_task_alloc(p))) goto bad_fork_cleanup_policy; if ((retval = audit_alloc(p))) @@ -1141,6 +1131,17 @@ static struct task_struct *copy_process( if (retval) goto bad_fork_cleanup_namespaces; + if (pid != &init_struct_pid) { + pid = alloc_pid(task_active_pid_ns(p)); + if (!pid) + goto bad_fork_cleanup_namespaces; + } + + p->pid = pid_nr(pid); + p->tgid = p->pid; + if (clone_flags & CLONE_THREAD) + p->tgid = current->tgid; + p->set_child_tid = (clone_flags & CLONE_CHILD_SETTID) ? child_tidp…
creator: Pavel Emelianov
date: 2007-07-26T14:52:33-00:00

[PATCH 10/15] Make each namespace has its own proc tree
type: item
description: The idea is the following: when a namespace is created the new proc superblock, pointing to this namespace, is created. When user accesses the proc tree from some pountmoint it oberves the tasks that live in the namespace, pointer by the according spuperblock. When task dies, its pids must be flushed from several proc trees, i.e. the ones that are owned by the namespaces that can see the taks's struct pid. Having the namespaces live without the proc mount is racy, so during the namespace creation we kern_mount the new proc instance. We cannot just get the reference on the struct pid_namespace from the superblock, otherwise we'll have the ns->proc_mnt->sb->ns loop. To break this, when the init task dies, it releases the kern_mount-ed instance. Signed-off-by: Pavel Emelyanov <xemul@openvz.org> --- fs/proc/base.c | 12 ++++++ fs/proc/inode.c | 3 + fs/proc/root.c | 83 +++++++++++++++++++++++++++++++++++++++--- include/linux/pid_namespace.h | 3 + include/linux/proc_fs.h | 15 +++++++ kernel/exit.c | 18 ++++++++- kernel/fork.c | 5 ++ kernel/pid.c | 1 8 files changed, 133 insertions(+), 7 deletions(-) diff -upr linux-2.6.23-rc1-mm1.orig/fs/proc/base.c linux-2.6.23-rc1-mm1-7/fs/proc/base.c --- linux-2.6.23-rc1-mm1.orig/fs/proc/base.c 2007-07-26 16:34:45.000000000 +0400 +++ linux-2.6.23-rc1-mm1-7/fs/proc/base.c 2007-07-26 16:36:37.000000000 +0400 @@ -2161,7 +2162,19 @@ out: void proc_flush_task(struct task_struct *task, struct pid *pid) { + int i; + struct upid *upid; + proc_flush_task_mnt(task, proc_mnt); + if (pid == NULL) + return; + + for (i = 1; i <= pid->level; i++) + proc_flush_task_mnt(task, pid->numbers[i].ns->proc_mnt); + + upid = &pid->numbers[pid->level]; + if (upid->nr == 1) + pid_ns_release_proc(upid->ns); } static struct dentry *proc_pid_instantiate(struct inode *dir, diff -upr linux-2.6.23-rc1-mm1.orig/fs/proc/inode.c linux-2.6.23-rc1-mm1-7/fs/proc/inode.c --- linux-2.6.23-rc1-mm1.orig/fs/proc/inode.c 2007-07-26 16:34:45.000000000 +0400 +++ linux-2.6.23-rc1-mm1…
creator: Pavel Emelianov
date: 2007-07-26T14:54:22-00:00

[PATCH 11/15] Signal semantics
type: item
description: From: Sukadev Bhattiprolu <sukadev@us.ibm.com> With support for multiple pid namespaces, each pid namespace has a separate child reaper and this process needs some special handling of signals. - The child reaper should appear like a normal process to other processes in its ancestor namespaces and so should be killable (or not) in the usual way. - The child reaper should receive, from processes in it's active and decendent namespaces, only those signals for which it has installed a signal handler. - System-wide signals (eg: kill signum -1) from within a child namespace should only affect processes within that namespace and descendant namespaces. They should not be posted to processes in ancestor or sibling namespaces. - If the sender of a signal does not have a pid_t in the receiver's namespace (eg: a process in init_pid_ns sends a signal to a process in a descendant namespace), the sender's pid should appear as 0 in the signal's siginfo structure. - Existing rules for SIGIO delivery still apply and a process can choose any other process in its namespace and descendant namespaces to receive the SIGIO signal. The following appears to be incorrect in the fcntl() man page for F_SETOWN. Sending a signal to the owner process (group) specified by F_SETOWN is subject to the same permissions checks as are described for kill(2), where the sending process is the one that employs F_SETOWN (but see BUGS below). Current behavior is that the SIGIO signal is delivered on behalf of the process that caused the event (eg: made data available on the file) and not the process that called fcntl(). To implement the above requirements, we: - Add a check in check_kill_permission() for a process within a namespace sending the fast-pathed, SIGKILL signal. - We use a flag, SIGQUEUE_CINIT, to tell the container-init if a signal posted to its queue is from a process within its own namespace. The flag is set in send_signal() if a process attempts to send a signal to its container-init. The SIGQUE…
creator: Pavel Emelianov
date: 2007-07-26T14:55:03-00:00

[PATCH 12/15] Miscelaneous stuff for pid namespaces
type: item
description: This includes * the headers dependency fix * task_child_reaper() correct declaration * fixes for is_global_init() and is_container_init() Maybe this should come before all the other stuff... Signed-off-by: Pavel Emelyanov <xemul@openvz.org> --- include/linux/pid_namespace.h | 4 ++-- include/linux/sched.h | 4 ++-- kernel/pid.c | 16 +++++++++++++--- 3 files changed, 17 insertions(+), 7 deletions(-) diff -upr linux-2.6.23-rc1-mm1.orig/include/linux/pid_namespace.h linux-2.6.23-rc1-mm1-7/include/linux/pid_namespace.h --- linux-2.6.23-rc1-mm1.orig/include/linux/pid_namespace.h 2007-07-26 16:34:45.000000000 +0400 +++ linux-2.6.23-rc1-mm1-7/include/linux/pid_namespace.h 2007-07-26 16:36:36.000000000 +0400 @@ -4,7 +4,6 @@ #include <linux/sched.h> #include <linux/mm.h> #include <linux/threads.h> -#include <linux/pid.h> #include <linux/nsproxy.h> #include <linux/kref.h> @@ -46,7 +53,8 @@ static inline struct pid_namespace *task static inline struct task_struct *task_child_reaper(struct task_struct *tsk) { - return init_pid_ns.child_reaper; + BUG_ON(tsk != current); + return tsk->nsproxy->pid_ns->child_reaper; } #endif /* _LINUX_PID_NS_H */ diff -upr linux-2.6.23-rc1-mm1.orig/include/linux/sched.h linux-2.6.23-rc1-mm1-7/include/linux/sched.h --- linux-2.6.23-rc1-mm1.orig/include/linux/sched.h 2007-07-26 16:34:45.000000000 +0400 +++ linux-2.6.23-rc1-mm1-7/include/linux/sched.h 2007-07-26 16:36:37.000000000 +0400 @@ -1277,13 +1366,13 @@ static inline int pid_alive(struct task_ * * TODO: We should inline this function after some cleanups in pid_namespace.h */ -extern int is_global_init(struct task_struct *tsk); +extern int is_container_init(struct task_struct *tsk); /* * is_container_init: * check whether in the task is init in its own pid namespace. */ -static inline int is_container_init(struct task_struct *tsk) +static inline int is_global_init(struct task_struct *tsk) { return tsk->pid == 1; } diff -upr linux-2.6.23-rc1-mm1.orig/kernel/pid.c linux-2.6.23-rc1-mm1-7/kernel/pid.…
creator: Pavel Emelianov
date: 2007-07-26T14:56:01-00:00

[PATCH 13/15] Clone the pid namespace
type: item
description: When clone() is invoked with CLONE_NEWPID, create a new pid namespace Since the active pid namespace is special and expected to be the first entry in pid->upid_list, preserve the order of pid namespaces. Pid namespaces can be nested and the nesting depth is unlimited. When a process clones its pid namespace, we create additional pid caches as necessary and use the pid cache to allocate 'struct pids' for that depth. TODO: One of the reasons the free_work() was introduced was to cleanup the proc in non-atomic context, but since proc is now released from proc_flush_task() this looks to be unneeded, but I have to recheck this. Signed-off-by: Pavel Emelyanov <xemul@openvz.org> --- include/linux/pid_namespace.h | 7 ++ include/linux/sched.h | 1 kernel/nsproxy.c | 3 - kernel/pid.c | 102 +++++++++++++++++++++++++++++++++++++----- 4 files changed, 101 insertions(+), 12 deletions(-) diff -upr linux-2.6.23-rc1-mm1.orig/include/linux/pid_namespace.h linux-2.6.23-rc1-mm1-7/include/linux/pid_namespace.h --- linux-2.6.23-rc1-mm1.orig/include/linux/pid_namespace.h 2007-07-26 16:34:45.000000000 +0400 +++ linux-2.6.23-rc1-mm1-7/include/linux/pid_namespace.h 2007-07-26 16:36:36.000000000 +0400 @@ -16,11 +15,16 @@ struct pidmap { #define PIDMAP_ENTRIES ((PID_MAX_LIMIT + 8*PAGE_SIZE - 1)/PAGE_SIZE/8) struct pid_namespace { - struct kref kref; + union { + struct kref kref; + struct work_struct free_work; + }; struct pidmap pidmap[PIDMAP_ENTRIES]; int last_pid; struct task_struct *child_reaper; struct kmem_cache *pid_cachep; + int level; + struct pid_namespace *parent; #ifdef CONFIG_PROC_FS struct vfsmount *proc_mnt; #endif diff -upr linux-2.6.23-rc1-mm1.orig/include/linux/sched.h linux-2.6.23-rc1-mm1-7/include/linux/sched.h --- linux-2.6.23-rc1-mm1.orig/include/linux/sched.h 2007-07-26 16:34:45.000000000 +0400 +++ linux-2.6.23-rc1-mm1-7/include/linux/sched.h 2007-07-26 16:36:37.000000000 +0400 @@ -27,6 +27,7 @@ #define CLONE_NEWUTS 0x04000000 /* New utsname group? */ #define CLONE_NEWIPC …
creator: Pavel Emelianov
date: 2007-07-26T14:56:50-00:00

[PATCH 14/15] Destroy pid namespace on init's death
type: item
description: From: Sukadev Bhattiprolu <sukadev@us.ibm.com> Terminate all processes in a namespace when the reaper of the namespace is exiting. We do this by walking the pidmap of the namespace and sending SIGKILL to all processes. Signed-off-by: Sukadev Bhattiprolu <sukadev@us.ibm.com> Acked-by: Pavel Emelyanov <xemul@openvz.org> --- include/linux/pid.h | 1 + include/linux/wait.h | 4 ++++ kernel/exit.c | 10 ++++++---- kernel/pid.c | 38 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 49 insertions(+), 4 deletions(-) diff -upr linux-2.6.23-rc1-mm1.orig/include/linux/pid.h linux-2.6.23-rc1-mm1-7/include/linux/pid.h --- linux-2.6.23-rc1-mm1.orig/include/linux/pid.h 2007-07-26 16:34:45.000000000 +0400 +++ linux-2.6.23-rc1-mm1-7/include/linux/pid.h 2007-07-26 16:36:37.000000000 +0400 @@ -83,6 +92,7 @@ extern void FASTCALL(detach_pid(struct t extern struct pid *alloc_pid(struct pid_namespace *ns); extern void FASTCALL(free_pid(struct pid *pid)); +extern void zap_pid_ns_processes(struct pid_namespace *pid_ns); /* * the helpers to get the pid's id seen from different namespaces diff -upr linux-2.6.23-rc1-mm1.orig/include/linux/wait.h linux-2.6.23-rc1-mm1-7/include/linux/wait.h --- linux-2.6.23-rc1-mm1.orig/include/linux/wait.h 2007-07-26 16:34:45.000000000 +0400 +++ linux-2.6.23-rc1-mm1-7/include/linux/wait.h 2007-07-26 16:36:36.000000000 +0400 @@ -22,9 +22,13 @@ #include <linux/list.h> #include <linux/stddef.h> #include <linux/spinlock.h> +#include <linux/resource.h> +#include <asm/siginfo.h> #include <asm/system.h> #include <asm/current.h> +long do_wait(pid_t pid, int options, struct siginfo __user *infop, + int __user *stat_addr, struct rusage __user *ru); typedef struct __wait_queue wait_queue_t; typedef int (*wait_queue_func_t)(wait_queue_t *wait, unsigned mode, int sync, void *key); int default_wake_function(wait_queue_t *wait, unsigned mode, int sync, void *key); diff -upr linux-2.6.23-rc1-mm1.orig/kernel/exit.c linux-2.6.23-rc1-mm1-7/kernel/exit.c --- linux-2.6.23-rc1-mm…
creator: Pavel Emelianov
date: 2007-07-26T14:57:39-00:00

[PATCH 15/15] Hooks over the code to show correct values to user
type: item
description: This is the largest patch in the set. Make all (I hope) the places where the pid is shown to or get from user operate on the virtual pids. The idea is: - all in-kernel data structures must store either struct pid itself or the pid's global nr, obtained with pid_nr() call; - when seeking the task from kernel code with the stored id one should use find_task_by_pid() call that works with global pids; - when showing pid's numerical value to the user the virtual one should be used, but however when one shows task's pid outside this task's namespace the global one is to be used; - when getting the pid from userspace one need to consider this as the virtual one and use appropriate task/pid-searching functions. Many thanks to Sukadev for pointing out many places I lost. Signed-off-by: Pavel Emelyanov <xemul@openvz.org> --- arch/ia64/kernel/signal.c | 4 +-- arch/parisc/kernel/signal.c | 2 - arch/sparc/kernel/sys_sunos.c | 2 - arch/sparc64/kernel/sys_sunos32.c | 2 - drivers/char/tty_io.c | 7 +++--- fs/binfmt_elf.c | 16 +++++++------- fs/binfmt_elf_fdpic.c | 16 +++++++------- fs/exec.c | 4 +-- fs/fcntl.c | 5 ++-- fs/ioprio.c | 5 ++-- fs/proc/array.c | 25 +++++++++++++++------- fs/proc/base.c | 37 ++++++++++++++++++++------------- include/net/scm.h | 4 ++- ipc/mqueue.c | 7 ++++-- ipc/msg.c | 6 ++--- ipc/sem.c | 8 +++---- ipc/shm.c | 6 ++--- kernel/capability.c | 14 +++++++----- kernel/exit.c | 31 ++++++++++++++++++---------- kernel/fork.c | 2 - kernel/futex.c | 20 +++++++++--------- kernel/ptrace.c | 4 ++- kernel/sched.c | 3 +- kernel/signal.c | 39 +++++++++++++++++++++++------------ kernel/sys.c | 42 ++++++++++++++++++++++---------------- kernel/sysctl.c | 2 - kernel/timer.c | 7 +++--- net/core/scm.c | 4 ++- net/unix/af_unix.c | 6 ++--- 29 files changed, 198 insertions(+), 132 deletions(-) diff -upr linux-2.6.23-rc1-mm1.orig/arch/ia64/kernel/signal.c linux-2.6.23-rc1-mm1-7/arch/ia64/kernel/signal.c --- linux-2.6.23-rc1-mm1.orig/arch/ia64/kernel/signal.c 2007-07-26 16:34:45.000…
creator: Pavel Emelianov
date: 2007-07-26T14:58:53-00:00

Re: [PATCH 1/15] Move exit_task_namespaces()
type: item
description: On Thu, 2007-07-26 at 18:46 +0400, Pavel Emelyanov wrote: > + write_unlock_irq(&tasklist_lock); > + exit_task_namespaces(tsk); > + write_lock_irq(&tasklist_lock); Are there any other side-effects of doing this? What was the tasklist_lock protecting here when it was released? -- Dave
creator: Dave Hansen
date: 2007-07-26T16:10:39-00:00

Re: [PATCH 1/15] Move exit_task_namespaces()
type: item
description: On 07/26, Pavel Emelyanov wrote: > > Make task release its namespaces after it has reparented all his > children to child_reaper, but before it notifies its parent about > its death. > > The reason to release namespaces after reparenting is that when task > exits it may send a signal to its parent (SIGCHLD), but if the parent > has already exited its namespaces there will be no way to decide what > pid to dever to him - parent can be from different namespace. > > The reason to release namespace before notifying the parent it that > when task sends a SIGCHLD to parent it can call wait() on this taks > and release it. But releasing the mnt namespace implies dropping > of all the mounts in the mnt namespace and NFS expects the task to > have valid sighand pointer. > > Signed-off-by: Pavel Emelyanov <xemul@openvz.org> > > --- > > exit.c | 5 ++++- > 1 files changed, 4 insertions(+), 1 deletion(-) > > diff -upr linux-2.6.23-rc1-mm1.orig/kernel/exit.c > linux-2.6.23-rc1-mm1-7/kernel/exit.c > --- linux-2.6.23-rc1-mm1.orig/kernel/exit.c 2007-07-26 > 16:34:45.000000000 +0400 > +++ linux-2.6.23-rc1-mm1-7/kernel/exit.c 2007-07-26 > 16:36:37.000000000 +0400 > @@ -788,6 +804,10 @@ static void exit_notify(struct task_stru > BUG_ON(!list_empty(&tsk->children)); > BUG_ON(!list_empty(&tsk->ptrace_children)); > > + write_unlock_irq(&tasklist_lock); > + exit_task_namespaces(tsk); > + write_lock_irq(&tasklist_lock); No. We "cleared" our ->children/->ptrace_children lists. Now suppose that another thread dies, and its forget_original_parent() choose us as a new reaper before we re-take tasklist. I'll try to read other patches tomorrow, but I can't avoid a stupid question: can we have a CONFIG_ for that? This series adds a lot of complications. OK, I guess the answer is "it is very difficult t achieve", but can't help myself :) Oleg.
creator: Oleg Nesterov
date: 2007-07-26T16:47:24-00:00

Re: [PATCH 1/15] Move exit_task_namespaces()
type: item
description: Oleg Nesterov wrote: > On 07/26, Pavel Emelyanov wrote: > >>Make task release its namespaces after it has reparented all his >>children to child_reaper, but before it notifies its parent about >>its death. >> >>The reason to release namespaces after reparenting is that when task >>exits it may send a signal to its parent (SIGCHLD), but if the parent >>has already exited its namespaces there will be no way to decide what >>pid to dever to him - parent can be from different namespace. >> >>The reason to release namespace before notifying the parent it that >>when task sends a SIGCHLD to parent it can call wait() on this taks >>and release it. But releasing the mnt namespace implies dropping >>of all the mounts in the mnt namespace and NFS expects the task to >>have valid sighand pointer. >> >>Signed-off-by: Pavel Emelyanov <xemul@openvz.org> >> >>--- >> >>exit.c | 5 ++++- >>1 files changed, 4 insertions(+), 1 deletion(-) >> >>diff -upr linux-2.6.23-rc1-mm1.orig/kernel/exit.c >>linux-2.6.23-rc1-mm1-7/kernel/exit.c >>--- linux-2.6.23-rc1-mm1.orig/kernel/exit.c 2007-07-26 >>16:34:45.000000000 +0400 >>+++ linux-2.6.23-rc1-mm1-7/kernel/exit.c 2007-07-26 >>16:36:37.000000000 +0400 >>@@ -788,6 +804,10 @@ static void exit_notify(struct task_stru >> BUG_ON(!list_empty(&tsk->children)); >> BUG_ON(!list_empty(&tsk->ptrace_children)); >> >>+ write_unlock_irq(&tasklist_lock); >>+ exit_task_namespaces(tsk); >>+ write_lock_irq(&tasklist_lock); > > > No. > > We "cleared" our ->children/->ptrace_children lists. Now suppose that > another thread dies, and its forget_original_parent() choose us as a > new reaper before we re-take tasklist. > > I'll try to read other patches tomorrow, but I can't avoid a stupid > question: can we have a CONFIG_ for that? This series adds a lot of > complications. the was a request from many people including Andrew that CONFIG_XXX is bad approach. > OK, I guess the answer is "it is very difficult t achieve", but can't > help myself :) First versions of Pa…
creator: dev
date: 2007-07-26T16:59:13-00:00

Re: [PATCH 7/15] Helpers to obtain pid numbers
type: item
description: On Thu, 2007-07-26 at 18:51 +0400, Pavel Emelyanov wrote: > > + * pid_nr() : global id, i.e. the id seen from the init namespace; > + * pid_vnr() : virtual id, i.e. the id seen from the namespace this pid > + * belongs to. this only makes sence when called in the > + * context of the task that belongs to the same namespace; Can we give these some better names? I think "virtual" is pretty bad, especially if you consider the multiple level of pid namespaces that we might have some day. Processes can belong to multiple pid namespaces, and thus have multiple "virtual" ids. Even though it will make the names longer, I think we need something in the names to say that "pid_nr()" is the top-level, global, init_pid_ns number. "pid_vnr()" is the pid for the lowest pid namespace in the hierarchy. Suka called this an "active pid namespace" because that is where the task actively interacts with its peers. But, I'm open to other suggestions, too. When writing code, people are going to need to know which one to use: pid_nr() or pid_vnr(). We can document the functions, but the names will help much more than any documentation. -- Dave
creator: Dave Hansen
date: 2007-07-26T19:03:01-00:00

Re: [PATCH 8/15] Helpers to find the task by its numerical ids
type: item
description: On Thu, 2007-07-26 at 18:51 +0400, Pavel Emelyanov wrote: > +extern struct task_struct *find_task_by_pid_type_ns(int type, int pid, > + struct pid_namespace *ns); > + > +#define find_task_by_pid_ns(nr, ns) \ > + find_task_by_pid_type_ns(PIDTYPE_PID, nr, ns) > +#define find_task_by_pid_type(type, nr) \ > + find_task_by_pid_type_ns(type, nr, &init_pid_ns) > +#define find_task_by_pid(nr) \ > + find_task_by_pid_type(PIDTYPE_PID, nr) > + > extern void __set_special_pids(pid_t session, pid_t pgrp); Do these _have_ to be macros? > /* per-UID process charging. */ > diff -upr linux-2.6.23-rc1-mm1.orig/kernel/pid.c linux-2.6.23-rc1-mm1-7/kernel/pid.c > --- linux-2.6.23-rc1-mm1.orig/kernel/pid.c 2007-07-26 16:34:45.000000000 +0400 > +++ linux-2.6.23-rc1-mm1-7/kernel/pid.c 2007-07-26 16:36:37.000000000 +0400 > @@ -204,19 +221,20 @@ static void delayed_put_pid(struct rcu_h > goto out; > } > > -struct pid * fastcall find_pid(int nr) > +struct pid * fastcall find_pid_ns(int nr, struct pid_namespace *ns) > { > struct hlist_node *elem; > - struct pid *pid; > + struct upid *pnr; > + > + hlist_for_each_entry_rcu(pnr, elem, > + &pid_hash[pid_hashfn(nr, ns)], pid_chain) > + if (pnr->nr == nr && pnr->ns == ns) > + return container_of(pnr, struct pid, > + numbers[ns->level]); Do we do this loop anywhere else? Should we have a for_each_pid() that makes this a little less messy? > - hlist_for_each_entry_rcu(pid, elem, > - &pid_hash[pid_hashfn(nr)], pid_chain) { > - if (pid->nr == nr) > - return pid; > - } > return NULL; > } > -EXPORT_SYMBOL_GPL(find_pid); > +EXPORT_SYMBOL_GPL(find_pid_ns); > > /* > * attach_pid() must be called with the tasklist_lock write-held. > @@ -318,12 +355,13 @@ struct task_struct * fastcall pid_task(s > /* > * Must be called under rcu_read_lock() or with tasklist_lock read-held. > */ > -struct task_struct *find_task_by_pid_type(int type, int nr) > +struct task_struct *find_task_by_pid_type_ns(int type, int nr, > + struct pid_namespace *ns) > { > - return pid_task(fi…
creator: Dave Hansen
date: 2007-07-26T19:05:41-00:00

Re: [RFC][PATCH 0/15] Pid namespaces
type: item
description: Pavel, We seem to have a memory leak. Its new in this patchset (i.e the following test ran fine on the 2.6.22-rc6-mm1 patchset). To repro: run "pidns_exec ./mypid" in a tight-loop - where mypid.c is: #include <stdio.h> #include <unistd.h> main() { printf("Pid %d, Ppid %d, Pgid %d, Sid %d\n", getpid(), getppid(), getpgid(0), getsid(0)); } I ran into OOM in about 30 mins. I am still investigating. BTW, can we include a simple test program like the pidns_exec in this patch-0 for whoever want to play with pidns ? Suka Pavel Emelianov [xemul@openvz.org] wrote: | (Comment is taken from Sukadev's patchset-v3) | | A pid namespace is a "view" of a particular set of tasks on the system. | They work in a similar way to filesystem namespaces. A file (or a process) | can be accessed in multiple namespaces, but it may have a different name | in each. In a filesystem, this name might be /etc/passwd in one namespace, | but /chroot/etc/passwd in another. | | For processes, a process may have pid 1234 in one namespace, but be pid 1 | in another. This allows new pid namespaces to have basically arbitrary | pids, and not have to worry about what pids exist in other namespaces. | This is essential for checkpoint/restart where a restarted process's pid | might collide with an existing process on the system's pid. | | In this particular implementation, pid namespaces have a parent-child | relationship, just like processes. A process in a pid namespace may see | all of the processes in the same namespace, as well as all of the processes | in all of the namespaces which are children of its namespace. Processes may | not, however, see others which are in their parent's namespace, but not in | their own. The same goes for sibling namespaces. | | This set is based on my patches, I sent before, but it includes some | comments | and patches that I received from Sukadev. Sukadev, please, add your | Acked-by, | Signed-off-by or From, to patches you want (everybody is also welcome :) ). | | The set…
creator: Sukadev Bhattiprolu
date: 2007-07-27T04:22:13-00:00

Re: [PATCH 15/15] Hooks over the code to show correct values to user
type: item
description: Pavel Emelianov [xemul@openvz.org] wrote: | diff -upr linux-2.6.23-rc1-mm1.orig/fs/proc/array.c | linux-2.6.23-rc1-mm1-7/fs/proc/array.c | --- linux-2.6.23-rc1-mm1.orig/fs/proc/array.c 2007-07-26 | 16:34:45.000000000 +0400 | +++ linux-2.6.23-rc1-mm1-7/fs/proc/array.c 2007-07-26 | 16:36:36.000000000 +0400 | @@ -77,6 +77,7 @@ | #include <linux/cpuset.h> | #include <linux/rcupdate.h> | #include <linux/delayacct.h> | +#include <linux/pid_namespace.h> | | #include <asm/pgtable.h> | #include <asm/processor.h> | @@ -161,7 +162,9 @@ static inline char *task_state(struct ta | struct group_info *group_info; | int g; | struct fdtable *fdt = NULL; | + struct pid_namespace *ns; | | + ns = current->nsproxy->pid_ns; | rcu_read_lock(); | buffer += sprintf(buffer, | "State:\t%s\n" | @@ -172,9 +175,12 @@ static inline char *task_state(struct ta | "Uid:\t%d\t%d\t%d\t%d\n" | "Gid:\t%d\t%d\t%d\t%d\n", | get_task_state(p), | - p->tgid, p->pid, | - pid_alive(p) ? rcu_dereference(p->real_parent)->tgid : 0, | - pid_alive(p) && p->ptrace ? rcu_dereference(p->parent)->pid | : 0, | + task_tgid_nr_ns(p, ns), | + task_pid_nr_ns(p, ns), | + pid_alive(p) ? | + task_ppid_nr_ns(p, ns) : 0, | + pid_alive(p) && p->ptrace ? | + task_tgid_nr_ns(rcu_dereference(p->parent), ns) : 0, Hmm. I think we have this reversed here. We should print the 'tgid' of task->real_parent and 'pid' of task->parent.
creator: Sukadev Bhattiprolu
date: 2007-07-27T05:57:36-00:00

Re: [RFC][PATCH 0/15] Pid namespaces
type: item
description: sukadev@us.ibm.com [sukadev@us.ibm.com] wrote: | Pavel, | | We seem to have a memory leak. Its new in this patchset (i.e the | following test ran fine on the 2.6.22-rc6-mm1 patchset). | | To repro: run "pidns_exec ./mypid" in a tight-loop - where mypid.c | is: | | #include <stdio.h> | #include <unistd.h> | | main() | { | printf("Pid %d, Ppid %d, Pgid %d, Sid %d\n", | getpid(), getppid(), getpgid(0), getsid(0)); | } | | I ran into OOM in about 30 mins. I am still investigating. | | BTW, can we include a simple test program like the pidns_exec in this | patch-0 for whoever want to play with pidns ? | | Suka | I think the problem is in create_pid_namespace(). kref_init() sets the refcount to 1 and then we do a get_pid_ns() which sets it to 2. free_nsproxy() frees one of this references, but the other is never freed. This patch seems to fix the leak. The patch also creates a slab cache for the pid_namespace --- Create a slab-cache for 'struct pid_namespace' and fix a memory leak due to an extra reference in create_pid_namespace(). --- kernel/pid.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) Index: lx26-23-rc1-mm1/kernel/pid.c =================================================================== --- lx26-23-rc1-mm1.orig/kernel/pid.c 2007-07-26 20:08:16.000000000 -0700 +++ lx26-23-rc1-mm1/kernel/pid.c 2007-07-26 22:31:42.000000000 -0700 @@ -35,6 +35,7 @@ static struct hlist_head *pid_hash; static int pidhash_shift; struct pid init_struct_pid = INIT_STRUCT_PID; +static struct kmem_cache *pid_ns_cachep; int pid_max = PID_MAX_DEFAULT; @@ -525,7 +526,7 @@ static struct pid_namespace *create_pid_ struct pid_namespace *ns; int i; - ns = kmalloc(sizeof(struct pid_namespace), GFP_KERNEL); + ns = kmem_cache_alloc(pid_ns_cachep, GFP_KERNEL); if (ns == NULL) goto out; @@ -544,7 +545,6 @@ static struct pid_namespace *create_pid_ set_bit(0, ns->pidmap[0].page); atomic_set(&ns->pidmap[0].nr_free, BITS_PER_PAGE - 1); - get_pid_ns(ns); for (i = 1; i < PIDMAP_ENTRIES; i+…
creator: Sukadev Bhattiprolu
date: 2007-07-27T06:08:56-00:00

Re: [PATCH 12/15] Miscelaneous stuff for pid namespaces
type: item
description: Pavel Emelianov [xemul@openvz.org] wrote: | This includes | * the headers dependency fix | * task_child_reaper() correct declaration | * fixes for is_global_init() and is_container_init() | | Maybe this should come before all the other stuff... | | Signed-off-by: Pavel Emelyanov <xemul@openvz.org> | | --- | | include/linux/pid_namespace.h | 4 ++-- | include/linux/sched.h | 4 ++-- | kernel/pid.c | 16 +++++++++++++--- | 3 files changed, 17 insertions(+), 7 deletions(-) | | diff -upr linux-2.6.23-rc1-mm1.orig/include/linux/pid_namespace.h | linux-2.6.23-rc1-mm1-7/include/linux/pid_namespace.h | --- linux-2.6.23-rc1-mm1.orig/include/linux/pid_namespace.h 2007-07-26 | 16:34:45.000000000 +0400 | +++ linux-2.6.23-rc1-mm1-7/include/linux/pid_namespace.h 2007-07-26 | 16:36:36.000000000 +0400 | @@ -4,7 +4,6 @@ | #include <linux/sched.h> | #include <linux/mm.h> | #include <linux/threads.h> | -#include <linux/pid.h> | #include <linux/nsproxy.h> | #include <linux/kref.h> | | @@ -46,7 +53,8 @@ static inline struct pid_namespace *task | | static inline struct task_struct *task_child_reaper(struct task_struct *tsk) | { | - return init_pid_ns.child_reaper; | + BUG_ON(tsk != current); | + return tsk->nsproxy->pid_ns->child_reaper; | } | | #endif /* _LINUX_PID_NS_H */ | diff -upr linux-2.6.23-rc1-mm1.orig/include/linux/sched.h | linux-2.6.23-rc1-mm1-7/include/linux/sched.h | --- linux-2.6.23-rc1-mm1.orig/include/linux/sched.h 2007-07-26 | 16:34:45.000000000 +0400 | +++ linux-2.6.23-rc1-mm1-7/include/linux/sched.h 2007-07-26 | 16:36:37.000000000 +0400 | @@ -1277,13 +1366,13 @@ static inline int pid_alive(struct task_ | * | * TODO: We should inline this function after some cleanups in | pid_namespace.h | */ We can now remove this TODO. | -extern int is_global_init(struct task_struct *tsk); | +extern int is_container_init(struct task_struct *tsk); | | /* | * is_container_init: | * check whether in the task is init in its own pid namespace. | */ The function header describes is_container_…
creator: Sukadev Bhattiprolu
date: 2007-07-27T06:22:13-00:00

Re: [PATCH 1/15] Move exit_task_namespaces()
type: item
description: Dave Hansen wrote: > On Thu, 2007-07-26 at 18:46 +0400, Pavel Emelyanov wrote: >> + write_unlock_irq(&tasklist_lock); >> + exit_task_namespaces(tsk); >> + write_lock_irq(&tasklist_lock); > > Are there any other side-effects of doing this? What was the Looks like there are :( Oleg pointed out, that if any thread dies it can re-parent all its children to this one, who already released its namespaces... :( > tasklist_lock protecting here when it was released? > > -- Dave > >
creator: xemul
date: 2007-07-27T06:38:02-00:00

Re: [PATCH 7/15] Helpers to obtain pid numbers
type: item
description: Dave Hansen wrote: > On Thu, 2007-07-26 at 18:51 +0400, Pavel Emelyanov wrote: >> + * pid_nr() : global id, i.e. the id seen from the init namespace; >> + * pid_vnr() : virtual id, i.e. the id seen from the namespace this pid >> + * belongs to. this only makes sence when called in the >> + * context of the task that belongs to the same namespace; > > Can we give these some better names? I think "virtual" is pretty bad, > especially if you consider the multiple level of pid namespaces that we > might have some day. Processes can belong to multiple pid namespaces, > and thus have multiple "virtual" ids. We do have them now (multiple levels). The "virtual" stands for "the one that task sees by his own". > Even though it will make the names longer, I think we need something in > the names to say that "pid_nr()" is the top-level, global, init_pid_ns > number. "pid_vnr()" is the pid for the lowest pid namespace in the > hierarchy. That's it. > Suka called this an "active pid namespace" because that is > where the task actively interacts with its peers. But, I'm open to > other suggestions, too. > > When writing code, people are going to need to know which one to use: > pid_nr() or pid_vnr(). We can document the functions, but the names > will help much more than any documentation. I do not mind, but what other names can we have? pid_nr() for init namespace, pid_nr_ns for arbitrary namespace and pid_<what> for the lower-level namespace? > > -- Dave > > Thanks, Pavel
creator: Pavel Emelianov
date: 2007-07-27T06:40:47-00:00

Re: [PATCH 8/15] Helpers to find the task by its numerical ids
type: item
description: Dave Hansen wrote: > On Thu, 2007-07-26 at 18:51 +0400, Pavel Emelyanov wrote: >> +extern struct task_struct *find_task_by_pid_type_ns(int type, int pid, >> + struct pid_namespace *ns); >> + >> +#define find_task_by_pid_ns(nr, ns) \ >> + find_task_by_pid_type_ns(PIDTYPE_PID, nr, ns) >> +#define find_task_by_pid_type(type, nr) \ >> + find_task_by_pid_type_ns(type, nr, &init_pid_ns) >> +#define find_task_by_pid(nr) \ >> + find_task_by_pid_type(PIDTYPE_PID, nr) >> + >> extern void __set_special_pids(pid_t session, pid_t pgrp); > > Do these _have_ to be macros? No, but why not? I can make them static inline functions, but why are macros that bad? >> /* per-UID process charging. */ >> diff -upr linux-2.6.23-rc1-mm1.orig/kernel/pid.c linux-2.6.23-rc1-mm1-7/kernel/pid.c >> --- linux-2.6.23-rc1-mm1.orig/kernel/pid.c 2007-07-26 16:34:45.000000000 +0400 >> +++ linux-2.6.23-rc1-mm1-7/kernel/pid.c 2007-07-26 16:36:37.000000000 +0400 >> @@ -204,19 +221,20 @@ static void delayed_put_pid(struct rcu_h >> goto out; >> } >> >> -struct pid * fastcall find_pid(int nr) >> +struct pid * fastcall find_pid_ns(int nr, struct pid_namespace *ns) >> { >> struct hlist_node *elem; >> - struct pid *pid; >> + struct upid *pnr; >> + >> + hlist_for_each_entry_rcu(pnr, elem, >> + &pid_hash[pid_hashfn(nr, ns)], pid_chain) >> + if (pnr->nr == nr && pnr->ns == ns) >> + return container_of(pnr, struct pid, >> + numbers[ns->level]); > > Do we do this loop anywhere else? Should we have a for_each_pid() that > makes this a little less messy? No. Iteration over the hash chain happens here only. >> - hlist_for_each_entry_rcu(pid, elem, >> - &pid_hash[pid_hashfn(nr)], pid_chain) { >> - if (pid->nr == nr) >> - return pid; >> - } >> return NULL; >> } >> -EXPORT_SYMBOL_GPL(find_pid); >> +EXPORT_SYMBOL_GPL(find_pid_ns); >> >> /* >> * attach_pid() must be called with the tasklist_lock write-held. >> @@ -318,12 +355,13 @@ struct task_struct * fastcall pid_task(s >> /* >> * Must be called under rcu_read_lock() or wit…
creator: Pavel Emelianov
date: 2007-07-27T06:43:38-00:00

Re: [PATCH 15/15] Hooks over the code to show correct values to user
type: item
description: sukadev@us.ibm.com wrote: > Pavel Emelianov [xemul@openvz.org] wrote: > | diff -upr linux-2.6.23-rc1-mm1.orig/fs/proc/array.c > | linux-2.6.23-rc1-mm1-7/fs/proc/array.c > | --- linux-2.6.23-rc1-mm1.orig/fs/proc/array.c 2007-07-26 > | 16:34:45.000000000 +0400 > | +++ linux-2.6.23-rc1-mm1-7/fs/proc/array.c 2007-07-26 > | 16:36:36.000000000 +0400 > | @@ -77,6 +77,7 @@ > | #include <linux/cpuset.h> > | #include <linux/rcupdate.h> > | #include <linux/delayacct.h> > | +#include <linux/pid_namespace.h> > | > | #include <asm/pgtable.h> > | #include <asm/processor.h> > | @@ -161,7 +162,9 @@ static inline char *task_state(struct ta > | struct group_info *group_info; > | int g; > | struct fdtable *fdt = NULL; > | + struct pid_namespace *ns; > | > | + ns = current->nsproxy->pid_ns; > | rcu_read_lock(); > | buffer += sprintf(buffer, > | "State:\t%s\n" > | @@ -172,9 +175,12 @@ static inline char *task_state(struct ta > | "Uid:\t%d\t%d\t%d\t%d\n" > | "Gid:\t%d\t%d\t%d\t%d\n", > | get_task_state(p), > | - p->tgid, p->pid, > | - pid_alive(p) ? rcu_dereference(p->real_parent)->tgid : 0, > | - pid_alive(p) && p->ptrace ? rcu_dereference(p->parent)->pid > | : 0, > | + task_tgid_nr_ns(p, ns), > | + task_pid_nr_ns(p, ns), > | + pid_alive(p) ? > | + task_ppid_nr_ns(p, ns) : 0, > | + pid_alive(p) && p->ptrace ? > | + task_tgid_nr_ns(rcu_dereference(p->parent), ns) : 0, > > Hmm. I think we have this reversed here. We should print the 'tgid' of > task->real_parent and 'pid' of task->parent. > You mean smth like task_tgid_nr_ns(p->real_parent) ? Yes, thanks :)
creator: Pavel Emelianov
date: 2007-07-27T06:44:54-00:00

Re: [RFC][PATCH 0/15] Pid namespaces
type: item
description: sukadev@us.ibm.com wrote: > sukadev@us.ibm.com [sukadev@us.ibm.com] wrote: > | Pavel, > | > | We seem to have a memory leak. Its new in this patchset (i.e the > | following test ran fine on the 2.6.22-rc6-mm1 patchset). > | > | To repro: run "pidns_exec ./mypid" in a tight-loop - where mypid.c > | is: > | > | #include <stdio.h> > | #include <unistd.h> > | > | main() > | { > | printf("Pid %d, Ppid %d, Pgid %d, Sid %d\n", > | getpid(), getppid(), getpgid(0), getsid(0)); > | } > | > | I ran into OOM in about 30 mins. I am still investigating. > | > | BTW, can we include a simple test program like the pidns_exec in this > | patch-0 for whoever want to play with pidns ? > | > | Suka > | > > I think the problem is in create_pid_namespace(). kref_init() sets the > refcount to 1 and then we do a get_pid_ns() which sets it to 2. > > free_nsproxy() frees one of this references, but the other is never > freed. That's it! I've switched from my old fast (and bad) reference counting scheme to new slow (and correct) one, but forgot to remove this "get". This is the real problem. Thanks :) > This patch seems to fix the leak. The patch also creates a slab cache > for the pid_namespace OK, I'll include this patch. Thanks. > --- > > Create a slab-cache for 'struct pid_namespace' and fix a memory leak > due to an extra reference in create_pid_namespace(). > > --- > kernel/pid.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > Index: lx26-23-rc1-mm1/kernel/pid.c > =================================================================== > --- lx26-23-rc1-mm1.orig/kernel/pid.c 2007-07-26 20:08:16.000000000 -0700 > +++ lx26-23-rc1-mm1/kernel/pid.c 2007-07-26 22:31:42.000000000 -0700 > @@ -35,6 +35,7 @@ > static struct hlist_head *pid_hash; > static int pidhash_shift; > struct pid init_struct_pid = INIT_STRUCT_PID; > +static struct kmem_cache *pid_ns_cachep; > > int pid_max = PID_MAX_DEFAULT; > > @@ -525,7 +526,7 @@ static struct pid_namespace *create_pid_ > struct pid_names…
creator: Pavel Emelianov
date: 2007-07-27T06:47:18-00:00

Re: [PATCH 12/15] Miscelaneous stuff for pid namespaces
type: item
description: sukadev@us.ibm.com wrote: > Pavel Emelianov [xemul@openvz.org] wrote: > | This includes > | * the headers dependency fix > | * task_child_reaper() correct declaration > | * fixes for is_global_init() and is_container_init() > | > | Maybe this should come before all the other stuff... > | > | Signed-off-by: Pavel Emelyanov <xemul@openvz.org> > | > | --- > | > | include/linux/pid_namespace.h | 4 ++-- > | include/linux/sched.h | 4 ++-- > | kernel/pid.c | 16 +++++++++++++--- > | 3 files changed, 17 insertions(+), 7 deletions(-) > | > | diff -upr linux-2.6.23-rc1-mm1.orig/include/linux/pid_namespace.h > | linux-2.6.23-rc1-mm1-7/include/linux/pid_namespace.h > | --- linux-2.6.23-rc1-mm1.orig/include/linux/pid_namespace.h 2007-07-26 > | 16:34:45.000000000 +0400 > | +++ linux-2.6.23-rc1-mm1-7/include/linux/pid_namespace.h 2007-07-26 > | 16:36:36.000000000 +0400 > | @@ -4,7 +4,6 @@ > | #include <linux/sched.h> > | #include <linux/mm.h> > | #include <linux/threads.h> > | -#include <linux/pid.h> > | #include <linux/nsproxy.h> > | #include <linux/kref.h> > | > | @@ -46,7 +53,8 @@ static inline struct pid_namespace *task > | > | static inline struct task_struct *task_child_reaper(struct task_struct *tsk) > | { > | - return init_pid_ns.child_reaper; > | + BUG_ON(tsk != current); > | + return tsk->nsproxy->pid_ns->child_reaper; > | } > | > | #endif /* _LINUX_PID_NS_H */ > | diff -upr linux-2.6.23-rc1-mm1.orig/include/linux/sched.h > | linux-2.6.23-rc1-mm1-7/include/linux/sched.h > | --- linux-2.6.23-rc1-mm1.orig/include/linux/sched.h 2007-07-26 > | 16:34:45.000000000 +0400 > | +++ linux-2.6.23-rc1-mm1-7/include/linux/sched.h 2007-07-26 > | 16:36:37.000000000 +0400 > | @@ -1277,13 +1366,13 @@ static inline int pid_alive(struct task_ > | * > | * TODO: We should inline this function after some cleanups in > | pid_namespace.h > | */ > > We can now remove this TODO. Ok. > | -extern int is_global_init(struct task_struct *tsk); > | +extern int is_container_init(struct task_struct *tsk); …
creator: Pavel Emelianov
date: 2007-07-27T06:53:08-00:00

Re: [PATCH 1/15] Move exit_task_namespaces()
type: item
description: On 07/26, Oleg Nesterov wrote: > > On 07/26, Pavel Emelyanov wrote: > > > > Make task release its namespaces after it has reparented all his > > children to child_reaper, but before it notifies its parent about > > its death. > > > > The reason to release namespaces after reparenting is that when task > > exits it may send a signal to its parent (SIGCHLD), but if the parent > > has already exited its namespaces there will be no way to decide what > > pid to dever to him - parent can be from different namespace. > > > > The reason to release namespace before notifying the parent it that > > when task sends a SIGCHLD to parent it can call wait() on this taks > > and release it. But releasing the mnt namespace implies dropping > > of all the mounts in the mnt namespace and NFS expects the task to > > have valid sighand pointer. > > > > Signed-off-by: Pavel Emelyanov <xemul@openvz.org> > > > > --- > > > > exit.c | 5 ++++- > > 1 files changed, 4 insertions(+), 1 deletion(-) > > > > diff -upr linux-2.6.23-rc1-mm1.orig/kernel/exit.c > > linux-2.6.23-rc1-mm1-7/kernel/exit.c > > --- linux-2.6.23-rc1-mm1.orig/kernel/exit.c 2007-07-26 > > 16:34:45.000000000 +0400 > > +++ linux-2.6.23-rc1-mm1-7/kernel/exit.c 2007-07-26 > > 16:36:37.000000000 +0400 > > @@ -788,6 +804,10 @@ static void exit_notify(struct task_stru > > BUG_ON(!list_empty(&tsk->children)); > > BUG_ON(!list_empty(&tsk->ptrace_children)); > > > > + write_unlock_irq(&tasklist_lock); > > + exit_task_namespaces(tsk); > > + write_lock_irq(&tasklist_lock); > > No. > > We "cleared" our ->children/->ptrace_children lists. Now suppose that > another thread dies, and its forget_original_parent() choose us as a > new reaper before we re-take tasklist. Perhaps, we can do something like the patch below. Roland, what do you think? We can check PF_EXITING instead of ->exit_state while choosing the new parent. Note that tasklits_lock acts as a barrier, everyone who takes tasklist after us (when forget_original_parent() drops it) mu…
creator: Oleg Nesterov
date: 2007-07-27T08:07:58-00:00

Re: [PATCH 1/15] Move exit_task_namespaces()
type: item
description: Oleg Nesterov wrote: > On 07/26, Oleg Nesterov wrote: >> On 07/26, Pavel Emelyanov wrote: >>> Make task release its namespaces after it has reparented all his >>> children to child_reaper, but before it notifies its parent about >>> its death. >>> >>> The reason to release namespaces after reparenting is that when task >>> exits it may send a signal to its parent (SIGCHLD), but if the parent >>> has already exited its namespaces there will be no way to decide what >>> pid to dever to him - parent can be from different namespace. >>> >>> The reason to release namespace before notifying the parent it that >>> when task sends a SIGCHLD to parent it can call wait() on this taks >>> and release it. But releasing the mnt namespace implies dropping >>> of all the mounts in the mnt namespace and NFS expects the task to >>> have valid sighand pointer. >>> >>> Signed-off-by: Pavel Emelyanov <xemul@openvz.org> >>> >>> --- >>> >>> exit.c | 5 ++++- >>> 1 files changed, 4 insertions(+), 1 deletion(-) >>> >>> diff -upr linux-2.6.23-rc1-mm1.orig/kernel/exit.c >>> linux-2.6.23-rc1-mm1-7/kernel/exit.c >>> --- linux-2.6.23-rc1-mm1.orig/kernel/exit.c 2007-07-26 >>> 16:34:45.000000000 +0400 >>> +++ linux-2.6.23-rc1-mm1-7/kernel/exit.c 2007-07-26 >>> 16:36:37.000000000 +0400 >>> @@ -788,6 +804,10 @@ static void exit_notify(struct task_stru >>> BUG_ON(!list_empty(&tsk->children)); >>> BUG_ON(!list_empty(&tsk->ptrace_children)); >>> >>> + write_unlock_irq(&tasklist_lock); >>> + exit_task_namespaces(tsk); >>> + write_lock_irq(&tasklist_lock); >> No. >> >> We "cleared" our ->children/->ptrace_children lists. Now suppose that >> another thread dies, and its forget_original_parent() choose us as a >> new reaper before we re-take tasklist. > > Perhaps, we can do something like the patch below. Roland, what do you > think? > > We can check PF_EXITING instead of ->exit_state while choosing the new Heh :) I've came to the same conclusion and now I'm checking for it. But my patch is much simpler th…
creator: Pavel Emelianov
date: 2007-07-27T08:24:33-00:00

Re: [PATCH 1/15] Move exit_task_namespaces()
type: item
description: On 07/27, Pavel Emelyanov wrote: > > Oleg Nesterov wrote: > > > >Perhaps, we can do something like the patch below. Roland, what do you > >think? > > > >We can check PF_EXITING instead of ->exit_state while choosing the new > > Heh :) I've came to the same conclusion and now I'm checking for it. > But my patch is much simpler that yours - it just checks for PF_EXITING > in forget_original_parent: > > --- ./kernel/exit.c.exitfix 2007-07-27 12:13:25.000000000 +0400 > +++ ./kernel/exit.c 2007-07-27 12:15:35.000000000 +0400 > @@ -712,7 +712,7 @@ forget_original_parent(struct task_struc > reaper = task_child_reaper(father); > break; > } > - } while (reaper->exit_state); > + } while (reaper->flags & PF_EXITING); Yes, other changes are just cleanups. They just move some code from exit_notify to forget_original_parent(). It is a bit silly to declare ptrace_dead in exit_notify(), take tasklist, pass ptrace_dead to forget_original_parent(), unlock-lock-unlock tasklist, and then use ptrace_dead. Oleg.
creator: Oleg Nesterov
date: 2007-07-27T08:35:33-00:00

Re: [PATCH 1/15] Move exit_task_namespaces()
type: item
description: Oleg Nesterov wrote: > On 07/27, Pavel Emelyanov wrote: >> Oleg Nesterov wrote: >>> Perhaps, we can do something like the patch below. Roland, what do you >>> think? >>> >>> We can check PF_EXITING instead of ->exit_state while choosing the new >> Heh :) I've came to the same conclusion and now I'm checking for it. >> But my patch is much simpler that yours - it just checks for PF_EXITING >> in forget_original_parent: >> >> --- ./kernel/exit.c.exitfix 2007-07-27 12:13:25.000000000 +0400 >> +++ ./kernel/exit.c 2007-07-27 12:15:35.000000000 +0400 >> @@ -712,7 +712,7 @@ forget_original_parent(struct task_struc >> reaper = task_child_reaper(father); >> break; >> } >> - } while (reaper->exit_state); >> + } while (reaper->flags & PF_EXITING); > > Yes, other changes are just cleanups. They just move some code from exit_notify > to forget_original_parent(). It is a bit silly to declare ptrace_dead in > exit_notify(), take tasklist, pass ptrace_dead to forget_original_parent(), > unlock-lock-unlock tasklist, and then use ptrace_dead. :) Ok. Thanks, I then will check with your patch. If you don't mind I will carry it together with this set. > Oleg. Thanks, Pavel
creator: Pavel Emelianov
date: 2007-07-27T08:37:21-00:00

Re: [PATCH 11/15] Signal semantics
type: item
description: Damn. I don't have time to read these patches today (will try tomorrow), but when I glanced at this patch yesterday I had some suspicions... On 07/26, Pavel Emelyanov wrote: > > +++ linux-2.6.23-rc1-mm1-7/kernel/signal.c 2007-07-26 > 16:36:37.000000000 +0400 > @@ -323,6 +325,9 @@ static int collect_signal(int sig, struc > if (first) { > list_del_init(&first->list); > copy_siginfo(info, &first->info); > + if (first->flags & SIGQUEUE_CINIT) > + kinfo->flags |= KERN_SIGINFO_CINIT; > + > > [...snip...] > > @@ -1852,7 +1950,7 @@ relock: > * within that pid space. It can of course get signals from > * its parent pid space. > */ > - if (current == task_child_reaper(current)) > + if (kinfo.flags & KERN_SIGINFO_CINIT) > continue; I think the whole idea is broken, it assumes the sender put something into "struct sigqueue". Suppose that /sbin/init has no handler for (say) SIGTERM, and we send this signal from the same namespace. send_signal() sets SIGQUEUE_CINIT, but it is lost because __group_complete_signal() silently "converts" sig_fatal() signals to SIGKILL using sigaddset(). > +static void encode_sender_info(struct task_struct *t, struct sigqueue *q) > +{ > + /* > + * If sender (i.e 'current') and receiver have the same active > + * pid namespace and the receiver is the container-init, set the > + * SIGQUEUE_CINIT flag. This tells the container-init that the > + * signal originated in its own namespace and so it can choose > + * to ignore the signal. > + * > + * If the receiver is the container-init of a pid namespace, > + * but the sender is from an ancestor pid namespace, the > + * container-init cannot ignore the signal. So clear the > + * SIGQUEUE_CINIT flag in this case. > + * > + * Also, if the sender does not have a pid_t in the receiver's > + * active pid namespace, set si_pid to 0 and pretend it originated > + * from the kernel. > + */ > + if (pid_ns_equal(t)) { > + if (is_container_init(t)) { > + q->flags |= SIGQUEUE_CINIT; Ironically, this change carefully pres…
creator: Oleg Nesterov
date: 2007-07-27T12:30:12-00:00

Re: [PATCH 11/15] Signal semantics
type: item
description: Oleg Nesterov wrote: > Damn. I don't have time to read these patches today (will try tomorrow), Oh, that's OK. I was about to send the set to Andrew only the next week. This patch is the most strange one and is to be discussed a lot. We try to do the following two things: 1. signals going from the namespace, that the target task doesn't see must be seen as SI_KERNEL if siginfo is allocated; 2. signals to init of any namespace must be allowed to send from one of the parent namespaces only. From child namespace, init needs only those, that it's ready to handle (SIGCHLD). As far as I understand Suka's approach (it's his patch, so I may be not 100% correct - it's better to wait for his comments) he is trying to carry the information about the signal up to the get_signal_to_deliver(). As far as the first issue is concerned, the solution is obvious - all the "calculations" can be done at the beginning of sending the signal, but the second issue is a bit more complicated and I have no good ideas of how to solve this :( yet. Thanks, Pavel > but when I glanced at this patch yesterday I had some suspicions... > > On 07/26, Pavel Emelyanov wrote: >> +++ linux-2.6.23-rc1-mm1-7/kernel/signal.c 2007-07-26 >> 16:36:37.000000000 +0400 >> @@ -323,6 +325,9 @@ static int collect_signal(int sig, struc >> if (first) { >> list_del_init(&first->list); >> copy_siginfo(info, &first->info); >> + if (first->flags & SIGQUEUE_CINIT) >> + kinfo->flags |= KERN_SIGINFO_CINIT; >> + >> >> [...snip...] >> >> @@ -1852,7 +1950,7 @@ relock: >> * within that pid space. It can of course get signals from >> * its parent pid space. >> */ >> - if (current == task_child_reaper(current)) >> + if (kinfo.flags & KERN_SIGINFO_CINIT) >> continue; > > I think the whole idea is broken, it assumes the sender put something into > "struct sigqueue". Yup. That's the problem. It seems to me that the only way to handle init's signals is to check for permissions in the sending path. > Suppose that /sbin/init has no handler…
creator: Pavel Emelianov
date: 2007-07-27T13:38:19-00:00

Re: [PATCH 9/15] Move alloc_pid() after the namespace is cloned
type: item
description: On 07/26, Pavel Emelyanov wrote: > > This is a fix for Sukadev's patch that moved the alloc_pid() call from > do_fork() into copy_process(). ... and this patch changes almost every line from Sukadev's patch. Sorry gents, but isn't it better to ask Andrew to drop that patch (which is quite useless by itself), and send a new one which incorporates all necessary changes? Imho, it would be much easier to understand. > @@ -1406,7 +1422,13 @@ long do_fork(unsigned long clone_flags, > if (!IS_ERR(p)) { > struct completion vfork; > > - nr = pid_nr(task_pid(p)); > + /* > + * this is enough to call pid_nr_ns here, but this if > + * improves optimisation of regular fork() > + */ > + nr = (clone_flags & CLONE_NEWPID) ? > + task_pid_nr_ns(p, current->nsproxy->pid_ns) : > + task_pid_vnr(p); Shouldn't we do the same for CLONE_PARENT_SETTID in copy_process() ? Otherwise *parent_tidptr may have a wrong value which doesn't match to what fork() returns. Oleg.
creator: Oleg Nesterov
date: 2007-07-27T15:10:54-00:00

Re: [PATCH 11/15] Signal semantics
type: item
description: Quoting sukadev@us.ibm.com (sukadev@us.ibm.com): > Pavel Emelianov [xemul@openvz.org] wrote: > | Oleg Nesterov wrote: > | >Damn. I don't have time to read these patches today (will try tomorrow), > | > | Oh, that's OK. I was about to send the set to Andrew only the next week. > | > | This patch is the most strange one and is to be discussed a lot. > | > | We try to do the following two things: > | 1. signals going from the namespace, that the target task doesn't > | see must be seen as SI_KERNEL if siginfo is allocated; > | 2. signals to init of any namespace must be allowed to send from > | one of the parent namespaces only. From child namespace, init > | needs only those, that it's ready to handle (SIGCHLD). > > Yes. > > | > | As far as I understand Suka's approach (it's his patch, so I may > | be not 100% correct - it's better to wait for his comments) he is > | trying to carry the information about the signal up to the > | get_signal_to_deliver(). > | > | As far as the first issue is concerned, the solution is obvious - > | all the "calculations" can be done at the beginning of sending the > | signal, but the second issue is a bit more complicated and I have > | no good ideas of how to solve this :( yet. > > Even I am looking for a better approach. > > | > | Thanks, > | Pavel > | > | >but when I glanced at this patch yesterday I had some suspicions... > | > > | >On 07/26, Pavel Emelyanov wrote: > | >>+++ linux-2.6.23-rc1-mm1-7/kernel/signal.c 2007-07-26 > | >>16:36:37.000000000 +0400 > | >>@@ -323,6 +325,9 @@ static int collect_signal(int sig, struc > | >> if (first) { > | >> list_del_init(&first->list); > | >> copy_siginfo(info, &first->info); > | >>+ if (first->flags & SIGQUEUE_CINIT) > | >>+ kinfo->flags |= KERN_SIGINFO_CINIT; > | >>+ > | >> > | >>[...snip...] > | >> > | >>@@ -1852,7 +1950,7 @@ relock: > | >> * within that pid space. It can of course get signals from > | >> * its parent pid space. > | >> */ > | >>- if (current == task_child_reaper(current)) >…
creator: serue
date: 2007-07-27T19:59:43-00:00

Re: [PATCH 11/15] Signal semantics
type: item
description: Serge E. Hallyn [serue@us.ibm.com] wrote: | Quoting sukadev@us.ibm.com (sukadev@us.ibm.com): | > Pavel Emelianov [xemul@openvz.org] wrote: | > | Oleg Nesterov wrote: | > | >(What about ptrace_attach() btw? If it is possible to send a signal to init | > | > from the "parent" namespace, perhaps it makes sense to allow ptracing as | > | > well). | > | | > | ptracing of tasks fro different namespaces is not possible at all, since | > | strace utility determines the fork()-ed child pid from the parent's eax | > | register, which would contain the pid value as this parent sees his child. | > | But if the strace is in different namespace - it won't be able to find | > | this child with the pid value from parent's eax. | > | | > | Maybe it's worth disabling cross-namespaces ptracing... | > | > I think so too. Its probably not a serious limitation ? | | Several people think we will implement 'namespace entering' through a | ptrace hack, where maybe the admin ptraces the init in a child pidns, | makes it fork, and makes the child execute what it wants (i.e. ps -ef). | | You're talking about killing that functionality? No. I was only thinking in terms of debugging container init and missed the namespace entering part. Pavel, I am not sure I understand your comment about being unable to ptrace() a child ns. BTW, I am able to gdb a process (incl container-init) from parent ns now. | | -serge
creator: Sukadev Bhattiprolu
date: 2007-07-27T20:23:37-00:00

Re: [PATCH 5/15] Introduce struct upid
type: item
description: On 07/26, Pavel Emelyanov wrote: > > --- linux-2.6.23-rc1-mm1.orig/include/linux/pid.h 2007-07-26 16:34:45.000000000 +0400 > +++ linux-2.6.23-rc1-mm1-7/include/linux/pid.h 2007-07-26 16:36:37.000000000 +0400 > @@ -40,15 +40,21 @@ enum pid_type > * processes. > */ > > -struct pid > -{ > - atomic_t count; > +struct upid { > /* Try to keep pid_chain in the same cacheline as nr for find_pid */ > int nr; > + struct pid_namespace *ns; > struct hlist_node pid_chain; > +}; > + > +struct pid > +{ > + atomic_t count; > /* lists of tasks that use this pid */ > struct hlist_head tasks[PIDTYPE_MAX]; > struct rcu_head rcu; > + int level; > + struct upid numbers[1]; > }; Well. Definitely, the kernel can't be compiled with this patch applied, this seems to be against the rules... So. The task has a single (PIDTYPE_MAX) pid no matter how many namespaces can see it, and "struct pid" has an array of numbers for each namespace. Still I can't understand why do we need upid->ns, can't we kill it? Suppose we add "struct pid_namespace *parent_ns" to "struct pid_namespace", init_pid_ns.parent_ns == NULL. Now, struct upid { int nr; struct hlist_node pid_chain; }; struct pid { atomic_t count; struct hlist_head tasks[PIDTYPE_MAX]; struct rcu_head rcu; struct pid_namespace *active_ns; struct upid numbers[0]; }; We populate pid->numbers in "reverse" order, so that pid->numbers[0] lives in pid->active_ns. Now, for example, void free_pid(struct pid *pid) { struct pid_namespace *ns; unsigned long flags; int i; spin_lock_irqsave(&pidmap_lock, flags); for (i = 0, ns = pid->active_ns; ns; i++, ns = ns->parent_ns) hlist_del_rcu(&pid->numbers[i].pid_chain); spin_unlock_irqrestore(&pidmap_lock, flags); for (i = 0, ns = pid->active_ns; ns; i++, ns = ns->parent_ns) free_pidmap(ns, pid->numbers[i].nr); call_rcu(&pid->rcu, delayed_put_pid); } Possible? Oleg.
creator: Oleg Nesterov
date: 2007-07-29T09:50:35-00:00

Re: [PATCH 6/15] Make alloc_pid(), free_pid() and put_pid() work with struct upid
type: item
description: On 07/26, Pavel Emelyanov wrote: > > -struct pid *alloc_pid(void) > +struct pid *alloc_pid(struct pid_namespace *ns) Why? We have the only caller, copy_process(), ns == task_active_pid_ns() always. > { > struct pid *pid; > enum pid_type type; > - int nr = -1; > - struct pid_namespace *ns; > + int i, nr; > + struct pid_namespace *tmp; > > - ns = task_active_pid_ns(current); > pid = kmem_cache_alloc(ns->pid_cachep, GFP_KERNEL); > if (!pid) > goto out; > > - nr = alloc_pidmap(ns); > - if (nr < 0) > - goto out_free; > + tmp = ns; > + for (i = ns->level; i >= 0; i--) { > + nr = alloc_pidmap(tmp); > + if (nr < 0) > + goto out_free; > + > + pid->numbers[i].nr = nr; > + pid->numbers[i].ns = tmp; > + tmp = tmp->parent; Hm... There is no ->parent in "struct pid_namespace", and this patch doesn't add it. > + if (ns != &init_pid_ns) > + get_pid_ns(ns); Q: put_pid() checks "ns != &init_pid_ns" as well, this is just an optimization, yes? Perhaps we can move this check into get_pid_ns/put_pid_ns. We are doing get_pid_ns() only for the "top namespace"... I guess this can work if pid_namespace does get_pid_ns() on its ->parent. This patch looks incomplete. Oleg.
creator: Oleg Nesterov
date: 2007-07-29T10:14:55-00:00

Re: [PATCH 14/15] Destroy pid namespace on init's death
type: item
description: On 07/26, Pavel Emelyanov wrote: > > @@ -895,6 +915,7 @@ fastcall NORET_TYPE void do_exit(long co > { > struct task_struct *tsk = current; > int group_dead; > + struct pid_namespace *pid_ns = tsk->nsproxy->pid_ns; > > profile_task_exit(tsk); > > @@ -905,9 +926,10 @@ fastcall NORET_TYPE void do_exit(long co > if (unlikely(!tsk->pid)) > panic("Attempted to kill the idle task!"); > if (unlikely(tsk == task_child_reaper(tsk))) { > - if (task_active_pid_ns(tsk) != &init_pid_ns) > - task_active_pid_ns(tsk)->child_reaper = > - init_pid_ns.child_reaper; > + if (pid_ns != &init_pid_ns) { > + zap_pid_ns_processes(pid_ns); > + pid_ns->child_reaper = init_pid_ns.child_reaper; > + } > else > panic("Attempted to kill init!"); No, no, this is wrong. Yes, the current code is buggy too, I'll send the fix. I think this code should be moved below under the "if (group_dead)", and we should use tsk->group_leader. > +void zap_pid_ns_processes(struct pid_namespace *pid_ns) > +{ > + int i; > + int nr; > + int nfree; > + int options = WNOHANG|WEXITED|__WALL; > + > +repeat: > + /* > + * We know pid == 1 is terminating. Find remaining pid_ts > + * in the namespace, signal them and then wait for them > + * exit. > + */ > + nr = next_pidmap(pid_ns, 1); > + while (nr > 0) { > + kill_proc_info(SIGKILL, SEND_SIG_PRIV, nr); > + nr = next_pidmap(pid_ns, nr); > + } > + > + nr = next_pidmap(pid_ns, 1); > + while (nr > 0) { > + do_wait(nr, options, NULL, NULL, NULL); When the first child of init exits, it sends SIGCHLD. After that, do_wait() will never sleep, so we are doing a busy-wait loop. Not good, especially when we have a niced child, can livelock. > + nr = next_pidmap(pid_ns, nr); > + } > + > + nfree = 0; > + for (i = 0; i < PIDMAP_ENTRIES; i++) > + nfree += atomic_read(&pid_ns->pidmap[i].nr_free); > + > + /* > + * If pidmap has entries for processes other than 0 and 1, retry. > + */ > + if (nfree < (BITS_PER_PAGE * PIDMAP_ENTRIES - 2)) > + goto repeat; This doesn't look right. Suppose that some…
creator: Oleg Nesterov
date: 2007-07-29T10:39:47-00:00

Re: [PATCH 11/15] Signal semantics
type: item
description: On 07/27, sukadev@us.ibm.com wrote: > > Pavel Emelianov [xemul@openvz.org] wrote: > | Oleg Nesterov wrote: > | >> > | >>@@ -1852,7 +1950,7 @@ relock: > | >> * within that pid space. It can of course get signals from > | >> * its parent pid space. > | >> */ > | >>- if (current == task_child_reaper(current)) > | >>+ if (kinfo.flags & KERN_SIGINFO_CINIT) > | >> continue; > | > > | >I think the whole idea is broken, it assumes the sender put something into > | >"struct sigqueue". > | > | Yup. That's the problem. It seems to me that the only way to handle init's > | signals is to check for permissions in the sending path. > > We can check permissions in the sending path - and in fact we do check for > SIGKILL case (deny_signal_to_container_init() below). > > But the receiver knows/decides whether or not the signal is wanted/not. No ? I can't understand your question. Yes, this is what we are doing currently, but this is broken by this patch. > Are you saying we should check/special case all fatal signals ? > > | > | >Suppose that /sbin/init has no handler for (say) SIGTERM, and we send this > | >signal from the same namespace. send_signal() sets SIGQUEUE_CINIT, but it > | >is lost because __group_complete_signal() silently "converts" sig_fatal() > | >signals to SIGKILL using sigaddset(). > > Yes, I should have called it out, but this patch currently assumes /sbin/init > (or container-init) has a handler for the fatal signals like SIGTERM Changelog says nothing about that. And in that case we don't need any complications except a) deny_signal_to_container_init() (should be named deny_SIGKILL_to_container_init) and b) "cross-namespace signals must have si_code == SI_KERNEL". I don't know whether this limitation (/sbin/init must install the handler for each fatal signal) acceptable or not. However, we should also take care about sig_kernel_stop() signals, and please note that it is not possible to install a handler for SIGSTOP. > | >>+static void encode_sender_info(struct t…
creator: Oleg Nesterov
date: 2007-07-29T11:23:34-00:00

Re: [PATCH 3/15] kern_siginfo helper
type: item
description: On 07/26, Pavel Emelyanov wrote: > > TODO: This is more an exploratory patch and modifies only interfaces > necessary to implement correct signal semantics in pid namespaces. > > If the approach is feasible, we could consistently use 'kern_siginfo' > in other signal interfaces and possibly in 'struct sigqueue'. > > We could modify dequeue_signal() to directly work with 'kern_siginfo' > and remove dequeue_signal_kern_info(). Well... I know, it is very easy to blame somebody else's patch, and probably my taste is not good... But honestly, I personally think this approach is a horror, and any alternative is better :) I'd rather change dequeue_signal() so that it takes "struct sigqueue *" parameter instead of "siginfo_t *", or add a new "int *flags". OK, this doesn't work anyway, we should do something different. Perhaps just do all checks on sender's side. It is a bit strange that this patch is 3/15, and the rest bits in 11/15, not very convenient for the review. Oleg.
creator: Oleg Nesterov
date: 2007-07-29T11:40:00-00:00

Re: [PATCH 7/15] Helpers to obtain pid numbers
type: item
description: On 07/26, Pavel Emelyanov wrote: > > --- linux-2.6.23-rc1-mm1.orig/include/linux/pid.h 2007-07-26 16:34:45.000000000 +0400 > +++ linux-2.6.23-rc1-mm1-7/include/linux/pid.h 2007-07-26 16:36:37.000000000 +0400 > @@ -83,12 +92,34 @@ extern void FASTCALL(detach_pid(struct t > > extern struct pid *alloc_pid(struct pid_namespace *ns); > extern void FASTCALL(free_pid(struct pid *pid)); > + > +/* > + * the helpers to get the pid's id seen from different namespaces > + * > + * pid_nr() : global id, i.e. the id seen from the init namespace; This looks a bit strange to me, but perhaps this is just matter of taste. I think pid_nr(pid) should be pid_nr_ns(pid, current->nsproxy->pid_ns), this is imho much closer to the current meaning. I won't persist though. > +pid_t pid_nr_ns(struct pid *pid, struct pid_namespace *ns) > +{ > + pid_t nr = 0; > + if (pid && ns->level <= pid->level) > + nr = pid->numbers[ns->level].nr; > + return nr; > +} I am not sure I understand the "ns->level <= pid->level" check. Isn't it a bug to use a "wrong" namespace here? In that case BUG_ON() looks better. If ns could be wrong, "ns->level <= pid->level" is not enough, we should also check "pid->numbers[ns->level].ns == ns", no? Oleg.
creator: Oleg Nesterov
date: 2007-07-29T12:08:57-00:00

Re: [PATCH 8/15] Helpers to find the task by its numerical ids
type: item
description: On 07/26, Pavel Emelyanov wrote: > > +#define find_pid(pid) find_pid_ns(pid, &init_pid_ns) Again, I think find_pid() should use current's active ns, not init_pid_ns. Just grep for find_pid/find_task_by_pid. > --- linux-2.6.23-rc1-mm1.orig/kernel/pid.c 2007-07-26 16:34:45.000000000 +0400 > +++ linux-2.6.23-rc1-mm1-7/kernel/pid.c 2007-07-26 16:36:37.000000000 +0400 > @@ -204,19 +221,20 @@ static void delayed_put_pid(struct rcu_h > goto out; > } > > -struct pid * fastcall find_pid(int nr) > +struct pid * fastcall find_pid_ns(int nr, struct pid_namespace *ns) > { > struct hlist_node *elem; > - struct pid *pid; > + struct upid *pnr; > + > + hlist_for_each_entry_rcu(pnr, elem, > + &pid_hash[pid_hashfn(nr, ns)], pid_chain) > + if (pnr->nr == nr && pnr->ns == ns) ^^^^^^^^^^^^^ Aha, that is why we need upid->ns. I am a bit surprised we don't move the global pid_hash into the "struct pid_namespace", this could speedup the search, and we don't need upid->ns. > -struct pid *find_ge_pid(int nr) > +struct pid *find_ge_pid(int nr, struct pid_namespace *ns) > { > struct pid *pid; > > do { > - pid = find_pid(nr); > + pid = find_pid_ns(nr, ns); > if (pid) > break; > - nr = next_pidmap(task_active_pid_ns(current), nr); > + nr = next_pidmap(ns, nr); > } while (nr > 0); > > return pid; This means we should fix the caller, next_tgid(), but this is done in 15/15. Oleg.
creator: Oleg Nesterov
date: 2007-07-29T12:39:07-00:00

Re: [PATCH 15/15] Hooks over the code to show correct values to user
type: item
description: On 07/26, Pavel Emelyanov wrote: > > int > kill_proc(pid_t pid, int sig, int priv) > { > - return kill_proc_info(sig, __si_special(priv), pid); > + int ret; > + > + rcu_read_lock(); > + ret = kill_pid_info(sig, __si_special(priv), find_pid(pid)); > + rcu_read_unlock(); > + return ret; > } I think this is wrong. kill_proc() should behave the same as kill_proc_info(), so this change is not needed. With this patch they use different namespaces to find the task, this is not consistent. (sadly, this patch is huge, very difficult to review). Oleg.
creator: Oleg Nesterov
date: 2007-07-29T14:29:59-00:00

Re: [PATCH 10/15] Make each namespace has its own proc tree
type: item
description: On 07/26, Pavel Emelyanov wrote: > > static int proc_get_sb(struct file_system_type *fs_type, > int flags, const char *dev_name, void *data, struct vfsmount *mnt) > { > [...snip...] > > + if (!sb->s_root) { > + sb->s_flags = flags; > + err = proc_fill_super(sb); > + if (err) { > + up_write(&sb->s_umount); > + deactivate_super(sb); > + return err; > + } > + > + ei = PROC_I(sb->s_root->d_inode); > + if (!ei->pid) { > + rcu_read_lock(); > + ei->pid = get_pid(find_pid_ns(1, ns)); Where do we "put" this pid? > void release_task(struct task_struct * p) > { > + struct pid *pid; > struct task_struct *leader; > int zap_leader; > repeat: > @@ -160,6 +161,20 @@ repeat: > write_lock_irq(&tasklist_lock); > ptrace_unlink(p); > BUG_ON(!list_empty(&p->ptrace_list) || > !list_empty(&p->ptrace_children)); > + /* > + * we have to keep this pid till proc_flush_task() to make > + * it possible to flush all dentries holding it. pid will > + * be put ibidem > + * > + * however if the pid belogs to init namespace only, we can > + * optimize this out > + */ > + pid = task_pid(p); > + if (pid->level != 0) > + get_pid(pid); > + else > + pid = NULL; > + > __exit_signal(p); > > /* > @@ -184,7 +199,8 @@ repeat: > } > > write_unlock_irq(&tasklist_lock); > - proc_flush_task(p, NULL); > + proc_flush_task(p, pid); > + put_pid(pid); Oh, this is not nice... Look, proc_flush_task() doesn't need pid, it can use active pid_ns to iterate the ->parent chain and do proc_flush_task_mnt(), and it can check "ns->child_reaper == current" for mntput(), yes? So, can't we do instead release_task() { struct pid_namespace *my_ns = ...; // get it from pid; ... write_unlock_irq(&tasklist_lock); // __exit_signal()->...->put_pid() drops the reference, // but this ns should still be valid because /proc itself // holds a reference to it, even if we are /sbin/init. proc_flush_task(p, my_ns); ... } No? Oleg.
creator: Oleg Nesterov
date: 2007-07-29T15:56:50-00:00

Re: [PATCH 10/15] Make each namespace has its own proc tree
type: item
description: On 07/29, Oleg Nesterov wrote: > > Look, proc_flush_task() doesn't need pid, it can use active pid_ns Oops. Yes, proc_flush_task() doesn't use ->numbers[].nr, but it should? proc_flush_task_mnt() uses task->pid to flush /proc/$pid, this looks wrong, we should use ->number[ns->level].nr, no? Oleg.
creator: Oleg Nesterov
date: 2007-07-29T17:02:53-00:00

Re: [PATCH 5/15] Introduce struct upid
type: item
description: Oleg Nesterov wrote: > On 07/26, Pavel Emelyanov wrote: >> --- linux-2.6.23-rc1-mm1.orig/include/linux/pid.h 2007-07-26 16:34:45.000000000 +0400 >> +++ linux-2.6.23-rc1-mm1-7/include/linux/pid.h 2007-07-26 16:36:37.000000000 +0400 >> @@ -40,15 +40,21 @@ enum pid_type >> * processes. >> */ >> >> -struct pid >> -{ >> - atomic_t count; >> +struct upid { >> /* Try to keep pid_chain in the same cacheline as nr for find_pid */ >> int nr; >> + struct pid_namespace *ns; >> struct hlist_node pid_chain; >> +}; >> + >> +struct pid >> +{ >> + atomic_t count; >> /* lists of tasks that use this pid */ >> struct hlist_head tasks[PIDTYPE_MAX]; >> struct rcu_head rcu; >> + int level; >> + struct upid numbers[1]; >> }; > > Well. Definitely, the kernel can't be compiled with this patch applied, > this seems to be against the rules... Yes. U forgot to mention, that this patchset is git-bisect-not-safe :) I sent the safe split earlier, but it was harder to make and understand, so I decided not to waste the time and sent a badly-split set just to get comments about the approach. The ways a big patch is split wouldn't affect the comments about the ideas, bugs, etc. > So. The task has a single (PIDTYPE_MAX) pid no matter how many namespaces > can see it, and "struct pid" has an array of numbers for each namespace. > > Still I can't understand why do we need upid->ns, can't we kill it? > Suppose we add "struct pid_namespace *parent_ns" to "struct pid_namespace", > init_pid_ns.parent_ns == NULL. We already have it :) > Now, > > struct upid { > int nr; > struct hlist_node pid_chain; > }; > > struct pid > { > atomic_t count; > struct hlist_head tasks[PIDTYPE_MAX]; > struct rcu_head rcu; > struct pid_namespace *active_ns; > struct upid numbers[0]; > }; > > We populate pid->numbers in "reverse" order, so that pid->numbers[0] lives > in pid->active_ns. > > Now, for example, > > void free_pid(struct pid *pid) > { > struct pid_namespace *ns; > unsigned long flags; > int i; > > spin_lock_irqsave(&pi…
creator: Pavel Emelianov
date: 2007-07-30T05:58:44-00:00

Re: [PATCH 6/15] Make alloc_pid(), free_pid() and put_pid() work with struct upid
type: item
description: Oleg Nesterov wrote: > On 07/26, Pavel Emelyanov wrote: >> -struct pid *alloc_pid(void) >> +struct pid *alloc_pid(struct pid_namespace *ns) > > Why? We have the only caller, copy_process(), ns == task_active_pid_ns() > always. task_active_pid_ns() by newly created task, not the current! That's why we need to pass something to alloc_pid() to find this new namespace. Task or namespace itself - is the matter of choice - I selected the most obvious argument :) >> { >> struct pid *pid; >> enum pid_type type; >> - int nr = -1; >> - struct pid_namespace *ns; >> + int i, nr; >> + struct pid_namespace *tmp; >> >> - ns = task_active_pid_ns(current); >> pid = kmem_cache_alloc(ns->pid_cachep, GFP_KERNEL); >> if (!pid) >> goto out; >> >> - nr = alloc_pidmap(ns); >> - if (nr < 0) >> - goto out_free; >> + tmp = ns; >> + for (i = ns->level; i >= 0; i--) { >> + nr = alloc_pidmap(tmp); >> + if (nr < 0) >> + goto out_free; >> + >> + pid->numbers[i].nr = nr; >> + pid->numbers[i].ns = tmp; >> + tmp = tmp->parent; > > Hm... There is no ->parent in "struct pid_namespace", and this > patch doesn't add it. Parent is added in another patch - 12/15. I will split it better when sending to Andrew - patches will be smaller and bisect-safe. >> + if (ns != &init_pid_ns) >> + get_pid_ns(ns); > > Q: put_pid() checks "ns != &init_pid_ns" as well, this is just > an optimization, yes? Perhaps we can move this check into It is :) > get_pid_ns/put_pid_ns. I think you're right. > We are doing get_pid_ns() only for the "top namespace"... I guess > this can work if pid_namespace does get_pid_ns() on its ->parent. > This patch looks incomplete. Yes. This set is not well split, sorry. I wanted to get comments about the approach, bugs, etc (I have already mentioned this in another letter...) > Oleg. > > Thanks, Pavel.
creator: Pavel Emelianov
date: 2007-07-30T06:03:05-00:00

Re: [PATCH 3/15] kern_siginfo helper
type: item
description: Oleg Nesterov wrote: > On 07/26, Pavel Emelyanov wrote: >> TODO: This is more an exploratory patch and modifies only interfaces >> necessary to implement correct signal semantics in pid namespaces. >> >> If the approach is feasible, we could consistently use 'kern_siginfo' >> in other signal interfaces and possibly in 'struct sigqueue'. >> >> We could modify dequeue_signal() to directly work with 'kern_siginfo' >> and remove dequeue_signal_kern_info(). > > Well... I know, it is very easy to blame somebody else's patch, and probably > my taste is not good... > > But honestly, I personally think this approach is a horror, and any alternative > is better :) > > I'd rather change dequeue_signal() so that it takes "struct sigqueue *" > parameter instead of "siginfo_t *", or add a new "int *flags". > > OK, this doesn't work anyway, we should do something different. Perhaps > just do all checks on sender's side. Yes. Signal handling in namespaces turned out to be the most complicated part of the set. I start thinking to drop this part till we have the "core" in -mm tree. Suka, what do you think? > It is a bit strange that this patch is 3/15, and the rest bits in 11/15, > not very convenient for the review. Well, I thought that a split like 1. patches for kernel to prepare it for the set 2. the set itself is the best to review. Maybe I was wrong, but how to make this then? E.g. I have a MS_KERNOUNT patch, but its changes are used *much* later. > Oleg. > > Thanks, Pavel
creator: Pavel Emelianov
date: 2007-07-30T06:07:46-00:00

Re: [PATCH 7/15] Helpers to obtain pid numbers
type: item
description: Oleg Nesterov wrote: > On 07/26, Pavel Emelyanov wrote: >> --- linux-2.6.23-rc1-mm1.orig/include/linux/pid.h 2007-07-26 16:34:45.000000000 +0400 >> +++ linux-2.6.23-rc1-mm1-7/include/linux/pid.h 2007-07-26 16:36:37.000000000 +0400 >> @@ -83,12 +92,34 @@ extern void FASTCALL(detach_pid(struct t >> >> extern struct pid *alloc_pid(struct pid_namespace *ns); >> extern void FASTCALL(free_pid(struct pid *pid)); >> + >> +/* >> + * the helpers to get the pid's id seen from different namespaces >> + * >> + * pid_nr() : global id, i.e. the id seen from the init namespace; > > This looks a bit strange to me, but perhaps this is just matter of taste. > I think pid_nr(pid) should be pid_nr_ns(pid, current->nsproxy->pid_ns), > this is imho much closer to the current meaning. I won't persist though. pid_nr, find_task_by_pid and all other stuff, that existed in the kernel before the set are intended to work with global ids only (just as it was before the set). >> +pid_t pid_nr_ns(struct pid *pid, struct pid_namespace *ns) >> +{ >> + pid_t nr = 0; >> + if (pid && ns->level <= pid->level) >> + nr = pid->numbers[ns->level].nr; >> + return nr; >> +} > > I am not sure I understand the "ns->level <= pid->level" check. Isn't it > a bug to use a "wrong" namespace here? In that case BUG_ON() looks better. Yes, that's a check for bad namespace passed. > If ns could be wrong, "ns->level <= pid->level" is not enough, we should > also check "pid->numbers[ns->level].ns == ns", no? Yes, we should, and you're right in that we must have BUG_ON() here. > Oleg. > > Thanks, Pavel
creator: Pavel Emelianov
date: 2007-07-30T06:11:25-00:00

Re: [PATCH 8/15] Helpers to find the task by its numerical ids
type: item
description: Oleg Nesterov wrote: > On 07/26, Pavel Emelyanov wrote: >> +#define find_pid(pid) find_pid_ns(pid, &init_pid_ns) > > Again, I think find_pid() should use current's active ns, not > init_pid_ns. Just grep for find_pid/find_task_by_pid. > >> --- linux-2.6.23-rc1-mm1.orig/kernel/pid.c 2007-07-26 16:34:45.000000000 +0400 >> +++ linux-2.6.23-rc1-mm1-7/kernel/pid.c 2007-07-26 16:36:37.000000000 +0400 >> @@ -204,19 +221,20 @@ static void delayed_put_pid(struct rcu_h >> goto out; >> } >> >> -struct pid * fastcall find_pid(int nr) >> +struct pid * fastcall find_pid_ns(int nr, struct pid_namespace *ns) >> { >> struct hlist_node *elem; >> - struct pid *pid; >> + struct upid *pnr; >> + >> + hlist_for_each_entry_rcu(pnr, elem, >> + &pid_hash[pid_hashfn(nr, ns)], pid_chain) >> + if (pnr->nr == nr && pnr->ns == ns) > ^^^^^^^^^^^^^ > Aha, that is why we need upid->ns. That's it :) > I am a bit surprised we don't move the global pid_hash into the > "struct pid_namespace", this could speedup the search, and we > don't need upid->ns. Hm... Worth thinking about, but this hash itself is large enough and its size depends on the node's number of pages, so we'll have 1. either to make per-namespace hash (much) smaller; 2. or to give (too) many memory for it. >> -struct pid *find_ge_pid(int nr) >> +struct pid *find_ge_pid(int nr, struct pid_namespace *ns) >> { >> struct pid *pid; >> >> do { >> - pid = find_pid(nr); >> + pid = find_pid_ns(nr, ns); >> if (pid) >> break; >> - nr = next_pidmap(task_active_pid_ns(current), nr); >> + nr = next_pidmap(ns, nr); >> } while (nr > 0); >> >> return pid; > > This means we should fix the caller, next_tgid(), but this is done > in 15/15. Sorry :) > Oleg. > > Thank, Pavel
creator: Pavel Emelianov
date: 2007-07-30T06:15:22-00:00

Re: [PATCH 9/15] Move alloc_pid() after the namespace is cloned
type: item
description: Oleg Nesterov wrote: > On 07/26, Pavel Emelyanov wrote: >> This is a fix for Sukadev's patch that moved the alloc_pid() call from >> do_fork() into copy_process(). > > ... and this patch changes almost every line from Sukadev's patch. It does. My bad :( I have reviewed Suka's patch badly and was sure it puts the alloc_pid() right where we need this. > Sorry gents, but isn't it better to ask Andrew to drop that patch > (which is quite useless by itself), and send a new one which incorporates > all necessary changes? Imho, it would be much easier to understand. Hm... Maybe it's better to ask him to fold these patches together? >> @@ -1406,7 +1422,13 @@ long do_fork(unsigned long clone_flags, >> if (!IS_ERR(p)) { >> struct completion vfork; >> >> - nr = pid_nr(task_pid(p)); >> + /* >> + * this is enough to call pid_nr_ns here, but this if >> + * improves optimisation of regular fork() >> + */ >> + nr = (clone_flags & CLONE_NEWPID) ? >> + task_pid_nr_ns(p, current->nsproxy->pid_ns) : >> + task_pid_vnr(p); > > Shouldn't we do the same for CLONE_PARENT_SETTID in copy_process() ? > Otherwise *parent_tidptr may have a wrong value which doesn't match > to what fork() returns. Oops. We should. Thanks :) > Oleg. Thanks, Pavel
creator: Pavel Emelianov
date: 2007-07-30T06:17:10-00:00

Re: [PATCH 10/15] Make each namespace has its own proc tree
type: item
description: Oleg Nesterov wrote: > On 07/26, Pavel Emelyanov wrote: >> static int proc_get_sb(struct file_system_type *fs_type, >> int flags, const char *dev_name, void *data, struct vfsmount *mnt) >> { >> [...snip...] >> >> + if (!sb->s_root) { >> + sb->s_flags = flags; >> + err = proc_fill_super(sb); >> + if (err) { >> + up_write(&sb->s_umount); >> + deactivate_super(sb); >> + return err; >> + } >> + >> + ei = PROC_I(sb->s_root->d_inode); >> + if (!ei->pid) { >> + rcu_read_lock(); >> + ei->pid = get_pid(find_pid_ns(1, ns)); > > Where do we "put" this pid? In proc_delete_inode() >> void release_task(struct task_struct * p) >> { >> + struct pid *pid; >> struct task_struct *leader; >> int zap_leader; >> repeat: >> @@ -160,6 +161,20 @@ repeat: >> write_lock_irq(&tasklist_lock); >> ptrace_unlink(p); >> BUG_ON(!list_empty(&p->ptrace_list) || >> !list_empty(&p->ptrace_children)); >> + /* >> + * we have to keep this pid till proc_flush_task() to make >> + * it possible to flush all dentries holding it. pid will >> + * be put ibidem >> + * >> + * however if the pid belogs to init namespace only, we can >> + * optimize this out >> + */ >> + pid = task_pid(p); >> + if (pid->level != 0) >> + get_pid(pid); >> + else >> + pid = NULL; >> + >> __exit_signal(p); >> >> /* >> @@ -184,7 +199,8 @@ repeat: >> } >> >> write_unlock_irq(&tasklist_lock); >> - proc_flush_task(p, NULL); >> + proc_flush_task(p, pid); >> + put_pid(pid); > > Oh, this is not nice... > > Look, proc_flush_task() doesn't need pid, it can use active pid_ns It cannot. By the time release_task() is called the task is already exit_task_namespaces()-ed :( > to iterate the ->parent chain and do proc_flush_task_mnt(), and it > can check "ns->child_reaper == current" for mntput(), yes? > > So, can't we do instead > > release_task() > { > struct pid_namespace *my_ns = ...; // get it from pid; So, that's what you mean. Well, that's sounds reasonable. Thanks. > ... > > write_unlock_irq(&tasklist_lock); > > // __exit_signal()->...->put_pi…
creator: Pavel Emelianov
date: 2007-07-30T06:43:37-00:00

Re: [PATCH 10/15] Make each namespace has its own proc tree
type: item
description: Oleg Nesterov wrote: > On 07/29, Oleg Nesterov wrote: >> Look, proc_flush_task() doesn't need pid, it can use active pid_ns > > Oops. Yes, proc_flush_task() doesn't use ->numbers[].nr, but it should? > > proc_flush_task_mnt() uses task->pid to flush /proc/$pid, this looks wrong, > we should use ->number[ns->level].nr, no? Indeed. Hmm, why then pids were released during my tests... Looks like we do still need struct pid for proc_flush_task(). > Oleg. Thanks, Pavel
creator: Pavel Emelianov
date: 2007-07-30T06:45:46-00:00

Re: [PATCH 15/15] Hooks over the code to show correct values to user
type: item
description: Oleg Nesterov wrote: > On 07/26, Pavel Emelyanov wrote: >> int >> kill_proc(pid_t pid, int sig, int priv) >> { >> - return kill_proc_info(sig, __si_special(priv), pid); >> + int ret; >> + >> + rcu_read_lock(); >> + ret = kill_pid_info(sig, __si_special(priv), find_pid(pid)); >> + rcu_read_unlock(); >> + return ret; >> } > > I think this is wrong. kill_proc() should behave the same as kill_proc_info(), > so this change is not needed. With this patch they use different namespaces > to find the task, this is not consistent. Actually, callers of this use tsk->pid (global pid) as an argument, so find_vpid() might return wrong value. > (sadly, this patch is huge, very difficult to review). This is *very* huge, but all it does is just replace tsk->pid, find_task_by_pid, pd_nr, etc with appropriate task_pid_nr(), pid_nr_ns() and find_task_by_vpid() etc. > Oleg. Thanks, Pavel
creator: Pavel Emelianov
date: 2007-07-30T06:49:10-00:00

Re: [PATCH 11/15] Signal semantics
type: item
description: [snip] >> | Maybe it's worth disabling cross-namespaces ptracing... >> >> I think so too. Its probably not a serious limitation ? > > Several people think we will implement 'namespace entering' through a > ptrace hack, where maybe the admin ptraces the init in a child pidns, Why not implement namespace entering w/o any hacks? :) > makes it fork, and makes the child execute what it wants (i.e. ps -ef). > > You're talking about killing that functionality? No. We're talking about disabling the things that are not supposed to work at all. > -serge >
creator: Pavel Emelianov
date: 2007-07-30T09:31:44-00:00

Re: [PATCH 11/15] Signal semantics
type: item
description: sukadev@us.ibm.com wrote: > Serge E. Hallyn [serue@us.ibm.com] wrote: > | Quoting sukadev@us.ibm.com (sukadev@us.ibm.com): > | > Pavel Emelianov [xemul@openvz.org] wrote: > | > | Oleg Nesterov wrote: > | > | >(What about ptrace_attach() btw? If it is possible to send a signal to init > | > | > from the "parent" namespace, perhaps it makes sense to allow ptracing as > | > | > well). > | > | > | > | ptracing of tasks fro different namespaces is not possible at all, since > | > | strace utility determines the fork()-ed child pid from the parent's eax > | > | register, which would contain the pid value as this parent sees his child. > | > | But if the strace is in different namespace - it won't be able to find > | > | this child with the pid value from parent's eax. > | > | > | > | Maybe it's worth disabling cross-namespaces ptracing... > | > > | > I think so too. Its probably not a serious limitation ? > | > | Several people think we will implement 'namespace entering' through a > | ptrace hack, where maybe the admin ptraces the init in a child pidns, > | makes it fork, and makes the child execute what it wants (i.e. ps -ef). > | > | You're talking about killing that functionality? > > No. I was only thinking in terms of debugging container init and missed > the namespace entering part. > > Pavel, I am not sure I understand your comment about being unable to > ptrace() a child ns. Ok. We have a task with pid 100, which tries to clone the new namespace. This task fork-s and we have a new task with a couple of pids (101, 1). Then this "init" forks again and we have the third task with pids (102, 2). The problem is that when the 3rd task appears the return value from fork(), that is - the new task's pid as it is seen by it's parent (2nd task), will go to eax register (for i386) and it will be 2! But the prtaces from the initial namespace cannot address this task with pid 2. > BTW, I am able to gdb a process (incl container-init) from parent ns now. Debugging separate proc…
creator: Pavel Emelianov
date: 2007-07-30T09:34:57-00:00

Re: [PATCH 14/15] Destroy pid namespace on init's death
type: item
description: Oleg Nesterov wrote: > On 07/26, Pavel Emelyanov wrote: >> @@ -895,6 +915,7 @@ fastcall NORET_TYPE void do_exit(long co >> { >> struct task_struct *tsk = current; >> int group_dead; >> + struct pid_namespace *pid_ns = tsk->nsproxy->pid_ns; >> >> profile_task_exit(tsk); >> >> @@ -905,9 +926,10 @@ fastcall NORET_TYPE void do_exit(long co >> if (unlikely(!tsk->pid)) >> panic("Attempted to kill the idle task!"); >> if (unlikely(tsk == task_child_reaper(tsk))) { >> - if (task_active_pid_ns(tsk) != &init_pid_ns) >> - task_active_pid_ns(tsk)->child_reaper = >> - init_pid_ns.child_reaper; >> + if (pid_ns != &init_pid_ns) { >> + zap_pid_ns_processes(pid_ns); >> + pid_ns->child_reaper = init_pid_ns.child_reaper; >> + } >> else >> panic("Attempted to kill init!"); > > No, no, this is wrong. Yes, the current code is buggy too, I'll send > the fix. > > I think this code should be moved below under the "if (group_dead)", > and we should use tsk->group_leader. > >> +void zap_pid_ns_processes(struct pid_namespace *pid_ns) >> +{ >> + int i; >> + int nr; >> + int nfree; >> + int options = WNOHANG|WEXITED|__WALL; >> + >> +repeat: >> + /* >> + * We know pid == 1 is terminating. Find remaining pid_ts >> + * in the namespace, signal them and then wait for them >> + * exit. >> + */ >> + nr = next_pidmap(pid_ns, 1); >> + while (nr > 0) { >> + kill_proc_info(SIGKILL, SEND_SIG_PRIV, nr); >> + nr = next_pidmap(pid_ns, nr); >> + } >> + >> + nr = next_pidmap(pid_ns, 1); >> + while (nr > 0) { >> + do_wait(nr, options, NULL, NULL, NULL); > > When the first child of init exits, it sends SIGCHLD. After that, > do_wait() will never sleep, so we are doing a busy-wait loop. > Not good, especially when we have a niced child, can livelock. > >> + nr = next_pidmap(pid_ns, nr); >> + } >> + >> + nfree = 0; >> + for (i = 0; i < PIDMAP_ENTRIES; i++) >> + nfree += atomic_read(&pid_ns->pidmap[i].nr_free); >> + >> + /* >> + * If pidmap has entries for processes other than 0 and 1, retry. >> + */ >> + if (nfree …
creator: Pavel Emelianov
date: 2007-07-30T11:56:50-00:00

Re: [PATCH 14/15] Destroy pid namespace on init's death
type: item
description: On 07/30, Pavel Emelyanov wrote: > > Oleg Nesterov wrote: > >>+ > >>+ nfree = 0; > >>+ for (i = 0; i < PIDMAP_ENTRIES; i++) > >>+ nfree += atomic_read(&pid_ns->pidmap[i].nr_free); > >>+ > >>+ /* > >>+ * If pidmap has entries for processes other than 0 and 1, retry. > >>+ */ > >>+ if (nfree < (BITS_PER_PAGE * PIDMAP_ENTRIES - 2)) > >>+ goto repeat; > > > >This doesn't look right. > > > >Suppose that some "struct pid" was pinned from the parent namespace. > >In that case zap_pid_ns_processes() will burn CPU until put_pid(), bad. > > Nope. struct pid can be pinned, but the pidmap "fingerprint" cannot. Heh. It was specially designed this way, but I managed to forget. You are right, thanks for correcting me. > So as soon as the release_task() is called the pidmap becomes free and > we can proceed. Well, it doesn't matter, but strictly speaking this is not true. release_task()->detach_pid(PIDTYPE_PID) doesn't necessary free pidmap, it could be "used" by other tasks as PGID/SID. > However I agree with the "burn CPU" issue - wait must sleep if needed. > > >I think we can rely on forget_original_child() and do something like > >this: > > > > zap_active_ns_processes(void) > > { > > // kill all tasks in our ns and below > > kill(-1, SIGKILL); > > That would be too slow to walk through all the tasks in a node searching > for a couple of them we need. fing_ge_pid() looks better to me. OK. I personally dislike the "retry" logic (and it is not safe if we want wait() to actually sleep waiting for the child), but we can do the "kill" loop under tasklist_lock. In any case, I don't think we should use next_pid() for wait(), or check pidmap[].nr_free, > > do { > > clear_thread_flag(TIF_SIGPENDING); > > } while (wait(NULL) != -ECHLD); > > } just wait for any child until -ECHLD. After that we can return safely. Oleg.
creator: Oleg Nesterov
date: 2007-07-30T15:44:57-00:00

Re: [PATCH 9/15] Move alloc_pid() after the namespace is cloned
type: item
description: Pavel Emelianov [xemul@openvz.org] wrote: | Oleg Nesterov wrote: | >On 07/26, Pavel Emelyanov wrote: | >>This is a fix for Sukadev's patch that moved the alloc_pid() call from | >>do_fork() into copy_process(). | > | >... and this patch changes almost every line from Sukadev's patch. | | It does. My bad :( I have reviewed Suka's patch badly and was sure it | puts the alloc_pid() right where we need this. I should have reviewed Pavel's closely too. Sorry. | | >Sorry gents, but isn't it better to ask Andrew to drop that patch | >(which is quite useless by itself), and send a new one which incorporates | >all necessary changes? Imho, it would be much easier to understand. | | Hm... Maybe it's better to ask him to fold these patches together? I think so, but even dropping my patch is fine with me. | | >>@@ -1406,7 +1422,13 @@ long do_fork(unsigned long clone_flags, | >> if (!IS_ERR(p)) { | >> struct completion vfork; | >> | >>- nr = pid_nr(task_pid(p)); | >>+ /* | >>+ * this is enough to call pid_nr_ns here, but this if | >>+ * improves optimisation of regular fork() | >>+ */ | >>+ nr = (clone_flags & CLONE_NEWPID) ? | >>+ task_pid_nr_ns(p, current->nsproxy->pid_ns) : | >>+ task_pid_vnr(p); | > | >Shouldn't we do the same for CLONE_PARENT_SETTID in copy_process() ? | >Otherwise *parent_tidptr may have a wrong value which doesn't match | >to what fork() returns. | | Oops. We should. Thanks :) | | >Oleg. | | Thanks, | Pavel
creator: Sukadev Bhattiprolu
date: 2007-07-30T23:43:11-00:00

Re: [PATCH 3/15] kern_siginfo helper
type: item
description: Pavel Emelianov [xemul@openvz.org] wrote: | Oleg Nesterov wrote: | >On 07/26, Pavel Emelyanov wrote: | >>TODO: This is more an exploratory patch and modifies only | >>interfaces | >> necessary to implement correct signal semantics in pid namespaces. | >> | >> If the approach is feasible, we could consistently use 'kern_siginfo' | >> in other signal interfaces and possibly in 'struct sigqueue'. | >> | >> We could modify dequeue_signal() to directly work with 'kern_siginfo' | >> and remove dequeue_signal_kern_info(). | > | >Well... I know, it is very easy to blame somebody else's patch, and | >probably | >my taste is not good... | > | >But honestly, I personally think this approach is a horror, and any | >alternative | >is better :) Hmm. My reasoning was that since siginfo_t was directly "shared" with user space, extending it even to add a flag was pain. | > | >I'd rather change dequeue_signal() so that it takes "struct sigqueue *" | >parameter instead of "siginfo_t *", or add a new "int *flags". My earlier version to Containers@ passed in "int *signal_cinit" couple of levels down and used that in get_signal_to_deliver() but that looked ugly :-) Passing in sigqueue to dequeue_signal() may be better, but anyway... | > | >OK, this doesn't work anyway, we should do something different. Perhaps | >just do all checks on sender's side. | | Yes. Signal handling in namespaces turned out to be the most complicated | part of the set. I start thinking to drop this part till we have the "core" | in -mm tree. Suka, what do you think? Yes. Lets focus on the core for now and allow privileged user in a child-ns to terminate the container-init. I will try the signal approach from sender side also. | | >It is a bit strange that this patch is 3/15, and the rest bits in 11/15, | >not very convenient for the review. | | Well, I thought that a split like | 1. patches for kernel to prepare it for the set | 2. the set itself | is the best to review. Maybe I was wrong, but how to make this th…
creator: Sukadev Bhattiprolu
date: 2007-07-31T00:21:38-00:00

Re: [PATCH 9/15] Move alloc_pid() after the namespace is cloned
type: item
description: Pavel Emelianov [xemul@openvz.org] wrote: | When we create new namespace we will need to allocate the struct pid, | that will have one extra struct upid in array, comparing to the parent. | Thus we need to know the new namespace (if any) in alloc_pid() to init | this struct upid properly, so move the alloc_pid() call lower in | copy_process(). | | This is a fix for Sukadev's patch that moved the alloc_pid() call from | do_fork() into copy_process(). | | Signed-off-by: Pavel Emelyanov <xemul@openvz.org> | | --- | | fork.c | 59 ++++++++++++++++++++++++++++++++++++++--------------------- | 1 files changed, 38 insertions(+), 21 deletions(-) | | diff -upr linux-2.6.23-rc1-mm1.orig/kernel/fork.c | linux-2.6.23-rc1-mm1-7/kernel/fork.c | --- linux-2.6.23-rc1-mm1.orig/kernel/fork.c 2007-07-26 | 16:34:45.000000000 +0400 | +++ linux-2.6.23-rc1-mm1-7/kernel/fork.c 2007-07-26 | 16:36:37.000000000 +0400 | @@ -1028,16 +1029,9 @@ static struct task_struct *copy_process( | if (p->binfmt && !try_module_get(p->binfmt->module)) | goto bad_fork_cleanup_put_domain; | | - if (pid != &init_struct_pid) { | - pid = alloc_pid(); | - if (!pid) | - goto bad_fork_put_binfmt_module; | - } | - | p->did_exec = 0; | delayacct_tsk_init(p); /* Must remain after dup_task_struct() */ | copy_flags(clone_flags, p); | - p->pid = pid_nr(pid); | INIT_LIST_HEAD(&p->children); | INIT_LIST_HEAD(&p->sibling); | p->vfork_done = NULL; | @@ -1112,10 +1106,6 @@ static struct task_struct *copy_process( | p->blocked_on = NULL; /* not blocked yet */ | #endif | | - p->tgid = p->pid; | - if (clone_flags & CLONE_THREAD) | - p->tgid = current->tgid; | - | if ((retval = security_task_alloc(p))) | goto bad_fork_cleanup_policy; | if ((retval = audit_alloc(p))) | @@ -1141,6 +1131,17 @@ static struct task_struct *copy_process( | if (retval) | goto bad_fork_cleanup_namespaces; | | + if (pid != &init_struct_pid) { | + pid = alloc_pid(task_active_pid_ns(p)); | + if (!pid) | + goto bad_fork_cleanup_namespaces; We should set retval …
creator: Sukadev Bhattiprolu
date: 2007-07-31T05:49:11-00:00

Re: [PATCH 14/15] Destroy pid namespace on init's death
type: item
description: On 07/30, sukadev@us.ibm.com wrote: > > --- lx26-23-rc1-mm1.orig/kernel/exit.c 2007-07-26 20:08:16.000000000 -0700 > +++ lx26-23-rc1-mm1/kernel/exit.c 2007-07-30 23:10:30.000000000 -0700 > @@ -915,6 +915,7 @@ fastcall NORET_TYPE void do_exit(long co > { > struct task_struct *tsk = current; > int group_dead; > + struct pid_namespace *pid_ns = tsk->nsproxy->pid_ns; > > profile_task_exit(tsk); > > @@ -925,9 +926,10 @@ fastcall NORET_TYPE void do_exit(long co > if (unlikely(!tsk->pid)) > panic("Attempted to kill the idle task!"); > if (unlikely(tsk == task_child_reaper(tsk))) { > - if (task_active_pid_ns(tsk) != &init_pid_ns) > - task_active_pid_ns(tsk)->child_reaper = > - init_pid_ns.child_reaper; > + if (pid_ns != &init_pid_ns) { > + zap_pid_ns_processes(pid_ns); > + pid_ns->child_reaper = init_pid_ns.child_reaper; > + } > else > panic("Attempted to kill init!"); > } Just to remind you, this is not right when init is multi-threaded, we should do this only when the last thread exits. > -static long do_wait(pid_t pid, int options, struct siginfo __user *infop, > +long do_wait(pid_t pid, int options, struct siginfo __user *infop, > int __user *stat_addr, struct rusage __user *ru) Small nit, other in-kernel reapers use sys_wait4(), perhaps we can use it too, in that case we don't need to export do_wait(). > +void zap_pid_ns_processes(struct pid_namespace *pid_ns) > +{ > + int nr; > + int rc; > + int options = WEXITED|__WALL; > + > + /* > + * We know pid == 1 is terminating. Find remaining pid_ts > + * in the namespace, signal them and then wait for them > + * exit. > + */ > + nr = next_pidmap(pid_ns, 1); > + while (nr > 0) { > + kill_proc_info(SIGKILL, SEND_SIG_PRIV, nr); > + nr = next_pidmap(pid_ns, nr); > + } Without tasklist_lock held this is not reliable. Oleg.
creator: Oleg Nesterov
date: 2007-07-31T09:07:20-00:00

Re: [PATCH 15/15] Hooks over the code to show correct values to user
type: item
description: On 07/30, Pavel Emelyanov wrote: > > Oleg Nesterov wrote: > >On 07/26, Pavel Emelyanov wrote: > >>int > >>kill_proc(pid_t pid, int sig, int priv) > >>{ > >>- return kill_proc_info(sig, __si_special(priv), pid); > >>+ int ret; > >>+ > >>+ rcu_read_lock(); > >>+ ret = kill_pid_info(sig, __si_special(priv), find_pid(pid)); > >>+ rcu_read_unlock(); > >>+ return ret; > >>} > > > >I think this is wrong. kill_proc() should behave the same as > >kill_proc_info(), > >so this change is not needed. With this patch they use different namespaces > >to find the task, this is not consistent. > > Actually, callers of this use tsk->pid (global pid) as an argument, so > find_vpid() might return wrong value. Yes I see. But still I don't agree on this issue. kill_proc() is a simple wrapper on top of kill_proc_info(), not good to break this. And with this patch they use different namespaces to search the pid. Imho, not consistent. Probably we can ignore this for now, but suppose we have some out-of-tree driver which does kill_proc(pid_number), and the application from non-init namespace does ioctl(SET_PID_NUMBER, getpid()). And this is why btw I think find_pid/pid_nr should use active namespace, not init_pid_ns. That driver can save "struct task_struct*" or "struct pid*". OK, I understand it is a pain to "fix" the in-tree callers of kill_proc() (say, we can introduce kill_pid_t() or something), so let's forget this. In fact, we'd better remove kill_proc(), we should avoid using pid_t, the callers should be converted to use struct pid. Oleg.
creator: Oleg Nesterov
date: 2007-07-31T10:04:20-00:00

Re: [PATCH 14/15] Destroy pid namespace on init's death
type: item
description: Oleg Nesterov [oleg@tv-sign.ru] wrote: | On 07/30, sukadev@us.ibm.com wrote: | > | > --- lx26-23-rc1-mm1.orig/kernel/exit.c 2007-07-26 20:08:16.000000000 -0700 | > +++ lx26-23-rc1-mm1/kernel/exit.c 2007-07-30 23:10:30.000000000 -0700 | > @@ -915,6 +915,7 @@ fastcall NORET_TYPE void do_exit(long co | > { | > struct task_struct *tsk = current; | > int group_dead; | > + struct pid_namespace *pid_ns = tsk->nsproxy->pid_ns; | > | > profile_task_exit(tsk); | > | > @@ -925,9 +926,10 @@ fastcall NORET_TYPE void do_exit(long co | > if (unlikely(!tsk->pid)) | > panic("Attempted to kill the idle task!"); | > if (unlikely(tsk == task_child_reaper(tsk))) { | > - if (task_active_pid_ns(tsk) != &init_pid_ns) | > - task_active_pid_ns(tsk)->child_reaper = | > - init_pid_ns.child_reaper; | > + if (pid_ns != &init_pid_ns) { | > + zap_pid_ns_processes(pid_ns); | > + pid_ns->child_reaper = init_pid_ns.child_reaper; | > + } | > else | > panic("Attempted to kill init!"); | > } | | Just to remind you, this is not right when init is multi-threaded, | we should do this only when the last thread exits. Sorry, I needed to clarify somethings about the multi-threaded init. I got the impresssion that you were sending a patch for the existing bug, and meant to review/clarify in the context of the patch. Anyways, re: requirements for multi-threaded init: Our current definition of is_container_init() and task_child_reaper() refer only to the main-thread of the container-init (since they check for pid_t == 1) If the main-thread is exiting and is the last thread in the group, we want terminate other processes in the pid ns (simple case). If the main thread is exiting, but is not the last thread in the group, should we let it exit and let the next thread in the group the reaper of the pid ns ? Then we would have the pid ns w/o a container-init (i.e reaper does not have a pid_t == 1, but probably does not matter). And, when this last thread is exiting, we want to terminate other processes in the ns righ…
creator: Sukadev Bhattiprolu
date: 2007-08-01T06:16:16-00:00

Re: [PATCH 14/15] Destroy pid namespace on init's death
type: item
description: On Tue, 2007-07-31 at 23:16 -0700, sukadev@us.ibm.com wrote: > Oleg Nesterov [oleg@tv-sign.ru] wrote: > | On 07/30, sukadev@us.ibm.com wrote: > | > > | > --- lx26-23-rc1-mm1.orig/kernel/exit.c 2007-07-26 20:08:16.000000000 -0700 > | > +++ lx26-23-rc1-mm1/kernel/exit.c 2007-07-30 23:10:30.000000000 -0700 > | > @@ -915,6 +915,7 @@ fastcall NORET_TYPE void do_exit(long co > | > { > | > struct task_struct *tsk = current; > | > int group_dead; > | > + struct pid_namespace *pid_ns = tsk->nsproxy->pid_ns; > | > > | > profile_task_exit(tsk); > | > > | > @@ -925,9 +926,10 @@ fastcall NORET_TYPE void do_exit(long co > | > if (unlikely(!tsk->pid)) > | > panic("Attempted to kill the idle task!"); > | > if (unlikely(tsk == task_child_reaper(tsk))) { > | > - if (task_active_pid_ns(tsk) != &init_pid_ns) > | > - task_active_pid_ns(tsk)->child_reaper = > | > - init_pid_ns.child_reaper; > | > + if (pid_ns != &init_pid_ns) { > | > + zap_pid_ns_processes(pid_ns); > | > + pid_ns->child_reaper = init_pid_ns.child_reaper; > | > + } > | > else > | > panic("Attempted to kill init!"); > | > } > | > | Just to remind you, this is not right when init is multi-threaded, > | we should do this only when the last thread exits. > > Sorry, I needed to clarify somethings about the multi-threaded init. I > got the impresssion that you were sending a patch for the existing bug, > and meant to review/clarify in the context of the patch. > > Anyways, re: requirements for multi-threaded init: > > Our current definition of is_container_init() and task_child_reaper() > refer only to the main-thread of the container-init (since they check > for pid_t == 1) Remember, the "pid" is actually a tgid: asmlinkage long sys_getpid(void) { return current->tgid; } So, there are multiple tasks with a "pid" == 1 with a multithreaded init. > If the main-thread is exiting and is the last thread in the group, > we want terminate other processes in the pid ns (simple case). > > If the main thread is exiting, but is not the la…
creator: Dave Hansen
date: 2007-08-01T16:00:30-00:00

Re: [PATCH 11/15] Signal semantics
type: item
description: Quoting Pavel Emelyanov (xemul@openvz.org): > [snip] > > >>| Maybe it's worth disabling cross-namespaces ptracing... > >> > >>I think so too. Its probably not a serious limitation ? > > > >Several people think we will implement 'namespace entering' through a > >ptrace hack, where maybe the admin ptraces the init in a child pidns, > > Why not implement namespace entering w/o any hacks? :) I did, as a patch on top of the nsproxy container subsystem. The response was that that is a hack, and ptrace is cleaner :) So the current options for namespace entering would be: * using Cedric's bind_ns() functionality, which assigns an integer global id to a namespace, and allows a process to enter a namespace by that global id * using my nsproxy container subsystem patch, which lets a process enter another namespace using echo pid > /container/some/cont/directory/tasks and eventually might allow construction of custom namespaces, i.e. mkdir /container/c1/c2 ln -s /container/c1/c1/network /container/c1/c2/network echo $$ > /container/c1/c2/tasks * using ptrace to coerce a process in the target namespace into forking and executing the desired program. > >makes it fork, and makes the child execute what it wants (i.e. ps -ef). > > > >You're talking about killing that functionality? > > No. We're talking about disabling the things that are not supposed > to work at all. Uh, well in the abstract that sounds like a sound policy... -serge
creator: serue
date: 2007-08-01T16:13:35-00:00

Re: [PATCH 14/15] Destroy pid namespace on init's death
type: item
description: On 07/31, sukadev@us.ibm.com wrote: > > Oleg Nesterov [oleg@tv-sign.ru] wrote: > | > > | > @@ -925,9 +926,10 @@ fastcall NORET_TYPE void do_exit(long co > | > if (unlikely(!tsk->pid)) > | > panic("Attempted to kill the idle task!"); > | > if (unlikely(tsk == task_child_reaper(tsk))) { > | > - if (task_active_pid_ns(tsk) != &init_pid_ns) > | > - task_active_pid_ns(tsk)->child_reaper = > | > - init_pid_ns.child_reaper; > | > + if (pid_ns != &init_pid_ns) { > | > + zap_pid_ns_processes(pid_ns); > | > + pid_ns->child_reaper = init_pid_ns.child_reaper; > | > + } > | > else > | > panic("Attempted to kill init!"); > | > } > | > | Just to remind you, this is not right when init is multi-threaded, > | we should do this only when the last thread exits. > > Sorry, I needed to clarify somethings about the multi-threaded init. I > got the impresssion that you were sending a patch for the existing bug, > and meant to review/clarify in the context of the patch. Ah, sorry, I forgot to send the patch to fix the bug in mainline. Will try to do tomorrow, please feel free to do this if you wish. > Our current definition of is_container_init() and task_child_reaper() > refer only to the main-thread of the container-init (since they check > for pid_t == 1) Yes. > If the main-thread is exiting and is the last thread in the group, > we want terminate other processes in the pid ns (simple case). Yes. > If the main thread is exiting, but is not the last thread in the > group, should we let it exit and let the next thread in the group > the reaper of the pid ns ? We can, but why? The main thread's task_struct can't go away until all sub-threads exit. Its ->nsproxy will be NULL, but this doesn't matter. > Then we would have the pid ns w/o a container-init (i.e reaper > does not have a pid_t == 1, but probably does not matter). > > And, when this last thread is exiting, we want to terminate other > processes in the ns right ? Yes, when this last thread is exiting, the entire process is exiting.…
creator: Oleg Nesterov
date: 2007-08-01T19:48:11-00:00

Re: [PATCH 14/15] Destroy pid namespace on init's death
type: item
description: On 08/01, Dave Hansen wrote: > > > If the main thread is exiting, but is not the last thread in the > > group, should we let it exit and let the next thread in the group > > the reaper of the pid ns ? > > Well, what happens with a multithreaded init today? As it was already discussed, the current code is buggy, and should be fixed. Oleg.
creator: Oleg Nesterov
date: 2007-08-01T19:51:13-00:00

Re: [PATCH 14/15] Destroy pid namespace on init's death
type: item
description: Oleg Nesterov [oleg@tv-sign.ru] wrote: | On 07/31, sukadev@us.ibm.com wrote: | > | > Oleg Nesterov [oleg@tv-sign.ru] wrote: | > | > | > | > @@ -925,9 +926,10 @@ fastcall NORET_TYPE void do_exit(long co | > | > if (unlikely(!tsk->pid)) | > | > panic("Attempted to kill the idle task!"); | > | > if (unlikely(tsk == task_child_reaper(tsk))) { | > | > - if (task_active_pid_ns(tsk) != &init_pid_ns) | > | > - task_active_pid_ns(tsk)->child_reaper = | > | > - init_pid_ns.child_reaper; | > | > + if (pid_ns != &init_pid_ns) { | > | > + zap_pid_ns_processes(pid_ns); | > | > + pid_ns->child_reaper = init_pid_ns.child_reaper; | > | > + } | > | > else | > | > panic("Attempted to kill init!"); | > | > } | > | | > | Just to remind you, this is not right when init is multi-threaded, | > | we should do this only when the last thread exits. | > | > Sorry, I needed to clarify somethings about the multi-threaded init. I | > got the impresssion that you were sending a patch for the existing bug, | > and meant to review/clarify in the context of the patch. | | Ah, sorry, I forgot to send the patch to fix the bug in mainline. | Will try to do tomorrow, please feel free to do this if you wish. I can do that, but am still a bit confused about this multi-threaded init :-) | | > Our current definition of is_container_init() and task_child_reaper() | > refer only to the main-thread of the container-init (since they check | > for pid_t == 1) | | Yes. This means that we cannot have a check like "tsk == task_child_reaper(tsk)" to properly detect the child reaper process right ? Its basically a very dumb question - How do we detect a container_init() in the multi-threaded case ? Should we use "task->tgid == 1" ? IOW to identify if the last thread of a child reaper is exiting, should we check "task->tgid == 1" and the "group_dead" flag in do_exit() ? | | > If the main-thread is exiting and is the last thread in the group, | > we want terminate other processes in the pid ns (simple case). | | Yes. | …
creator: Sukadev Bhattiprolu
date: 2007-08-02T07:29:58-00:00

Re: [PATCH 14/15] Destroy pid namespace on init's death
type: item
description: Dave Hansen [haveblue@us.ibm.com] wrote: | On Tue, 2007-07-31 at 23:16 -0700, sukadev@us.ibm.com wrote: | > Oleg Nesterov [oleg@tv-sign.ru] wrote: | > | On 07/30, sukadev@us.ibm.com wrote: | > | > | > | > --- lx26-23-rc1-mm1.orig/kernel/exit.c 2007-07-26 20:08:16.000000000 -0700 | > | > +++ lx26-23-rc1-mm1/kernel/exit.c 2007-07-30 23:10:30.000000000 -0700 | > | > @@ -915,6 +915,7 @@ fastcall NORET_TYPE void do_exit(long co | > | > { | > | > struct task_struct *tsk = current; | > | > int group_dead; | > | > + struct pid_namespace *pid_ns = tsk->nsproxy->pid_ns; | > | > | > | > profile_task_exit(tsk); | > | > | > | > @@ -925,9 +926,10 @@ fastcall NORET_TYPE void do_exit(long co | > | > if (unlikely(!tsk->pid)) | > | > panic("Attempted to kill the idle task!"); | > | > if (unlikely(tsk == task_child_reaper(tsk))) { | > | > - if (task_active_pid_ns(tsk) != &init_pid_ns) | > | > - task_active_pid_ns(tsk)->child_reaper = | > | > - init_pid_ns.child_reaper; | > | > + if (pid_ns != &init_pid_ns) { | > | > + zap_pid_ns_processes(pid_ns); | > | > + pid_ns->child_reaper = init_pid_ns.child_reaper; | > | > + } | > | > else | > | > panic("Attempted to kill init!"); | > | > } | > | | > | Just to remind you, this is not right when init is multi-threaded, | > | we should do this only when the last thread exits. | > | > Sorry, I needed to clarify somethings about the multi-threaded init. I | > got the impresssion that you were sending a patch for the existing bug, | > and meant to review/clarify in the context of the patch. | > | > Anyways, re: requirements for multi-threaded init: | > | > Our current definition of is_container_init() and task_child_reaper() | > refer only to the main-thread of the container-init (since they check | > for pid_t == 1) | | Remember, the "pid" is actually a tgid: | | asmlinkage long sys_getpid(void) | { | return current->tgid; | } | | So, there are multiple tasks with a "pid" == 1 with a multithreaded | init. Yes, and so am now wondering if is_containe…
creator: Sukadev Bhattiprolu
date: 2007-08-02T07:37:00-00:00

Re: [PATCH 11/15] Signal semantics
type: item
description: Serge E. Hallyn wrote: > Quoting Pavel Emelyanov (xemul@openvz.org): > >>[snip] >> >> >>>>| Maybe it's worth disabling cross-namespaces ptracing... >>>> >>>>I think so too. Its probably not a serious limitation ? >>> >>>Several people think we will implement 'namespace entering' through a >>>ptrace hack, where maybe the admin ptraces the init in a child pidns, >> >>Why not implement namespace entering w/o any hacks? :) > > > I did, as a patch on top of the nsproxy container subsystem. The > response was that that is a hack, and ptrace is cleaner :) > > So the current options for namespace entering would be: > > * using Cedric's bind_ns() functionality, which assigns an > integer global id to a namespace, and allows a process to > enter a namespace by that global id looks more or less good and what OVZ actually does. So I would prefer this one. > * using my nsproxy container subsystem patch, which lets > a process enter another namespace using > echo pid > /container/some/cont/directory/tasks > and eventually might allow construction of custom > namespaces, i.e. > mkdir /container/c1/c2 > ln -s /container/c1/c1/network /container/c1/c2/network > echo $$ > /container/c1/c2/tasks Sound ok and logical as well. > * using ptrace to coerce a process in the target namespace > into forking and executing the desired program. you'll need to change ptrace interface in this case imho... doesn't sound ok at all... at least for me. So I agree with Pavel. >>>makes it fork, and makes the child execute what it wants (i.e. ps -ef). >>> >>>You're talking about killing that functionality? >> >>No. We're talking about disabling the things that are not supposed >>to work at all. > > > Uh, well in the abstract that sounds like a sound policy... Pavel simply meant that no one plans to disable functionality in question. Thanks, Kirill _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listin…
creator: dev
date: 2007-08-02T08:35:32-00:00

Re: [PATCH 14/15] Destroy pid namespace on init's death
type: item
description: Oleg Nesterov wrote: > On 08/01, Dave Hansen wrote: > >>> If the main thread is exiting, but is not the last thread in the >>> group, should we let it exit and let the next thread in the group >>> the reaper of the pid ns ? >> >>Well, what happens with a multithreaded init today? > > > As it was already discussed, the current code is buggy, and should be > fixed. I'm not that sure it MUST be fixed. There are no multi-threaded init's anywhere. Oleg, does it worth changing without reasons? Thanks, Kirill _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containers
creator: dev
date: 2007-08-02T08:37:55-00:00

Re: [PATCH 14/15] Destroy pid namespace on init's death
type: item
description: On 08/02, sukadev@us.ibm.com wrote: > > Oleg Nesterov [oleg@tv-sign.ru] wrote: > | On 07/31, sukadev@us.ibm.com wrote: > | > > | > Oleg Nesterov [oleg@tv-sign.ru] wrote: > | > | > > | > | > @@ -925,9 +926,10 @@ fastcall NORET_TYPE void do_exit(long co > | > | > if (unlikely(!tsk->pid)) > | > | > panic("Attempted to kill the idle task!"); > | > | > if (unlikely(tsk == task_child_reaper(tsk))) { > | > | > - if (task_active_pid_ns(tsk) != &init_pid_ns) > | > | > - task_active_pid_ns(tsk)->child_reaper = > | > | > - init_pid_ns.child_reaper; > | > | > + if (pid_ns != &init_pid_ns) { > | > | > + zap_pid_ns_processes(pid_ns); > | > | > + pid_ns->child_reaper = init_pid_ns.child_reaper; OOPS. I didn't notice this before, but this is not right too (regardless of multi-threaded init problems). We should not "reset" ->child_reaper here, we may have exiting tasks which will re-parent their ->children to global init. No, we are still /sbin/init of this namespace even if we are exiting, ->child_reaper should point to us, at least until zap_pid_ns_processes() completes. > | > Our current definition of is_container_init() and task_child_reaper() > | > refer only to the main-thread of the container-init (since they check > | > for pid_t == 1) > | > | Yes. > > This means that we cannot have a check like "tsk == task_child_reaper(tsk)" > to properly detect the child reaper process right ? Yes, we should use "tsk->group_leader == task_child_reaper(tsk)" > Its basically a very dumb question - How do we detect a container_init() > in the multi-threaded case ? Good point. I think is_container_init(tsk) needs a fix: - pid = task_pid(tsk); + pid = task_pid(tsk->group_leader); Or, perhaps better, change the callers to use tsk->group_leader. > Should we use "task->tgid == 1" ? No, no, "task->tgid == 1" means "global" init. > IOW to identify if the last thread of a child reaper is exiting, should we > check "task->tgid == 1" and the "group_dead" flag in do_exit() ? See above, but yes, as I sa…
creator: Oleg Nesterov
date: 2007-08-02T15:39:34-00:00

Re: [PATCH 14/15] Destroy pid namespace on init's death
type: item
description: On 08/02, Kirill Korotaev wrote: > > Oleg Nesterov wrote: > > On 08/01, Dave Hansen wrote: > > > >>> If the main thread is exiting, but is not the last thread in the > >>> group, should we let it exit and let the next thread in the group > >>> the reaper of the pid ns ? > >> > >>Well, what happens with a multithreaded init today? > > > > > > As it was already discussed, the current code is buggy, and should be > > fixed. > > I'm not that sure it MUST be fixed. There are no multi-threaded init's anywhere. > Oleg, does it worth changing without reasons? I don't know. But the kernel already tries to support multi-threaded init's. Look at de_thread(), it could be simplified a bit (and we don't need tasklist lock for zap_other_threads()) if we forbid them. Still. A non-root user does clone(CLONE_PIDNS), then clone(CLONE_THREAD), and sys_exit() from the main thread, then proceeds with fork()s. Now this ns has the global init as a child reaper, and admin can't kill entire pid_ns by killing its init. Worse, (see the reply to Sukadev' message), we should not reset pid_ns->child_reaper before zap_pid_ns_processes(). In that case ->child_reaper points to the freed task when the last thread exits, this means the non-root user can crash the kernel. Or, some embedded system uses multi-threaded init, and the kernel panics when the main thread exits. Perhaps this is just a "quality of implementation" question. sys_exit() from the main thread should be OK, why /sbin/init should be special? That said, I personally do not think that multi-threaded init is terribly useful. Oleg. _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containers
creator: Oleg Nesterov
date: 2007-08-02T16:08:51-00:00

Re: [PATCH 1/15] Move exit_task_namespaces()
type: item
description: On 07/26, Pavel Emelyanov wrote: > > The reason to release namespaces after reparenting is that when task > exits it may send a signal to its parent (SIGCHLD), but if the parent > has already exited its namespaces there will be no way to decide what > pid to dever to him - parent can be from different namespace. I almost forgot about this one... After reading the whole series, I can't understand the above explanation any longer. The parent can't be from different namespace, either we have another sub-thread, or we reparent the child to /sbin/init which should be from the same namespace. If we are /sbin/init, we kill all tasks from the same namespace before doing exit_notify(). Could you clarify? Oleg.
creator: Oleg Nesterov
date: 2007-08-02T16:18:24-00:00

Re: [PATCH 14/15] Destroy pid namespace on init's death
type: item
description: On 08/02, Oleg Nesterov wrote: > > On 08/02, Kirill Korotaev wrote: > > > > Oleg Nesterov wrote: > > > > > > As it was already discussed, the current code is buggy, and should be > > > fixed. > > > > I'm not that sure it MUST be fixed. There are no multi-threaded init's anywhere. > > Oleg, does it worth changing without reasons? > > I don't know. But the kernel already tries to support multi-threaded init's. > Look at de_thread(), it could be simplified a bit (and we don't need tasklist > lock for zap_other_threads()) if we forbid them. > > Still. A non-root user does clone(CLONE_PIDNS), then clone(CLONE_THREAD), > and sys_exit() from the main thread, then proceeds with fork()s. Now this > ns has the global init as a child reaper, and admin can't kill entire pid_ns > by killing its init. Worse, (see the reply to Sukadev' message), we should > not reset pid_ns->child_reaper before zap_pid_ns_processes(). In that case > ->child_reaper points to the freed task when the last thread exits, this > means the non-root user can crash the kernel. > > Or, some embedded system uses multi-threaded init, and the kernel panics > when the main thread exits. > > Perhaps this is just a "quality of implementation" question. sys_exit() > from the main thread should be OK, why /sbin/init should be special? > > That said, I personally do not think that multi-threaded init is terribly > useful. So I think the patch below makes sense for now. Note that it removes the games with pid_ns->child_reaper: this doesn't work currently, and this has to be modified when we actually support pid namespaces anyway. Oleg. --- t/kernel/exit.c~MTINIT 2007-07-28 16:58:17.000000000 +0400 +++ t/kernel/exit.c 2007-08-02 20:59:59.000000000 +0400 @@ -895,6 +895,14 @@ static void check_stack_usage(void) static inline void check_stack_usage(void) {} #endif +static inline void exit_child_reaper(struct task_struct *tsk) +{ + if (likely(tsk->group_leader != child_reaper(tsk))) + return; + + panic("Attempted to kill …
creator: Oleg Nesterov
date: 2007-08-02T17:08:20-00:00

Re: [PATCH 14/15] Destroy pid namespace on init's death
type: item
description: Oleg Nesterov [oleg@tv-sign.ru] wrote: | On 08/02, sukadev@us.ibm.com wrote: | > | > Oleg Nesterov [oleg@tv-sign.ru] wrote: | > | On 07/31, sukadev@us.ibm.com wrote: | > | > | > | > Oleg Nesterov [oleg@tv-sign.ru] wrote: | > | > | > | > | > | > @@ -925,9 +926,10 @@ fastcall NORET_TYPE void do_exit(long co | > | > | > if (unlikely(!tsk->pid)) | > | > | > panic("Attempted to kill the idle task!"); | > | > | > if (unlikely(tsk == task_child_reaper(tsk))) { | > | > | > - if (task_active_pid_ns(tsk) != &init_pid_ns) | > | > | > - task_active_pid_ns(tsk)->child_reaper = | > | > | > - init_pid_ns.child_reaper; | > | > | > + if (pid_ns != &init_pid_ns) { | > | > | > + zap_pid_ns_processes(pid_ns); | > | > | > + pid_ns->child_reaper = init_pid_ns.child_reaper; | | OOPS. I didn't notice this before, but this is not right too (regardless | of multi-threaded init problems). | | We should not "reset" ->child_reaper here, we may have exiting tasks | which will re-parent their ->children to global init. | | No, we are still /sbin/init of this namespace even if we are exiting, | ->child_reaper should point to us, at least until zap_pid_ns_processes() | completes. Yes, we are resetting the reaper _after_ zap_pid_ns_processes() completes right ? (all other processes in the namespace must have exited). | | > | > Our current definition of is_container_init() and task_child_reaper() | > | > refer only to the main-thread of the container-init (since they check | > | > for pid_t == 1) | > | | > | Yes. | > | > This means that we cannot have a check like "tsk == task_child_reaper(tsk)" | > to properly detect the child reaper process right ? | | Yes, we should use "tsk->group_leader == task_child_reaper(tsk)" Ok. | | > Its basically a very dumb question - How do we detect a container_init() | > in the multi-threaded case ? | | Good point. I think is_container_init(tsk) needs a fix: | | - pid = task_pid(tsk); | + pid = task_pid(tsk->group_leader); | Ok. | Or, perhaps better, change the caller…
creator: Sukadev Bhattiprolu
date: 2007-08-02T17:20:33-00:00

Re: [PATCH 14/15] Destroy pid namespace on init's death
type: item
description: On 08/02, sukadev@us.ibm.com wrote: > > Oleg Nesterov [oleg@tv-sign.ru] wrote: > | > | > | > + if (pid_ns != &init_pid_ns) { > | > | > | > + zap_pid_ns_processes(pid_ns); > | > | > | > + pid_ns->child_reaper = init_pid_ns.child_reaper; > | > | OOPS. I didn't notice this before, but this is not right too (regardless > | of multi-threaded init problems). > | > | We should not "reset" ->child_reaper here, we may have exiting tasks > | which will re-parent their ->children to global init. > | > | No, we are still /sbin/init of this namespace even if we are exiting, > | ->child_reaper should point to us, at least until zap_pid_ns_processes() > | completes. > > Yes, we are resetting the reaper _after_ zap_pid_ns_processes() completes > right ? (all other processes in the namespace must have exited). OOPS again :) Can't understand how I managed to misread this code. This means that we should take care about multi-thread init exit, otherwise the non-root user can crash the kernel. >From reply to Kirill's message: > Still. A non-root user does clone(CLONE_PIDNS), then clone(CLONE_THREAD), > and sys_exit() from the main thread, then proceeds with fork()s. Now this > ns has the global init as a child reaper, and admin can't kill entire pid_ns > by killing its init. Worse, (see the reply to Sukadev' message), we should > not reset pid_ns->child_reaper before zap_pid_ns_processes(). In that case > ->child_reaper points to the freed task when the last thread exits, this > means the non-root user can crash the kernel. Oleg.
creator: Oleg Nesterov
date: 2007-08-02T17:29:32-00:00

Re: [PATCH 14/15] Destroy pid namespace on init's death
type: item
description: Oleg Nesterov [oleg@tv-sign.ru] wrote: | On 08/02, sukadev@us.ibm.com wrote: | > | > Oleg Nesterov [oleg@tv-sign.ru] wrote: | > | > | > | > + if (pid_ns != &init_pid_ns) { | > | > | > | > + zap_pid_ns_processes(pid_ns); | > | > | > | > + pid_ns->child_reaper = init_pid_ns.child_reaper; | > | | > | OOPS. I didn't notice this before, but this is not right too (regardless | > | of multi-threaded init problems). | > | | > | We should not "reset" ->child_reaper here, we may have exiting tasks | > | which will re-parent their ->children to global init. | > | | > | No, we are still /sbin/init of this namespace even if we are exiting, | > | ->child_reaper should point to us, at least until zap_pid_ns_processes() | > | completes. | > | > Yes, we are resetting the reaper _after_ zap_pid_ns_processes() completes | > right ? (all other processes in the namespace must have exited). | | OOPS again :) Can't understand how I managed to misread this code. | | This means that we should take care about multi-thread init exit, | otherwise the non-root user can crash the kernel. | | >From reply to Kirill's message: | | > Still. A non-root user does clone(CLONE_PIDNS), then clone(CLONE_THREAD), Agree we should fix the crash. But we need CAP_SYS_ADMIN to clone pid or other namespaces - this is enforced in copy_namespaces() and unshare_nsproxy_namespaces() Suka
creator: Sukadev Bhattiprolu
date: 2007-08-02T18:36:08-00:00

Re: [PATCH 14/15] Destroy pid namespace on init's death
type: item
description: On 08/02, sukadev@us.ibm.com wrote: > > Oleg Nesterov [oleg@tv-sign.ru] wrote: > | > | This means that we should take care about multi-thread init exit, > | otherwise the non-root user can crash the kernel. > | > | >From reply to Kirill's message: > | > | > Still. A non-root user does clone(CLONE_PIDNS), then clone(CLONE_THREAD), > > Agree we should fix the crash. But we need CAP_SYS_ADMIN to clone > pid or other namespaces - this is enforced in copy_namespaces() and > unshare_nsproxy_namespaces() Hmm. sys_unshare(CLONE_PIDNS) doesn't (and shouldn't) work anyway, but I don't see the CAP_SYS_ADMIN check in copy_process()->copy_namespaces() path. Perhaps I just missed it (sorry, I already cleared my mbox, so I can't look at theses patches), but is it a good idea to require CAP_SYS_ADMIN? I think it would be nice if a normal user can create containers, no? Oleg.
creator: Oleg Nesterov
date: 2007-08-02T18:49:50-00:00

Re: [PATCH 14/15] Destroy pid namespace on init's death
type: item
description: Quoting Oleg Nesterov (oleg@tv-sign.ru): > On 08/02, sukadev@us.ibm.com wrote: > > > > Oleg Nesterov [oleg@tv-sign.ru] wrote: > > | > > | This means that we should take care about multi-thread init exit, > > | otherwise the non-root user can crash the kernel. > > | > > | >From reply to Kirill's message: > > | > > | > Still. A non-root user does clone(CLONE_PIDNS), then clone(CLONE_THREAD), > > > > Agree we should fix the crash. But we need CAP_SYS_ADMIN to clone > > pid or other namespaces - this is enforced in copy_namespaces() and > > unshare_nsproxy_namespaces() > > Hmm. sys_unshare(CLONE_PIDNS) doesn't (and shouldn't) work anyway, but > I don't see the CAP_SYS_ADMIN check in copy_process()->copy_namespaces() > path. > > Perhaps I just missed it (sorry, I already cleared my mbox, so I can't > look at theses patches), but is it a good idea to require CAP_SYS_ADMIN? > I think it would be nice if a normal user can create containers, no? For pid namespaces I can't think of any reason why CAP_SYS_ADMIN should be needed, since you can't hide processes that way. Same for uts namespaces. However for ipc and mount namespaces I'd want to think about it some more. Any case I can think of right now where they'd be unsafe, is unsafe anyway, so maybe it's fine... -serge
creator: serue
date: 2007-08-02T19:13:35-00:00

Re: [PATCH 11/15] Signal semantics
type: item
description: Quoting Kirill Korotaev (dev@sw.ru): > Serge E. Hallyn wrote: > > Quoting Pavel Emelyanov (xemul@openvz.org): > > > >>[snip] > >> > >> > >>>>| Maybe it's worth disabling cross-namespaces ptracing... > >>>> > >>>>I think so too. Its probably not a serious limitation ? > >>> > >>>Several people think we will implement 'namespace entering' through a > >>>ptrace hack, where maybe the admin ptraces the init in a child pidns, > >> > >>Why not implement namespace entering w/o any hacks? :) > > > > > > I did, as a patch on top of the nsproxy container subsystem. The > > response was that that is a hack, and ptrace is cleaner :) > > > > So the current options for namespace entering would be: > > > > * using Cedric's bind_ns() functionality, which assigns an > > integer global id to a namespace, and allows a process to > > enter a namespace by that global id > > looks more or less good and what OVZ actually does. > So I would prefer this one. I think this was Cedric's last post of it: https://lists.linux-foundation.org/pipermail/containers/2007-June/005665.html However I'm pretty sure Eric would be soundly against this. > > * using my nsproxy container subsystem patch, which lets > > a process enter another namespace using > > echo pid > /container/some/cont/directory/tasks > > and eventually might allow construction of custom > > namespaces, i.e. > > mkdir /container/c1/c2 > > ln -s /container/c1/c1/network /container/c1/c2/network > > echo $$ > /container/c1/c2/tasks > > Sound ok and logical as well. I last posted this here: http://www.mail-archive.com/devel@openvz.org/msg00295.html In the ensuing thread, the ptrace-based solution is also discussed. > > * using ptrace to coerce a process in the target namespace > > into forking and executing the desired program. > > you'll need to change ptrace interface in this case imho... > doesn't sound ok at all... at least for me. So I agree with Pavel. Well maybe the problem is that while I think of it as ptrace-based, it's really on…
creator: serue
date: 2007-08-02T20:09:39-00:00

Re: [PATCH 14/15] Destroy pid namespace on init's death
type: item
description: Here is the modified patch that applies on top of Oleg's patch to fix the error for threaded-init. Hope the "big-fat-comment" in exit_child_reaper() (about child-reaper inheriting children even after exiting) makes sense. --- From: Pavel Emelyanov <xemul@openvz.org> Subject: [PATCH 14/15] Destroy pid namespace on init's death From: Sukadev Bhattiprolu <sukadev@us.ibm.com> Terminate all processes in a namespace when the reaper of the namespace is exiting. We do this by walking the pidmap of the namespace and sending SIGKILL to all processes. Changelog: [Oleg Nesterov]: In zap_pid_ns_processes() wait for any child rather than iterating over all pid_ts in the pidmap. Clear TIF_SIGPENDING flag for successive wait() calls. [Oleg Nesterov]: Ensure the logic works even with multi-threaded container-init process. Signed-off-by: Sukadev Bhattiprolu <sukadev@us.ibm.com> Acked-by: Pavel Emelyanov <xemul@openvz.org> --- include/linux/pid.h | 2 ++ kernel/exit.c | 27 ++++++++++++++++++++++++++- kernel/pid.c | 41 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 69 insertions(+), 1 deletion(-) Index: lx26-23-rc1-mm1/include/linux/pid.h =================================================================== --- lx26-23-rc1-mm1.orig/include/linux/pid.h 2007-08-02 11:03:39.000000000 -0700 +++ lx26-23-rc1-mm1/include/linux/pid.h 2007-08-02 11:06:47.000000000 -0700 @@ -118,6 +118,8 @@ extern struct pid *find_ge_pid(int nr, s extern struct pid *alloc_pid(struct pid_namespace *ns); extern void FASTCALL(free_pid(struct pid *pid)); +extern void zap_pid_ns_processes(struct pid_namespace *pid_ns, + struct task_struct *tsk); /* * the helpers to get the pid's id seen from different namespaces Index: lx26-23-rc1-mm1/kernel/exit.c =================================================================== --- lx26-23-rc1-mm1.orig/kernel/exit.c 2007-08-02 11:06:36.000000000 -0700 +++ lx26-23-rc1-mm1/kernel/exit.c 2007-08-02 23:06:47.000000000 -0700 @@ -916,7 +916,32 @@ static inline void exit_child_…
creator: Sukadev Bhattiprolu
date: 2007-08-03T06:22:27-00:00

Re: [PATCH 14/15] Destroy pid namespace on init's death
type: item
description: On 08/02, sukadev@us.ibm.com wrote: > > --- lx26-23-rc1-mm1.orig/kernel/exit.c 2007-08-02 11:06:36.000000000 -0700 > +++ lx26-23-rc1-mm1/kernel/exit.c 2007-08-02 23:06:47.000000000 -0700 > @@ -916,7 +916,32 @@ static inline void exit_child_reaper(str > if (likely(tsk->group_leader != task_child_reaper(tsk))) > return; > > - panic("Attempted to kill init!"); > + if (tsk->nsproxy->pid_ns == &init_pid_ns) > + panic("Attempted to kill init!"); > + > + /* > + * @tsk is the last thread in the 'container-init' and is exiting. > + * Terminate all remaining processes in the namespace and reap them > + * before exiting @tsk. > + * > + * Note that @tsk (last thread of container-init) may not necessarily > + * be the child-reaper (i.e main thread of container-init) of the > + * namespace i.e the child_reaper may have already exited. > + * > + * Even after a child_reaper exits, we let it inherit orphaned children, > + * because, pid_ns->child_reaper remains valid as long as there is > + * at least one living sub-thread in the container init. > + > + * This living sub-thread of the container-init will be notified when > + * a child inherited by the 'child-reaper' exits (do_notify_parent() > + * uses __group_send_sig_info()). Further, when reaping child processes, > + * do_wait() iterates over children of all living sub threads. > + > + * i.e even though 'child_reaper' thread is listed as the parent of the > + * orphaned children, any living sub-thread in the container-init can > + * receive notification of the child exiting and reap the child. > + */ Great, thanks. > + zap_pid_ns_processes(tsk->nsproxy->pid_ns, tsk); > } > > +void zap_pid_ns_processes(struct pid_namespace *pid_ns, > + struct task_struct *reaper) > +{ > + int nr; > + int rc; > + pid_t reaper_pid = pid_nr_ns(task_pid(reaper), pid_ns); I personally dislike these paramaters. reaper == current, and it is /sbin/init always. Not because zap_pid_ns_processes() is always called with reaper == current, but because zap_pid_…
creator: Oleg Nesterov
date: 2007-08-03T10:55:57-00:00

Re: [PATCH 14/15] Destroy pid namespace on init's death
type: item
description: Oleg Nesterov [oleg@tv-sign.ru] wrote: | This is harmless (and note that it is possible that current was actually killed | with SIGKILL from the parent namespace), but the code imho looks confusing. | | I'd suggest to make zap_pid_ns_processes(void), and start the loop from nr == 1. | Or zap_pid_ns_processes(struct pid_namespace *pid_ns). | | Oleg. Agree. Here is the modified patch. --- From: Pavel Emelyanov <xemul@openvz.org> Subject: [PATCH 14/15] Destroy pid namespace on init's death From: Sukadev Bhattiprolu <sukadev@us.ibm.com> Terminate all processes in a namespace when the reaper of the namespace is exiting. We do this by walking the pidmap of the namespace and sending SIGKILL to all processes. Changelog: [Oleg Nesterov]: In zap_pid_ns_processes() wait for any child rather than iterating over all pid_ts in the pidmap. Clear TIF_SIGPENDING flag for successive wait() calls. [Oleg Nesterov]: Ensure the logic works even with multi-threaded container-init process. Signed-off-by: Sukadev Bhattiprolu <sukadev@us.ibm.com> Acked-by: Pavel Emelyanov <xemul@openvz.org> --- include/linux/pid.h | 1 + kernel/exit.c | 27 ++++++++++++++++++++++++++- kernel/pid.c | 38 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 65 insertions(+), 1 deletion(-) Index: lx26-23-rc1-mm1/include/linux/pid.h =================================================================== --- lx26-23-rc1-mm1.orig/include/linux/pid.h 2007-08-02 11:03:39.000000000 -0700 +++ lx26-23-rc1-mm1/include/linux/pid.h 2007-08-03 13:29:40.000000000 -0700 @@ -118,6 +118,7 @@ extern struct pid *find_ge_pid(int nr, s extern struct pid *alloc_pid(struct pid_namespace *ns); extern void FASTCALL(free_pid(struct pid *pid)); +extern void zap_pid_ns_processes(struct pid_namespace *pid_ns); /* * the helpers to get the pid's id seen from different namespaces Index: lx26-23-rc1-mm1/kernel/exit.c =================================================================== --- lx26-23-rc1-mm1.orig/kernel/exit.c 2007-08-02 11:06:36.0000…
creator: Sukadev Bhattiprolu
date: 2007-08-03T21:36:36-00:00

Re: [PATCH 1/15] Move exit_task_namespaces()
type: item
description: Oleg Nesterov wrote: > On 07/26, Pavel Emelyanov wrote: >> The reason to release namespaces after reparenting is that when task >> exits it may send a signal to its parent (SIGCHLD), but if the parent >> has already exited its namespaces there will be no way to decide what >> pid to dever to him - parent can be from different namespace. > > I almost forgot about this one... > > After reading the whole series, I can't understand the above explanation > any longer. The parent can't be from different namespace, either we have > another sub-thread, or we reparent the child to /sbin/init which should > be from the same namespace. If the child that is a new namespace's init is exiting its parent is from the different namespace. Moreover, we will probably want to implement "entering" the pid namespace, so having tasks with parents from another namespace will be OK. > If we are /sbin/init, we kill all tasks from the same namespace before > doing exit_notify(). > > Could you clarify? > > Oleg. > >
creator: Pavel Emelianov
date: 2007-08-06T08:00:44-00:00

Re: [PATCH 1/15] Move exit_task_namespaces()
type: item
description: On 08/06, Pavel Emelyanov wrote: > Oleg Nesterov wrote: > >On 07/26, Pavel Emelyanov wrote: > >>The reason to release namespaces after reparenting is that when task > >>exits it may send a signal to its parent (SIGCHLD), but if the parent > >>has already exited its namespaces there will be no way to decide what > >>pid to dever to him - parent can be from different namespace. > > > >I almost forgot about this one... > > > >After reading the whole series, I can't understand the above explanation > >any longer. The parent can't be from different namespace, either we have > >another sub-thread, or we reparent the child to /sbin/init which should > >be from the same namespace. > > If the child that is a new namespace's init is exiting its parent is from the > different namespace. In that case it doesn't have childs. The were SIGKILL'ed before exit_notify(). > Moreover, we will probably want to implement "entering" > the pid namespace, so having tasks with parents from another namespace will > be OK. Well. I saw this word "entering", but I don't know the meaning. Just curious, could you explain? And, if an exiting task has a child which is already from another namespace, why can't we release our namespace before re-parenting? I guess I need to know what "entering" means to understand this... Oleg.
creator: Oleg Nesterov
date: 2007-08-06T09:54:21-00:00

Re: [PATCH 1/15] Move exit_task_namespaces()
type: item
description: Oleg Nesterov wrote: > On 08/06, Pavel Emelyanov wrote: >> Oleg Nesterov wrote: >>> On 07/26, Pavel Emelyanov wrote: >>>> The reason to release namespaces after reparenting is that when task >>>> exits it may send a signal to its parent (SIGCHLD), but if the parent >>>> has already exited its namespaces there will be no way to decide what >>>> pid to dever to him - parent can be from different namespace. >>> I almost forgot about this one... >>> >>> After reading the whole series, I can't understand the above explanation >>> any longer. The parent can't be from different namespace, either we have >>> another sub-thread, or we reparent the child to /sbin/init which should >>> be from the same namespace. >> If the child that is a new namespace's init is exiting its parent is from the >> different namespace. > > In that case it doesn't have childs. The were SIGKILL'ed before exit_notify(). It does not, but it's parent - does :) >> Moreover, we will probably want to implement "entering" >> the pid namespace, so having tasks with parents from another namespace will >> be OK. > > Well. I saw this word "entering", but I don't know the meaning. Just curious, > could you explain? "Entering" means "moving task to arbitrary namespace" > And, if an exiting task has a child which is already from another namespace, > why can't we release our namespace before re-parenting? I guess I need to > know what "entering" means to understand this... One of the desired actions was the following: 1. task X clones the new namespace with the child Y as this namespace's init; 2. task X waits for SIGCHILD to come informing that the namespace is dead. In this scenario we need to set the Y's pid as it is seen from X's namespace in siginfo. > Oleg. > >
creator: Pavel Emelianov
date: 2007-08-06T09:58:50-00:00

Re: [PATCH 1/15] Move exit_task_namespaces()
type: item
description: On 08/06, Pavel Emelyanov wrote: > > Oleg Nesterov wrote: > >On 08/06, Pavel Emelyanov wrote: > >>Oleg Nesterov wrote: > >>>On 07/26, Pavel Emelyanov wrote: > >>>>The reason to release namespaces after reparenting is that when task > >>>>exits it may send a signal to its parent (SIGCHLD), but if the parent > >>>>has already exited its namespaces there will be no way to decide what > >>>>pid to dever to him - parent can be from different namespace. > >>>I almost forgot about this one... > >>> > >>>After reading the whole series, I can't understand the above explanation > >>>any longer. The parent can't be from different namespace, either we have > >>>another sub-thread, or we reparent the child to /sbin/init which should > >>>be from the same namespace. > >>If the child that is a new namespace's init is exiting its parent is from > >>the > >>different namespace. > > > >In that case it doesn't have childs. The were SIGKILL'ed before > >exit_notify(). > > It does not, but it's parent - does :) Yes. But in that case forget_original_parent() is no-op! This means that it is not needed to move exit_task_namepsace() after. > >>Moreover, we will probably want to implement "entering" > >>the pid namespace, so having tasks with parents from another namespace > >>will > >>be OK. > > > >Well. I saw this word "entering", but I don't know the meaning. Just > >curious, > >could you explain? > > "Entering" means "moving task to arbitrary namespace" Heh. Very much nontrivial, good luck :) At least we need to grow ->numbers[], and if its pid was pinned... > >And, if an exiting task has a child which is already from another > >namespace, > >why can't we release our namespace before re-parenting? I guess I need to > >know what "entering" means to understand this... > > One of the desired actions was the following: > 1. task X clones the new namespace with the child Y as this namespace's > init; > 2. task X waits for SIGCHILD to come informing that the namespace is dead. > In this scenar…
creator: Oleg Nesterov
date: 2007-08-06T10:38:31-00:00

Re: [PATCH 1/15] Move exit_task_namespaces()
type: item
description: Oleg Nesterov wrote: > On 08/06, Pavel Emelyanov wrote: >> Oleg Nesterov wrote: >>> On 08/06, Pavel Emelyanov wrote: >>>> Oleg Nesterov wrote: >>>>> On 07/26, Pavel Emelyanov wrote: >>>>>> The reason to release namespaces after reparenting is that when task >>>>>> exits it may send a signal to its parent (SIGCHLD), but if the parent >>>>>> has already exited its namespaces there will be no way to decide what >>>>>> pid to dever to him - parent can be from different namespace. >>>>> I almost forgot about this one... >>>>> >>>>> After reading the whole series, I can't understand the above explanation >>>>> any longer. The parent can't be from different namespace, either we have >>>>> another sub-thread, or we reparent the child to /sbin/init which should >>>>> be from the same namespace. >>>> If the child that is a new namespace's init is exiting its parent is from >>>> the >>>> different namespace. >>> In that case it doesn't have childs. The were SIGKILL'ed before >>> exit_notify(). >> It does not, but it's parent - does :) > > Yes. But in that case forget_original_parent() is no-op! This means that > it is not needed to move exit_task_namepsace() after. > >>>> Moreover, we will probably want to implement "entering" >>>> the pid namespace, so having tasks with parents from another namespace >>>> will >>>> be OK. >>> Well. I saw this word "entering", but I don't know the meaning. Just >>> curious, >>> could you explain? >> "Entering" means "moving task to arbitrary namespace" > > Heh. Very much nontrivial, good luck :) At least we need to grow ->numbers[], > and if its pid was pinned... > >>> And, if an exiting task has a child which is already from another >>> namespace, >>> why can't we release our namespace before re-parenting? I guess I need to >>> know what "entering" means to understand this... >> One of the desired actions was the following: >> 1. task X clones the new namespace with the child Y as this namespace's >> init; >> 2. task X waits for SIGCHILD to c…
creator: Pavel Emelianov
date: 2007-08-06T11:21:46-00:00

Re: [PATCH 1/15] Move exit_task_namespaces()
type: item
description: Oleg Nesterov wrote: > On 08/06, Pavel Emelyanov wrote: >> Oleg Nesterov wrote: >>> On 08/06, Pavel Emelyanov wrote: >>>> Oleg Nesterov wrote: >>>>> On 07/26, Pavel Emelyanov wrote: >>>>>> The reason to release namespaces after reparenting is that when task >>>>>> exits it may send a signal to its parent (SIGCHLD), but if the parent >>>>>> has already exited its namespaces there will be no way to decide what >>>>>> pid to dever to him - parent can be from different namespace. >>>>> I almost forgot about this one... >>>>> >>>>> After reading the whole series, I can't understand the above explanation >>>>> any longer. The parent can't be from different namespace, either we have >>>>> another sub-thread, or we reparent the child to /sbin/init which should >>>>> be from the same namespace. >>>> If the child that is a new namespace's init is exiting its parent is from >>>> the >>>> different namespace. >>> In that case it doesn't have childs. The were SIGKILL'ed before >>> exit_notify(). >> It does not, but it's parent - does :) > > Yes. But in that case forget_original_parent() is no-op! This means that > it is not needed to move exit_task_namepsace() after. > >>>> Moreover, we will probably want to implement "entering" >>>> the pid namespace, so having tasks with parents from another namespace >>>> will >>>> be OK. >>> Well. I saw this word "entering", but I don't know the meaning. Just >>> curious, >>> could you explain? >> "Entering" means "moving task to arbitrary namespace" > > Heh. Very much nontrivial, good luck :) At least we need to grow ->numbers[], > and if its pid was pinned... > >>> And, if an exiting task has a child which is already from another >>> namespace, >>> why can't we release our namespace before re-parenting? I guess I need to >>> know what "entering" means to understand this... >> One of the desired actions was the following: >> 1. task X clones the new namespace with the child Y as this namespace's >> init; >> 2. task X waits for SIGCHILD to c…
creator: Pavel Emelianov
date: 2007-08-06T11:29:18-00:00

Re: [PATCH 1/15] Move exit_task_namespaces()
type: item
description: On 08/06, Pavel Emelyanov wrote: > > Oleg Nesterov wrote: > >On 08/06, Pavel Emelyanov wrote: > >>Oleg Nesterov wrote: > >>>On 08/06, Pavel Emelyanov wrote: > >>>>Oleg Nesterov wrote: > >>>>>On 07/26, Pavel Emelyanov wrote: > >>>>>>The reason to release namespaces after reparenting is that when task > >>>>>>exits it may send a signal to its parent (SIGCHLD), but if the parent > >>>>>>has already exited its namespaces there will be no way to decide what > >>>>>>pid to dever to him - parent can be from different namespace. > >>>>>I almost forgot about this one... > >>>>> > >I guess I missed something stupid and simple... > > In other words. Let task X live in init_pid_ns, task Y is his child and > lives > int another namespace. task X and task Y both die. This will happen: > > 1. Task X call exit_task_namespaces() > and sets its nsproxy to NULL Ah, got it, thanks. So the problem is not namespace itself (parent's or child's), there are still valid (even if different but related). We just can't get ->parent->nsproxy. I was greatly confused by the "parent can be from different namespace" above. We have exactly same problem if namespaces are not differ. IOW, the problem is: we can't clear ->nsproxy (exit_task_namespaces) until we get rid of ->children. This have nothing to do with different namespace. Oleg.
creator: Oleg Nesterov
date: 2007-08-06T12:48:20-00:00

Re: [PATCH 1/15] Move exit_task_namespaces()
type: item
description: On 08/06, Pavel Emelyanov wrote: > > If task X is exiting and has already exit_task_namespaces()-ed task > Y will OOPs during its exit in determining parent's namespace. I agree > that in that case this is not important what namespace X belongs to, > but we need to handle the race with changing the nsproxy from not-NULL > to NULL. This is OK to make this under task_lock() but what to add > extra locking for if we can avoid it? No, we can't take task_lock() under write_lock(tasklist). Oleg.
creator: Oleg Nesterov
date: 2007-08-06T12:52:08-00:00

Re: [PATCH 1/15] Move exit_task_namespaces()
type: item
description: Oleg Nesterov wrote: > On 08/06, Pavel Emelyanov wrote: >> Oleg Nesterov wrote: >>> On 08/06, Pavel Emelyanov wrote: >>>> Oleg Nesterov wrote: >>>>> On 08/06, Pavel Emelyanov wrote: >>>>>> Oleg Nesterov wrote: >>>>>>> On 07/26, Pavel Emelyanov wrote: >>>>>>>> The reason to release namespaces after reparenting is that when task >>>>>>>> exits it may send a signal to its parent (SIGCHLD), but if the parent >>>>>>>> has already exited its namespaces there will be no way to decide what >>>>>>>> pid to dever to him - parent can be from different namespace. >>>>>>> I almost forgot about this one... >>>>>>> >>> I guess I missed something stupid and simple... >> In other words. Let task X live in init_pid_ns, task Y is his child and >> lives >> int another namespace. task X and task Y both die. This will happen: >> >> 1. Task X call exit_task_namespaces() >> and sets its nsproxy to NULL > > Ah, got it, thanks. So the problem is not namespace itself (parent's or > child's), there are still valid (even if different but related). > > We just can't get ->parent->nsproxy. I was greatly confused by the "parent > can be from different namespace" above. We have exactly same problem if > namespaces are not differ. > > IOW, the problem is: we can't clear ->nsproxy (exit_task_namespaces) until > we get rid of ->children. This have nothing to do with different namespace. No. If the parent is always in the same namespace we do not need to get its nsproxy :) Problem is exactly in that the parent's namespace is to be known. > Oleg. > >
creator: Pavel Emelianov
date: 2007-08-06T13:36:51-00:00

Re: [PATCH 1/15] Move exit_task_namespaces()
type: item
description: Oleg Nesterov wrote: > On 08/06, Pavel Emelyanov wrote: >> If task X is exiting and has already exit_task_namespaces()-ed task >> Y will OOPs during its exit in determining parent's namespace. I agree >> that in that case this is not important what namespace X belongs to, >> but we need to handle the race with changing the nsproxy from not-NULL >> to NULL. This is OK to make this under task_lock() but what to add >> extra locking for if we can avoid it? > > No, we can't take task_lock() under write_lock(tasklist). I meant that we could save the namspace earlier (w/o tasklist), carry it up to the place we need and release it later. But all this is too complex and it's easier to get rid of children and then release the namespaces. > Oleg. > >
creator: Pavel Emelianov
date: 2007-08-06T13:38:03-00:00