OpenVZ Forum - RDF feed
type: channel
items: [PATCH 7/9] sig: Handle pid namespace crossing when sending signals.
items: [PATCH 8/9] signal: Drop signals before sending them to init.
items: [PATCH 9/9] signal: Ignore signals sent to the pid namespace init
items: [PATCH 0/4] pid namespace infrastructure cleanups
items: [PATCH 1/4] pidns: Remove the child_reaper special case from de_thread.
items: [PATCH 2/4] proc: Simplify proc_get_sb.
items: [PATCH 3/4] proc: Remove the unnecessary global proc_mnt
items: [PATCH 4/4] pid: Move all of the pid_namespace logic into copy_pid_ns.
items: Re: [PATCH 2/4] proc: Simplify proc_get_sb.
items: Re: [PATCH 8/9] signal: Drop signals before sending them to init.
items: Re: [PATCH 9/9] signal: Ignore signals sent to the pid namespace init

[PATCH 7/9] sig: Handle pid namespace crossing when sending signals.
type: item
description: Setting si_pid correctly in the context of pid namespaces is tricky. Currently with the special cases in do_notify_parent and do_notify_parent_cldstop we handle all of the day to day cases properly except sending a signal to a task in a child pid namespace. For that case we need to pretend to be the kernel and set si_pid to 0. There are also a few theoretical cases where we can trigger sending a signal from a task in one pid namespace to a task in another pid namespace. With no necessary correlation between one or the other. In those cases when the source pid namespace is a child of the destination pid namespace we actually have a valid pid value we can and should report to user space. This patch modifies the code to handle the full general case for setting si_pid. The code is a little longer but occurs only once and making it some easier to understand and verify it is correct. I add a struct pid sender parameter to __group_send_sig_info, as that is the only function called with si_pid != task_tgid_vnr(current). So we can correctly handle the sending of a signal to the parent of an arbitrary task. Signed-off-by: Eric W. Biederman <ebiederm@xmission.com> --- drivers/char/tty_io.c | 4 +- include/linux/signal.h | 3 +- ipc/mqueue.c | 2 +- kernel/posix-cpu-timers.c | 8 ++-- kernel/signal.c | 88 +++++++++++++++++++++++++++----------------- 5 files changed, 63 insertions(+), 42 deletions(-) diff --git a/drivers/char/tty_io.c b/drivers/char/tty_io.c index 613ec81..c121cdb 100644 --- a/drivers/char/tty_io.c +++ b/drivers/char/tty_io.c @@ -1435,8 +1435,8 @@ static void do_tty_hangup(struct work_struct *work) spin_unlock_irq(&p->sighand->siglock); continue; } - __group_send_sig_info(SIGHUP, SEND_SIG_PRIV, p); - __group_send_sig_info(SIGCONT, SEND_SIG_PRIV, p); + __group_send_sig_info(SIGHUP, SEND_SIG_PRIV, p, NULL); + __group_send_sig_info(SIGCONT, SEND_SIG_PRIV, p, NULL); put_pid(p->signal->tty_old_pgrp); /* A noop */ if (tty->pgrp) p->signal->tty_old_pgrp = get_pid(tty->pgrp…
creator: ebiederm
date: 2007-12-12T12:52:41-00:00

[PATCH 8/9] signal: Drop signals before sending them to init.
type: item
description: Currently init drops all signals including SIGKILL that are sent to it that it does not ignore and does have a handler for. Extending this to pid namespaces where we want to maintain this semantic for signals sent from inside the pid namespace (descendents of init) but we want the namespace init to appear as a normal process is problematic, as a naive approach requires always tracking the sender of a signal. By making the rule (for init dropping signals): When sending a signal to init, the presence of a signal handler that is not SIG_DFL allows the signal to be sent to init. If the signal is not sent it is silently dropped without becoming pending. The only noticeable user space difference from todays init is that it no longer needs to worry about signals becoming pending when it has them marked as SIG_DFL and blocked. This change by making the presence of a signal handler effectively a permission check allows us to do all of the work before we enqueue the signal, and there is no need for any fancy tracking of the signal sender. Which means that we can now allow force_sig_info to send signals to init, that panic the kernel instead of going into an infinite busy loop taking an exception sending a signal and then retaking the same exception, eating all of the cpu time but accomplishing nothing. This change also makes it possible to easily implement the desired semantics of /sbin/init for pid namespaces where outer processes can kill init but processes inside the pid namespace can not. While it is now easy to remove the dropping of signals from individual code paths such as force_sig_info this patch does not implement that, to retain as much of the current behavior as possible. The only behavioral difference besides not queuing blocked SIG_DFL signals are signals directly added with sigaddset. In practice a threaded init now receives SIGKILL sent from a core dump, a thread group exit, or an exec shutting down extraneous threads. This patch was inspired by a patch from …
creator: ebiederm
date: 2007-12-12T12:57:13-00:00

[PATCH 9/9] signal: Ignore signals sent to the pid namespace init
type: item
description: This patch extends sit_init_drop to take a sender pid so it can make decisions about dropping signals based on the sender of the signal. With knowledge of the signal sender sig_init_drop is extended to drop signals meant for the pid namespace init processes as well as the global init process. However only signals with senders contained in the pid namespace of the init process are dropped. Ensuring that outside of the pid namespace the pid namespace init process continues to look like an ordinary process. Signed-off-by: Eric W. Biederman <ebiederm@xmission.com> --- kernel/signal.c | 24 +++++++++++++++--------- 1 files changed, 15 insertions(+), 9 deletions(-) diff --git a/kernel/signal.c b/kernel/signal.c index 029a45d..074905f 100644 --- a/kernel/signal.c +++ b/kernel/signal.c @@ -64,20 +64,26 @@ static int sig_ignored(struct task_struct *t, int sig) (handler == SIG_DFL && sig_kernel_ignore(sig)); } -static int is_sig_init(struct task_struct *tsk) +static int is_sig_init(struct task_struct *init, struct pid *sender) { - if (likely(!is_global_init(tsk->group_leader))) + if (!is_container_init(init)) + return 0; + + if (!sender) + sender = task_tgid(current); + + if (!pid_in_pid_ns(sender, task_active_pid_ns(init))) return 0; return 1; } -static int sig_init_drop(struct task_struct *tsk, int sig) +static int sig_init_drop(struct task_struct *tsk, int sig, struct pid *sender) { /* All signals for which init has a SIG_DFL handler are * silently dropped without being sent. */ - if (!is_sig_init(tsk)) + if (!is_sig_init(tsk, sender)) return 0; return (tsk->sighand->action[sig-1].sa.sa_handler == SIG_DFL); @@ -854,7 +860,7 @@ force_sig_info(int sig, struct siginfo *info, struct task_struct *t) spin_lock_irqsave(&t->sighand->siglock, flags); ret = 0; - if (sig_init_drop(t, sig)) + if (sig_init_drop(t, sig, NULL)) goto out; action = &t->sighand->action[sig-1]; ignored = action->sa.sa_handler == SIG_IGN; @@ -985,7 +991,7 @@ __group_send_sig_info(int sig, struct siginfo *info,…
creator: ebiederm
date: 2007-12-12T12:58:27-00:00

[PATCH 0/4] pid namespace infrastructure cleanups
type: item
description: These next 4 patches are just generally nice cleanups to the pid namespace infrastructure. There is a slight context dependence on the previous patchset but nothing fundamental. These patches remove yet another special case from de_thread. Remove special cases from proc_get_sb Kill proc_mnt Move the pid_namespace logic out of the fork fast path. Just generally making those code paths much nicer to live with. Eric _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containers
creator: ebiederm
date: 2007-12-12T13:09:42-00:00

[PATCH 1/4] pidns: Remove the child_reaper special case from de_thread.
type: item
description: This patch inspired by Oleg's leader_pid change for the timer code now tracks the child_reaper by pid instead of by task_struct, allowing us to not need to update anything when de_thread is run. The logic in de_thread that is supposed to let a threaded init call exec has bit rotted. Since the signal drop code does not look at child_reaper changing child_reaper early is of no help. Since zap_other_threads uses sigaddset my previous patch to drop signals sent to init before queueing them actually fixes this case. Since the child_reaper never exits we don't have to worry about reference counting the pid. By tracking the child_reaper of a pid namespace by it's task group id, we don't need to do anything when the leading task in the task group exits. Signed-off-by: Eric W. Biederman <ebiederm@xmission.com> --- fs/exec.c | 8 -------- include/linux/pid_namespace.h | 9 ++------- init/main.c | 2 +- kernel/fork.c | 2 +- kernel/pid.c | 10 +++++++++- 5 files changed, 13 insertions(+), 18 deletions(-) diff --git a/fs/exec.c b/fs/exec.c index f73edfc..96b4822 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -770,14 +770,6 @@ static int de_thread(struct task_struct *tsk) return -EAGAIN; } - /* - * child_reaper ignores SIGKILL, change it now. - * Reparenting needs write_lock on tasklist_lock, - * so it is safe to do it under read_lock. - */ - if (unlikely(tsk->group_leader == task_child_reaper(tsk))) - task_active_pid_ns(tsk)->child_reaper = tsk; - sig->group_exit_task = tsk; zap_other_threads(tsk); read_unlock(&tasklist_lock); diff --git a/include/linux/pid_namespace.h b/include/linux/pid_namespace.h index ab13faa..2e67033 100644 --- a/include/linux/pid_namespace.h +++ b/include/linux/pid_namespace.h @@ -18,7 +18,7 @@ struct pid_namespace { struct kref kref; struct pidmap pidmap[PIDMAP_ENTRIES]; int last_pid; - struct task_struct *child_reaper; + struct pid *child_reaper; struct kmem_cache *pid_cachep; int level; struct pid_namespace *parent; @@ -75,11 +75,6 @@ static inline void zap_pid…
creator: ebiederm
date: 2007-12-12T13:27:00-00:00

[PATCH 2/4] proc: Simplify proc_get_sb.
type: item
description: The idle_thread now has a struct pid, so we can always find out know the pid of the child_reaper before we mount proc. Therefore we can remove the special cases for getting the pid of the child_reaper from proc_get_sb. Signed-off-by: Eric W. Biederman <ebiederm@xmission.com> --- fs/proc/root.c | 17 +---------------- 1 files changed, 1 insertions(+), 16 deletions(-) diff --git a/fs/proc/root.c b/fs/proc/root.c index 81f99e6..f442967 100644 --- a/fs/proc/root.c +++ b/fs/proc/root.c @@ -46,17 +46,6 @@ static int proc_get_sb(struct file_system_type *fs_type, struct pid_namespace *ns; struct proc_inode *ei; - if (proc_mnt) { - /* Seed the root directory with a pid so it doesn't need - * to be special in base.c. I would do this earlier but - * the only task alive when /proc is mounted the first time - * is the init_task and it doesn't have any pids. - */ - ei = PROC_I(proc_mnt->mnt_sb->s_root->d_inode); - if (!ei->pid) - ei->pid = find_get_pid(1); - } - if (flags & MS_KERNMOUNT) ns = (struct pid_namespace *)data; else @@ -76,11 +65,7 @@ static int proc_get_sb(struct file_system_type *fs_type, } ei = PROC_I(sb->s_root->d_inode); - if (!ei->pid) { - rcu_read_lock(); - ei->pid = get_pid(find_pid_ns(1, ns)); - rcu_read_unlock(); - } + ei->pid = get_pid(ns->child_reaper); sb->s_flags |= MS_ACTIVE; ns->proc_mnt = mnt; -- 1.5.3.rc6.17.g1911 _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containers
creator: ebiederm
date: 2007-12-12T13:30:15-00:00

[PATCH 3/4] proc: Remove the unnecessary global proc_mnt
type: item
description: There are no longer any users of the proc_mnt global and having it suggests that we have a single instance of proc, so just kill it. Signed-off-by: Eric W. Biederman <ebiederm@xmission.com> --- fs/proc/inode.c | 2 -- fs/proc/root.c | 5 ++--- include/linux/proc_fs.h | 1 - 3 files changed, 2 insertions(+), 6 deletions(-) diff --git a/fs/proc/inode.c b/fs/proc/inode.c index 82b3a1b..96446ac 100644 --- a/fs/proc/inode.c +++ b/fs/proc/inode.c @@ -71,8 +71,6 @@ static void proc_delete_inode(struct inode *inode) clear_inode(inode); } -struct vfsmount *proc_mnt; - static struct kmem_cache * proc_inode_cachep; static struct inode *proc_alloc_inode(struct super_block *sb) diff --git a/fs/proc/root.c b/fs/proc/root.c index f442967..e4b7c5a 100644 --- a/fs/proc/root.c +++ b/fs/proc/root.c @@ -97,9 +97,8 @@ void __init proc_root_init(void) err = register_filesystem(&proc_fs_type); if (err) return; - proc_mnt = kern_mount_data(&proc_fs_type, &init_pid_ns); - err = PTR_ERR(proc_mnt); - if (IS_ERR(proc_mnt)) { + err = pid_ns_prepare_proc(&init_pid_ns); + if (err) { unregister_filesystem(&proc_fs_type); return; } diff --git a/include/linux/proc_fs.h b/include/linux/proc_fs.h index a5ab8ad..40da820 100644 --- a/include/linux/proc_fs.h +++ b/include/linux/proc_fs.h @@ -128,7 +128,6 @@ extern struct proc_dir_entry *create_proc_entry(const char *name, mode_t mode, struct proc_dir_entry *parent); extern void remove_proc_entry(const char *name, struct proc_dir_entry *parent); -extern struct vfsmount *proc_mnt; struct pid_namespace; extern int proc_fill_super(struct super_block *); extern struct inode *proc_get_inode(struct super_block *, unsigned int, struct proc_dir_entry *); -- 1.5.3.rc6.17.g1911 _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containers
creator: ebiederm
date: 2007-12-12T13:31:22-00:00

[PATCH 4/4] pid: Move all of the pid_namespace logic into copy_pid_ns.
type: item
description: Currently we have a bunch of pid namespace logic unnecessarily scattered through fork, this patch moves it into copy_pid_ns where it is easy to find and review. To do this the prototype of copy_pid_ns is changed to take a task parameter and the pid passed into copy_process is passed in the task_pid field of that task. After this cleanup things are actually clean enough we can contemplate what an unshare of the pid namespace would mean. Signed-off-by: Eric W. Biederman <ebiederm@xmission.com> --- include/linux/pid_namespace.h | 5 +++-- kernel/fork.c | 16 +++++----------- kernel/nsproxy.c | 2 +- kernel/pid_namespace.c | 31 ++++++++++++++++++++++++++++--- 4 files changed, 37 insertions(+), 17 deletions(-) diff --git a/include/linux/pid_namespace.h b/include/linux/pid_namespace.h index 2e67033..8e2e346 100644 --- a/include/linux/pid_namespace.h +++ b/include/linux/pid_namespace.h @@ -37,7 +37,7 @@ static inline struct pid_namespace *get_pid_ns(struct pid_namespace *ns) return ns; } -extern struct pid_namespace *copy_pid_ns(unsigned long flags, struct pid_namespace *ns); +extern struct pid_namespace *copy_pid_ns(unsigned long flags, struct task_struct *tsk); extern void free_pid_ns(struct kref *kref); extern void zap_pid_ns_processes(struct pid_namespace *pid_ns); @@ -56,8 +56,9 @@ static inline struct pid_namespace *get_pid_ns(struct pid_namespace *ns) } static inline struct pid_namespace * -copy_pid_ns(unsigned long flags, struct pid_namespace *ns) +copy_pid_ns(unsigned long flags, struct task_struct *task) { + struct pid_namespace *ns = task->nsproxy->pid_ns; if (flags & CLONE_NEWPID) ns = ERR_PTR(-EINVAL); return ns; diff --git a/kernel/fork.c b/kernel/fork.c index 20d26a5..bbd8bcd 100644 --- a/kernel/fork.c +++ b/kernel/fork.c @@ -50,7 +50,6 @@ #include <linux/taskstats_kern.h> #include <linux/random.h> #include <linux/tty.h> -#include <linux/proc_fs.h> #include <linux/memcontrol.h> #include <asm/pgtable.h> @@ -1013,6 +1012,9 @@ static struct task_struct *copy_proce…
creator: ebiederm
date: 2007-12-12T13:33:27-00:00

Re: [PATCH 2/4] proc: Simplify proc_get_sb.
type: item
description: Eric W. Biederman wrote: > The idle_thread now has a struct pid, so we can always find out know > the pid of the child_reaper before we mount proc. > > Therefore we can remove the special cases for getting the pid of the > child_reaper from proc_get_sb. > > Signed-off-by: Eric W. Biederman <ebiederm@xmission.com> > --- > fs/proc/root.c | 17 +---------------- > 1 files changed, 1 insertions(+), 16 deletions(-) > > diff --git a/fs/proc/root.c b/fs/proc/root.c > index 81f99e6..f442967 100644 > --- a/fs/proc/root.c > +++ b/fs/proc/root.c > @@ -46,17 +46,6 @@ static int proc_get_sb(struct file_system_type *fs_type, > struct pid_namespace *ns; > struct proc_inode *ei; > > - if (proc_mnt) { > - /* Seed the root directory with a pid so it doesn't need > - * to be special in base.c. I would do this earlier but > - * the only task alive when /proc is mounted the first time > - * is the init_task and it doesn't have any pids. > - */ > - ei = PROC_I(proc_mnt->mnt_sb->s_root->d_inode); > - if (!ei->pid) > - ei->pid = find_get_pid(1); > - } > - > if (flags & MS_KERNMOUNT) > ns = (struct pid_namespace *)data; > else > @@ -76,11 +65,7 @@ static int proc_get_sb(struct file_system_type *fs_type, > } > > ei = PROC_I(sb->s_root->d_inode); > - if (!ei->pid) { > - rcu_read_lock(); > - ei->pid = get_pid(find_pid_ns(1, ns)); > - rcu_read_unlock(); > - } > + ei->pid = get_pid(ns->child_reaper); That's not git-bisect safe - you move the child_reaper initialization before the call to prepare_proc only in the 4th patch. > sb->s_flags |= MS_ACTIVE; > ns->proc_mnt = mnt; _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containers
creator: Pavel Emelianov
date: 2007-12-12T13:42:21-00:00

Re: [PATCH 2/4] proc: Simplify proc_get_sb.
type: item
description: Pavel Emelyanov <xemul@openvz.org> writes: > Eric W. Biederman wrote: >> The idle_thread now has a struct pid, so we can always find out know >> the pid of the child_reaper before we mount proc. >> >> Therefore we can remove the special cases for getting the pid of the >> child_reaper from proc_get_sb. >> >> Signed-off-by: Eric W. Biederman <ebiederm@xmission.com> >> --- >> fs/proc/root.c | 17 +---------------- >> 1 files changed, 1 insertions(+), 16 deletions(-) >> >> diff --git a/fs/proc/root.c b/fs/proc/root.c >> index 81f99e6..f442967 100644 >> --- a/fs/proc/root.c >> +++ b/fs/proc/root.c >> @@ -46,17 +46,6 @@ static int proc_get_sb(struct file_system_type *fs_type, >> struct pid_namespace *ns; >> struct proc_inode *ei; >> >> - if (proc_mnt) { >> - /* Seed the root directory with a pid so it doesn't need >> - * to be special in base.c. I would do this earlier but >> - * the only task alive when /proc is mounted the first time >> - * is the init_task and it doesn't have any pids. >> - */ >> - ei = PROC_I(proc_mnt->mnt_sb->s_root->d_inode); >> - if (!ei->pid) >> - ei->pid = find_get_pid(1); >> - } >> - >> if (flags & MS_KERNMOUNT) >> ns = (struct pid_namespace *)data; >> else >> @@ -76,11 +65,7 @@ static int proc_get_sb(struct file_system_type *fs_type, >> } >> >> ei = PROC_I(sb->s_root->d_inode); >> - if (!ei->pid) { >> - rcu_read_lock(); >> - ei->pid = get_pid(find_pid_ns(1, ns)); >> - rcu_read_unlock(); >> - } >> + ei->pid = get_pid(ns->child_reaper); > > That's not git-bisect safe - you move the child_reaper initialization > before the call to prepare_proc only in the 4th patch. Bugger. I had overlooked that dependency. So patch 4 really should have been patch 2. Eric _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containers
creator: ebiederm
date: 2007-12-12T14:06:18-00:00

Re: [PATCH 8/9] signal: Drop signals before sending them to init.
type: item
description: Quoting Eric W. Biederman (ebiederm@xmission.com): > > Currently init drops all signals including SIGKILL that are sent to it > that it does not ignore and does have a handler for. Extending this to Description is crucial in this patchset, so should the above not be: "does not have a handler for" ? Otherwise, this whole patchset looks good to me (deferring to Pavel for his NAK on patch 5 of course). Acked-by: Serge Hallyn <serue@us.ibm.com> thanks, -serge > pid namespaces where we want to maintain this semantic for signals sent > from inside the pid namespace (descendents of init) but we want the > namespace init to appear as a normal process is problematic, as a > naive approach requires always tracking the sender of a signal. > > By making the rule (for init dropping signals): > When sending a signal to init, the presence of a signal handler that > is not SIG_DFL allows the signal to be sent to init. If the signal > is not sent it is silently dropped without becoming pending. > > The only noticeable user space difference from todays init is that it > no longer needs to worry about signals becoming pending when it has > them marked as SIG_DFL and blocked. > > This change by making the presence of a signal handler effectively > a permission check allows us to do all of the work before we enqueue > the signal, and there is no need for any fancy tracking of the > signal sender. > > Which means that we can now allow force_sig_info to send signals to > init, that panic the kernel instead of going into an infinite busy > loop taking an exception sending a signal and then retaking the same > exception, eating all of the cpu time but accomplishing nothing. > > This change also makes it possible to easily implement the desired > semantics of /sbin/init for pid namespaces where outer processes can > kill init but processes inside the pid namespace can not. > > While it is now easy to remove the dropping of signals from individual > code paths such as force_sig_info this patc…
creator: serue
date: 2007-12-12T19:00:42-00:00

Re: [PATCH 8/9] signal: Drop signals before sending them to init.
type: item
description: "Serge E. Hallyn" <serue@us.ibm.com> writes: > Quoting Eric W. Biederman (ebiederm@xmission.com): >> >> Currently init drops all signals including SIGKILL that are sent to it >> that it does not ignore and does have a handler for. Extending this to > > Description is crucial in this patchset, so should the above not be: > > "does not have a handler for" Yes. It should say "does not have a handler for." Instead of "does have a handler for." Oops. I thought that was what I wrote! > > ? Eric _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containers
creator: ebiederm
date: 2007-12-12T19:33:08-00:00

Re: [PATCH 8/9] signal: Drop signals before sending them to init.
type: item
description: On 12/12, Eric W. Biederman wrote: > > By making the rule (for init dropping signals): > When sending a signal to init, the presence of a signal handler that > is not SIG_DFL allows the signal to be sent to init. If the signal > is not sent it is silently dropped without becoming pending. But isn't it better to modify sig_ignore() and handle_stop_signal() instead? This way we seem to need less changes, http://marc.info/?l=linux-kernel&m=118753610515859 (the patch above itself is not complete and a bit obsolete) > The only noticeable user space difference from todays init is that it > no longer needs to worry about signals becoming pending when it has > them marked as SIG_DFL and blocked. Ugh. I have to apologize again. I got a fever, and it turns out I just can't read English. So, do you mean we can ignore the problems with the signals which are currently blocked by /sbin/init? I personally agree, but I'm not sure I understand this right. > +static int sig_init_drop(struct task_struct *tsk, int sig) > +{ > + /* All signals for which init has a SIG_DFL handler are > + * silently dropped without being sent. > + */ > + if (!is_sig_init(tsk)) > + return 0; > + > + return (tsk->sighand->action[sig-1].sa.sa_handler == SIG_DFL); > +} What if /sbin/init has a handler, but before this signal is delivered /sbin/init does signal(SIG_DFL) ? We should modify so_sigaction() to prevent this. Note again the patch above. Oleg. _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containers
creator: Oleg Nesterov
date: 2007-12-13T16:25:02-00:00

Re: [PATCH 9/9] signal: Ignore signals sent to the pid namespace init
type: item
description: On 12/12, Eric W. Biederman wrote: > > -static int is_sig_init(struct task_struct *tsk) > +static int is_sig_init(struct task_struct *init, struct pid *sender) > { > - if (likely(!is_global_init(tsk->group_leader))) > + if (!is_container_init(init)) > + return 0; > + > + if (!sender) > + sender = task_tgid(current); What if this signal is sent from the interrupt and sender == NULL? > + > + if (!pid_in_pid_ns(sender, task_active_pid_ns(init))) > return 0; In that case the result of the above check can be wrong, no? Oleg. _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containers
creator: Oleg Nesterov
date: 2007-12-13T16:28:11-00:00

Re: [PATCH 8/9] signal: Drop signals before sending them to init.
type: item
description: Oleg Nesterov <oleg@tv-sign.ru> writes: > On 12/12, Eric W. Biederman wrote: >> >> By making the rule (for init dropping signals): >> When sending a signal to init, the presence of a signal handler that >> is not SIG_DFL allows the signal to be sent to init. If the signal >> is not sent it is silently dropped without becoming pending. > > But isn't it better to modify sig_ignore() and handle_stop_signal() > instead? This way we seem to need less changes, > > http://marc.info/?l=linux-kernel&m=118753610515859 > > (the patch above itself is not complete and a bit obsolete) No. That way leads to semantic disaster. In particular if it is traced or blocked your patch did not ignore the signal, the signal would be queued up. We would not know who the sender is and thus not know the proper disposition for the signal. For a clean definition that we can maintain into the future either we must either drop the signal before it is placed on the queue or otherwise processed. Or we must track the sender. Tracking the sender is expensive. >> The only noticeable user space difference from todays init is that it >> no longer needs to worry about signals becoming pending when it has >> them marked as SIG_DFL and blocked. > > Ugh. I have to apologize again. I got a fever, and it turns out I just > can't read English. > > So, do you mean we can ignore the problems with the signals which are > currently blocked by /sbin/init? Yes. Further I am saying those signals will never become pending if we do not have a signal handler installed. > I personally agree, but I'm not sure I understand this right. > >> +static int sig_init_drop(struct task_struct *tsk, int sig) >> +{ >> + /* All signals for which init has a SIG_DFL handler are >> + * silently dropped without being sent. >> + */ >> + if (!is_sig_init(tsk)) >> + return 0; >> + >> + return (tsk->sighand->action[sig-1].sa.sa_handler == SIG_DFL); >> +} > > What if /sbin/init has a handler, but before this signal is delivered > /sbin/init doe…
creator: ebiederm
date: 2007-12-13T17:50:59-00:00

Re: [PATCH 9/9] signal: Ignore signals sent to the pid namespace init
type: item
description: Oleg Nesterov <oleg@tv-sign.ru> writes: > On 12/12, Eric W. Biederman wrote: >> >> -static int is_sig_init(struct task_struct *tsk) >> +static int is_sig_init(struct task_struct *init, struct pid *sender) >> { >> - if (likely(!is_global_init(tsk->group_leader))) >> + if (!is_container_init(init)) >> + return 0; >> + >> + if (!sender) >> + sender = task_tgid(current); > > What if this signal is sent from the interrupt and sender == NULL? > >> + >> + if (!pid_in_pid_ns(sender, task_active_pid_ns(init))) >> return 0; > > In that case the result of the above check can be wrong, no? Yes. If we are not in process context (in_interrupt) we do infer the sender incorrectly. Duh. I saw something in earlier patches people had posted didn't understand it, and didn't get an answer when I asked about it. I guess I should have thought about that corner case and looked a little harder. In this case the sender would always be the kernel. Oleg do you know which signals are sent from interrupt context and through which signal sending entry points? I'm inclined to say: if (!sender && in_interrupt()) sender = &init_struct_pid; Which will cause the signal to be dropped if SIG_DFL for the real init (same pid namespace), and otherwise cause the signal to be sent. Which sight unseen feels like the right thing. It worries me that we are likely going through check_kill_permission and all of the rest. I expect instead of testing for in_interrupt I should be doing something like the siginfo test in check_kill_permssion. And just lumping all of the kernel related signals into the same bin. I place this one on the backburner of my thoughts and come back to it in a bit. I am close enough that it should be a simple straight forward code change whatever the outcome. Eric _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containers
creator: ebiederm
date: 2007-12-13T18:16:21-00:00

Re: [PATCH 8/9] signal: Drop signals before sending them to init.
type: item
description: On 12/13, Eric W. Biederman wrote: > > Oleg Nesterov <oleg@tv-sign.ru> writes: > > > So, do you mean we can ignore the problems with the signals which are > > currently blocked by /sbin/init? > > Yes. Further I am saying those signals will never become pending if > we do not have a signal handler installed. OK, if we change the semantics for /sbin/init signals we can avoid a lot of problems, > > I personally agree, but I'm not sure I understand this right. > > > >> +static int sig_init_drop(struct task_struct *tsk, int sig) > >> +{ > >> + /* All signals for which init has a SIG_DFL handler are > >> + * silently dropped without being sent. > >> + */ > >> + if (!is_sig_init(tsk)) > >> + return 0; > >> + > >> + return (tsk->sighand->action[sig-1].sa.sa_handler == SIG_DFL); > >> +} > > > > What if /sbin/init has a handler, but before this signal is delivered > > /sbin/init does signal(SIG_DFL) ? We should modify so_sigaction() to > > prevent this. Note again the patch above. > > No. We should treat signals that we process for /sbin/init completely > normally. ... including this one. I am not arguing. > This gives /sbin/init completely normal signal handling if the signal is > ever enqueued. Something trivial to implement and explain. Well, I am not sure about "explain" though. Unless I missed something this makes the semantics a bit special. Suppose that init does sigtimedwait() but the handler == SIG_DFL. Oleg. _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containers
creator: Oleg Nesterov
date: 2007-12-13T18:18:02-00:00

Re: [PATCH 9/9] signal: Ignore signals sent to the pid namespace init
type: item
description: ebiederm@xmission.com (Eric W. Biederman) writes: > > Yes. If we are not in process context (in_interrupt) we do infer > the sender incorrectly. Duh. I saw something in earlier patches > people had posted didn't understand it, and didn't get an answer > when I asked about it. Grr. Rather I didn't see a reply when I asked about it. I just spotted Suka's old reply telling me it was to attempt to prevent init from getting sigio. Clearly I have a big fat blind spot here you could drive a truck through. Kernel generated signals are a pain because they are neither fish nor fowl. They are sent privileged, but we don't necessarily want to give them all curtsies and deliver them. Grr. Eric _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containers
creator: ebiederm
date: 2007-12-13T18:33:10-00:00

Re: [PATCH 8/9] signal: Drop signals before sending them to init.
type: item
description: Oleg Nesterov <oleg@tv-sign.ru> writes: > OK, if we change the semantics for /sbin/init signals we can avoid > a lot of problems, Yes. Otherwise we must track the source of the signals. >> No. We should treat signals that we process for /sbin/init completely >> normally. > > ... including this one. I am not arguing. > >> This gives /sbin/init completely normal signal handling if the signal is >> ever enqueued. Something trivial to implement and explain. > > Well, I am not sure about "explain" though. Unless I missed something > this makes the semantics a bit special. Well the semantics are a bit special for init period. I just make them special in a slightly different way. > Suppose that init does sigtimedwait() but the handler == SIG_DFL. Yes that is a bit surprising. However it is still easy to explain. The signal is never enqueued so sigtimedwait never gets the chance to do anything with it. Interestingly enough this is not a problem for the current sysvinit. sysvinit does this at start up: /* * Ignore all signals. */ for(f = 1; f <= NSIG; f++) SETSIG(sa, f, SIG_IGN, SA_RESTART); So everything is initialized to SIG_IGN by userspace, in the common case. Which means none of this special case logic will actually kick in, except for SIGKILL and SIGSTOP. The signals we can't change. Eric _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containers
creator: ebiederm
date: 2007-12-13T18:50:09-00:00

Re: [PATCH 8/9] signal: Drop signals before sending them to init.
type: item
description: On 12/13, Eric W. Biederman wrote: > > Oleg Nesterov <oleg@tv-sign.ru> writes: > > > OK, if we change the semantics for /sbin/init signals we can avoid > > a lot of problems, > > Yes. Otherwise we must track the source of the signals. Yes. > > Well, I am not sure about "explain" though. Unless I missed something > > this makes the semantics a bit special. > > Well the semantics are a bit special for init period. I just > make them special in a slightly different way. Yes. But still I personally can't agree with the new behaviour. > > Suppose that init does sigtimedwait() but the handler == SIG_DFL. > > Yes that is a bit surprising. However it is still easy to explain. > The signal is never enqueued so sigtimedwait never gets the chance > to do anything with it. But this precisely means that sigtimedwait() is just broken for init. Another example. /sbin/init execs, and tries to make sure it doesn't miss the important signal (say, SIGCHLD). So it blocks SIGCHLD, execs, and the new program installs the handler. However, the handler == SIG_DFL right after exec, so signal could be missed. Eric, I think the blocked signal should be enqueued. If application blocks the signal, this is the strong indication it does care about it, we shouldn't throw it out. Yes, this also has surprises. If init unblocks the signal without installing the handler, it could be killed. But this can happen with your patch as well. sig_init_drop() returns false if we have a handler, but this races with sys_rt_sigaction() which can set SIG_DFL, so init could be killed. IOW, I still have a strong feeling that this patch http://marc.info/?l=linux-kernel&m=118753610515859 is better, and more correct. That said, this all is very subjective, I can't "prove" this of course. > Interestingly enough this is not a problem > for the current sysvinit. > > sysvinit does this at start up: > /* > * Ignore all signals. > */ > for(f = 1; f <= NSIG; f++) > SETSIG(sa, f, SIG_IGN, SA_RESTART); Yes sure, the "good" init…
creator: Oleg Nesterov
date: 2007-12-16T15:52:44-00:00

Re: [PATCH 8/9] signal: Drop signals before sending them to init.
type: item
description: Oleg Nesterov <oleg@tv-sign.ru> writes: > On 12/13, Eric W. Biederman wrote: >> >> Oleg Nesterov <oleg@tv-sign.ru> writes: >> >> > OK, if we change the semantics for /sbin/init signals we can avoid >> > a lot of problems, >> >> Yes. Otherwise we must track the source of the signals. > > Yes. Good. We have fundamental agreement that we should slightly change the definition of how signals to init are dropped. For me the really important part is that we have a clean definition so that we can fix bugs in the implementation. Rather then declaring a particular implementation is the definition and getting lost there. >> > Well, I am not sure about "explain" though. Unless I missed something >> > this makes the semantics a bit special. >> >> Well the semantics are a bit special for init period. I just >> make them special in a slightly different way. > > Yes. > > But still I personally can't agree with the new behaviour. You argument that a program that blocks signals wants them is compelling, especially since blocking signals is not a common thing to do. So I would have no problem with a definition said signals will be dropped when sent to init if at the time they are sent the signal is SIG_DFL and unblocked. >> > Suppose that init does sigtimedwait() but the handler == SIG_DFL. >> >> Yes that is a bit surprising. However it is still easy to explain. >> The signal is never enqueued so sigtimedwait never gets the chance >> to do anything with it. > > But this precisely means that sigtimedwait() is just broken for init. > > Another example. /sbin/init execs, and tries to make sure it doesn't > miss the important signal (say, SIGCHLD). So it blocks SIGCHLD, execs, > and the new program installs the handler. However, the handler == SIG_DFL > right after exec, so signal could be missed. > > Eric, I think the blocked signal should be enqueued. If application > blocks the signal, this is the strong indication it does care about > it, we shouldn't throw it out. A reasonable argume…
creator: ebiederm
date: 2007-12-18T04:06:52-00:00

Re: [PATCH 9/9] signal: Ignore signals sent to the pid namespace init
type: item
description: ebiederm@xmission.com (Eric W. Biederman) writes: > ebiederm@xmission.com (Eric W. Biederman) writes: >> >> Yes. If we are not in process context (in_interrupt) we do infer >> the sender incorrectly. Duh. I saw something in earlier patches >> people had posted didn't understand it, and didn't get an answer >> when I asked about it. > > Grr. Rather I didn't see a reply when I asked about it. I just > spotted Suka's old reply telling me it was to attempt to prevent > init from getting sigio. Clearly I have a big fat blind spot > here you could drive a truck through. > > Kernel generated signals are a pain because they are neither fish nor > fowl. They are sent privileged, but we don't necessarily want to give > them all curtsies and deliver them. Grr. I solved this in my thinking earlier but I should really mention it. There are two kinds of kernel generated signals. - Services like timers, SIGIO, etc where the kernel is doing something on behalf of the process. - Process management where we send SIGKILL to make a process go away. For services we clearly want to ignore the signal. For the kernel flexing's it's muscle we want to accept the signal, and let init die and accept the consequences. Unfortunately we don't do that today. Since most signals are services and since we want to preserve today semantics we want to drop kernel generated signals. For those cases where it matters we can have the kernel send a signal by a different path, that doesn't give init the option of ignoring the signal. Eric _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containers
creator: ebiederm
date: 2007-12-18T08:37:40-00:00

Re: [PATCH 8/9] signal: Drop signals before sending them to init.
type: item
description: On 12/17, Eric W. Biederman wrote: > > So I would have no problem with a definition said signals > will be dropped when sent to init if at the time they are > sent the signal is SIG_DFL and unblocked. Great! > > But this can happen with > > your patch as well. sig_init_drop() returns false if we have a handler, > > but this races with sys_rt_sigaction() which can set SIG_DFL, so init > > could be killed. > > I am checking under the sighand lock so we should not race, > at least not internally to the kernel. Yes, but as soon as we drop ->siglock /sbin/init can set SIG_DFL before noticing the signal. > > IOW, I still have a strong feeling that this patch > > > > http://marc.info/?l=linux-kernel&m=118753610515859 > > > > is better, and more correct. That said, this all is very subjective, > > I can't "prove" this of course. > > My fundamental problem with that patch is that it drops signals > after we have started processing them, and it modifies the code > of an optimization. > > To have a clean definition and clean semantics I think we need > to drop the signal earlier in the path. Which is what I > really object to in your patch. Hmm. Could you look at this patch again? I'm attaching it at the end. (re-diffed against the current code) It modifies sig_ignored(), so we drop the signal before we started processing. And in fact it is more "optimized", because we don't need to check sa_handler twice. Btw. I don't think we should change force_sig_info(). Suppose that init blocks/ignores SIGSEGV and do_page_fault() does force_sig_info_fault(). In that case it is better to die explicitely than go into the endless loop. Oleg. --- t/kernel/signal.c~INITSIGS 2007-08-19 14:39:35.000000000 +0400 +++ t/kernel/signal.c 2007-08-19 19:00:27.000000000 +0400 @@ -39,11 +39,33 @@ static struct kmem_cache *sigqueue_cachep; +static int sig_init_ignore(struct task_struct *tsk) +{ + if (likely(!is_container_init(tsk->group_leader))) + return 0; + + // ---------------- Multiple pid namespace…
creator: Oleg Nesterov
date: 2007-12-18T12:22:41-00:00

Re: [PATCH 8/9] signal: Drop signals before sending them to init.
type: item
description: Oleg Nesterov <oleg@tv-sign.ru> writes: > On 12/17, Eric W. Biederman wrote: >> >> So I would have no problem with a definition said signals >> will be dropped when sent to init if at the time they are >> sent the signal is SIG_DFL and unblocked. > > Great! My only issue is that with including the blocked signals in the definition is that it makes things a little harder to explain and understand. >> > But this can happen with >> > your patch as well. sig_init_drop() returns false if we have a handler, >> > but this races with sys_rt_sigaction() which can set SIG_DFL, so init >> > could be killed. >> >> I am checking under the sighand lock so we should not race, >> at least not internally to the kernel. > > Yes, but as soon as we drop ->siglock /sbin/init can set SIG_DFL before > noticing the signal. I guess I don't see this as a race. The definition in my head is all about permission to send a signal to init. That permission happens if init shows interest in the incoming signal. So it is an instant in time decision. At another instant the permissions may have changed and we are allowed to send the signal. You seem to be implying that something is wrong. If something is wrong if something can be wrong either we have the definition wrong or the implementation wrong. If we can not implement things to be correct with our new definition then it is the wrong definition. So races are totally NOT ok. I don't think you have made the switch to the permission check mentality that I am proposing in my definition. Can you restate from the perspective of allowing or denying a signal how looking at the blocked state of the signal is better then just looking at SIG_DFL? >> > IOW, I still have a strong feeling that this patch >> > >> > http://marc.info/?l=linux-kernel&m=118753610515859 >> > >> > is better, and more correct. That said, this all is very subjective, >> > I can't "prove" this of course. >> >> My fundamental problem with that patch is that it drops signals >> after we ha…
creator: ebiederm
date: 2007-12-18T13:36:55-00:00

Re: [PATCH 8/9] signal: Drop signals before sending them to init.
type: item
description: On 12/18, Eric W. Biederman wrote: > > Oleg Nesterov <oleg@tv-sign.ru> writes: > > > On 12/17, Eric W. Biederman wrote: > >> > >> So I would have no problem with a definition said signals > >> will be dropped when sent to init if at the time they are > >> sent the signal is SIG_DFL and unblocked. > > > > Great! > > My only issue is that with including the blocked signals in > the definition is that it makes things a little harder to > explain and understand. Perhaps yes. > >> > But this can happen with > >> > your patch as well. sig_init_drop() returns false if we have a handler, > >> > but this races with sys_rt_sigaction() which can set SIG_DFL, so init > >> > could be killed. > >> > >> I am checking under the sighand lock so we should not race, > >> at least not internally to the kernel. > > > > Yes, but as soon as we drop ->siglock /sbin/init can set SIG_DFL before > > noticing the signal. > > I guess I don't see this as a race. The definition in my head is > all about permission to send a signal to init. That permission happens > if init shows interest in the incoming signal. So it is an > instant in time decision. Agreed. > You seem to be implying that something is wrong. If something > is wrong if something can be wrong either we have the definition wrong > or the implementation wrong. I think that the resulting behaviour is not right. /sbin/init should not die after signal(SIG_DFL) if it has a pending !sig_kernel_ignore() signal. Of course this doesn't depend on "should we look at the blocked state or not" issue. > >> My fundamental problem with that patch is that it drops signals > >> after we have started processing them, and it modifies the code > >> of an optimization. > >> > >> To have a clean definition and clean semantics I think we need > >> to drop the signal earlier in the path. Which is what I > >> really object to in your patch. > > > > Hmm. Could you look at this patch again? I'm attaching it at the end. > > (re-diffed against the current code) …
creator: Oleg Nesterov
date: 2007-12-18T15:30:07-00:00

Re: [PATCH 8/9] signal: Drop signals before sending them to init.
type: item
description: Oleg Nesterov <oleg@tv-sign.ru> writes: > >> > @@ -2303,8 +2316,7 @@ int do_sigaction(int sig, struct k_sigac >> > * (for example, SIGCHLD), shall cause the pending signal to >> > * be discarded, whether or not it is blocked" >> > */ >> > - if (act->sa.sa_handler == SIG_IGN || >> > - (act->sa.sa_handler == SIG_DFL && sig_kernel_ignore(sig))) { >> > + if (sig_task_ignore(current, sig)) { >> > struct task_struct *t = current; >> > sigemptyset(&mask); >> > sigaddset(&mask, sig); >> >> Any time you start ignoring the signal here you are not thinking >> in terms of never sending the signal or not. Once a signal is sent >> we must treat it like normal (to have a clean definition). > > We don't agree here. Which seems to be the heart of the matter. >> In the namespace case we can not look at a pending signal and decide >> if we should drop it or not. So changing sigaction is impossible. > > You mean that it is possible that this signal has come from the parent > namespace, and so we should die but not just discard the signal. Yes. > I think we can ignore this problem. If we had a handler before (when > the signal was sent), this is - imho - the correct behaviour. If not > then yes, /sbin/init can "accidentally" survive. But the parent namespace > can always use SIGKILL to really kill us. Only because we can't change SIGKILL to SIG_DFL. Think of force_siginfo and what happens when we stop dropping signals on that path. We send the signal and then before we process it user space does signal(SIG_DFL), and we drop SIGSEGV. Ouch! > But yes I agree, this changes one corner case to another. And let me > repeat, I don't claim that "I am right and you are not", and I can't > really prove that my approach is "technically" better. Just a personal > feeling about the "better" tradeoff. And I already said my taste is > perverted ;) In this instance I can prove that my choice is better. When the code is called into question and we must decided if a code behavior is a bug or not we requi…
creator: ebiederm
date: 2007-12-18T21:34:26-00:00

Re: [PATCH 8/9] signal: Drop signals before sending them to init.
type: item
description: On 12/18, Eric W. Biederman wrote: > > Oleg Nesterov <oleg@tv-sign.ru> writes: > > > >> In the namespace case we can not look at a pending signal and decide > >> if we should drop it or not. So changing sigaction is impossible. > > > > You mean that it is possible that this signal has come from the parent > > namespace, and so we should die but not just discard the signal. > > Yes. > > > I think we can ignore this problem. If we had a handler before (when > > the signal was sent), this is - imho - the correct behaviour. If not > > then yes, /sbin/init can "accidentally" survive. But the parent namespace > > can always use SIGKILL to really kill us. > > Only because we can't change SIGKILL to SIG_DFL. > > Think of force_siginfo and what happens when we stop dropping signals > on that path. We send the signal and then before we process it > user space does signal(SIG_DFL), and we drop SIGSEGV. Ouch! Not a problem. First of all, this has nothing to do with init's problems, any application can can do this with signal(SIG_IGN). The most important case is SIGSEGV sent from do_trap/do_page_fault/etc. Another sub-thread can "steal" the signal, but this is harmless. The signal will be re-generated when application returns from the exception and restarts the faulting instruction. > > But yes I agree, this changes one corner case to another. And let me > > repeat, I don't claim that "I am right and you are not", and I can't > > really prove that my approach is "technically" better. Just a personal > > feeling about the "better" tradeoff. And I already said my taste is > > perverted ;) > > In this instance I can prove that my choice is better. > > When the code is called into question and we must decided if > a code behavior is a bug or not we require a definition of > what the code is supposed to do. > > Given our technical constraints of not being able to track > the source of the signal, and needing to appear as a normal > process to signal senders outside of the pid namespa…
creator: Oleg Nesterov
date: 2007-12-19T13:42:46-00:00