[PATCH 0/4] signal: force_sig cleanups

[PATCH 0/4] signal: force_sig cleanups

[Oleg Nesterov]

The usage of force_sig/etc is almost always wrong outside
of do_trap-like paths, and this interface needs the fixes/
cleanups itself.

This series starts the necessary cleanups. Initially it had
one more patch, send_sig_all()->force_sig() is absolutely
wrong and should be replaced with SEND_SIG_FORCED too. But
this conflicts with other changes in tty.git, I need to
discuss this with Anton/Greg.

Oleg.

[PATCH 1/4] signal: give SEND_SIG_FORCED more power to beat SIGNAL_UNKILLABLE

[Oleg Nesterov]

force_sig_info() and friends have the special semantics for
synchronous signals, this interface should not be used if the
target is not current. And it needs the fixes, in particular
the clearing of SIGNAL_UNKILLABLE is not exactly right.

However there are callers which have to use force_ exactly because
it clears SIGNAL_UNKILLABLE and thus it can kill the CLONE_NEWPID
tasks, although this is almost always is wrong by various reasons.

With this patch SEND_SIG_FORCED ignores SIGNAL_UNKILLABLE, like
we do if the signal comes from the ancestor namespace.

This makes the naming in prepare_signal() paths insane, fixed by
the next cleanup.

Note: this only affects SIGKILL/SIGSTOP, but this is enough for
force_sig() abusers.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 kernel/signal.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/kernel/signal.c b/kernel/signal.c
index c73c428..bfb2b97 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -1059,7 +1059,8 @@ static int __send_signal(int sig, struct siginfo *info, struct task_struct *t,
 
 	assert_spin_locked(&t->sighand->siglock);
 
-	if (!prepare_signal(sig, t, from_ancestor_ns))
+	if (!prepare_signal(sig, t,
+			from_ancestor_ns || (info == SEND_SIG_FORCED)))
 		return 0;
 
 	pending = group ? &t->signal->shared_pending : &t->pending;
-- 
1.5.5.1

Re: [PATCH 1/4] signal: give SEND_SIG_FORCED more power to beat SIGNAL_UNKILLABLE

[Tejun Heo]

Hello, Oleg,.

I can't provide any meaningful constructive criticism but have one
bike-shedding one.

On Fri, Feb 10, 2012 at 09:00:21PM +0100, Oleg Nesterov wrote:
> diff --git a/kernel/signal.c b/kernel/signal.c
> index c73c428..bfb2b97 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -1059,7 +1059,8 @@ static int __send_signal(int sig, struct siginfo *info, struct task_struct *t,
>  
>  	assert_spin_locked(&t->sighand->siglock);
>  
> -	if (!prepare_signal(sig, t, from_ancestor_ns))
> +	if (!prepare_signal(sig, t,
> +			from_ancestor_ns || (info == SEND_SIG_FORCED)))

How about the following indentation instead?  :)

	if (!prepare_signal(sig, t,
	    from_ancestor_ns || (info == SEND_SIG_FORCED)))

Overall, the changes look sane to me but I haven't really thought
about it deeply.  Please feel free to add Reviewed-by for 2-4.

Thank you.

-- 
tejun

Re: [PATCH 1/4] signal: give SEND_SIG_FORCED more power to beat SIGNAL_UNKILLABLE

[Oleg Nesterov]

Hi Tejun,

On 02/10, Tejun Heo wrote:
>
> > --- a/kernel/signal.c
> > +++ b/kernel/signal.c
> > @@ -1059,7 +1059,8 @@ static int __send_signal(int sig, struct siginfo *info, struct task_struct *t,
> >
> >  	assert_spin_locked(&t->sighand->siglock);
> >
> > -	if (!prepare_signal(sig, t, from_ancestor_ns))
> > +	if (!prepare_signal(sig, t,
> > +			from_ancestor_ns || (info == SEND_SIG_FORCED)))
>
> How about the following indentation instead?  :)
>
> 	if (!prepare_signal(sig, t,
> 	    from_ancestor_ns || (info == SEND_SIG_FORCED)))

Well, I am not sure this looks better, although I do not really mind.
But since this patch is already in -mm, I think I won't send v2 ;)

> Please feel free to add Reviewed-by for 2-4.

Thanks!

Oleg.

Re: [PATCH 1/4] signal: give SEND_SIG_FORCED more power to beat SIGNAL_UNKILLABLE

[Tejun Heo]

On Tue, Feb 14, 2012 at 06:45:28PM +0100, Oleg Nesterov wrote:
> > How about the following indentation instead?  :)
> >
> > 	if (!prepare_signal(sig, t,
> > 	    from_ancestor_ns || (info == SEND_SIG_FORCED)))
> 
> Well, I am not sure this looks better, although I do not really mind.
> But since this patch is already in -mm, I think I won't send v2 ;)

Yeah, they are different but equally ugly, I suppose. :)

-- 
tejun

[PATCH 2/4] signal: cosmetic, s/from_ancestor_ns/force/ in prepare_signal() paths

[Oleg Nesterov]

Cosmetic, rename the from_ancestor_ns argument in prepare_signal()
paths. After the previous change it doesn't match the reality.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 kernel/signal.c |   15 +++++++--------
 1 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/kernel/signal.c b/kernel/signal.c
index bfb2b97..541905b 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -58,21 +58,20 @@ 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,
-		int from_ancestor_ns)
+static int sig_task_ignored(struct task_struct *t, int sig, bool force)
 {
 	void __user *handler;
 
 	handler = sig_handler(t, sig);
 
 	if (unlikely(t->signal->flags & SIGNAL_UNKILLABLE) &&
-			handler == SIG_DFL && !from_ancestor_ns)
+			handler == SIG_DFL && !force)
 		return 1;
 
 	return sig_handler_ignored(handler, sig);
 }
 
-static int sig_ignored(struct task_struct *t, int sig, int from_ancestor_ns)
+static int sig_ignored(struct task_struct *t, int sig, bool force)
 {
 	/*
 	 * Blocked signals are never ignored, since the
@@ -82,7 +81,7 @@ static int sig_ignored(struct task_struct *t, int sig, int from_ancestor_ns)
 	if (sigismember(&t->blocked, sig) || sigismember(&t->real_blocked, sig))
 		return 0;
 
-	if (!sig_task_ignored(t, sig, from_ancestor_ns))
+	if (!sig_task_ignored(t, sig, force))
 		return 0;
 
 	/*
@@ -855,7 +854,7 @@ static void ptrace_trap_notify(struct task_struct *t)
  * Returns true if the signal should be actually delivered, otherwise
  * it should be dropped.
  */
-static int prepare_signal(int sig, struct task_struct *p, int from_ancestor_ns)
+static int prepare_signal(int sig, struct task_struct *p, bool force)
 {
 	struct signal_struct *signal = p->signal;
 	struct task_struct *t;
@@ -915,7 +914,7 @@ static int prepare_signal(int sig, struct task_struct *p, int from_ancestor_ns)
 		}
 	}
 
-	return !sig_ignored(p, sig, from_ancestor_ns);
+	return !sig_ignored(p, sig, force);
 }
 
 /*
@@ -1595,7 +1594,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, 0))
+	if (!prepare_signal(sig, t, false))
 		goto out;
 
 	ret = 0;
-- 
1.5.5.1

[PATCH 3/4] signal: oom_kill_task: use SEND_SIG_FORCED instead of force_sig()

[Oleg Nesterov]

Change oom_kill_task() to use do_send_sig_info(SEND_SIG_FORCED)
instead of force_sig(SIGKILL). With the recent changes we do not
need force_ to kill the CLONE_NEWPID tasks.

And this is more correct. force_sig() can race with the exiting
thread even if oom_kill_task() checks p->mm != NULL, while
do_send_sig_info(group => true) kille the whole process.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 mm/oom_kill.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index 2958fd8..b1e9643 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -472,11 +472,11 @@ static int oom_kill_task(struct task_struct *p)
 			pr_err("Kill process %d (%s) sharing same memory\n",
 				task_pid_nr(q), q->comm);
 			task_unlock(q);
-			force_sig(SIGKILL, q);
+			do_send_sig_info(SIGKILL, SEND_SIG_FORCED, q, true);
 		}
 
 	set_tsk_thread_flag(p, TIF_MEMDIE);
-	force_sig(SIGKILL, p);
+	do_send_sig_info(SIGKILL, SEND_SIG_FORCED, p, true);
 
 	return 0;
 }
-- 
1.5.5.1

Re: [PATCH 3/4] signal: oom_kill_task: use SEND_SIG_FORCED instead of force_sig()

[KOSAKI Motohiro]

2012/2/10 Oleg Nesterov <oleg@redhat.com>:
> Change oom_kill_task() to use do_send_sig_info(SEND_SIG_FORCED)
> instead of force_sig(SIGKILL). With the recent changes we do not
> need force_ to kill the CLONE_NEWPID tasks.
>
> And this is more correct. force_sig() can race with the exiting
> thread even if oom_kill_task() checks p->mm != NULL, while
> do_send_sig_info(group => true) kille the whole process.
>
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
>  mm/oom_kill.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> index 2958fd8..b1e9643 100644
> --- a/mm/oom_kill.c
> +++ b/mm/oom_kill.c
> @@ -472,11 +472,11 @@ static int oom_kill_task(struct task_struct *p)
>                        pr_err("Kill process %d (%s) sharing same memory\n",
>                                task_pid_nr(q), q->comm);
>                        task_unlock(q);
> -                       force_sig(SIGKILL, q);
> +                       do_send_sig_info(SIGKILL, SEND_SIG_FORCED, q, true);
>                }
>
>        set_tsk_thread_flag(p, TIF_MEMDIE);
> -       force_sig(SIGKILL, p);
> +       do_send_sig_info(SIGKILL, SEND_SIG_FORCED, p, true);
>
>        return 0;

I don't think I clearly understand this series. But, at least, this
patch is ok to me. thanks.

[PATCH 4/4] signal: zap_pid_ns_processes: s/SEND_SIG_NOINFO/SEND_SIG_FORCED/

[Oleg Nesterov]

Change zap_pid_ns_processes() to use SEND_SIG_FORCED, it looks more
clear compared to SEND_SIG_NOINFO which relies on from_ancestor_ns
logic send_signal().

It is also more effecient if we need to kill a lot of tasks because
it doesn't alloc sigqueue.

While at it, add the __fatal_signal_pending(task) check as a minor
optimization.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 kernel/pid_namespace.c |    8 ++------
 1 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/kernel/pid_namespace.c b/kernel/pid_namespace.c
index a896839..17b2328 100644
--- a/kernel/pid_namespace.c
+++ b/kernel/pid_namespace.c
@@ -168,13 +168,9 @@ void zap_pid_ns_processes(struct pid_namespace *pid_ns)
 	while (nr > 0) {
 		rcu_read_lock();
 
-		/*
-		 * Any nested-container's init processes won't ignore the
-		 * SEND_SIG_NOINFO signal, see send_signal()->si_fromuser().
-		 */
 		task = pid_task(find_vpid(nr), PIDTYPE_PID);
-		if (task)
-			send_sig_info(SIGKILL, SEND_SIG_NOINFO, task);
+		if (task && !__fatal_signal_pending(task))
+			send_sig_info(SIGKILL, SEND_SIG_FORCED, task);
 
 		rcu_read_unlock();
 
-- 
1.5.5.1