linux-kernel - Re: Kernel threads

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20070309213852.059FC1801C4@magilla.sf.frob.com>
Date:	Fri,  9 Mar 2007 13:38:51 -0800 (PST)
From:	Roland McGrath <roland@...hat.com>
To:	Oleg Nesterov <oleg@...sign.ru>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	"linux-os (Dick Johnson)" <linux-os@...logic.com>,
	linux-kernel@...r.kernel.org
Subject: Re: Kernel threads

> 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 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);
	else
		BUG_ON((clone_flags & (CLONE_THREAD|CLONE_UNTRACED))
		       != CLONE_UNTRACED);

> 	zap_other_threads:
> 
> 		if (t != p->group_leader)
> 			t->exit_signal = -1;
> 
> looks like another leftover to me, we already depend on the fact that
> all sub-threads have ->exit_signal == -1 (otherwise, for example, a
> thread group just can't exit properly).

Yes, I agree it looks superfluous.

> 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.

> 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).  There might be other strange caveats like that I'm not
thinking of.  

Thanks,
Roland

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/