Support Id mapped mounts for shared volumes #3429

AlexeyPerevalov · 2022-03-23T12:05:16Z

This pull request adds support for id map mount feature for shared volumes.
For rootfs this is already implemented in containerd/containerd#5890.

Both commit has a code in common, which should be placed on the same place in the future.

There are two possibilities to use mount_setattr syscall with fd returned by open_tree or with fd returned by fsmount syscall. This PR uses open_tree, since this approach was used in examples, as well as in crun.

As a fallback scenario, mount syscall still used for bind mount. Open_tree syscall as well as mount_setattr are quite new syscall, to use it latest kernel version is required. In this patch set, kernel version is checked, but as an alternative approach we could just check syscall existence, any suggestions are welcome.
Also this PR includes specs-go which could be moved to separate PR.

AkihiroSuda · 2022-03-23T12:22:05Z

go.mod

@@ -24,3 +25,5 @@ require (
 	golang.org/x/sys v0.0.0-20211116061358-0a5406a5449c
 	google.golang.org/protobuf v1.27.1
 )
+
+replace github.com/opencontainers/runtime-spec/specs-go => ./staging/src/github.com/opencontainers/runtime-spec/specs-go


Please open PR for runtime-spec

@AkihiroSuda opencontainers/runtime-spec#1143

AkihiroSuda · 2022-03-23T12:22:34Z

libcontainer/init_linux.go

@@ -97,6 +97,7 @@ func newContainerInit(t initType, pipe *os.File, consoleSocket *os.File, fifoFd,
 			logFd:         logFd,
 		}, nil
 	case initStandard:
+		logrus.Infof("mountFds: %v", mountFds)


Suggested change

logrus.Infof("mountFds: %v", mountFds)

logrus.Debugf("mountFds: %v", mountFds)

AkihiroSuda · 2022-03-23T12:23:39Z

go.mod

 	github.com/opencontainers/selinux v1.10.0
+	github.com/pkg/errors v0.9.1


kolyshkin · 2022-03-23T17:29:04Z

libcontainer/mount_linux.go

+	OPEN_TREE_CLONE         = 1
+	OPEN_TREE_CLOEXEC       = 02000000
+	MOVE_MOUNT_F_EMPTY_PATH = 0x00000004
+	AT_FDCWD                = -100


Should these be defined in x/sys/unix? Please open a CL against it -- they are usually more than happy to accept such stuff.

yeah, I wrote about it, in cover message, but definitely x/sys/unix is better candidate.

@kolyshkin I've seen your https://go-review.googlesource.com/c/sys/+/363444 I assume this change was made w/o code generation e.g. by sys/unix/mkall.sh ?

kolyshkin · 2022-03-23T17:29:35Z

libcontainer/mount_linux.go

