Container-init signal semantics

Patch 5/7 is new in this set and fixes a bug. Remaining patches are just a forward-port from previous version and I believe they address all comments I have received.

Oleg please sign-off/ack if you agree.

---

Container-init must behave like global-init to processes within the container and hence it must be immune to unhandled fatal signals from within the container (i.e SIG_DFL signals that terminate the process).

But the same container-init must behave like a normal process to processes in ancestor namespaces and so if it receives the same fatal signal from a process in ancestor namespace, the signal must be processed.

Implementing these semantics requires that send_signal() determine pid namespace of the sender but since signals can originate from workqueues/ interrupt-handlers, determining pid namespace of sender may not always be possible or safe.

This patchset implements the design/simplified semantics suggested by Oleg Nesterov. The simplified semantics for container-init are:

- container-init must never be terminated by a signal from a descendant process.

- container-init must never be immune to SIGKILL from an ancestor namespace (so a process in parent namespace must always be able to terminate a descendant container).

- container-init may be immune to unhandled fatal signals (like SIGUSR1) even if they are from ancestor namespace. SIGKILL/SIGSTOP are the only reliable signals to a container-init from ancestor namespace.

Patches in this set:

[PATCH 1/7] Remove 'handler' parameter to tracehook functions [PATCH 2/7] Protect init from unwanted signals more [PATCH 3/7] Add from_ancestor_ns parameter to send_signal() [PATCH 4/7] Protect cinit from unblocked SIG_DFL signals [PATCH 5/7] zap_pid_ns_process() should use force_sig() [PATCH 6/7] Protect cinit from blocked fatal signals [PATCH 7/7] SI_USER: Masquerade si_pid when crossing pid ns boundary

Changelog[v8]:

- Bugfix (new patch, 5/7): Nested container-init not terminated when parent container-init exits and calls zap_pid_ns_processes().
- Dropped old patch 7/7 which showed SIG_DFL signals to init as "ignored" in /proc (we were undecided on whether its good or bad).

Changelog[v7]:
- siginfo_from_user() and siginfo_from_ancestor_ns() are fairly simple and used only in send_signal(). Remove them and move the logic into send_signal() (Patch 4/7)

- Update /proc/pid/status to include SIG_DFL signals to init in the "ignored" set (and remove the TODO in Patch 0/7) (Patch 7/7)

Changelog[v6]:

- Patches 3,4: Have kill_pid_info_as_uid() pass in 'from_ancestor_ns' parameter to __send_signal() and remove SI_ASYNCIO check in siginfo_from_user().
- Patches 4,6: Update changelog and simplify code

Changelog[v5]:
- Patch 2/6: Remove SIG_IGN check in sig_task_ignored() and let sig_handler_ignored() check SIG_IGN.
- Patch 3/6. Put siginfo_from_ancestor_ns() back under CONFIG_PID_NS and remove warning in rt_sigqueueinfo().
- (Patch 5/6)Simplify check in get_signal_to_deliver()
- (Patch 6/6)Simplify masquerading pid
- LTP-20081219-intermediate showed no new errors on 2.6.28-rc5-mm2.

Changelog[v4]:
- [Bugfix] Patch 3/7. Check ns == NULL in siginfo_from_ancestor_ns(). Although http://lkml.org/lkml/2008/12/16/502 makes it less likely that ns == NULL, looks like an explicit check won't hurt ?
- Remove SIGNAL_UNKILLABLE_FROM_NS flag and simplify logic as suggested by Oleg Nesterov.
- Dropped patch that set SIGNAL_UNKILLABLE_FROM_NS and set SIGNAL_UNKILLABLE in patch 5/7 to be bisect-safe.
- Add a warning in rt_sigqueueinfo() if SI_ASYNCIO is used (patch 3/7)
- Added two patches (6/7 and 7/7) to masquerade si_pid for SI_USER and SI_TKILL

Changelog[v3]:
Changes based on discussions of previous version:
http://lkml.org/lkml/2008/11/25/458

Major changes:

- Define SIGNAL_UNKILLABLE_FROM_NS and use in container-inits to skip fatal signals from same namespace but process SIGKILL/SIGSTOP from ancestor namespace.
- Use SI_FROMUSER() and si_code != SI_ASYNCIO to determine if it is safe to dereference pid-namespace of caller. Highly experimental :-)
- Masquerading si_pid when crossing namespace boundary: relevant patches merged in -mm and dropped from this set.

Minor changes:

- Remove 'handler' parameter to tracehook functions
- Update sig_ignored() to drop SIG_DFL signals to global init early (tried to address Roland's and Oleg's comments)
- Use 'same_ns' flag to drop SIGKILL/SIGSTOP to cinit from same namespace

Limitations/side-effects of current design

	- Container-init is immune to suicide - kill(getpid(), SIGKILL) is
	  ignored. Use exit() :-)

From: Oleg Nesterov <[email protected]>
Date: Wed, 24 Dec 2008 13:35:17 -0800
Subject: [PATCH 1/7][v8] Remove 'handler' parameter to tracehook functions

