linux-kernel - Re: [PATCH] pidns: Fix wait for zombies to be reaped in zap_pid_ns

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]

Date:	Wed, 14 Jul 2010 13:53:24 -0700
From:	Sukadev Bhattiprolu <sukadev@...ux.vnet.ibm.com>
To:	"Eric W. Biederman" <ebiederm@...ssion.com>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Linux Containers <containers@...ts.osdl.org>,
	linux-kernel@...r.kernel.org, Pavel Emelyanov <xemul@...nvz.org>,
	Oleg Nesterov <oleg@...hat.com>,
	"Serge E. Hallyn" <serge@...lyn.com>
Subject: Re: [PATCH] pidns: Fix wait for zombies to be reaped in
	zap_pid_ns_processes

Eric W. Biederman [ebiederm@...ssion.com] wrote:
| 
|   Changing zap_pid_ns_processes to fix the problem instead of
|   changing the code elsewhere is one of the few solutions I have
|   seen that does not increase the cost of the lat_proc test from
|   lmbench.

I think its a good fix for the problem. but I have a nit and a minor
comment below.

Thanks,

Sukadev

| 
| Reported-by: Louis Rilling <Louis.Rilling@...labs.com>

Reviewed-by: Sukadev Bhattiprolu <sukadev@...ux.vnet.ibm.com>

| Signed-off-by: Eric W. Biederman <ebiederm@...ssion.com>
| ---
|  kernel/pid_namespace.c |   50 ++++++++++++++++++++++++++++++++++++-----------
|  1 files changed, 38 insertions(+), 12 deletions(-)
| 
| diff --git a/kernel/pid_namespace.c b/kernel/pid_namespace.c
| index a5aff94..aaf2ab0 100644
| --- a/kernel/pid_namespace.c
| +++ b/kernel/pid_namespace.c
| @@ -139,16 +139,20 @@ void free_pid_ns(struct kref *kref)
| 
|  void zap_pid_ns_processes(struct pid_namespace *pid_ns)
|  {
| +	struct task_struct *me = current;
|  	int nr;
|  	int rc;
|  	struct task_struct *task;
| 
|  	/*
| -	 * The last thread in the cgroup-init thread group is terminating.
| -	 * Find remaining pid_ts in the namespace, signal and wait for them
| -	 * to exit.
| +	 * The last task in the pid namespace-init threa group is terminating.

nit:                                               thread

| +	 * Find remaining pids in the namespace, signal and wait for them
| +	 * to to be reaped.
|  	 *
| -	 * Note:  This signals each threads in the namespace - even those that
| +	 * By waiting for all of the tasks to be reaped before init is reaped
| +	 * we provide the invariant that no task can escape the pid namespace.
| +	 *
| +	 * Note:  This signals each task in the namespace - even those that
|  	 * 	  belong to the same thread group, To avoid this, we would have
|  	 * 	  to walk the entire tasklist looking a processes in this
|  	 * 	  namespace, but that could be unnecessarily expensive if the
| @@ -157,28 +161,50 @@ void zap_pid_ns_processes(struct pid_namespace *pid_ns)
|  	 *
|  	 */
|  	read_lock(&tasklist_lock);
| -	nr = next_pidmap(pid_ns, 1);
| -	while (nr > 0) {
| -		rcu_read_lock();
| +	for (nr = next_pidmap(pid_ns, 0); nr > 0; nr = next_pidmap(pid_ns, nr)) {

Is it necessary to start the search at nr == 0 ? We will find nr == 1
first and then immediately skip over it - bc same_thread_group() will
be TRUE.

| 
|  		/*
|  		 * 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)
| +		rcu_read_lock();
| +		task = pid_task(find_pid_ns(nr, pid_ns), PIDTYPE_PID);
| +		if (task && !same_thread_group(task, me))
|  			send_sig_info(SIGKILL, SEND_SIG_NOINFO, task);

Also, if we start the search at 1, we could skip the only the other possible
thread in the group with

	(nr != my_pid_nr)

but its not really an optimization.
--
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/