Sukadev Bhattiprolu <sukadev@xxxxxxxxxxxxxxxxxx> writes:
> Eric W. Biederman [ebiederm@xxxxxxxxxxxx] 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@xxxxxxxxxxx>
>
> Reviewed-by: Sukadev Bhattiprolu <sukadev@xxxxxxxxxxxxxxxxxx>
>
> | Signed-off-by: Eric W. Biederman <ebiederm@xxxxxxxxxxxx>
> | ---
> | 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
_______________________________________________
Containers mailing list
Containers@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linux-foundation.org/mailman/listinfo/containers
