2007-02-26 21:53:42

by Oleg Nesterov

Subject: [PATCH] ____call_usermodehelper: don't flush_signals()

____call_usermodehelper() has no reason for flush_signals(). It is a fresh
forked process which is going to exec a user-space application or exit on
failure.

Signed-off-by: Oleg Nesterov <[email protected]>

--- WQ/kernel/kmod.c~ 2007-02-18 22:56:49.000000000 +0300
+++ WQ/kernel/kmod.c 2007-02-27 00:05:08.000000000 +0300
@@ -256,7 +256,6 @@ static int ____call_usermodehelper(void

/* Unblock all signals and set the session keyring. */
new_session = key_get(sub_info->ring);
- flush_signals(current);
spin_lock_irq(&current->sighand->siglock);
old_session = __install_session_keyring(current, new_session);
flush_signal_handlers(current, 1);

2007-02-26 23:46:29

by Rusty Russell

Subject: Re: [PATCH] ____call_usermodehelper: don't flush_signals()

On Tue, 2007-02-27 at 00:53 +0300, Oleg Nesterov wrote:
> ____call_usermodehelper() has no reason for flush_signals(). It is a fresh
> forked process which is going to exec a user-space application or exit on
> failure.

And the flush_signal_handlers() call?

Your patch looks correct; this code was presumably lying around from
before someone (I?) adapted this code to use kthread.

Thanks,
Rusty.

2007-02-27 09:22:17

by Oleg Nesterov

Subject: Re: [PATCH] ____call_usermodehelper: don't flush_signals()

On 02/27, Rusty Russell wrote:
>
> On Tue, 2007-02-27 at 00:53 +0300, Oleg Nesterov wrote:
> > ____call_usermodehelper() has no reason for flush_signals(). It is a fresh
> > forked process which is going to exec a user-space application or exit on
> > failure.
>
> And the flush_signal_handlers() call?

Well... flush_old_exec() calls flush_signal_handlers(force_default = 0),
this is not enough. We don't want to start user-space application with
SIGKILL ignored (yes, parent doesn't ignores it currently).

> Your patch looks correct; this code was presumably lying around from
> before someone (I?) adapted this code to use kthread.

Thanks! What I can't understand is why kthread/daemonize block all signals.
This doesn't look good to me. This can't prevent signal delivery, this only
blocks signal_wake_up(). We can even set SIGNAL_GROUP_EXIT for the kernel
thread. Of course, only root can do that, but isn't it better to just ignore
all signals instead of blocking them?

Note that we can't free the memory if we send a signal to (for example)
khelper.

Oleg.