Based on an earlier patch submitted by Oleg Nesterov and comments from Roland McGrath (http://lkml.org/lkml/2008/11/19/258).

The handler parameter is currently unused in the tracehook functions. Besides, the tracehook functions are called with siglock held, so the functions can check the handler if they later need to.

Removing the parameter simiplifies changes to sig_ignored() in a follow-on patch.

---
 arch/x86/kernel/ptrace.c  |    2 +-
 include/linux/tracehook.h |   13 ++++---------
 kernel/signal.c           |    6 +++---
 3 files changed, 8 insertions(+), 13 deletions(-)

index 5a4c23d..2117492 100644
@@ -1461,6 +1461,6 @@ asmregparm void syscall_trace_leave(struct pt_regs *regs)
 	 * system call instruction.
 	 */
 	if (test_thread_flag(TIF_SINGLESTEP) &&
-	    tracehook_consider_fatal_signal(current, SIGTRAP, SIG_DFL))
+	    tracehook_consider_fatal_signal(current, SIGTRAP))
 		send_sigtrap(current, regs, 0, TRAP_BRKPT);
 }

index c913155..c7aa154 100644
@@ -388,17 +388,14 @@ static inline void tracehook_signal_handler(int sig, siginfo_t *info,
  * tracehook_consider_ignored_signal - suppress short-circuit of ignored signal
  * @task:		task receiving the signal
  * @sig:		signal number being sent
- * @handler:		%SIG_IGN or %SIG_DFL
  *
  * Return zero iff tracing doesn't care to examine this ignored signal,
  * so it can short-circuit normal delivery and never even get queued.
- * Either @handler is %SIG_DFL and @sig's default is ignore, or it's %SIG_IGN.
  *
  * Called with @task->sighand->siglock held.
  */
 static inline int tracehook_consider_ignored_signal(struct task_struct *task,
-						    int sig,
-						    void __user *handler)
+						    int sig)
 {
 	return (task_ptrace(task) & PT_PTRACED) != 0;
 }

@@ -407,19 +404,17 @@ static inline int tracehook_consider_ignored_signal(struct task_struct *task,
  * tracehook_consider_fatal_signal - suppress special handling of fatal signal
  * @task:		task receiving the signal
  * @sig:		signal number being sent
- * @handler:		%SIG_DFL or %SIG_IGN
  *
  * Return nonzero to prevent special handling of this termination signal.
- * Normally @handler is %SIG_DFL.  It can be %SIG_IGN if @sig is ignored,
- * in which case force_sig() is about to reset it to %SIG_DFL.
+ * Normally handler for signal is %SIG_DFL.  It can be %SIG_IGN if @sig is
+ * ignored, in which case force_sig() is about to reset it to %SIG_DFL.
  * When this returns zero, this signal might cause a quick termination
  * that does not give the debugger a chance to intercept the signal.
  *
  * Called with or without @task->sighand->siglock held.
  */
 static inline int tracehook_consider_fatal_signal(struct task_struct *task,
-						  int sig,
-						  void __user *handler)
+						  int sig)
 {
 	return (task_ptrace(task) & PT_PTRACED) != 0;
 }

index 2a74fe8..ee8a8f1 100644
@@ -74,7 +74,7 @@ static int sig_ignored(struct task_struct *t, int sig)
 	/*
 	 * Tracers may want to know about even ignored signals.
 	 */
-	return !tracehook_consider_ignored_signal(t, sig, handler);
+	return !tracehook_consider_ignored_signal(t, sig);
 }
 
 /*

@@ -318,7 +318,7 @@ int unhandled_signal(struct task_struct *tsk, int sig)
 		return 1;
 	if (handler != SIG_IGN && handler != SIG_DFL)
 		return 0;
-	return !tracehook_consider_fatal_signal(tsk, sig, handler);
+	return !tracehook_consider_fatal_signal(tsk, sig);
 }
 
 

@@ -777,7 +777,7 @@ static void complete_signal(int sig, struct task_struct *p, int group)
 	    !(signal->flags & (SIGNAL_UNKILLABLE | SIGNAL_GROUP_EXIT)) &&
 	    !sigismember(&t->real_blocked, sig) &&
 	    (sig == SIGKILL ||
-	     !tracehook_consider_fatal_signal(t, sig, SIG_DFL))) {
+	     !tracehook_consider_fatal_signal(t, sig))) {
 		/*
 		 * This signal will be fatal to the whole group.
 		 */

1.5.2.5

From: Oleg Nesterov <[email protected]>
Date: Wed, 24 Dec 2008 13:35:23 -0800
Subject: [PATCH 2/7][v8] Protect init from unwanted signals more

(This is a modified version of the patch submitted by Oleg Nesterov
http://lkml.org/lkml/2008/11/18/249 and tries to address comments that came up in that discussion)

init ignores the SIG_DFL signals but we queue them anyway, including SIGKILL. This is mostly OK, the signal will be dropped silently when dequeued, but the pending SIGKILL has 2 bad implications:

- it implies fatal_signal_pending(), so we confuse things like wait_for_completion_killable/lock_page_killable.

- for the sub-namespace inits, the pending SIGKILL can mask (legacy_queue) the subsequent SIGKILL from the parent namespace which must kill cinit reliably. (preparation, cinits don't have SIGNAL_UNKILLABLE yet)

The patch can't help when init is ptraced, but ptracing of init is not "safe" anyway.

Changelog[v5]:
- (Oleg Nesterov) Remove SIG_IGN check in sig_task_ignored() and let sig_handler_ignored() check SIG_IGN.

---
 kernel/signal.c |   16 +++++++++++++---
 1 files changed, 13 insertions(+), 3 deletions(-)

index ee8a8f1..8761e4c 100644
@@ -55,10 +55,21 @@ static int sig_handler_ignored(void __user *handler, int sig)
 		(handler == SIG_DFL && sig_kernel_ignore(sig));
 }
 
-static int sig_ignored(struct task_struct *t, int sig)
+static int sig_task_ignored(struct task_struct *t, int sig)
 {
 	void __user *handler;
 
+	handler = sig_handler(t, sig);
+
+	if (unlikely(t->signal->flags & SIGNAL_UNKILLABLE) &&
+			handler == SIG_DFL)
+		return 1;
+
+	return sig_handler_ignored(handler, sig);
+}
+
+static int sig_ignored(struct task_struct *t, int sig)
+{
 	/*
 	 * Blocked signals are never ignored, since the
 	 * signal handler may change by the time it is

@@ -67,8 +78,7 @@ static int sig_ignored(struct task_struct *t, int sig)
 	if (sigismember(&t->blocked, sig) || sigismember(&t->real_blocked, sig))
 		return 0;
 
-	handler = sig_handler(t, sig);
-	if (!sig_handler_ignored(handler, sig))
+	if (!sig_task_ignored(t, sig))
 		return 0;
 
 	/*

1.5.2.5

From: Sukadev Bhattiprolu <[email protected]>
Date: Tue, 6 Jan 2009 17:28:05 -0800
Subject: [PATCH 3/7][v8] Add from_ancestor_ns parameter to send_signal()

send_signal() (or its helper) needs to determine the pid namespace of the sender. But a signal sent via kill_pid_info_as_uid() comes from within the kernel and send_signal() does not need to determine the pid namespace of the sender. So define a helper for send_signal() which takes an additional parameter, 'from_ancestor_ns' and have kill_pid_info_as_uid() use that helper directly.

The 'from_ancestor_ns' parameter will be used in a follow-on patch.

Changelog[v6]:
- New patch added to this patchset, based on suggestions from Roland McGrath and Oleg Nesterov.

---
 kernel/signal.c |   12 +++++++++---
 1 files changed, 9 insertions(+), 3 deletions(-)

index 8761e4c..ff1dff9 100644
@@ -823,8 +823,8 @@ static inline int legacy_queue(struct sigpending *signals, int sig)
 	return (sig < SIGRTMIN) && sigismember(&signals->signal, sig);
 }
 
-static int send_signal(int sig, struct siginfo *info, struct task_struct *t,
-			int group)
+static int __send_signal(int sig, struct siginfo *info, struct task_struct *t,
+			int group, int from_ancestor_ns)
 {
 	struct sigpending *pending;
 	struct sigqueue *q;

@@ -899,6 +899,12 @@ out_set:
 	return 0;
 }
 
+static int send_signal(int sig, struct siginfo *info, struct task_struct *t,
+			int group)
+{
+	return __send_signal(sig, info, t, group, 0);
+}
+
 int print_fatal_signals;
 
 static void print_fatal_signal(struct pt_regs *regs, int signr)

@@ -1143,7 +1149,7 @@ int kill_pid_info_as_uid(int sig, struct siginfo *info, struct pid *pid,
 	if (sig && p->sighand) {
 		unsigned long flags;
 		spin_lock_irqsave(&p->sighand->siglock, flags);
-		ret = __group_send_sig_info(sig, info, p);
+		ret = __send_signal(sig, info, p, 1, 0);
 		spin_unlock_irqrestore(&p->sighand->siglock, flags);
 	}
 out_unlock:

1.5.2.5

From: Sukadev Bhattiprolu <[email protected]>
Date: Wed, 24 Dec 2008 14:03:57 -0800
Subject: [PATCH 4/7][v8] Protect cinit from unblocked SIG_DFL signals

Drop early any SIG_DFL or SIG_IGN signals to container-init from within the same container. But queue SIGSTOP and SIGKILL to the container-init if they are from an ancestor container.

Blocked, fatal signals (i.e when SIG_DFL is to terminate) from within the container can still terminate the container-init. That will be addressed in the next patch.

Note: To be bisect-safe, SIGNAL_UNKILLABLE will be set for container-inits
in a follow-on patch. Until then, this patch is just a preparatory step.

Changelog[v7]:
- (Oleg Nesterov) Bugfix: send_sigqueue() should use from_ancestor_ns=0
- siginfo_from_user(), siginfo_from_ancestor_ns() are needed in only one place. Remove them and move their logic into send_signal(). Seems to make code more easier to read :-)

Changelog[v6]:
- (Roland McGrath) Remove unnecessary helper signal_task_unkillable() and fold checks into sig_task_ignored().

Changelog[v4]:
- (Oleg Nesterov) Remove SIGNAL_UNKILLABLE_FROM_NS and rename 'same_ns' to 'from_ancestor_ns'.
- SIGNAL_UNKILLABLE is not yet set for container-inits (will be set in follow-on patch).

---
 kernel/signal.c |   28 +++++++++++++++++++---------
 1 files changed, 19 insertions(+), 9 deletions(-)

index ff1dff9..a3a1304 100644
@@ -55,20 +55,21 @@ static int sig_handler_ignored(void __user *handler, int sig)
 		(handler == SIG_DFL && sig_kernel_ignore(sig));
 }
 
-static int sig_task_ignored(struct task_struct *t, int sig)
+static int sig_task_ignored(struct task_struct *t, int sig,
+		int from_ancestor_ns)
 {
 	void __user *handler;
 
 	handler = sig_handler(t, sig);
 
 	if (unlikely(t->signal->flags & SIGNAL_UNKILLABLE) &&
-			handler == SIG_DFL)
+			handler == SIG_DFL && !from_ancestor_ns)
 		return 1;
 
 	return sig_handler_ignored(handler, sig);
 }
 
-static int sig_ignored(struct task_struct *t, int sig)
+static int sig_ignored(struct task_struct *t, int sig, int from_ancestor_ns)
 {
 	/*
 	 * Blocked signals are never ignored, since the

@@ -78,7 +79,7 @@ static int sig_ignored(struct task_struct *t, int sig)
 	if (sigismember(&t->blocked, sig) || sigismember(&t->real_blocked, sig))
 		return 0;
 
-	if (!sig_task_ignored(t, sig))
+	if (!sig_task_ignored(t, sig, from_ancestor_ns))
 		return 0;
 
 	/*

@@ -634,7 +635,7 @@ static int check_kill_permission(int sig, struct siginfo *info,
  * Returns true if the signal should be actually delivered, otherwise
  * it should be dropped.
  */
-static int prepare_signal(int sig, struct task_struct *p)
+static int prepare_signal(int sig, struct task_struct *p, int from_ancestor_ns)
 {
 	struct signal_struct *signal = p->signal;
 	struct task_struct *t;

@@ -718,7 +719,7 @@ static int prepare_signal(int sig, struct task_struct *p)
 		}
 	}
 
-	return !sig_ignored(p, sig);
+	return !sig_ignored(p, sig, from_ancestor_ns);
 }
 
 /*

@@ -832,7 +833,8 @@ static int __send_signal(int sig, struct siginfo *info, struct task_struct *t,
 	trace_sched_signal_send(sig, t);
 
 	assert_spin_locked(&t->sighand->siglock);
-	if (!prepare_signal(sig, t))
+
+	if (!prepare_signal(sig, t, from_ancestor_ns))
 		return 0;
 
 	pending = group ? &t->signal->shared_pending : &t->pending;

@@ -902,7 +904,15 @@ out_set:
 static int send_signal(int sig, struct siginfo *info, struct task_struct *t,
 			int group)
 {
-	return __send_signal(sig, info, t, group, 0);
+	int from_ancestor_ns = 0;
+
+#ifdef CONFIG_PID_NS
+	if (!is_si_special(info) && SI_FROMUSER(info) &&
+			task_pid_nr_ns(current, task_active_pid_ns(t)) <= 0)
+		from_ancestor_ns = 1;
+#endif
+
+	return __send_signal(sig, info, t, group, from_ancestor_ns);
 }
 
 int print_fatal_signals;

@@ -1336,7 +1346,7 @@ int send_sigqueue(struct sigqueue *q, struct task_struct *t, int group)
 		goto ret;
 
 	ret = 1; /* the signal is ignored */
