signal: force_sig cleanups

PATCH In review Oleg Nesterov 3 participants 9 messages Feb 10, 2012 – Feb 14, 2012

Subsystems (3)

Memory management Memory management - OOM killer Namespaces:

lore

Share & tools

lore

Oleg Nesterov

author

2012-02-10 20:06 UTC #0 lore raw

cover signal: force_sig cleanups

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.

Oleg Nesterov

author

2012-02-10 20:06 UTC #1 lore raw

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

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 <[email protected]>

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

kernel/signal.c +2 -1

index c73c428..bfb2b97 100644
@@ -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;

Signature · 1 line

1.5.5.1

Tejun Heo

2012-02-10 21:25 UTC #2 lore raw

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

Hello, Oleg,.

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

kernel/signal.c static int __send_signal(int sig, struct siginfo *info, struct task_struct *t, · line 1063

Quoted 11 lines · Oleg Nesterov

kernel/signal.c +2 -1

index c73c428..bfb2b97 100644
@@ -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.

Signature · 1 line

tejun

Oleg Nesterov

author

2012-02-14 17:52 UTC #3 lore raw

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

Hi Tejun,

Quoted 7 lines · Tejun Heo

Quoted 7 lines

kernel/signal.c +0 -0

@@ -1059,7 +1059,8 @@ static int __send_signal(int sig, struct siginfo *info, struct task_struct *t,
 
 	assert_spin_locked(&t->sighand->siglock);

Quoted 8 lines

kernel/signal.c static int __send_signal(int sig, struct siginfo *info, struct task_struct *t, · line 1063
Quoted 3 lines
kernel/signal.c +2 -1
-	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 ;)

Quoted 1 line

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

Thanks!

Oleg.

Tejun Heo

2012-02-14 17:53 UTC #4 lore raw

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

Quoted 7 lines · Oleg Nesterov

Quoted 4 lines
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. :)

Signature · 1 line

tejun

Oleg Nesterov

author

2012-02-10 20:07 UTC #5 lore raw

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

In reply to 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 <[email protected]>

---
 kernel/signal.c |   15 +++++++--------
 1 files changed, 7 insertions(+), 8 deletions(-)

kernel/signal.c +7 -8

index bfb2b97..541905b 100644
@@ -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;

Signature · 1 line

1.5.5.1

Oleg Nesterov

author

2012-02-10 20:07 UTC #6 lore raw

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

In reply to 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 <[email protected]>

---
 mm/oom_kill.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

mm/oom_kill.c +2 -2

index 2958fd8..b1e9643 100644
@@ -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;
 }

Signature · 1 line

1.5.5.1

KOSAKI Motohiro

2012-02-13 16:49 UTC #7 lore raw

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

2012/2/10 Oleg Nesterov <[email protected]>:

Quoted 20 lines

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 <[email protected]>
---
mm/oom_kill.c | 4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
mm/oom_kill.c +0 -0
index 2958fd8..b1e9643 100644
@@ -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);

mm/oom_kill.c static int oom_kill_task(struct task_struct *p) · line 481

Quoted 10 lines

mm/oom_kill.c +2 -2

                        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.

Oleg Nesterov

author

2012-02-10 20:07 UTC #8 lore raw

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

In reply to 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 <[email protected]>

---
 kernel/pid_namespace.c |    8 ++------
 1 files changed, 2 insertions(+), 6 deletions(-)

kernel/pid_namespace.c +2 -6

index a896839..17b2328 100644
@@ -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();

Signature · 1 line

1.5.5.1