Re: Kernel threads

[Oleg Nesterov <oleg@tv-sign.ru>]

On 03/09, Roland McGrath wrote:
>
> > Yes sure, this change shoud be tested in -mm tree (I'll send the patch
> > on Sunday after some testing). The only (afaics) problem is that with
> > this change a kernel thread must not do do_fork(CLONE_THREAD). 
> 
> To clarify, the danger here is that an exit_signal=-1 leader would
> self-reap and leave behind live threads with dangling ->group_leader
> pointers.  This danger doesn't exist for normal user group leaders with
> parents ignoring SIGCHLD, because exit_signal is never set to -1 until
> do_notify_parent, which is never called until the last thread in the group
> dies (except when ptrace'd, but then do_notify_parent never resets
> exit_signal at all).  Is that right?

I think yes.

> > I think it should not, but currently this is technically
> > possible. Perhaps it makes sense to add BUG_ON(CLONE_THREAD &&
> > group_leader->exit_signal==-1) in copy_process().
>
> It probably wouldn't hurt to make it:
>
> 	if (user_mode(regs))
> 		BUG_ON(current->group_leader->exit_signal == -1);

Well, this is of course right, but a bit strange. Because we can add
this check to any function which can't be called after exit_notify().

> 	else
> 		BUG_ON((clone_flags & (CLONE_THREAD|CLONE_UNTRACED))
> 		       != CLONE_UNTRACED);

I think this _should_ be right, but please note that fork_idle() does
copy_process(CLONE_VM). Also, we may have some external driver which
plays with do_fork/copy_process.

> > While we are talking about kernel threads, there is something I can't
> > undestand. kthread/daemonize use sigprocmask(SIG_BLOCK) to protect
> > against signals. This doesn't look right to me, because this doesn't
> > prevent the signal delivery, this only blocks signal_wake_up(). Every
> > "killall -33 khelper" means a "struct siginfo" leak.
>
> It does prevent the delivery (signal_pending() never set), but not the queuing.

Yep.

> > Imho, the kernel thread shouldn't play with ->blocked at all. Instead
> > it should set SIG_IGN for all handlers. If it really needs, say, SIGCHLD,
> > it should call allow_signal() anyway. Do you see any problems with this
> > approach?
>
> That sounds reasonable to me generally.  However, if kernel threads ever
> spawn user children, they may not want the self-reaping behavior of
> ignoring SIGCHLD even if they never dequeue the signal (because they want
> to call do_wait).

Yes. That is why wait_for_helper() does allow_signal(SIGCHLD). I think a
kernel thread must not make any assumption about ->action[SIGCHLD] if it
wants to call wait4, but we may break some "buggy" external driver.

In fact, most threads inherit action[SIGCHLD] == SIG_IGN from worker_thread().

BTW, wait_for_helper() does do_sigaction() before allow_signal(). Looks
unneeded to me.

>                    There might be other strange caveats like that I'm not
> thinking of.

Yes, this makes me worry too :)

Oleg.