Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:8 as permitted sender) client-ip=2620:137:e000::3:8;
From:   "Eric W. Biederman" <ebiederm@xmission.com>
To:     Yunhui Cui <cuiyunhui@bytedance.com>
Cc:     akpm@linux-foundation.org, keescook@chromium.org,
        brauner@kernel.org, jeffxu@google.com, frederic@kernel.org,
        mcgrof@kernel.org, cyphar@cyphar.com, rongtao@cestc.cn,
        linux-kernel@vger.kernel.org,
        Linux Containers <containers@lists.linux.dev>
References: <20231011065446.53034-1-cuiyunhui@bytedance.com>
Date:   Wed, 11 Oct 2023 22:30:24 -0500
In-Reply-To: <20231011065446.53034-1-cuiyunhui@bytedance.com> (Yunhui Cui's
        message of "Wed, 11 Oct 2023 14:54:46 +0800")
Message-ID: <87sf6gcyb3.fsf@email.froward.int.ebiederm.org>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Subject: Re: [PATCH] pid_ns: support pidns switching between sibling
Precedence: bulk

Yunhui Cui <cuiyunhui@bytedance.com> writes:

> In the scenario of container acceleration,

What is container acceleration?

Are you perhaps performing what is essentially checkpoint/restart
from one set of processes to a new set of processes so you can
get a container starting faster?

> when a target pstree is cloned from a temp pstree, we hope that the
> cloned process is inherently in the target's pid namespace.

I am having a hard time figuring out what you are saying here.

> Examples of what we expected:
>
> /* switch to target ns first. */
> setns(target_ns, CLONE_NEWPID);
  ^-------- Is this the line that fails for you?

> if(!fork()) {
> /* Child */
> ...
> }
> /* switch back */
> setns(temp_ns, CLONE_NEWPID);

Assuming that the "switch back" means returning to your
task_active_pid_ns that should always work.

If I had to guess I think what you are missing is that entire pid
namespaces can be inside other pid namespaces.

So there is no reason to believe that any random pid namespace
that happens to pass the CAP_SYS_ADMIN permission check is also in
your processes task_active_pid_ns.


> However, it is limited by the existing implementation, CAP_SYS_ADMIN
> has been checked in pidns_install(), so remove the limitation that only
> by traversing parent can switch pidns.

The check you are deleting is what verifies the pid namespaces you are
attempting to change pid_ns_for_children to is a member of the tasks
current pid namespace (aka task_active_pid_ns).


There is a perfectly good comment describing why what you are attempting
to do is unsupportable.

	/*
	 * Only allow entering the current active pid namespace
	 * or a child of the current active pid namespace.
	 *
	 * This is required for fork to return a usable pid value and
	 * this maintains the property that processes and their
	 * children can not escape their current pid namespace.
	 */


If you pick a pid namespace that does not meet the restrictions you are
removing the pid of the new child can not be mapped into the pid
namespace of the parent that called setns.

AKA the following code can not work.

pid = fork();
if (!pid) {
	/* child */
        do_something();
        _exit(0);
}
waitpid(pid);


So no.  The suggested change to pidns_install makes no sense at all.

The whole not being able to escape your current pid namespace is
also an important invariant when reasoning about pid namespaces.

It would have to be a strong well thought out case for me to agree
it makes sense to abandon the invariant that a process can not escape
it's pid namespace.


> Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
> ---
>  kernel/pid_namespace.c | 8 +-------
>  1 file changed, 1 insertion(+), 7 deletions(-)
>
> diff --git a/kernel/pid_namespace.c b/kernel/pid_namespace.c
> index 3028b2218aa4..774db1f268f1 100644
> --- a/kernel/pid_namespace.c
> +++ b/kernel/pid_namespace.c
> @@ -389,7 +389,7 @@ static int pidns_install(struct nsset *nsset, struct ns_common *ns)
>  {
>  	struct nsproxy *nsproxy = nsset->nsproxy;
>  	struct pid_namespace *active = task_active_pid_ns(current);
> -	struct pid_namespace *ancestor, *new = to_pid_ns(ns);
> +	struct pid_namespace *new = to_pid_ns(ns);
>  
>  	if (!ns_capable(new->user_ns, CAP_SYS_ADMIN) ||
>  	    !ns_capable(nsset->cred->user_ns, CAP_SYS_ADMIN))
> @@ -406,12 +406,6 @@ static int pidns_install(struct nsset *nsset, struct ns_common *ns)
>  	if (new->level < active->level)
>  		return -EINVAL;
>  
> -	ancestor = new;
> -	while (ancestor->level > active->level)
> -		ancestor = ancestor->parent;
> -	if (ancestor != active)
> -		return -EINVAL;
> -
>  	put_pid_ns(nsproxy->pid_ns_for_children);
>  	nsproxy->pid_ns_for_children = get_pid_ns(new);
>  	return 0;


Eric