RFC: Initial support of idmapped mount points #5890

artqzn · 2021-08-17T18:10:40Z

Hi,
this is this is initial support of idmapped mount points in containerd. The original PR was published by @mauriciovasquezbernal here #4734.

Updates:

Original PR was rebased on top of latest containerd.
Idmapped mount support was reimplemented using in Go.
Small changes in error handling.
Added some comments.
Updated container start time performance measurement results (numbers are in seconds).

Image	ID mapped mount	WithRemappedSnapshot
BusyBox	00.135	04.964
Ubuntu	00.171	15.713
Fedora	00.143	38.799

In current patch set the only idmapped mount points functionality is implemented. I think that user namespace support for containerd should be integrated separately, please share your opinion, maybe I am wrong here. I don't know if anybody is working on it, if no I would like to work on user namespaces support also.

Dear reviewers, could you please check this PR.
Thank you in advance!

cc @alban @rata @mauriciovasquezbernal @AkihiroSuda

k8s-ci-robot · 2021-08-17T18:10:48Z

Hi @artqzn. Thanks for your PR.

I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

tuananh · 2021-08-18T01:18:16Z

Cant wait :)

artqzn · 2021-08-18T10:49:30Z

Previous PR's: #5075 #4734 #5888

kzys

We may have a bigger discussion first though.

kzys · 2021-08-19T17:04:48Z

mount/mount_id_mapped.go