-	if (!prepare_signal(sig, t))
+	if (!prepare_signal(sig, t, 0))
 		goto out;
 
 	ret = 0;

1.5.2.5

From: Sukadev Bhattiprolu <[email protected]>
Date: Wed, 18 Feb 2009 15:12:30 -0800
Subject: [PATCH 5/7][v8] zap_pid_ns_process() should use force_sig()

send_signal() assumes that signals with SEND_SIG_PRIV are generated from within the same namespace. So any nested container-init processes become immune to the SIGKILL generated by kill_proc_info() in zap_pid_ns_processes().

Use force_sig() in zap_pid_ns_processes() instead - force_sig() clears the SIGNAL_UNKILLABLE flag ensuring the signal is processed by container-inits.

---
 kernel/pid_namespace.c |   14 +++++++++++++-
 1 files changed, 13 insertions(+), 1 deletions(-)

index fab8ea8..33815ae 100644
@@ -152,6 +152,7 @@ void zap_pid_ns_processes(struct pid_namespace *pid_ns)
 {
 	int nr;
 	int rc;
+	struct task_struct *task;
 
 	/*
 	 * The last thread in the cgroup-init thread group is terminating.

@@ -169,7 +170,18 @@ void zap_pid_ns_processes(struct pid_namespace *pid_ns)
 	read_lock(&tasklist_lock);
 	nr = next_pidmap(pid_ns, 1);
 	while (nr > 0) {
-		kill_proc_info(SIGKILL, SEND_SIG_PRIV, nr);
+		rcu_read_lock();
+
+		/*
+		 * Use force_sig() since it clears SIGNAL_UNKILLABLE ensuring
+		 * any nested-container's init processes don't ignore the
+		 * signal
+		 */
+		task = pid_task(find_vpid(nr), PIDTYPE_PID);
+		force_sig(SIGKILL, task);
+
+		rcu_read_unlock();
+
 		nr = next_pidmap(pid_ns, nr);
 	}
 	read_unlock(&tasklist_lock);

1.5.2.5

Oleg Nesterov [[email protected]] wrote:

