Prev: KVM: MMU: track dirty page in speculative path properly
Next: [PATCH] x86: only set early_serial_base after port is initialized
From: Eric W. Biederman on 13 Jul 2010 21:50
Louis Rilling <Louis.Rilling(a)kerlabs.com> writes:

> This patch looks like it is working (only a small RCU issue shown below). I
> couldn't try it yet though.

It certainly worked in my testing.

> I must admit that I am using a similar back-off solution in Kerrighed (not to
> solve the issue of proc_flush_task(), but for one of the reasons that you stated
> above: we want to be sure that all tasks of the namespace have been reaped), but
> I considered it too ugly to propose it for Linux ;)

Well sometimes you have to go with what works.

Thanks for spotting those issue with my patch. I guess it needs one more
pass before I can call it done.

> That said, this is probably the least intrusive solution we have seen yet.

Thanks for the review. The bug where processes can escape a pid
namespace is really the reason I did it this way. I also have the
patches needed to cleanly fix the pid namespace ref counting.
(Hopefully I can get them posted soon). So if there was just the
ref counting bug I would drop this patch.

Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Eric W. Biederman on 14 Jul 2010 17:40
Sukadev Bhattiprolu <sukadev(a)linux.vnet.ibm.com> writes:

> Eric W. Biederman [ebiederm(a)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(a)kerlabs.com>
>
> Reviewed-by: Sukadev Bhattiprolu <sukadev(a)linux.vnet.ibm.com>
>
> | Signed-off-by: Eric W. Biederman <ebiederm(a)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

Agreed.

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

Which means we exercise that code path, and ensure we have same_thread_group
test working properly. Given how rare threaded inits are every little bit
of extra test coverage that doesn't really cost us anything seems important.

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

It is possible that other threads of a multi-threaded init are in the PF_EXITING
state and still visible for sending signals to. I really don't want to send
SIG_KILL to another thread of init. There is a chance of messing up the return
code if I do that, and do not want to need to think about that case.

Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
 | 
Pages: 1
Prev: KVM: MMU: track dirty page in speculative path properly
Next: [PATCH] x86: only set early_serial_base after port is initialized