idmapped: use pidfd to avoid pid reuse issue#9218
Merged
dmcgowan merged 1 commit intocontainerd:mainfrom Oct 20, 2023
Merged
Conversation
|
Skipping CI for Draft Pull Request. |
2ca300e to
2abd31c
Compare
2abd31c to
f82c751
Compare
It's followup for containerd#5890. The containerd-shim process depends on the mount package to init rootfs for container. For the container enable user namespace, the mount package needs to fork child process to get the brand-new user namespace. However, there are two reapers in one process (described by the following list) and there are race-condition cases. 1. mount package 2. sys.Reaper as global one which watch all the SIGCHLD. === [kill(2)][kill] the wrong process === Currently, we use pipe to ensure that child process is alive. However, the pide file descriptor can be hold by other process, which the child process cannot exit by self. We should use [kill(2)][kill] to ensure the child process. But we might kill the wrong process if the child process might be reaped by containerd-shim and the PID might be reused by other process. === [waitid(2)][waitid] on the wrong child process === ``` containerd-shim process: Goroutine 1(GetUsernsFD): Goroutine 2(Reaper) 1. Ready to wait for child process X 2. Received SIGCHLD from X 3. Reaped the zombie child process X (X has been reused by other child process) 4. Wait on process X The goroutine 1 will be stuck until the process X has been terminated. ``` === open `/proc/X/ns/user` on the wrong child process === There is also pid-reused risk between opening `/proc/$pid/ns/user` and writing `/proc/$pid/u[g]id_map`. ``` containerd-shim process: Goroutine 1(GetUsernsFD): Goroutine 2(Reaper) 1. Fork child process X 2. Write /proc/X/uid_map,gid_map 3. Received SIGCHLD from X 4. Reaped the zombie child process X (X has been reused by other process) 5. Open /proc/X/ns/user file as usernsFD The usernsFD links to the wrong X!!! ``` In order to fix the race-condition, we should use [CLONE_PIDFD][clone2] (Since Linux v5.2). When we fork child process `X`, the kernel will return a process file descriptor `X_PIDFD` referencing to child process `X`. With the pidfd, we can use [pidfd_send_signal(2)][pidfd_send_signal] (Since Linux v5.1) to send signal(0) to ensure the child process `X` is alive. If the `X` has terminated and its PID has been recycled for another process. The pidfd_send_signal fails with the error ESRCH. Therefore, we can open `/proc/X/{ns/user,uid_map,gid_map}` file descriptors as first and then use pidfd_send_signal to check the process is still alive. If so, we can ensure the file descriptors are valid and reference to the child process `X`. Even if the `X` PID has been reused after pidfd_send_signal call, the file descriptors are still valid. ```code X, pidfd = clone2(CLONE_PIDFD) usernsFD = open /proc/X/ns/user uidmapFD = open /proc/X/uid_map gidmapFD = open /proc/X/gid_map pidfd_send_signal pidfd, signal(0) return err if no such process == When we arrive here, we can ensure usernsFD/uidmapFD/gidmapFD are correct == even if X has been reused after pidfd_send_signal call. update uid/gid mapping by uidmapFD/gidmapFD return usernsFD ``` And the [waitid(2)][waitid] also supports pidfd type (Since Linux 5.4). We can use pidfd type waitid to ensure we are waiting for the correct process. All the PID related race-condition issues can be resolved by pidfd. ```bash ➜ mount git:(followup-idmapped) pwd /home/fuwei/go/src/github.com/containerd/containerd/mount ➜ mount git:(followup-idmapped) sudo go test -test.root -run TestGetUsernsFD -count=1000 -failfast -p 100 ./... PASS ok github.com/containerd/containerd/mount 3.446s ``` [kill]: <https://man7.org/linux/man-pages/man2/kill.2.html> [clone2]: <https://man7.org/linux/man-pages/man2/clone.2.html> [pidfd_send_signal]: <https://man7.org/linux/man-pages/man2/pidfd_send_signal.2.html> [waitid]: <https://man7.org/linux/man-pages/man2/waitid.2.html> Signed-off-by: Wei Fu <[email protected]>
f82c751 to
3742f7f
Compare
fuweid
commented
Oct 12, 2023
| goto err | ||
| } | ||
|
|
||
| _, _, err = syscall.RawSyscall(syscall.SYS_PPOLL, 0, 0, 0) |
Member
Author
There was a problem hiding this comment.
I didn't see sys_pause syscall in riscv64 so I use ppoll on empty fd to make the thread into infinite sleep status
mikebrow
reviewed
Oct 13, 2023
| } | ||
|
|
||
| syscall.Close(pipeMap[0]) | ||
| pidFD := os.NewFile(pidfd, "pidfd") |
Member
There was a problem hiding this comment.
should probably check for pidFD == nil before continuing
Member
Author
There was a problem hiding this comment.
Hi @mikebrow ! Thanks for the comment.
The kernel returns the pidfd when we enable CLONE_PIDFD option. Basically, it won't be zero.
If the kernel doesn't support CLONE_PIDFD, the ForkUserns will return error.
So, I think we don't need to check pidFD == nil here.
Member
Author
|
Ping @AkihiroSuda @dmcgowan ~ |
dmcgowan
approved these changes
Oct 20, 2023
mxpv
approved these changes
Oct 20, 2023
kiashok
added a commit
to kiashok/containerd
that referenced
this pull request
Oct 23, 2024
Update fork-external main with upstream main @ 452ec25 Related work items: containerd#5890, containerd#7647, containerd#9218, containerd#9233, containerd#9258, containerd#9270, containerd#9274, containerd#9279, containerd#9283, containerd#9286, containerd#9289, containerd#9290, containerd#9294, containerd#9295, containerd#9297, containerd#9305, containerd#9306, containerd#9308, containerd#9316, containerd#9317, containerd#9319, containerd#9320, containerd#9321
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
It's followup for #5890.
1. Race-condition
The containerd-shim process depends on the mount package to init rootfs
for container. For the container enable user namespace, the mount
package needs to fork child process to get the brand-new user namespace.
However, there are two reapers in one process (described by the
following list) and there are race-condition cases.
1.1 kill(2) the wrong process
Currently, we use pipe to ensure that child process is alive. However,
the pide file descriptor can be hold by other process, which the child
process cannot exit by self. We should use kill(2) to ensure the
child process exit. But we might kill the wrong process if the child process
might be reaped by containerd-shim and the PID might be reused by other
process.
1.2 waitid(2) on the wrong child process
1.3 open
/proc/X/ns/useron the wrong child processThere is also pid-reused risk between opening
/proc/$pid/ns/userandwriting
/proc/$pid/u[g]id_map.2. Solution
In order to fix the race-condition, we should use CLONE_PIDFD (Since
Linux v5.2).
When we fork child process
X, the kernel will return a process filedescriptor
X_PIDFDreferencing to child processX. With the pidfd, we canuse pidfd_send_signal(2) (Since Linux v5.1)
to send signal(0) to ensure the child process
Xis alive. If theXhasterminated and its PID has been recycled for another process. The
pidfd_send_signal fails with the error ESRCH.
Therefore, we can open
/proc/X/{ns/user,uid_map,gid_map}filedescriptors as first and then use pidfd_send_signal to check the process
is still alive. If so, we can ensure the file descriptors are valid and
reference to the child process
X. Even if theXPID has been reusedafter pidfd_send_signal call, the file descriptors are still valid.
And the waitid(2) also supports pidfd type (Since Linux 5.4).
We can use pidfd type waitid to ensure we are waiting for the correct
process. All the PID related race-condition issues can be resolved by
pidfd.
Signed-off-by: Wei Fu [email protected]