| On 02/18, Sukadev Bhattiprolu wrote:
| >
| >  	read_lock(&tasklist_lock);
| >  	nr = next_pidmap(pid_ns, 1);
| >  	while (nr > 0) {
| > -		kill_proc_info(SIGKILL, SEND_SIG_PRIV, nr);
| > +		rcu_read_lock();
| > +
| > +		/*
| > +		 * Use force_sig() since it clears SIGNAL_UNKILLABLE ensuring
| > +		 * any nested-container's init processes don't ignore the
| > +		 * signal
| > +		 */
| > +		task = pid_task(find_vpid(nr), PIDTYPE_PID);
| > +		force_sig(SIGKILL, task);
| 
| Shouldn't we check task != NULL ?

Yes. Here is the updated patch.
---

From: Sukadev Bhattiprolu <[email protected]>
Date: Wed, 18 Feb 2009 15:12:30 -0800
Subject: [PATCH 5/7][v8] zap_pid_ns_process() should use force_sig()

send_signal() assumes that signals with SEND_SIG_PRIV are generated from within the same namespace. So any nested container-init processes become immune to the SIGKILL generated by kill_proc_info() in zap_pid_ns_processes().

Use force_sig() in zap_pid_ns_processes() instead - force_sig() clears the SIGNAL_UNKILLABLE flag ensuring the signal is processed by container-inits.

---
 kernel/pid_namespace.c |   15 ++++++++++++++-
 1 files changed, 14 insertions(+), 1 deletions(-)

index fab8ea8..2d1001b 100644
@@ -152,6 +152,7 @@ void zap_pid_ns_processes(struct pid_namespace *pid_ns)
 {
 	int nr;
 	int rc;
+	struct task_struct *task;
 
 	/*
 	 * The last thread in the cgroup-init thread group is terminating.

@@ -169,7 +170,19 @@ void zap_pid_ns_processes(struct pid_namespace *pid_ns)
 	read_lock(&tasklist_lock);
 	nr = next_pidmap(pid_ns, 1);
 	while (nr > 0) {
-		kill_proc_info(SIGKILL, SEND_SIG_PRIV, nr);
+		rcu_read_lock();
+
+		/*
+		 * Use force_sig() since it clears SIGNAL_UNKILLABLE ensuring
+		 * any nested-container's init processes don't ignore the
+		 * signal
+		 */
+		task = pid_task(find_vpid(nr), PIDTYPE_PID);
+		if (task)
+			force_sig(SIGKILL, task);
+
+		rcu_read_unlock();
+
 		nr = next_pidmap(pid_ns, nr);
 	}
 	read_unlock(&tasklist_lock);

1.5.2.5

From: Sukadev Bhattiprolu <[email protected]>
Date: Wed, 24 Dec 2008 14:04:24 -0800
Subject: [PATCH 6/7][v8] Protect cinit from blocked fatal signals

Normally SIG_DFL signals to global and container-init are dropped early. But if a signal is blocked when it is posted, we cannot drop the signal since the receiver may install a handler before unblocking the signal. Once this signal is queued however, the receiver container-init has no way of knowing if the signal was sent from an ancestor or descendant namespace. This patch ensures that contianer-init drops all SIG_DFL signals in get_signal_to_deliver() except SIGKILL/SIGSTOP.

If SIGSTOP/SIGKILL originate from a descendant of container-init they are never queued (i.e dropped in sig_ignored() in an earler patch).

If SIGSTOP/SIGKILL originate from parent namespace, the signal is queued and container-init processes the signal.

IOW, if get_signal_to_deliver() sees a sig_kernel_only() signal for global or container-init, the signal must have been generated internally or must have come from an ancestor ns and we process the signal.

Further, the signal_group_exit() check was needed to cover the case of a multi-threaded init sending SIGKILL to other threads when doing an exit() or exec(). But since the new sig_kernel_only() check covers the SIGKILL, the signal_group_exit() check is no longer needed and can be removed.

Finally, now that we have all pieces in place, set SIGNAL_UNKILLABLE for container-inits.

Changelog[v6]:
- Add a note regarding the signal_group_exit() in patch description.

Changelog[v5]:
- (Oleg Nesterov) Drop signal_unkillable(), simplify check in get_signal_to_deliver() and drop check for signal_group_exit() since it is covered by sig_kernel_only().

Changelog[v4]:
- Rename sig_unkillable() to unkillable_by_sig()
- Remove SIGNAL_UNKILLABLE_FROM_NS flag and simplify (Oleg Nesterov)
- Set SIGNAL_UNKILLABLE for container-init in this patch.

---
 kernel/fork.c   |    2 ++
 kernel/signal.c |    9 ++++++++-
 2 files changed, 10 insertions(+), 1 deletions(-)

index 2b45bc5..ddfa413 100644
@@ -841,6 +841,8 @@ static int copy_signal(unsigned long clone_flags, struct task_struct *tsk)
 	atomic_set(&sig->live, 1);
 	init_waitqueue_head(&sig->wait_chldexit);
 	sig->flags = 0;
+	if (clone_flags & CLONE_NEWPID)
+		sig->flags |= SIGNAL_UNKILLABLE;
 	sig->group_exit_code = 0;
 	sig->group_exit_task = NULL;
 	sig->group_stop_count = 0;

index a3a1304..c94355b 100644
@@ -1862,9 +1862,16 @@ relock:
 
 		/*
 		 * Global init gets no signals it doesn't want.
+		 * Container-init gets no signals it doesn't want from same
+		 * container.
+		 *
+		 * Note that if global/container-init sees a sig_kernel_only()
+		 * signal here, the signal must have been generated internally
+		 * or must have come from an ancestor namespace. In either
+		 * case, the signal cannot be dropped.
 		 */
 		if (unlikely(signal->flags & SIGNAL_UNKILLABLE) &&
-		    !signal_group_exit(signal))
+				!sig_kernel_only(signr))
 			continue;
 
 		if (sig_kernel_stop(signr)) {

1.5.2.5

From: Sukadev Bhattiprolu <[email protected]>
Date: Wed, 24 Dec 2008 14:14:18 -0800
Subject: [PATCH 7/7][v8] SI_USER: Masquerade si_pid when crossing pid ns boundary

When sending a signal to a descendant namespace, set ->si_pid to 0 since the sender does not have a pid in the receiver's namespace.

Note:
- If rt_sigqueueinfo() sets si_code to SI_USER when sending a signal across a pid namespace boundary, the value in ->si_pid will be cleared to 0.

Changelog[v5]:
- (Oleg Nesterov) Address both sys_kill() and sys_tkill() cases in send_signal() to simplify code (this drops patch 7/7 from earlier version of patchset).

---
 kernel/signal.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

index c94355b..a416d77 100644
@@ -883,6 +883,8 @@ static int __send_signal(int sig, struct siginfo *info, struct task_struct *t,
 			break;
 		default:
 			copy_siginfo(&q->info, info);
+			if (from_ancestor_ns)
+				q->info.si_pid = 0;
 			break;
 		}
 	} else if (!is_si_special(info)) {

1.5.2.5

SI_FROMUSER() == T, unless we have more (hopefully not) in-kernel users which send SI_FROMUSER() signals, .si_pid must be valid?

kill_pid_info_as_uid() was the only known sender of SI_FROMUSER signal, it was converted to use __send_signal(from_ancestor_ns => 0).

Oleg.

So the argument is that while things such as force_sig_info(SIGSEGV) don't have a si_pid we don't care because from_ancestor_ns == 0.

Interesting. Then I don't know if we have any kernel senders that cross the namespace boundaries.

That said I still object to this code.

sys_kill(-pgrp, SIGUSR1)
kill_something_info(SIGUSR1, &info, 0)

    __kill_pgrp_info(SIGUSR1, &info task_pgrp(current))
      group_send_sig_info(SIGUSR1, &info, tsk)
        __group_send_sig_info(SIGUSR1, &info, tsk)
          send_signal(SIGUSR1, &info, tsk, 1)
            __send_signal(SIGUSR1, &info, tsk, 1)

Process groups and sessions can have processes in multiple pid namespaces, which is very useful for not messing up your controlling terminal.

In which case sys_kill cannot possibly set the si_pid value correct and from_ancestor_ns is not enough either.

So I see two valid policies with setting si_pid. Push the work out to the callers of send_signal (kill_pgrp in this case). And know you have a valid set of siginfo values. Or handle the work in send_signal.

Given that except for process groups we don't send the same siginfo to multiple processes simply generating the right siginfo values from the start appears easy enough.

I am not current with the current rule: the caller of send_signal will do all of the work except for sometimes. I don't see how we can figure out which code path has the bug in it with a rule like that.

Eric

(I know, I shouldn't reply today because I am already sleeping ;)

Why? send_signal() should calculate the correct value of from_parent and pass it to __send_signal(). If it is true, then we clear .si_pid in the copied siginfo (which was already queued). We don't mangle the original siginfo.

This happens for each process we send the signal.

Or I misunderstood you?

Oleg.

Suppose I have 3 processes in a process group in three separate pid namespaces.

Looking from the init pid namespace I have:

     pid pgrp ppid
      10 10    1
      11 10    10
      12 10    11

Looking from the pid namespace of pid 11 I have:

     pid pgrp ppid
      0  0     0
      1  0     0
      2  0     1

Looking from the pid namespace of pid 12 I have:

     pid pgrp ppid
      0  0     0
      0  0     0
      1  0     0

So if the process with pid 12 in the initial pid namespace sends to process group 0.

pid 10 should see si_pid 12.
pid 11 should see si_pid 2.

Neither should see si_pid 0, as from_ancestor_ns will not be true.

Eric

There is no "process group 0". 0 means "the sender's pgrp".

One possibility is that perhaps what people really want the pid_ns to mean is that "the sender's pgrp" in the view of the sender does not include any processes outside its pid_ns scope. That would be consistent with the behavior of kill (kill_something_info) on -1; it's described as "all processes", but in fact means "all processes within my pid_ns scope".

What I mean to describe there is changing kill_something_info, so that e.g. killpg() inside the NS would affect only the NS init itself but e.g. ^Z (effectively an implicit killpg() that's always from the global NS) would also go to that init's "mother" pgrp in the outer NS.

Another possibility is to decide that's just not worth having at all, and CLONE_NEWNS should just implicitly reset pgrp to self. That is simple. But perhaps today someone has a script running a pid_ns-world whose init is gracefully killed by ^C of the whole script and we wouldn't want to break that if it is actually useful now.

We indeed have this problem if we think it's useful to continue to have a concept of pgrp for the sub-init that can see outside its own NS.

Perhaps replace from_ancestor_ns with struct pid_namespace *sender_ns? (I don't know if there was already a can of worms with such an idea before.) Then si_pid could be translated as appropriate for each recipient. (Or perhaps just struct pid *sender and reset si_pid from that.)

Thanks,
Roland

Exactly. It just happens in this case that pid_nr_ns returns 0 for the process group number as well as the process group the process is a member of, that was created outside of the current pid namespace.

It is especially useful, and this is a deliberate feature. Having sessions and process groups extend across pid namespace borders means you can share a tty and job control functions correctly. Very handy for circumstances where you want a light weight temporary container, and something I am actively using today. The practical benefit is that you can upgrade from situations where you would previous use chroot without extra hassle.

In practice I don't care about si_pid and I doubt I care about processes sending signals outside of their pid namespace. But I do care about sharing a tty and a session and having job control work.

The last was my original line of thinking. I seem to recall Oleg figuring the code gets pretty ugly when you add in the necessary test to see if si_pid is actually present.

There are several other cases where we also signal a process outside of our current pid namespace, where we have a pid inside the recipients pid namespace. do_notify_parent is the easiest example. However those cases can get the value right because they are unicast signals and know their recipient when the set the si_pid originally.

My current line of thinking is either:
a) We pass in struct pid *sender and we reset si_pid in send_signal.
b) We make the rule that send_signal must receive a valid siginfo from the caller and we only do the extra work for process groups.

Eric

Ok, I thought that might be so.

Understood.

Well, the existing test to set from_ancestor_ns is in one place and we think that its logic is OK. What I had in mind was that when that logic says "0", we pass NULL and the innards don't touch .si_pid (same as now); when it says "1", we pass a pointer and the innards do rewrite it.

It's the only example that Oleg has mentioned. What others are there?

That's what I said. ;-) The a) option seems cleaner to me regardless, to the extent that the "from_ancestor_ns" approach is a "clean" one. But I think it would be best to fully elucidate what we think about desireable semantics for the whole spectrum of cross-NS signal-sending cases before actually choosing the implementation details.

Thanks,
Roland

With respect to si_pid I think the value should be:

    pid_nr_ns(sender, task_active_pid_ns(tsk));

With respect to init receiving signals.
- SIGKILL and SIGSTOP are ignored not coming from an ancestor pid namespace.

For the other signals since init can set it's handlers I really don't care how we handle them. If we treat them all normally /sbin/init should work fine.

For signals from the kernel I'm inclined to always call those from an ancestor pid namespace. It seems we don't have any legitimate reason to special case signals ignoring that we either asked for or that if we don't handle will cause an infinite signal handling loop in /sbin/init.

But as long as we get si_pid set correctly and we get handling of SIGKILL and SIGSTOP correct. I'm happy.

Eric

... and then you answered all the questions that are already well settled, and did not address the new question that you had raised earlier today.

To which processes should a pgrp-wide signal sent from user mode inside a pid_ns go? Should they go to a pgrp member in a different pid_ns, or not?

If your answer is that you don't care, my inclination is to leave it as it is ("my pgrp" can include processes outside your pid_ns, which you could not explicitly target in any other way). The way we are going just for the sake of cleanliness happens to make the si_pid values all work out right for this. Possibly the semantics are even what you want: If e.g. the sub-init acts like many terminal apps and might use the tty in raw mode but then handle something like ^Z by fiddling the tty and then kill(0,SIGTSTP) to act like ^Z was hit in cooked mode, then this preserves the proper effect of that suspending a whole script/pipeline.

I don't call tty signals "sending a signal to outside the PID namespace". I suspect Oleg did not intend it to be read that way either. To me, a tty signal comes from "the device"--not from some process--and I presume all such sources we'd consider to "live in the global namespace" (really they just don't exist in any PID namespace, they aren't processes). So I don't think Oleg's comment is meant to apply to such cases at all.

Ah, this is the clear example of "any to any", since all the sender and recipient have to share is the mqueue they each have a descriptor on. But, as you say, it's got no problems because the sender is just "current in mq_timedsend" to a single recipient, no different than "current in sys_kill" when that is going to a single recipient.

Agreed.

Thanks,
Roland

Oh sorry. I misunderstood what you were asking.

As it is are the easiest and most intuitive semantics to me. It is simply weird to people expecting that signals will never exit out of a pid namespace. Additionally I like having a prominent easy to create case because it makes it much easier for people to realize it can happen.

What I don't have is a compelling usage that means we must send to every process in our process group if our process group spans multiple pid namespaces. Your description of manually implementing ^Z sounds as close as I can come to a compelling case.

I simply have a compelling case for process groups and sessions that span pid namespaces where the tty sends the signals to all of the processes.

A nearly compelling case for the current process group semantics and comes from using pid namespaces as inescapable process groups. In that use case I would find it very convenient to be able to set SIGCHLD to SIG_IGN and exec an arbitrary program a pid namespace leader.

Ouch!

I have just recalled a use case that will cause problems with the current ignoring of signals in this patchset. Currently a container init can not send SIGSTOP to itself. And I have been taking advantage of that in usages such as supporting the bash suspend command or the M-x suspend-emacs. And it is very handy for getting back to a shell outside of a chroot like container. SIGTSTP will still work, but SIGSTOP which I'm pretty certain bash sends itself will not.

So I have the question. How few special cases do we need to implement to signal handling in a container init and still support running programs written to be /sbin/init, on linux. Can we limit this our special case to just ignoring SIGKILL and SIGSTOP when sent from other process in the same pid namespace? Or do we actually need more?

I suspect there are others buried in the kernel somewhere or there will be others in the future. We have a very similar pattern with fcntl and SIGIO and SIGURG, but they all look they are coming from the kernel. Everything except the tty code appears to slowly approach the general case.

Eric

But this is the different problem, it is not that we clear si_pid while we shouldn't, just the .si_pid passed from kill_something_info() is not right.

Personally, I think we should not allow to send signals outside our namespace (except SIGCHLD on exit), this looks just wrong to me. And some time ago copy_process(CLONE_PID) did "setsid".

Hmm... that was changed by your commit 5cd17569fd0eeca510735e63a6061291e3971bf6. And while I agree with this commit, I think that cinit should do sys_setsid() itself to detach itself from the parent namespace.

Or. We can fix the case you described. We can move "si_pid = task_tgid_vnr()" from sys_kill/do_tkill/etc to send_signal(), it can calculate the correct .si_pid looking at sender/receiver namespaces.

Oleg.

There are cases that happen, and it very much simplifies dealing with tty's if we allow it.

Another case where we can send signals between namespaces is posix message queues. Implemented in ipc/mqueue.c. In that case because it is a unicast message we are generating the proper si_pid when we generate the signal.

When programmers get lazy it is all to easy to have signals that are useful and cross namespace boundaries. In practice it is possible to close those holes if don't want the confusion in your userspace programs. For the people who take advantage of the flexibility of namespace to mix things together I don't think we can get away in the kernel assuming weird things won't happen.

In general that should indeed happen. There are specific cases where it is advantageous not to call setsid(). It is all under control of the creator of the pid namespace.

I think that is where we need to go, to be safe and to be certain weird things won't sneak up on us. We already handle half of the logic in send_signal anyway. We might as well handle the other half.

Eric

Hi Suka,

I agree with these semantics, they look good.

What is planned to have the init process to die when a system container shuts down ?

Let's say we use the "shutdown" command, it will telinit to go to the runlevel 0, and will kill -1.
At this point, the container finishes with a sys_reboot (we take care to do nothing otherwise the real system shuts down). But the init process will stay there and the launcher of the container will never know if the container has stopped or not.

Gregory Kurz proposed a solution:
* when shutdown is called and we are not in the init pidns, then we
kill the process 1 of the pidnamespace.
* when reboot is called and we are not in the init pidns, then we
reexec the init process, using the same command line. I guess this one could be easily retrieved if we are able to display /proc/1/cmdline ;)

IMHO, this is a good proposition because it is generic and intuitive, no ?

What do you thing ?

-- Daniel

Yes, I think it makes sense. Do we have any prototype patches that implement this behavior ?

I have a simple prototype for the first case, added in attachment.
For the second case, I didn't had time to do it yet as it is not so trivial because we force an exec of another process.

Subject: kill the pid 1 process at shutdown
From: Daniel Lezcano <[email protected]>

This patch makes the pid 1 to be killed when we shutdown the host and we are not in the init namespace.

---
 include/linux/pid_namespace.h |    5 +++++
 kernel/pid_namespace.c        |   17 +++++++++++++++++
 kernel/sys.c                  |   17 +++++++++++------
 3 files changed, 33 insertions(+), 6 deletions(-)

Index: 2.6.29-rc3/include/linux/pid_namespace.h
===================================================================
@@ -44,6 +44,7 @@ static inline struct pid_namespace *get_
 
 extern struct pid_namespace *copy_pid_ns(unsigned long flags, struct pid_namespace *ns);
 extern void free_pid_ns(struct kref *kref);
+extern int power_off_pid_ns(struct pid_namespace *ns);
 extern void zap_pid_ns_processes(struct pid_namespace *pid_ns);
 
 static inline void put_pid_ns(struct pid_namespace *ns)

@@ -72,6 +73,10 @@ static inline void put_pid_ns(struct pid
 {
 }
 
+static inline int power_off_pid_ns(struct pid_namespace *ns)
+{
+	return -EINVAL;
+}
 
 static inline void zap_pid_ns_processes(struct pid_namespace *ns)
 {

Index: 2.6.29-rc3/kernel/pid_namespace.c
===================================================================
@@ -148,6 +148,23 @@ void free_pid_ns(struct kref *kref)
 		put_pid_ns(parent);
 }
 
+int power_off_pid_ns(struct pid_namespace *pid_ns)
+{
+	struct task_struct *task;
+
+	if (pid_ns == &init_pid_ns)
+		return -EINVAL;
+
+	rcu_read_lock();
+
+	task = pid_task(find_vpid(1), PIDTYPE_PID);
+	force_sig(SIGKILL, task);
+
+	rcu_read_unlock();
+
+	return 0;
+}
+
 void zap_pid_ns_processes(struct pid_namespace *pid_ns)
 {
 	int nr;

Index: 2.6.29-rc3/kernel/sys.c
===================================================================
@@ -34,6 +34,7 @@
 #include <linux/seccomp.h>
 #include <linux/cpu.h>
 #include <linux/ptrace.h>
+#include <linux/pid_namespace.h>
 
 #include <linux/compat.h>
 #include <linux/syscalls.h>

@@ -393,15 +394,19 @@ SYSCALL_DEFINE4(reboot, int, magic1, int
 		break;
 
 	case LINUX_REBOOT_CMD_HALT:
-		kernel_halt();
-		unlock_kernel();
-		do_exit(0);
+		if (power_off_pid_ns(current->nsproxy->pid_ns)) {
+			kernel_halt();
+			unlock_kernel();
+			do_exit(0);
+		}
 		break;
 
 	case LINUX_REBOOT_CMD_POWER_OFF:
-		kernel_power_off();
-		unlock_kernel();
-		do_exit(0);
+		if (power_off_pid_ns(current->nsproxy->pid_ns)) {
+			kernel_power_off();
+			unlock_kernel();
+			do_exit(0);
+		}
 		break;
 
 	case LINUX_REBOOT_CMD_RESTART2:

Right and unlock_kernel too :)

To clarify, the current code is buggy, and the fix doesn't depend on any other patch, afaics.