Re: [PATCH v3 2/4] pid: check init is created first after idr alloc

[Andrei Vagin <avagin@gmail.com>]

On Tue, Feb 24, 2026 at 9:02 AM Pavel Tikhomirov
<ptikhomirov@virtuozzo.com> wrote:
>
> This moves the condition (tid != 1 && !tmp->child_reaper) to after idr
> alloc, so it not only covers that first process in pid namespace has pid
> 1 in case of clone3(set_tid) requesting wrong pid, but also if idr
> itself gives wrong pid for some reason.
>
> This could've been the case before this patch, when creating first
> process the alloc_pid()->pidfs_add_pid() code path fails, so that the
> idr->idr_next is non zero anymore and next process calling to
> alloc_pid(), will get 2 as a pid from idr_alloc_cyclic(). Effectively
> leading to init-less pid namespace, which is a bug.

I don't think this is an issue right now, as we only allow one attempt
to create the init. If that first attempt fails, free_pid will disable
further PID allocations in the target namespace.

It is a sort of off-topic, because we will definitely need this logic
with the next patch in this series.

>
> Note: This is also a preparation for the next patch in the series, which
> will introduce an ability of creating init from the task different to
> the task which had created the pid namespace. Needed to make sure that
> init is always first, even in this new case.
>
> Suggested-by: Oleg Nesterov <oleg@redhat.com>
> Signed-off-by: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>
> --
> v3: Split from main commit. Merge two checks of ->child_reaper into one.
> ---
>  kernel/pid.c | 17 ++++++++++-------
>  1 file changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/kernel/pid.c b/kernel/pid.c
> index 76c2744493e2..ebf013f35cb3 100644
> --- a/kernel/pid.c
> +++ b/kernel/pid.c
> @@ -215,12 +215,6 @@ struct pid *alloc_pid(struct pid_namespace *ns, pid_t *arg_set_tid,
>                         retval = -EINVAL;
>                         if (tid < 1 || tid >= pid_max[ns->level - i])
>                                 goto out_abort;
> -                       /*
> -                        * Also fail if a PID != 1 is requested and
> -                        * no PID 1 exists.
> -                        */
> -                       if (tid != 1 && !READ_ONCE(tmp->child_reaper))
> -                               goto out_abort;
>                         retval = -EPERM;
>                         if (!checkpoint_restore_ns_capable(tmp->user_ns))
>                                 goto out_abort;
> @@ -296,9 +290,18 @@ struct pid *alloc_pid(struct pid_namespace *ns, pid_t *arg_set_tid,
>
>                 pid->numbers[i].nr = nr;
>                 pid->numbers[i].ns = tmp;
> -               tmp = tmp->parent;
>                 i--;
>                 retried_preload = false;
> +
> +               /*
> +                * PID 1 (init) must be created first.
> +                */
> +               if (!READ_ONCE(tmp->child_reaper) && nr != 1) {
> +                       retval = -EINVAL;

According to the comment below, we should probably return ENOMEM if this
is a subsequent attempt to create the init process. In the next patch,
we may want to return EAGAIN if there is a concurrent attempt to create
the first process.

This isn't directly related to this patch, but why do we check
"ns->pid_allocated & PIDNS_ADDING" after allocating all pids?  Wouldn't
it be more reasonable to do that right after taking the pidmap_lock?

> +                       goto out_free;
> +               }
> +
> +               tmp = tmp->parent;
>         }
>
>         /*
> --
> 2.53.0
>
>