+	)
+
+	path = fmt.Sprintf("/proc/%d/ns/user", pid)
+	if userNsFile, err = os.OpenFile(path, os.O_RDONLY, 0); err != nil {


How about using os.Open() here instead?

kzys · 2021-08-19T18:29:58Z

mount/mount_id_mapped.go

+
+	path = fmt.Sprintf("/proc/%d/ns/user", pid)
+	if userNsFile, err = os.OpenFile(path, os.O_RDONLY, 0); err != nil {
+		return errors.Wrapf(err, "Unable to get user ns file descriptor for - %s\n", path)


Don't include \n

kzys · 2021-08-19T18:30:13Z

mount/mount_id_mapped.go

+	attr.userNs = uint64(userNsFile.Fd())
+
+	defer userNsFile.Close()
+	if targetDir, err = os.OpenFile(target, os.O_RDONLY, 0); err != nil {


kzys · 2021-08-19T18:31:34Z

mount/mount_linux.go

@@ -198,11 +209,13 @@ func UnmountAll(mount string, flags int) error {

 // parseMountOptions takes fstab style mount options and parses them for
 // use with a standard mount() syscall
-func parseMountOptions(options []string) (int, string, bool) {
+func parseMountOptions(options []string) (int, string, bool, string, string) {


It may be the time to return a struct.

Agreed, it will be much better

theopenlab-ci · 2021-08-20T10:50:05Z

Build succeeded.

containerd-build-arm64 : SUCCESS in 4m 05s (non-voting)
containerd-test-arm64 : SUCCESS in 5m 27s (non-voting)
containerd-integration-test-arm64 : SUCCESS in 24m 25s (non-voting)

artqzn · 2021-08-20T11:31:25Z

/retest

k8s-ci-robot · 2021-08-20T11:31:40Z

@artqzn: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

theopenlab-ci · 2021-08-20T12:34:54Z

Build succeeded.

containerd-build-arm64 : SUCCESS in 4m 16s (non-voting)
containerd-test-arm64 : SUCCESS in 5m 28s (non-voting)
containerd-integration-test-arm64 : FAILURE in 29m 49s (non-voting)

artqzn · 2021-08-20T12:42:35Z

@kzys Thanks for comment! Fixed

theopenlab-ci · 2021-09-15T08:42:34Z

Build succeeded.

containerd-build-arm64 : SUCCESS in 4m 51s (non-voting)
containerd-test-arm64 : SUCCESS in 5m 35s (non-voting)
containerd-integration-test-arm64 : SUCCESS in 23m 47s (non-voting)

theopenlab-ci · 2021-09-15T09:15:11Z

Build succeeded.

containerd-build-arm64 : SUCCESS in 4m 46s (non-voting)
containerd-test-arm64 : SUCCESS in 5m 54s (non-voting)
containerd-integration-test-arm64 : FAILURE in 23m 34s (non-voting)

kzys · 2021-09-15T20:56:44Z

mount/mount_id_mapped.go

+// MapMount applies GID/UID shift according to gidmap/uidmap for target path
+func MapMount(uidmap string, gidmap string, target string) (err error) {
+	const (
+		userNsHelperBinary = "/bin/sh"


Could it be /bin/echo or something smaller?

Sure, we can use any standard non-blocking program. Here we just need to run some small application in user namespace to create gid/uid mappings to run mount_setattr() before /proc/{pid}/ns/user becomes unavailable.
Possibly let's use /bin/true, it has the same size as echo.

-rwxr-xr-x 1 root root 39288 Sep 5 2019 uname
-rwxr-xr-x 1 root root 39256 Sep 5 2019 echo
-rwxr-xr-x 1 root root 39256 Sep 5 2019 false
-rwxr-xr-x 1 root root 39256 Sep 5 2019 sleep
-rwxr-xr-x 1 root root 39256 Sep 5 2019 sync
-rwxr-xr-x 1 root root 39256 Sep 5 2019 true

I'll update the PR.

integration/client/container_idmapped_test.go

rata

This mostly LGTM. Most of my comments are minor and easy to fix

snapshots/overlay/overlay_test.go

rata · 2023-09-04T10:27:34Z

snapshots/overlay/overlayutils/check.go

+		}
+	}()
+
+	if err = os.Lchown(filepath.Join(td, "upper"), uidMap.HostID, gidMap.HostID); err != nil {


Hmm, lchown won't follow a symlink, right? Why do we want to change the symlink?

This will fail with new kernels (from https://lwn.net/SubscriberLink/942954/4a108b4f66074f2a/ ):

The kernel will no longer allow the changing of permissions on symbolic links.

Although I'm unsure if this means the owner/group or the read/write/exec... 🤔

Do we really need this?

I'm really not sure that there's a real case for that, but even if there isn't, I think it should be fixed separately, since main branch has this too (I didn't change it), so we're risking to break backward compatibility if there's a case for that, but it seems you're right, there might be issues with this later

I think maintainers can provide more info on this (@fuweid @AkihiroSuda @dmcgowan @kzys)

@rata let's keep it as it is. And the upper in this context(function) is unlikely to be symlink right?

Yes, let's keep it like that if it is already like that in master

integration/client/container_idmapped_linux_test.go

Previously remapping of a snapshotter has been done using recursive chown. Commit containerd@31a6449 added a support for "remap-ids" capability which allows snapshotter internals do remappings in case of idmapped mounts support to avoid recursive chown and creating a new remapped snapshot. Signed-off-by: Ilya Hanov <ilya.hanov@huawei-partners.com>

Previously the only fuse-overlayfs supports "--remap-labels" option. Since idmapped mounts were landed to Linux kernel v5.12 it becomes possible to use it with overlayfs via mount_setattr() system call. The changes are based on experimental patchset published by Mauricio Vásquez containerd#4734. Signed-off-by: Mauricio Vásquez <mauricio@kinvolk.io> Signed-off-by: Artem Kuzin <artem.kuzin@huawei.com> Signed-off-by: Ilya Hanov <ilya.hanov@huawei-partners.com>

This patch introduces idmapped mounts support for container rootfs. The idmapped mounts support was merged in Linux kernel 5.12 torvalds/linux@7d6beb7. This functionality allows to address chown overhead for containers that use user namespace. The changes are based on experimental patchset published by Mauricio Vásquez containerd#4734. Current version reiplements support of idmapped mounts using Golang. Performance measurement results: Image idmapped mount recursive chown BusyBox 00.135 04.964 Ubuntu 00.171 15.713 Fedora 00.143 38.799 Signed-off-by: Mauricio Vásquez <mauricio@kinvolk.io> Signed-off-by: Artem Kuzin <artem.kuzin@huawei.com> Signed-off-by: Alexey Perevalov <alexey.perevalov@huawei.com> Signed-off-by: Ilya Hanov <ilya.hanov@huawei-partners.com>

Signed-off-by: Ilya Hanov <ilya.hanov@huawei-partners.com>

Signed-off-by: Alexey Perevalov <alexey.perevalov@huawei.com> Signed-off-by: Ilya Hanov <ilya.hanov@huawei-partners.com>

Signed-off-by: Ilya Hanov <ilya.hanov@huawei-partners.com>

fuweid

LGTM

rata

LGTM

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 <fuweid89@gmail.com>

k8s-ci-robot added the needs-ok-to-test label Aug 17, 2021

artqzn mentioned this pull request Aug 17, 2021

RFC: Initial support of idmapped mount points #5888

Closed

artqzn force-pushed the idmapped_mounts branch from 79e548b to 3d49872 Compare August 17, 2021 18:31

artqzn force-pushed the idmapped_mounts branch 4 times, most recently from 1edbe6e to cfb1212 Compare August 18, 2021 09:48

dmcgowan added this to New in Code Review via automation Aug 18, 2021

dmcgowan moved this from New to Needs Discussion in Code Review Aug 18, 2021

containerd deleted a comment from theopenlab-ci bot Aug 19, 2021

kzys reviewed Aug 19, 2021

View reviewed changes

artqzn force-pushed the idmapped_mounts branch from cfb1212 to 74a57dc Compare August 20, 2021 10:24

artqzn force-pushed the idmapped_mounts branch from 74a57dc to 0819ce2 Compare August 20, 2021 12:00

artqzn requested a review from kzys August 20, 2021 12:44

artqzn force-pushed the idmapped_mounts branch from 0819ce2 to c0314c1 Compare September 15, 2021 08:15

artqzn force-pushed the idmapped_mounts branch from c0314c1 to 6b4dde9 Compare September 15, 2021 08:49

kzys reviewed Sep 15, 2021

View reviewed changes

fuweid added this to the 2.0 milestone Aug 29, 2023

AkihiroSuda reviewed Sep 4, 2023

View reviewed changes

integration/client/container_idmapped_test.go Outdated Show resolved Hide resolved

AkihiroSuda approved these changes Sep 4, 2023

View reviewed changes

AkihiroSuda requested review from fuweid and rata September 4, 2023 02:40

IlyaHanov force-pushed the idmapped_mounts branch from e21cfae to 19ce879 Compare September 4, 2023 07:39

rata approved these changes Sep 4, 2023

View reviewed changes

IlyaHanov added 6 commits September 5, 2023 01:23

snapshotter: implement slow path for idmapped mounts check for overlay

e49e6d6

Signed-off-by: Ilya Hanov <ilya.hanov@huawei-partners.com>

integration: add test for idmapped mounts

9d01ed1

Signed-off-by: Alexey Perevalov <alexey.perevalov@huawei.com> Signed-off-by: Ilya Hanov <ilya.hanov@huawei-partners.com>

snapshotter: implement unit tests for idmapped mounts for overlay

295bcec

Signed-off-by: Ilya Hanov <ilya.hanov@huawei-partners.com>

IlyaHanov force-pushed the idmapped_mounts branch from 19ce879 to 295bcec Compare September 4, 2023 22:23

fuweid approved these changes Sep 5, 2023

View reviewed changes

rata approved these changes Sep 5, 2023

View reviewed changes

AkihiroSuda approved these changes Sep 5, 2023

View reviewed changes

AkihiroSuda merged commit 0ee2433 into containerd:main Sep 5, 2023
45 checks passed

Code Review automation moved this from Ready For Review to Done Sep 5, 2023

AkihiroSuda mentioned this pull request Sep 5, 2023

Support idmapped mounts (kernel 5.12) #5075

Closed

pacoxu mentioned this pull request Sep 6, 2023

Minimal kernel version plan: probably 4.18/4.19+ kubernetes/kubernetes#116799

Open

22 tasks

rata mentioned this pull request Sep 7, 2023

Require opt-in for rootfs chown when idmap mounts is not supported #9068

Closed

bergwolf mentioned this pull request Oct 8, 2023

support user namespace kata-containers/kata-containers#8170

Open

fuweid mentioned this pull request Oct 12, 2023

idmapped: use pidfd to avoid pid reuse issue #9218

Merged

samuelkarp mentioned this pull request Oct 30, 2023

containerd 1.7.x support idmap mount containers. #9310

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RFC: Initial support of idmapped mount points #5890

RFC: Initial support of idmapped mount points #5890

artqzn commented Aug 17, 2021

k8s-ci-robot commented Aug 17, 2021

tuananh commented Aug 18, 2021

artqzn commented Aug 18, 2021

kzys left a comment

kzys Aug 19, 2021

artqzn Aug 20, 2021

kzys Aug 19, 2021

artqzn Aug 20, 2021

kzys Aug 19, 2021

kzys Aug 19, 2021

artqzn Aug 20, 2021

theopenlab-ci bot commented Aug 20, 2021

artqzn commented Aug 20, 2021

k8s-ci-robot commented Aug 20, 2021

theopenlab-ci bot commented Aug 20, 2021

artqzn commented Aug 20, 2021

theopenlab-ci bot commented Sep 15, 2021

theopenlab-ci bot commented Sep 15, 2021

kzys Sep 15, 2021

artqzn Sep 16, 2021

rata left a comment •

edited

rata Sep 4, 2023 •

edited

IlyaHanov Sep 4, 2023

fuweid Sep 5, 2023

rata Sep 5, 2023

fuweid left a comment

rata left a comment

RFC: Initial support of idmapped mount points #5890

RFC: Initial support of idmapped mount points #5890

Conversation

artqzn commented Aug 17, 2021

k8s-ci-robot commented Aug 17, 2021

tuananh commented Aug 18, 2021

artqzn commented Aug 18, 2021

kzys left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theopenlab-ci bot commented Aug 20, 2021

artqzn commented Aug 20, 2021

k8s-ci-robot commented Aug 20, 2021

theopenlab-ci bot commented Aug 20, 2021

artqzn commented Aug 20, 2021

theopenlab-ci bot commented Sep 15, 2021

theopenlab-ci bot commented Sep 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rata left a comment • edited

Choose a reason for hiding this comment

rata Sep 4, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fuweid left a comment

Choose a reason for hiding this comment

rata left a comment

Choose a reason for hiding this comment

rata left a comment •

edited

rata Sep 4, 2023 •

edited