+
+//move_mount(int from_dfd, const char *from_pathname, int to_dfd,
+// const char *to_pathname, unsigned int flags)
+func moveMount(from_dfd int, fromPathName string, to_dfd int, toPathName string, flags uint) (err error) {


Same here -- seems that this belongs to x/sys/unix.

kolyshkin · 2022-03-23T17:29:41Z

libcontainer/mount_linux.go

+}
+
+// int dfd, const char *filename, unsigned int flags)
+func openTree(dfd int, fileName string, flags uint) (r int, err error) {


Same here -- seems that this belongs to x/sys/unix.

kolyshkin · 2022-03-23T17:30:28Z

libcontainer/rootfs_linux.go

@@ -68,6 +74,7 @@ func prepareRootfs(pipe io.ReadWriter, iConfig *initConfig, mountFds []int) (err
 		cgroupns:        config.Namespaces.Contains(configs.NEWCGROUP),
 	}
 	setupDev := needsSetupDev(config)
+	//time.Sleep(30 * time.Second)


Debug session leftover?

yes, initial version, after moving helpers to x/sys/unix and runtime-spec, it would be clear.

kolyshkin · 2022-03-23T17:30:41Z

libcontainer/rootfs_linux.go

@@ -380,6 +387,9 @@ func mountToRootfs(m *configs.Mount, c *mountConfig) error {
 		return err
 	}

+	if mountFd != nil {
+		logrus.Infof("mountFd: %d", *mountFd)


either logrus.Debugf or remove it.

kolyshkin · 2022-03-23T17:31:21Z

libcontainer/rootfs_linux.go

+		logrus.Infof("m.Data: %v\n", m.Data)
+		logrus.Infof("flags: %v\n", flags)


No need for \n here. Also seems like a debug leftover.

kolyshkin · 2022-03-23T17:31:38Z

libcontainer/rootfs_linux.go

@@ -1055,7 +1079,9 @@ func remount(m *configs.Mount, rootfs string, mountFd *int) error {
 		}
 		// ... and retry the mount with ro flag set.
 		flags |= unix.MS_RDONLY
-		return mount(source, m.Destination, procfd, m.Device, flags, "")
+
+		logrus.Infof("m.Data: %v\n", m.Data)


No need for \n here. Also seems like a debug leftover.

kolyshkin · 2022-03-23T17:33:09Z

OK, this should be a draft / DNM until changes to runtime-spec and x/sys/unix are landed.

kolyshkin · 2022-03-23T17:40:45Z

In this patch set, kernel version is checked

This obviously won't work because of e.g. RHEL kernels that backport lots of stuff.

but as an alternative approach we could just check syscall existence, any suggestions are welcome.

Right, a simple test checking if a syscall is available (perhaps calling it with wrong arguments and expecting EINVAL in return is sufficient), when wrap this into once.Do and use it freely.

As an example, see how we check if openat2(2) is available here:

runc/libcontainer/cgroups/file.go

Lines 79 to 98 in c4c4889

    
           	cgroupFd     int = -1 
        
           	prepOnce     sync.Once 
        
           	prepErr      error 
        
           	resolveFlags uint64 
        
           ) 
        
           func prepareOpenat2() error { 
        
           	prepOnce.Do(func() { 
        
           		fd, err := unix.Openat2(-1, cgroupfsDir, &unix.OpenHow{ 
        
           			Flags: unix.O_DIRECTORY | unix.O_PATH, 
        
           		}) 
        
           		if err != nil { 
        
           			prepErr = &os.PathError{Op: "openat2", Path: cgroupfsDir, Err: err} 
        
           			if err != unix.ENOSYS { //nolint:errorlint // unix errors are bare 
        
           				logrus.Warnf("falling back to securejoin: %s", prepErr) 
        
           			} else { 
        
           				logrus.Debug("openat2 not available, falling back to securejoin") 
        
           			} 
        
           			return 
        
           		}

AlexeyPerevalov · 2022-04-13T07:40:10Z

This PR depends on golang's x/sys library modifications:
https://go-review.googlesource.com/c/sys/+/397095
https://go-review.googlesource.com/c/sys/+/397094
and depends on opencontainers/runtime-spec#1143.
Patches for x/sys are merged, just need to wait for appropriate version release of lib.
runtime-spec change is stuck on review, how to initiate it?

jiangliu · 2022-05-13T08:02:45Z

libcontainer/rootfs_linux.go

+
+func mountByMove(m *configs.Mount, dest string) error {
+	// Check once for all mount point that syscall exists
+	syscallOnce.Do(func() {


Could this be exported as an interface so other parts may query the syscall availability status? We face the same issue too and seems it could be reused.

rata · 2022-07-12T16:46:12Z

@AlexeyPerevalov the runtime spec PR was already merged (opencontainers/runtime-spec#1143). It would be great if you can finish this PR now :)

AlexeyPerevalov · 2022-07-14T10:26:30Z

runtime-spec changes are not yet tagged, latest tag is v1.0.2
the same with x/sys golang lib, not yet published

rata · 2022-07-14T13:40:50Z

@AlexeyPerevalov No need to wait for a runtime-spec tag, IMHO. We are not using a tag already:

runc/go.mod

Line 15 in a776ec9

    
           github.com/opencontainers/runtime-spec v1.0.3-0.20210326190908-1c3f411f0417

For the x/sys pkg, I think it makes sense to define ourselves the constants if that is not going to be released soon. Do you happen to know when they plan to release that with the constants we need? If there is no visibility into that, I don't know what others think, but I would guess it seems reasonable to use some tmp file with the constants defined here. We can switch whenever that becomes available.

kolyshkin · 2022-07-21T00:54:23Z

runtime-spec changes are not yet tagged, latest tag is v1.0.2

We are already using a sha for it (due to the lack of runtime-spec maintainers I guess).

the same with x/sys golang lib, not yet published

This one is never tagged, i.e. all users are just pin it to a particular sha.

AlexeyPerevalov · 2022-07-29T13:33:09Z

in centos-stream-9 two tests are failed:
TestUsernsCheckpoint
TestCheckpoint

Tests related to checkpoint restore failed due to segfault in criu, I reproduced it on Centos Stream 8 (I don't have Stream 9), but It is not reproducible on Ubuntu 20.04, with working criu.

rata · 2022-07-29T13:56:41Z

libcontainer/rootfs_linux.go

+		if mountByMove(m, dest) != nil {
+			if err := mountPropagate(m, rootfs, mountLabel, mountFd); err != nil {
+				return err


This means, if we fail to mount with idmap mounts, it just does a bind-mount?

I'm not sure what is the right behavior, because just mounting and ignoring the mapping for that mount will probably create issues to access the files (if the container running inside a userns).

I'm not sure what the right behavior should be. What do you think? And other maintainers?

I'm leaning towards in case of errors just failing the mount, rather than doing a bind-mount and ignoring the mapping.

It will definitely cause access issues, or even worse: files might get written with completely different UIDs/GIDs and it will be painful to fix once that happens.

@AlexeyPerevalov What do you think?

@AkihiroSuda @kolyshkin What do you think?

Thinking a little bit how higher layers will use this, yes, I think we need to error out. At the k8s layer we want to start the container with an id mapped mount or not start it at all. If it is started without the id mapped mounts, the files created will have just random garbage as UID/GID, won't be able to read files... it will be a complete mess.

I think if we just return an error if an id mapped mount was requested and we failed to create it is the right path here.

Also, even if in the future we want to open this door (doubt about it), we can always open a door. But we won't be able to close it if we open it now.

@AlexeyPerevalov Can we make this error out if the id map mount fails?

rata · 2022-07-29T13:58:31Z

@AlexeyPerevalov but it seems all test have passed... Was that maybe before the latest fix merged yesterday to fix the CI? (although IIRC it was only about one of those test, not both)

rata · 2022-08-09T15:37:01Z

@AlexeyPerevalov friendly ping? It would be great to have this in for runc 1.2.

(otherwise, we will have to wait for another runc release, that takes a while, some more time for stable versions of high level container runtimes to include it, etc. :))

Signed-off-by: Alexey Perevalov <[email protected]>

to configs.Mount from spec This patch is using mount_setattr for idmapped mounts mount_setattr syscall requires anonymous mount point, IOW mount point should not be published, but it published by mount syscall. So either fsmount or open_tree should be used here. So open_tree syscall is used instead of mount, for bind mount. Signed-off-by: Alexey Perevalov <[email protected]>

This is not based on kernel version, just to check syscall existence. Signed-off-by: Alexey Perevalov <[email protected]>

rata

@AlexeyPerevalov Hey, I was testing this and can't make it work.

It seems the code to create an idmap mount is fine (except probably the move mount should be last), but as this code is run already inside the userns, we always get EPERM for the mountSetAttr() syscall, because that can only be run from the initial userns to create a IDMAP mount.

Have you tested this with runc and it worked for you? It would be great if I'm missing something and there is a simple way to make this work :)

rata · 2023-01-27T15:11:39Z

libcontainer/rootfs_linux.go

-		if err := mountPropagate(m, rootfs, mountLabel, mountFd); err != nil {
-			return err
+
+		if mountByMove(m, dest) != nil {


I'm testing this, and it seems this can't work. At this point we are inside the userns (I've added some logrus.Debugf to print the content of /proc/self/uid_map to verify this) and therefore we create an idmap mount. They only work from the initial userns.

Did you made this runc code work for you at some point? Or was this a c&p from working code in a standalone binary (that you tried from the host, so the initial userns) that never was tested in the runc binary (that IIUC this part runs inside the userns).

Am I missing something? Or do we need a big rework to make this work?

rata · 2023-01-27T17:48:30Z

I really wish to include this functionality in the next runc release. This is required for our userns work in Kubernetes. I'll try to make this work.

If anyone has any thoughts on the approach to take, please let me know :)

AkihiroSuda · 2023-01-30T18:01:23Z

I really wish to include this functionality in the next runc release. This is required for our userns work in Kubernetes. I'll try to make this work.

If anyone has any thoughts on the approach to take, please let me know :)

Thanks @rata , could you open a new PR to carry this PR?

rata · 2023-01-31T10:21:03Z

@AkihiroSuda Thanks! Yes, I'm working on it and I'll open a new PR in the next days.

I need to start from scratch and do most things in nsenter/nsexec, so nothing really to carry from here. And quite more involved, that is why I haven't opened it yet :)

rata · 2023-02-01T11:41:07Z

Opened this draft PR with an approach that works: #3717

AlexeyPerevalov changed the title ~~Id map mounts move to open tree~~ Support Id mapped mounts for shared volumes Mar 23, 2022

AkihiroSuda added the enhancement label Mar 23, 2022

AkihiroSuda added this to the 1.2.0 milestone Mar 23, 2022

AkihiroSuda reviewed Mar 23, 2022

View reviewed changes

kolyshkin reviewed Mar 23, 2022

View reviewed changes

kolyshkin marked this pull request as draft March 23, 2022 17:33

AlexeyPerevalov force-pushed the IdMapMountsMoveToOpenTree branch from c1a18c3 to 26b6744 Compare April 5, 2022 14:28

AlexeyPerevalov force-pushed the IdMapMountsMoveToOpenTree branch from 26b6744 to 27d6967 Compare April 18, 2022 07:49

jiangliu reviewed May 13, 2022

View reviewed changes

AlexeyPerevalov force-pushed the IdMapMountsMoveToOpenTree branch from 27d6967 to b1187e9 Compare July 14, 2022 10:25

AlexeyPerevalov marked this pull request as ready for review July 14, 2022 10:25

AlexeyPerevalov force-pushed the IdMapMountsMoveToOpenTree branch 3 times, most recently from 1ba3c92 to 28c40da Compare July 25, 2022 13:49

kolyshkin force-pushed the IdMapMountsMoveToOpenTree branch from 28c40da to 7fdbd19 Compare July 28, 2022 19:08

rata reviewed Jul 29, 2022

View reviewed changes

AlexeyPerevalov added 3 commits August 12, 2022 08:04

ID mapping field in config.Mount

b93a25d

Signed-off-by: Alexey Perevalov <[email protected]>

Fallback for mount_setattr

7b34ecb

This is not based on kernel version, just to check syscall existence. Signed-off-by: Alexey Perevalov <[email protected]>

AlexeyPerevalov force-pushed the IdMapMountsMoveToOpenTree branch from 7fdbd19 to 7b34ecb Compare August 12, 2022 08:51

rata reviewed Jan 27, 2023

View reviewed changes

AkihiroSuda mentioned this pull request Feb 1, 2023

go:mod: runtime-spec v1.1.0-rc.1; add docs/spec-conformance.md #3716

Merged

rata mentioned this pull request Feb 1, 2023

Support idmap mounts for volumes #3717

Merged

5 tasks

AkihiroSuda removed this from the 1.2.0 milestone Feb 1, 2023

lifubang closed this in #3717 Jul 17, 2023

	logrus.Infof("mountFds: %v", mountFds)
	logrus.Debugf("mountFds: %v", mountFds)

		github.com/opencontainers/selinux v1.10.0
		github.com/pkg/errors v0.9.1

		logrus.Infof("m.Data: %v\n", m.Data)
		logrus.Infof("flags: %v\n", flags)

Support Id mapped mounts for shared volumes #3429

Support Id mapped mounts for shared volumes #3429

Conversation

AlexeyPerevalov commented Mar 23, 2022

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

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

kolyshkin commented Mar 23, 2022

kolyshkin commented Mar 23, 2022

AlexeyPerevalov commented Apr 13, 2022

Choose a reason for hiding this comment

rata commented Jul 12, 2022

AlexeyPerevalov commented Jul 14, 2022

rata commented Jul 14, 2022

kolyshkin commented Jul 21, 2022

AlexeyPerevalov commented Jul 29, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rata commented Jul 29, 2022

rata commented Aug 9, 2022

rata left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rata commented Jan 27, 2023

AkihiroSuda commented Jan 30, 2023

rata commented Jan 31, 2023

rata commented Feb 1, 2023