Skip to content

Conversation

@fuweid
Copy link
Member

@fuweid fuweid commented Oct 11, 2023

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. mount package
  2. sys.Reaper as global one which watch all the SIGCHLD.

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

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.

1.3 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!!!

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 file
descriptor X_PIDFD referencing to child process X. With the pidfd, we can
use pidfd_send_signal(2) (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.

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

➜  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

Signed-off-by: Wei Fu [email protected]

@k8s-ci-robot
Copy link

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@fuweid fuweid force-pushed the followup-idmapped branch 3 times, most recently from 2ca300e to 2abd31c Compare October 12, 2023 14:19
@fuweid fuweid changed the title feat(idmapped): use pidfd to avoid pid reuse issue idmapped: use pidfd to avoid pid reuse issue Oct 12, 2023
@fuweid fuweid force-pushed the followup-idmapped branch from 2abd31c to f82c751 Compare October 12, 2023 14:29
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]>
@fuweid fuweid force-pushed the followup-idmapped branch from f82c751 to 3742f7f Compare October 12, 2023 16:59
@fuweid fuweid marked this pull request as ready for review October 12, 2023 16:59
goto err
}

_, _, err = syscall.RawSyscall(syscall.SYS_PPOLL, 0, 0, 0)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't see sys_pause syscall in riscv64 so I use ppoll on empty fd to make the thread into infinite sleep status

}

syscall.Close(pipeMap[0])
pidFD := os.NewFile(pidfd, "pidfd")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should probably check for pidFD == nil before continuing

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@fuweid
Copy link
Member Author

fuweid commented Oct 17, 2023

Ping @AkihiroSuda @dmcgowan ~

@fuweid fuweid added this to the 2.0 milestone Oct 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants