[PATCH] change kernel threads to ignore signals instead of blocking them

[PATCH] change kernel threads to ignore signals instead of blocking them

[Oleg Nesterov]

On top of Eric's

	kthread-dont-depend-on-work-queues-take-2.patch

Currently kernel threads use sigprocmask(SIG_BLOCK) to protect against signals.
This doesn't prevent the signal delivery, this only blocks signal_wake_up().
Every "killall -33 kthreadd" means a "struct siginfo" leak.

Change kthreadd_setup() to set all handlers to SIG_IGN instead of blocking them
(make a new helper ignore_signals() for that). If the kernel thread needs some
signal, it should use allow_signal() anyway, and in that case it should not use
CLONE_SIGHAND.

Note that we can't change daemonize() (should die!) in the same way, because
it can be used along with CLONE_SIGHAND. This means that allow_signal() still
should unblock the signal to work correctly with daemonize()ed threads.

However, disallow_signal() doesn't block the signal any longer but ignores it.

NOTE: with or without this patch the kernel threads are not protected from
handle_stop_signal(), this seems harmless, but not good.

Signed-off-by: Oleg Nesterov <oleg@tv-sign.ru>

 include/linux/sched.h |    1 +
 kernel/exit.c         |    2 +-
 kernel/kthread.c      |   17 +++--------------
 kernel/signal.c       |   10 ++++++++++
 4 files changed, 15 insertions(+), 15 deletions(-)

--- 2.6.21-rc5/include/linux/sched.h~1_SIGIGN	2007-04-05 12:18:28.000000000 +0400
+++ 2.6.21-rc5/include/linux/sched.h	2007-04-13 00:09:56.000000000 +0400
@@ -1299,6 +1299,7 @@ extern int in_egroup_p(gid_t);
 
 extern void proc_caches_init(void);
 extern void flush_signals(struct task_struct *);
+extern void ignore_signals(struct task_struct *);
 extern void flush_signal_handlers(struct task_struct *, int force_default);
 extern int dequeue_signal(struct task_struct *tsk, sigset_t *mask, siginfo_t *info);
 
--- 2.6.21-rc5/kernel/signal.c~1_SIGIGN	2007-04-05 12:18:28.000000000 +0400
+++ 2.6.21-rc5/kernel/signal.c	2007-04-13 02:14:06.000000000 +0400
@@ -329,6 +329,16 @@ void flush_signals(struct task_struct *t
 	spin_unlock_irqrestore(&t->sighand->siglock, flags);
 }
 
+void ignore_signals(struct task_struct *t)
+{
+	int i;
+
+	for (i = 0; i < _NSIG; ++i)
+		t->sighand->action[i].sa.sa_handler = SIG_IGN;
+
+	flush_signals(t);
+}
+
 /*
  * Flush all handlers for a task.
  */
--- 2.6.21-rc5/kernel/kthread.c~1_SIGIGN	2007-04-12 23:18:09.000000000 +0400
+++ 2.6.21-rc5/kernel/kthread.c	2007-04-13 02:27:39.000000000 +0400
@@ -215,24 +215,13 @@ EXPORT_SYMBOL(kthread_stop);
 static __init void kthreadd_setup(void)
 {
 	struct task_struct *tsk = current;
-	struct k_sigaction sa;
-	sigset_t blocked;
 
 	set_task_comm(tsk, "kthreadd");
 
-	/* Block and flush all signals */
-	sigfillset(&blocked);
-	sigprocmask(SIG_BLOCK, &blocked, NULL);
-	flush_signals(tsk);
-
-	/* SIG_IGN makes children autoreap: see do_notify_parent(). */
-	sa.sa.sa_handler = SIG_IGN;
-	sa.sa.sa_flags = 0;
-	siginitset(&sa.sa.sa_mask, sigmask(SIGCHLD));
-	do_sigaction(SIGCHLD, &sa, (struct k_sigaction *)0);
+	ignore_signals(tsk);
 
-	set_user_nice(current, -5);
-	set_cpus_allowed(current, CPU_MASK_ALL);
+	set_user_nice(tsk, -5);
+	set_cpus_allowed(tsk, CPU_MASK_ALL);
 }
 
 int kthreadd(void *unused)
--- 2.6.21-rc5/kernel/exit.c~1_SIGIGN	2007-04-12 23:23:50.000000000 +0400
+++ 2.6.21-rc5/kernel/exit.c	2007-04-13 10:17:06.000000000 +0400
@@ -348,7 +348,7 @@ int disallow_signal(int sig)
 		return -EINVAL;
 
 	spin_lock_irq(&current->sighand->siglock);
-	sigaddset(&current->blocked, sig);
+	current->sighand->action[(sig)-1].sa.sa_handler = SIG_IGN;
 	recalc_sigpending();
 	spin_unlock_irq(&current->sighand->siglock);
 	return 0;

Re: [PATCH] change kernel threads to ignore signals instead of blocking them

[Eric W. Biederman]

Oleg Nesterov <oleg@tv-sign.ru> writes:

> On top of Eric's
>
> 	kthread-dont-depend-on-work-queues-take-2.patch
>
> Currently kernel threads use sigprocmask(SIG_BLOCK) to protect against signals.
> This doesn't prevent the signal delivery, this only blocks signal_wake_up().
> Every "killall -33 kthreadd" means a "struct siginfo" leak.
>
> Change kthreadd_setup() to set all handlers to SIG_IGN instead of blocking them
> (make a new helper ignore_signals() for that). If the kernel thread needs some
> signal, it should use allow_signal() anyway, and in that case it should not use
> CLONE_SIGHAND.
>
> Note that we can't change daemonize() (should die!) in the same way, because
> it can be used along with CLONE_SIGHAND. This means that allow_signal() still
> should unblock the signal to work correctly with daemonize()ed threads.
>
> However, disallow_signal() doesn't block the signal any longer but ignores it.
>
> NOTE: with or without this patch the kernel threads are not protected from
> handle_stop_signal(), this seems harmless, but not good.

Hmm.  I like it all except for disallow_signal.

disallow_signal currently only has one user, jffs2.  While jffs2
currently doesn't care, given the way jffs2 is using disallow_signal I
would expect it would prefer to have the signal blocked.

Thinking about this some more if jffs2 or anyone else wants blocked
signal behavior they can go ahead and block the signal.  Keeping
disallow_signal in sync with allow_signal seems to make sense.

Nothing in the kernel should break with this change so:

Acked-by: "Eric W. Biederman" <ebiederm@xmission.com>

Re: [PATCH] change kernel threads to ignore signals instead of blocking them

[Andrew Morton]

On Fri, 13 Apr 2007 08:13:32 -0600
ebiederm@xmission.com (Eric W. Biederman) wrote:

> Oleg Nesterov <oleg@tv-sign.ru> writes:
> 
> > On top of Eric's
> >
> > 	kthread-dont-depend-on-work-queues-take-2.patch
> >
> > Currently kernel threads use sigprocmask(SIG_BLOCK) to protect against signals.
> > This doesn't prevent the signal delivery, this only blocks signal_wake_up().
> > Every "killall -33 kthreadd" means a "struct siginfo" leak.
> >
> > Change kthreadd_setup() to set all handlers to SIG_IGN instead of blocking them
> > (make a new helper ignore_signals() for that). If the kernel thread needs some
> > signal, it should use allow_signal() anyway, and in that case it should not use
> > CLONE_SIGHAND.
> >
> > Note that we can't change daemonize() (should die!) in the same way, because
> > it can be used along with CLONE_SIGHAND. This means that allow_signal() still
> > should unblock the signal to work correctly with daemonize()ed threads.
> >
> > However, disallow_signal() doesn't block the signal any longer but ignores it.
> >
> > NOTE: with or without this patch the kernel threads are not protected from
> > handle_stop_signal(), this seems harmless, but not good.
> 
> Hmm.  I like it all except for disallow_signal.
> 
> disallow_signal currently only has one user, jffs2.  While jffs2
> currently doesn't care, given the way jffs2 is using disallow_signal I
> would expect it would prefer to have the signal blocked.
> 
> Thinking about this some more if jffs2 or anyone else wants blocked
> signal behavior they can go ahead and block the signal.  Keeping
> disallow_signal in sync with allow_signal seems to make sense.

jffs2 actually wants its head examined.  W. T. F. does it think it's
doing in there?

Sigh.

Re: [PATCH] change kernel threads to ignore signals instead of blocking them

[Eric W. Biederman]

Andrew Morton <akpm@linux-foundation.org> writes:

> jffs2 actually wants its head examined.  W. T. F. does it think it's
> doing in there?

Good question, especially with respect to SIGHUP.

It is on my short list of very annoying kernel threads...

NFS and a few kernel threads others currently need a way to abort an
interruptible sleep when the are terminated.  Which has been a pain
finding a simple way to allow that without too much trouble.

I'm within about a day of having everything converted over to
kthreads, allowing me to kill daemonize.  Hopefully things I don't run
into any serious snags in testing...

Eric

Re: [PATCH] change kernel threads to ignore signals instead of blocking them

[Andrew Morton]

On Fri, 13 Apr 2007 11:31:16 +0400 Oleg Nesterov <oleg@tv-sign.ru> wrote:

> On top of Eric's
> 
> 	kthread-dont-depend-on-work-queues-take-2.patch
> 
> Currently kernel threads use sigprocmask(SIG_BLOCK) to protect against signals.
> This doesn't prevent the signal delivery, this only blocks signal_wake_up().
> Every "killall -33 kthreadd" means a "struct siginfo" leak.
> 
> Change kthreadd_setup() to set all handlers to SIG_IGN instead of blocking them
> (make a new helper ignore_signals() for that). If the kernel thread needs some
> signal, it should use allow_signal() anyway, and in that case it should not use
> CLONE_SIGHAND.
> 
> Note that we can't change daemonize() (should die!) in the same way, because
> it can be used along with CLONE_SIGHAND. This means that allow_signal() still
> should unblock the signal to work correctly with daemonize()ed threads.
> 
> However, disallow_signal() doesn't block the signal any longer but ignores it.
> 
> NOTE: with or without this patch the kernel threads are not protected from
> handle_stop_signal(), this seems harmless, but not good.

I'm seeing 500 zombied instances of khelper (from udev startup).  It only
happens when the utrace patches are applied.  Presumably an interaction
between utrace and one of these kthread changes.

I'll drop utrace for now.  I don't think it's getting much help from being
in -mm at present and it's getting increasingly painful to keep it merged
against all the other stuff which is happening.

Roland, I'll squirt all the extra utrace patches which I have in your direction.
Please merge them or hang on to them for later on.