[Devel] Re: [PATCH] pidns: Fix wait for zombies to be reaped in zap_pid_ns_processes

[Sukadev Bhattiprolu]

Eric W. Biederman [ebiederm at xmission.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 at kerlabs.com>

Reviewed-by: Sukadev Bhattiprolu <sukadev at linux.vnet.ibm.com>

| Signed-off-by: Eric W. Biederman <ebiederm at xmission.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.
_______________________________________________
Containers mailing list
Containers at lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers