Fix clone(2) with double fork

[yihuaf]

Fix #185

This PR replaces clone(2) with double fork to create the container process. Double fork was the original implementation, and we wisely thought that we could improve it by using a single clone call. Turns out jokes on us that we can't use a single clone call. With clone, if we enter into a new pid namespace, we are OK. However, if we need to set_ns into an existing pid namespace, we are out of luck. So the right path forward needs to be double fork. For a more detailed discussion, see #185 why it doesn't work.

Also, turns out, this is a bigger change than I intended. So I apologize up front for the big change.

[utam0k]

@yihuaf
Could you give me an example command that I can use to check that this has been fixed correctly?

[utam0k]

Maybe, but if we don't use rootless, do we need to do double-fork?

[yihuaf]

@yihuaf
Could you give me an example command that I can use to check that this has been fixed correctly?

Try

docker run -it --rm --runtime youki --name youki busybox

and then

docker exec -it youki sh

In the shell using docker exec, check /proc/self and /proc/2 are set up correctly. Before, because exec didn't enter the correct pid namespace, /proc/<pid> was not configured correctly. In the main shell by docker run, you should also be able to see /proc/2 for the docker exec shell.
Pid 1 -> docker run shell
Pid 2 -> docker exec shell

[yihuaf]

Maybe, but if we don't use rootless, do we need to do double-fork?

There are 2 cases where we don't need to double fork.

1. If we don't use rootless and/or don't enter into an existing user namespace, we can enter into Pid namespace on the Youki main process and then fork the init process.
2. If we don't need to enter an existing Pid namespace through set_ns, we can use clone to enter user and pid namespace together and create the init process directly.

[utam0k]

@yihuaf
Could you give me an example command that I can use to check that this has been fixed correctly?

Try

docker run -it --rm --runtime youki --name youki busybox

and then

docker exec -it youki sh

In the shell using docker exec, check /proc/self and /proc/2 are set up correctly. Before, because exec didn't enter the correct pid namespace, /proc/<pid> was not configured correctly. In the main shell by docker run, you should also be able to see /proc/2 for the docker exec shell.
Pid 1 -> docker run shell
Pid 2 -> docker exec shell

I was able to confirm the change. Thank you very much.

main

$ docker exec -it youki sh
/ # ps
PID   USER     TIME  COMMAND
    1 root      0:00 sh
    4 root      0:00 ps

this pr

$ docker exec -it youki sh
/ # ps
PID   USER     TIME  COMMAND
    1 root      0:00 sh
    2 root      0:00 sh
    3 root      0:00 ps
/ #

[utam0k]

There are 2 cases where we don't need to double fork.

1. If we don't use rootless and/or don't enter into an existing user namespace, we can enter into Pid namespace on the Youki main process and then fork the init process.
2. If we don't need to enter an existing Pid namespace through set_ns, we can use clone to enter user and pid namespace together and create the init process directly.

Does this mean that if uid_mapping and gid_mapping are not needed, double-fork is not needed?
I thought it might be a good idea to improve the case where double-fork is not needed. However, once this PR is done, it's good.

[utam0k]

I was hoping these PRs would pass with this PR fix, but it looks like not.
https://github.com/containers/youki/blob/9acc899239ee0b9d58ea1a81b69884d13ba561fe/integration_test.sh#L37-L40

[utam0k]

I will not point out any details in this PR. We will not point out details in this PR, because this PR is very important and will be very hard to merge because of the large number of changes. Those corrections will be made in the PR little by little. I am looking forward to your reviews.
If you respond to some of my comments, I'll try to merge them.
Finally, it would be helpful if you could leave a brief summary of this issue on issue or on this PR.

[yihuaf]

I was hoping these PRs would pass with this PR fix, but it looks like not.
https://github.com/containers/youki/blob/9acc899239ee0b9d58ea1a81b69884d13ba561fe/integration_test.sh#L37-L40

I will take a look at these tests right after this. #221 filed.

[yihuaf]

There are 2 cases where we don't need to double fork.

1. If we don't use rootless and/or don't enter into an existing user namespace, we can enter into Pid namespace on the Youki main process and then fork the init process.
2. If we don't need to enter an existing Pid namespace through set_ns, we can use clone to enter user and pid namespace together and create the init process directly.

Does this mean that if uid_mapping and gid_mapping are not needed, double-fork is not needed?
I thought it might be a good idea to improve the case where double-fork is not needed. However, once this PR is done, it's good.

Actually no. Without uid/gid mapping, we would still require double fork unless we are in one of two cases mentioned above. I think we can potentially improve this by differentiating which case to use, but it would take some effort to get all the corner cases right. The double fork implementation would cover all the cases and should be good enough for the moment.

[yihuaf]

I will not point out any details in this PR. We will not point out details in this PR, because this PR is very important and will be very hard to merge because of the large number of changes. Those corrections will be made in the PR little by little. I am looking forward to your reviews.
If you respond to some of my comments, I'll try to merge them.
Finally, it would be helpful if you could leave a brief summary of this issue on issue or on this PR.

That's probably a good idea, and I am happy to continue to fix all the details. I will summarize the issue here again, and #185 contains a history of how we got here.

Before this PR, the implementation was to use clone to create the container init process, joining new user and new pid namespaces together. However, in the case of docker exec, the container process will join existing namespaces user and pid namespaces. The set_ns to pid namespaces requires an extra fork to enter into the correct pid namespace. As a result, the container process didn't join the right pid namespaces. Many of the pid related operations were failing, such as looking at /proc/self and /proc/<pid>. The solution in this PR is to always double fork, one to enter user namespace and another fork to enter into pid namespace. We can't enter user and pid namespace together because entering into pid namespace requires CAP_SYSADMIN so we have to enter into user namespace first.

[utam0k]

Superb!