Re: [PATCH] pid_ns: support pidns switching between sibling

Re: [PATCH] pid_ns: support pidns switching between sibling

[Eric W. Biederman]

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

Re: [External] Re: [PATCH] pid_ns: support pidns switching between sibling

[yunhui cui]

Hi Eric,

On Thu, Oct 12, 2023 at 11:31 AM Eric W. Biederman
<ebiederm@xmission.com> wrote:
>
> 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?
Yeah, you are right .

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

I think I need to describe in detail our needs and problems we face.
What we need to do is fork a container into a new container, which
means that all
processes of the original container need to be forked out and added to
the new container.
Then the forked process needs to be added to the namespace and cgroup
of the new container.

What we are talking about here is the pid namespace.

for example:
Assume that there are three processes A, B, and C in the original container.
What we need to do is A fork A_new, B fork B_new, C fork C_new.

However, in the existing pidns implementation, the parent process
first joins pidns, and then
the forked child process will get the new pidns (the pid of the child
process is what we expected),
and the parent process's own pidns has not actually changed (at least
pid is still existing).

To make A_new, B_new, and C_new inherently in the pidns of the new container,
A, B, and C must first switch to the pidns of the new container, right?
From my understanding there is no better way to implement it.

But the existing implementation (the part to be changed in this patch)
is blocking our progress.

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

In the scenario I described, "switch back" would certainly work.

dst_pidns = open("/proc/%d/ns/pid");
src_pidns = open("/proc/self/ns/pid");

setns(dst_pidns, CLONE_NEWPID);
if(!fork()) {
/* Child */
/* The child process is born in the pidns of the new container. */
...
}
/* switch back */
setns(src_pidns, CLONE_NEWPID);

>
> 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);

Sorry, I don't understand what you mean here.

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

I think we'd better have a good understanding of the problems we face first,
and then think of a more comprehensive way to solve it.
Although the modification of this patch is not perfect, do we have a better way?


Thanks,
Yunhui

Re: [External] Re: [PATCH] pid_ns: support pidns switching between sibling

[yunhui cui]

Hi Eric,

On Fri, Oct 13, 2023 at 10:44 AM yunhui cui <cuiyunhui@bytedance.com> wrote:
>
> Hi Eric,
>
> On Thu, Oct 12, 2023 at 11:31 AM Eric W. Biederman
> <ebiederm@xmission.com> wrote:
> >
> > 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?
> Yeah, you are right .
>
> >
> > > 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.
>
> I think I need to describe in detail our needs and problems we face.
> What we need to do is fork a container into a new container, which
> means that all
> processes of the original container need to be forked out and added to
> the new container.
> Then the forked process needs to be added to the namespace and cgroup
> of the new container.
>
> What we are talking about here is the pid namespace.
>
> for example:
> Assume that there are three processes A, B, and C in the original container.
> What we need to do is A fork A_new, B fork B_new, C fork C_new.
>
> However, in the existing pidns implementation, the parent process
> first joins pidns, and then
> the forked child process will get the new pidns (the pid of the child
> process is what we expected),
> and the parent process's own pidns has not actually changed (at least
> pid is still existing).
>
> To make A_new, B_new, and C_new inherently in the pidns of the new container,
> A, B, and C must first switch to the pidns of the new container, right?
> From my understanding there is no better way to implement it.
>
> But the existing implementation (the part to be changed in this patch)
> is blocking our progress.
>
> >
> > > Examples of what we expected:
> > >
> > > /* switch to target ns first. */
> > > setns(target_ns, CLONE_NEWPID);
> >   ^-------- Is this the line that fails for you?

Yes, it failed here.

Thanks,
Yunhui

Re: [External] Re: [PATCH] pid_ns: support pidns switching between sibling

[Eric W. Biederman]

yunhui cui <cuiyunhui@bytedance.com> writes:

> Hi Eric,
>
> On Thu, Oct 12, 2023 at 11:31 AM Eric W. Biederman
> <ebiederm@xmission.com> wrote:
>>
>> 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);
>
> Sorry, I don't understand what you mean here.

What I mean is that if your simple patch was adopted,
then the classic way of controlling a fork would fail.

	pid = fork()
        ^--------------- Would return 0 for both parent and child
        ^--------------- Look at pid_nr_ns to understand.
        if (!pid() {
		/* child */
		do_something();
		_exit(0);
	}
	waitpid(pid);

For your use case there are more serious problems as well.  The entire
process hierarchy built would be incorrect.   Which means children
signaling parents when they exit would be incorrect, and that parents
would not be able to wait on their children.

I do understand the desire to want to cow the memory space of all of the
processes.  That can potentially save a lot of resources.

In other checkpoint/restart scenarios people have been using userfaultfd
to get a similar benefit.

I suggest you look at the CRIU project.

Eric

Re: [External] Re: [PATCH] pid_ns: support pidns switching between sibling

[yunhui cui]

Hi Eric，

On Fri, Oct 13, 2023 at 9:04 PM Eric W. Biederman <ebiederm@xmission.com> wrote:
>
> yunhui cui <cuiyunhui@bytedance.com> writes:
>
> > Hi Eric,
> >
> > On Thu, Oct 12, 2023 at 11:31 AM Eric W. Biederman
> > <ebiederm@xmission.com> wrote:
> >>
> >> 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);
> >
> > Sorry, I don't understand what you mean here.
>
> What I mean is that if your simple patch was adopted,
> then the classic way of controlling a fork would fail.
>
>         pid = fork()
>         ^--------------- Would return 0 for both parent and child
>         ^--------------- Look at pid_nr_ns to understand.
>         if (!pid() {
>                 /* child */
>                 do_something();
>                 _exit(0);
>         }
>         waitpid(pid);

okay, The reason here is that pid_nr_ns has no pid in the current
pidns of the child process, and returns 0.
Can this also support sibling traversal? If so, it means that the
process also has a pid in its sibling's pidns.


>
> For your use case there are more serious problems as well.  The entire
> process hierarchy built would be incorrect.   Which means children
> signaling parents when they exit would be incorrect, and that parents
> would not be able to wait on their children.

Therefore, support for slibing pidns must be added to the entire logic of pidns.
Do you have any plans to support this, or what are the good reasons
for not supporting it?

Thanks,
Yunhui

Re: [External] Re: [PATCH] pid_ns: support pidns switching between sibling

[Eric W. Biederman]

yunhui cui <cuiyunhui@bytedance.com> writes:

> Hi Eric，
>
> On Fri, Oct 13, 2023 at 9:04 PM Eric W. Biederman <ebiederm@xmission.com> wrote:
>>
>> yunhui cui <cuiyunhui@bytedance.com> writes:
>>
>> > Hi Eric,
>> >
>> > On Thu, Oct 12, 2023 at 11:31 AM Eric W. Biederman
>> > <ebiederm@xmission.com> wrote:
>> >>
>> >> 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);
>> >
>> > Sorry, I don't understand what you mean here.
>>
>> What I mean is that if your simple patch was adopted,
>> then the classic way of controlling a fork would fail.
>>
>>         pid = fork()
>>         ^--------------- Would return 0 for both parent and child
>>         ^--------------- Look at pid_nr_ns to understand.
>>         if (!pid() {
>>                 /* child */
>>                 do_something();
>>                 _exit(0);
>>         }
>>         waitpid(pid);
>
> okay, The reason here is that pid_nr_ns has no pid in the current
> pidns of the child process, and returns 0.
> Can this also support sibling traversal?

Not without a complete redesign.

> If so, it means that the process also has a pid in its sibling's pidns.



>> For your use case there are more serious problems as well.  The entire
>> process hierarchy built would be incorrect.   Which means children
>> signaling parents when they exit would be incorrect, and that parents
>> would not be able to wait on their children.
>
> Therefore, support for slibing pidns must be added to the entire logic of pidns.
> Do you have any plans to support this,

No plans to support it.

> or what are the good reasons for not supporting it?

I see no point, it is a lot of work, and your container acceleration
still won't work.

By forking from your original processes instead of properly building
the process hierarchy.  If a pair of your original processes are doing:

	pid = fork()
	if (!pid() {
	        /* child */
                <-------------------------- clone created here
	        do_something();
	        _exit(0);
	}
        <---------------------------------- clone created here
	waitpid(pid);


Their clones won't work.  Not because the pids aren't the same, but
because the clones are not parent and child.  Which causes waitpid
not to see the other process.



I believe you want to do this sibling pid_ns fork so that you can
have copy-on-write of the anonymous pages of the original process.
Which is a completely reasonable thing to want.




For performing copy-on-write between machines we have userfaultfd.

For simply reading the pages we have process_vm_readv.

I think what you want is essentially process_vm_cow_map.  Unfortunately
no one has built that yet.

Maybe memfd is a better model to start from?  Something where you pause
process a, setup the cow in process a, and place the pages in process
b.  With the final result that either process a or process b writing
to the page will cause the copy on write to happen the and page to be
unshared.

I really think you need something that will decouple the copy-on-write
mechanism of fork from the rest of fork, so you can build a proper
process hierarchy.

Eric