Re: [PATCH v2 1/2] pid_namespace: allow opening pid_for_children before init was created

[Andrei Vagin <avagin@google.com>]

On Mon, Feb 23, 2026 at 3:03 PM Pavel Tikhomirov
<ptikhomirov@virtuozzo.com> wrote:
>
> This effectively gives us an ability to create the pid namespace init as
> a child of the process (setns-ed to the pid namespace) different to the
> process which created the pid namespace itself.
>
> Original problem:
>
> There is a cool set_tid feature in clone3() syscall, it allows you to
> create process with desired pids on multiple pid namespace levels. Which
> is useful to restore processes in CRIU for nested pid namespace case.
>
> In nested container case we can potentially see this kind of pid/user
> namespace tree:
>
>                             Process
>                           ┌─────────┐
>     User NS0 ──▶ Pid NS0 ──▶ Pid p0 │
>         │           │     │         │
>         ▼           ▼     │         │
>     User NS1 ──▶ Pid NS1 ──▶ Pid p1 │
>         │           │     │         │
>        ...         ...    │   ...   │
>         │           │     │         │
>         ▼           ▼     │         │
>     User NSn ──▶ Pid NSn ──▶ Pid pn │
>                           └─────────┘
>
> So to create the "Process" and set pids {p0, p1, ... pn} for it on all
> pid namespace levels we can use clone3() syscall set_tid feature, BUT
> the syscall does not allow you to set pid on pid namespace levels you
> don't have permission to. So basically you have to be in "User NS0" when
> creating the "Process" to actually be able to set pids on all levels.
>
> It is ok for almost any process, but with pid namespace init this does
> not work, as currently we can only create pid namespace init and the pid
> namespace itself simultaneously, so to make "Pid NSn" owned by "User
> NSn" we have to be in the "User NSn".
>
> We can't possibly be in "User NS0" and "User NSn" at the same time,
> hence the problem.
>
> Alternative solution:
>
> Yes, for the case of pid namespace init we can use old and gold
> /proc/sys/kernel/ns_last_pid interface on the levels lower than n. But
> it is much more complicated and introduces tons of extra code to do. It
> would be nice to make clone3() set_tid interface also aplicable to this
> corner case.
>
> Implementation:
>
> Now when anyone can setns to the pid namespace before the creation of
> init, and thus multiple processes can fork children to the pid
> namespace, we enforce that the first process created is always the init,
> and only allow other processes after the init sets
> pid_namespace->child_reaper.
>
> To avoid possible problems related to cpu/compiler optimizations around
> ->child_reaper, let's use WRITE_ONCE (additional to task_list lock)
> everywhere we write it and use READ_ONCE everywhere we read it without
> explicit lock. Note: we already had READ_ONCE in nsfs_fh_to_dentry().
>
> Signed-off-by: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>
>
> --
> v2: Use *_ONCE for ->child_reaper accesses atomicity, and avoid taking
> task_list lock for reading it. Rebase to master, and thus remove
> now excess pidns_ready variable.
>
> Note: I didn't find anything in copy_process() around setting the
> ->child_reaper which can influence the pid namespace, so it looks like
> the pid namespace is fully setup at the point when init sets
> ->child_reaper to receive more processes. Thus tasklist lock looks
> excess in pidns_for_children_get()'s ->child_reaper check and it should
> be safe not to have it in the corresponding checks in alloc_pid().
> ---
>  kernel/exit.c          | 2 +-
>  kernel/fork.c          | 2 +-
>  kernel/pid.c           | 5 +++--
>  kernel/pid_namespace.c | 9 ---------
>  4 files changed, 5 insertions(+), 13 deletions(-)
>
> diff --git a/kernel/exit.c b/kernel/exit.c
> index 8a87021211ae..567fc3b7b0f9 100644
> --- a/kernel/exit.c
> +++ b/kernel/exit.c
> @@ -608,7 +608,7 @@ static struct task_struct *find_child_reaper(struct task_struct *father,
>
>         reaper = find_alive_thread(father);
>         if (reaper) {
> -               pid_ns->child_reaper = reaper;
> +               WRITE_ONCE(pid_ns->child_reaper, reaper);
>                 return reaper;
>         }
>
> diff --git a/kernel/fork.c b/kernel/fork.c
> index e832da9d15a4..27d0cdbca67e 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -2423,7 +2423,7 @@ __latent_entropy struct task_struct *copy_process(
>                         init_task_pid(p, PIDTYPE_SID, task_session(current));
>
>                         if (is_child_reaper(pid)) {
> -                               ns_of_pid(pid)->child_reaper = p;
> +                               WRITE_ONCE(ns_of_pid(pid)->child_reaper, p);
>                                 p->signal->flags |= SIGNAL_UNKILLABLE;
>                         }
>                         p->signal->shared_pending.signal = delayed.signal;
> diff --git a/kernel/pid.c b/kernel/pid.c
> index 3b96571d0fe6..e6116e131d8d 100644
> --- a/kernel/pid.c
> +++ b/kernel/pid.c
> @@ -219,7 +219,7 @@ struct pid *alloc_pid(struct pid_namespace *ns, pid_t *arg_set_tid,
>                          * Also fail if a PID != 1 is requested and
>                          * no PID 1 exists.
>                          */
> -                       if (tid != 1 && !tmp->child_reaper)
> +                       if (tid != 1 && !READ_ONCE(tmp->child_reaper))
>                                 goto out_abort;
>                         retval = -EPERM;
>                         if (!checkpoint_restore_ns_capable(tmp->user_ns))
> @@ -247,8 +247,9 @@ struct pid *alloc_pid(struct pid_namespace *ns, pid_t *arg_set_tid,
>                          * alreay in use. Return EEXIST in that case.
>                          */
>                         if (nr == -ENOSPC)
> -
>                                 nr = -EEXIST;
> +               } else if (!READ_ONCE(tmp->child_reaper) && idr_get_cursor(&tmp->idr) != 0) {

I think it is better to update pid_ns_ctl_handler to prevent setting
ns_last_pid in a pidns
without init. Otherwise, figuring out why fork returns EINVAL can be tricky.

> +                       nr = -EINVAL;
>                 } else {
>                         int pid_min = 1;
>                         /*