From c47f58c4e9a6b5a4cb36bb953ae24ef6e81b38f0 Mon Sep 17 00:00:00 2001 From: Francis Laniel Date: Tue, 18 Jul 2023 10:38:47 +0200 Subject: [PATCH 1/3] Capitalize [UG]idMappings as [UG]IDMappings Signed-off-by: Francis Laniel --- libcontainer/README.md | 4 +-- libcontainer/configs/config.go | 8 ++--- libcontainer/configs/config_linux.go | 8 ++--- libcontainer/configs/config_linux_test.go | 4 +-- libcontainer/configs/validate/rootless.go | 8 ++--- .../configs/validate/rootless_test.go | 26 +++++++------- libcontainer/configs/validate/validator.go | 8 ++--- .../configs/validate/validator_test.go | 34 +++++++++---------- libcontainer/container_linux.go | 10 +++--- libcontainer/integration/exec_test.go | 4 +-- libcontainer/integration/template_test.go | 4 +-- libcontainer/specconv/spec_linux.go | 4 +-- 12 files changed, 61 insertions(+), 61 deletions(-) diff --git a/libcontainer/README.md b/libcontainer/README.md index 26f7b25c2..303dd6125 100644 --- a/libcontainer/README.md +++ b/libcontainer/README.md @@ -184,14 +184,14 @@ config := &configs.Config{ Flags: defaultMountFlags | unix.MS_RDONLY, }, }, - UidMappings: []configs.IDMap{ + UIDMappings: []configs.IDMap{ { ContainerID: 0, HostID: 1000, Size: 65536, }, }, - GidMappings: []configs.IDMap{ + GIDMappings: []configs.IDMap{ { ContainerID: 0, HostID: 1000, diff --git a/libcontainer/configs/config.go b/libcontainer/configs/config.go index d43ea7860..576db5952 100644 --- a/libcontainer/configs/config.go +++ b/libcontainer/configs/config.go @@ -159,11 +159,11 @@ type Config struct { // More information about kernel oom score calculation here: https://lwn.net/Articles/317814/ OomScoreAdj *int `json:"oom_score_adj,omitempty"` - // UidMappings is an array of User ID mappings for User Namespaces - UidMappings []IDMap `json:"uid_mappings"` + // UIDMappings is an array of User ID mappings for User Namespaces + UIDMappings []IDMap `json:"uid_mappings"` - // GidMappings is an array of Group ID mappings for User Namespaces - GidMappings []IDMap `json:"gid_mappings"` + // GIDMappings is an array of Group ID mappings for User Namespaces + GIDMappings []IDMap `json:"gid_mappings"` // MaskPaths specifies paths within the container's rootfs to mask over with a bind // mount pointing to /dev/null as to prevent reads of the file. diff --git a/libcontainer/configs/config_linux.go b/libcontainer/configs/config_linux.go index 8c02848b7..41a365612 100644 --- a/libcontainer/configs/config_linux.go +++ b/libcontainer/configs/config_linux.go @@ -13,10 +13,10 @@ var ( // different when user namespaces are enabled. func (c Config) HostUID(containerId int) (int, error) { if c.Namespaces.Contains(NEWUSER) { - if c.UidMappings == nil { + if c.UIDMappings == nil { return -1, errNoUIDMap } - id, found := c.hostIDFromMapping(containerId, c.UidMappings) + id, found := c.hostIDFromMapping(containerId, c.UIDMappings) if !found { return -1, errNoUserMap } @@ -36,10 +36,10 @@ func (c Config) HostRootUID() (int, error) { // different when user namespaces are enabled. func (c Config) HostGID(containerId int) (int, error) { if c.Namespaces.Contains(NEWUSER) { - if c.GidMappings == nil { + if c.GIDMappings == nil { return -1, errNoGIDMap } - id, found := c.hostIDFromMapping(containerId, c.GidMappings) + id, found := c.hostIDFromMapping(containerId, c.GIDMappings) if !found { return -1, errNoGroupMap } diff --git a/libcontainer/configs/config_linux_test.go b/libcontainer/configs/config_linux_test.go index 68d33e61a..b94ffaed2 100644 --- a/libcontainer/configs/config_linux_test.go +++ b/libcontainer/configs/config_linux_test.go @@ -34,7 +34,7 @@ func TestHostRootUIDNoUSERNS(t *testing.T) { func TestHostRootUIDWithUSERNS(t *testing.T) { config := &Config{ Namespaces: Namespaces{{Type: NEWUSER}}, - UidMappings: []IDMap{ + UIDMappings: []IDMap{ { ContainerID: 0, HostID: 1000, @@ -67,7 +67,7 @@ func TestHostRootGIDNoUSERNS(t *testing.T) { func TestHostRootGIDWithUSERNS(t *testing.T) { config := &Config{ Namespaces: Namespaces{{Type: NEWUSER}}, - GidMappings: []IDMap{ + GIDMappings: []IDMap{ { ContainerID: 0, HostID: 1000, diff --git a/libcontainer/configs/validate/rootless.go b/libcontainer/configs/validate/rootless.go index 99ab3f8f3..6d32704a3 100644 --- a/libcontainer/configs/validate/rootless.go +++ b/libcontainer/configs/validate/rootless.go @@ -42,10 +42,10 @@ func rootlessEUIDMappings(config *configs.Config) error { return errors.New("rootless container requires user namespaces") } - if len(config.UidMappings) == 0 { + if len(config.UIDMappings) == 0 { return errors.New("rootless containers requires at least one UID mapping") } - if len(config.GidMappings) == 0 { + if len(config.GIDMappings) == 0 { return errors.New("rootless containers requires at least one GID mapping") } return nil @@ -68,7 +68,7 @@ func rootlessEUIDMount(config *configs.Config) error { // Ignore unknown mount options. continue } - if !hasIDMapping(uid, config.UidMappings) { + if !hasIDMapping(uid, config.UIDMappings) { return errors.New("cannot specify uid= mount options for unmapped uid in rootless containers") } } @@ -79,7 +79,7 @@ func rootlessEUIDMount(config *configs.Config) error { // Ignore unknown mount options. continue } - if !hasIDMapping(gid, config.GidMappings) { + if !hasIDMapping(gid, config.GIDMappings) { return errors.New("cannot specify gid= mount options for unmapped gid in rootless containers") } } diff --git a/libcontainer/configs/validate/rootless_test.go b/libcontainer/configs/validate/rootless_test.go index 0e8df6eb2..7b0887399 100644 --- a/libcontainer/configs/validate/rootless_test.go +++ b/libcontainer/configs/validate/rootless_test.go @@ -16,14 +16,14 @@ func rootlessEUIDConfig() *configs.Config { {Type: configs.NEWUSER}, }, ), - UidMappings: []configs.IDMap{ + UIDMappings: []configs.IDMap{ { HostID: 1337, ContainerID: 0, Size: 1, }, }, - GidMappings: []configs.IDMap{ + GIDMappings: []configs.IDMap{ { HostID: 7331, ContainerID: 0, @@ -52,7 +52,7 @@ func TestValidateRootlessEUIDUserns(t *testing.T) { func TestValidateRootlessEUIDMappingUid(t *testing.T) { config := rootlessEUIDConfig() - config.UidMappings = nil + config.UIDMappings = nil if err := Validate(config); err == nil { t.Errorf("Expected error to occur if no uid mappings provided") } @@ -60,7 +60,7 @@ func TestValidateRootlessEUIDMappingUid(t *testing.T) { func TestValidateNonZeroEUIDMappingGid(t *testing.T) { config := rootlessEUIDConfig() - config.GidMappings = nil + config.GIDMappings = nil if err := Validate(config); err == nil { t.Errorf("Expected error to occur if no gid mappings provided") } @@ -93,15 +93,15 @@ func TestValidateRootlessEUIDMountUid(t *testing.T) { } config.Mounts[0].Data = "uid=2" - config.UidMappings[0].Size = 10 + config.UIDMappings[0].Size = 10 if err := Validate(config); err != nil { - t.Errorf("Expected error to not occur when setting uid=2 in mount options and UidMapping[0].size is 10") + t.Errorf("Expected error to not occur when setting uid=2 in mount options and UIDMappings[0].size is 10") } config.Mounts[0].Data = "uid=20" - config.UidMappings[0].Size = 10 + config.UIDMappings[0].Size = 10 if err := Validate(config); err == nil { - t.Errorf("Expected error to occur when setting uid=20 in mount options and UidMapping[0].size is 10") + t.Errorf("Expected error to occur when setting uid=20 in mount options and UIDMappings[0].size is 10") } } @@ -130,21 +130,21 @@ func TestValidateRootlessEUIDMountGid(t *testing.T) { } config.Mounts[0].Data = "gid=5" - config.GidMappings[0].Size = 10 + config.GIDMappings[0].Size = 10 if err := Validate(config); err != nil { - t.Errorf("Expected error to not occur when setting gid=5 in mount options and GidMapping[0].size is 10") + t.Errorf("Expected error to not occur when setting gid=5 in mount options and GIDMappings[0].size is 10") } config.Mounts[0].Data = "gid=11" - config.GidMappings[0].Size = 10 + config.GIDMappings[0].Size = 10 if err := Validate(config); err == nil { - t.Errorf("Expected error to occur when setting gid=11 in mount options and GidMapping[0].size is 10") + t.Errorf("Expected error to occur when setting gid=11 in mount options and GIDMappings[0].size is 10") } } func BenchmarkRootlessEUIDMount(b *testing.B) { config := rootlessEUIDConfig() - config.GidMappings[0].Size = 10 + config.GIDMappings[0].Size = 10 config.Mounts = []*configs.Mount{ { Source: "devpts", diff --git a/libcontainer/configs/validate/validator.go b/libcontainer/configs/validate/validator.go index 5f065a384..196b431db 100644 --- a/libcontainer/configs/validate/validator.go +++ b/libcontainer/configs/validate/validator.go @@ -95,7 +95,7 @@ func namespaces(config *configs.Config) error { return errors.New("USER namespaces aren't enabled in the kernel") } } else { - if config.UidMappings != nil || config.GidMappings != nil { + if config.UIDMappings != nil || config.GIDMappings != nil { return errors.New("User namespace mappings specified, but USER namespace isn't enabled in the config") } } @@ -264,13 +264,13 @@ func checkIDMapMounts(config *configs.Config, m *configs.Mount) error { if config.RootlessEUID { return fmt.Errorf("gidMappings/uidMappings is not supported when runc is being launched with EUID != 0, needs CAP_SYS_ADMIN on the runc parent's user namespace") } - if len(config.UidMappings) == 0 || len(config.GidMappings) == 0 { + if len(config.UIDMappings) == 0 || len(config.GIDMappings) == 0 { return fmt.Errorf("not yet supported to use gidMappings/uidMappings in a mount without also using a user namespace") } - if !sameMapping(config.UidMappings, m.UIDMappings) { + if !sameMapping(config.UIDMappings, m.UIDMappings) { return fmt.Errorf("not yet supported for the mount uidMappings to be different than user namespace uidMapping") } - if !sameMapping(config.GidMappings, m.GIDMappings) { + if !sameMapping(config.GIDMappings, m.GIDMappings) { return fmt.Errorf("not yet supported for the mount gidMappings to be different than user namespace gidMapping") } if !filepath.IsAbs(m.Source) { diff --git a/libcontainer/configs/validate/validator_test.go b/libcontainer/configs/validate/validator_test.go index fc92a7bb8..58aae7a68 100644 --- a/libcontainer/configs/validate/validator_test.go +++ b/libcontainer/configs/validate/validator_test.go @@ -192,7 +192,7 @@ func TestValidateUsernamespaceWithoutUserNS(t *testing.T) { uidMap := configs.IDMap{ContainerID: 123} config := &configs.Config{ Rootfs: "/var", - UidMappings: []configs.IDMap{uidMap}, + UIDMappings: []configs.IDMap{uidMap}, } err := Validate(config) @@ -405,8 +405,8 @@ func TestValidateIDMapMounts(t *testing.T) { name: "idmap mount without bind opt specified", isErr: true, config: &configs.Config{ - UidMappings: mapping, - GidMappings: mapping, + UIDMappings: mapping, + GIDMappings: mapping, Mounts: []*configs.Mount{ { Source: "/abs/path/", @@ -422,8 +422,8 @@ func TestValidateIDMapMounts(t *testing.T) { isErr: true, config: &configs.Config{ RootlessEUID: true, - UidMappings: mapping, - GidMappings: mapping, + UIDMappings: mapping, + GIDMappings: mapping, Mounts: []*configs.Mount{ { Source: "/abs/path/", @@ -454,8 +454,8 @@ func TestValidateIDMapMounts(t *testing.T) { name: "idmap mounts with different userns and mount mappings", isErr: true, config: &configs.Config{ - UidMappings: mapping, - GidMappings: mapping, + UIDMappings: mapping, + GIDMappings: mapping, Mounts: []*configs.Mount{ { Source: "/abs/path/", @@ -477,8 +477,8 @@ func TestValidateIDMapMounts(t *testing.T) { name: "idmap mounts with different userns and mount mappings", isErr: true, config: &configs.Config{ - UidMappings: mapping, - GidMappings: mapping, + UIDMappings: mapping, + GIDMappings: mapping, Mounts: []*configs.Mount{ { Source: "/abs/path/", @@ -500,8 +500,8 @@ func TestValidateIDMapMounts(t *testing.T) { name: "idmap mounts without abs source path", isErr: true, config: &configs.Config{ - UidMappings: mapping, - GidMappings: mapping, + UIDMappings: mapping, + GIDMappings: mapping, Mounts: []*configs.Mount{ { Source: "./rel/path/", @@ -517,8 +517,8 @@ func TestValidateIDMapMounts(t *testing.T) { name: "idmap mounts without abs dest path", isErr: true, config: &configs.Config{ - UidMappings: mapping, - GidMappings: mapping, + UIDMappings: mapping, + GIDMappings: mapping, Mounts: []*configs.Mount{ { Source: "/abs/path/", @@ -534,8 +534,8 @@ func TestValidateIDMapMounts(t *testing.T) { { name: "simple idmap mount", config: &configs.Config{ - UidMappings: mapping, - GidMappings: mapping, + UIDMappings: mapping, + GIDMappings: mapping, Mounts: []*configs.Mount{ { Source: "/another-abs/path/", @@ -550,8 +550,8 @@ func TestValidateIDMapMounts(t *testing.T) { { name: "idmap mount with more flags", config: &configs.Config{ - UidMappings: mapping, - GidMappings: mapping, + UIDMappings: mapping, + GIDMappings: mapping, Mounts: []*configs.Mount{ { Source: "/another-abs/path/", diff --git a/libcontainer/container_linux.go b/libcontainer/container_linux.go index 21c9905e6..e4a1e6d7d 100644 --- a/libcontainer/container_linux.go +++ b/libcontainer/container_linux.go @@ -2219,7 +2219,7 @@ func (c *Container) bootstrapData(cloneFlags uintptr, nsMaps map[configs.Namespa _, joinExistingUser := nsMaps[configs.NEWUSER] if !joinExistingUser { // write uid mappings - if len(c.config.UidMappings) > 0 { + if len(c.config.UIDMappings) > 0 { if c.config.RootlessEUID { // We resolve the paths for new{u,g}idmap from // the context of runc to avoid doing a path @@ -2231,7 +2231,7 @@ func (c *Container) bootstrapData(cloneFlags uintptr, nsMaps map[configs.Namespa }) } } - b, err := encodeIDMapping(c.config.UidMappings) + b, err := encodeIDMapping(c.config.UIDMappings) if err != nil { return nil, err } @@ -2242,8 +2242,8 @@ func (c *Container) bootstrapData(cloneFlags uintptr, nsMaps map[configs.Namespa } // write gid mappings - if len(c.config.GidMappings) > 0 { - b, err := encodeIDMapping(c.config.GidMappings) + if len(c.config.GIDMappings) > 0 { + b, err := encodeIDMapping(c.config.GIDMappings) if err != nil { return nil, err } @@ -2356,5 +2356,5 @@ func requiresRootOrMappingTool(c *configs.Config) bool { gidMap := []configs.IDMap{ {ContainerID: 0, HostID: os.Getegid(), Size: 1}, } - return !reflect.DeepEqual(c.GidMappings, gidMap) + return !reflect.DeepEqual(c.GIDMappings, gidMap) } diff --git a/libcontainer/integration/exec_test.go b/libcontainer/integration/exec_test.go index 03d166390..9795964ca 100644 --- a/libcontainer/integration/exec_test.go +++ b/libcontainer/integration/exec_test.go @@ -1774,8 +1774,8 @@ func TestBindMountAndUser(t *testing.T) { }) // Set HostID to 1000 to avoid DAC_OVERRIDE bypassing the purpose of this test. - config.UidMappings[0].HostID = 1000 - config.GidMappings[0].HostID = 1000 + config.UIDMappings[0].HostID = 1000 + config.GIDMappings[0].HostID = 1000 // Set the owner of rootfs to the effective IDs in the host to avoid errors // while creating the folders to perform the mounts. diff --git a/libcontainer/integration/template_test.go b/libcontainer/integration/template_test.go index 3de1d3e32..63c46b28f 100644 --- a/libcontainer/integration/template_test.go +++ b/libcontainer/integration/template_test.go @@ -193,8 +193,8 @@ func newTemplateConfig(t *testing.T, p *tParam) *configs.Config { } if p.userns { - config.UidMappings = []configs.IDMap{{HostID: 0, ContainerID: 0, Size: 1000}} - config.GidMappings = []configs.IDMap{{HostID: 0, ContainerID: 0, Size: 1000}} + config.UIDMappings = []configs.IDMap{{HostID: 0, ContainerID: 0, Size: 1000}} + config.GIDMappings = []configs.IDMap{{HostID: 0, ContainerID: 0, Size: 1000}} config.Namespaces = append(config.Namespaces, configs.Namespace{Type: configs.NEWUSER}) } else { config.Mounts = append(config.Mounts, &configs.Mount{ diff --git a/libcontainer/specconv/spec_linux.go b/libcontainer/specconv/spec_linux.go index d7ca7a53d..7fb67d8ee 100644 --- a/libcontainer/specconv/spec_linux.go +++ b/libcontainer/specconv/spec_linux.go @@ -945,8 +945,8 @@ next: func setupUserNamespace(spec *specs.Spec, config *configs.Config) error { if spec.Linux != nil { - config.UidMappings = toConfigIDMap(spec.Linux.UIDMappings) - config.GidMappings = toConfigIDMap(spec.Linux.GIDMappings) + config.UIDMappings = toConfigIDMap(spec.Linux.UIDMappings) + config.GIDMappings = toConfigIDMap(spec.Linux.GIDMappings) } rootUID, err := config.HostRootUID() if err != nil { From 46ada59ba2339d4a7bef5a7682dba70d351551bc Mon Sep 17 00:00:00 2001 From: Francis Laniel Date: Tue, 18 Jul 2023 10:56:15 +0200 Subject: [PATCH 2/3] Use an *int for srcFD Previously to this commit, we used a string for srcFD as /proc/self/fd/NN. This commit modified to this behavior, so srcFD is only an *int and the full path is constructed in mountViaFDs() if srcFD is different than nil. Signed-off-by: Francis Laniel --- libcontainer/container_linux.go | 5 +---- libcontainer/mount_linux.go | 22 ++++++++++++---------- libcontainer/rootfs_linux.go | 22 +++++++++++----------- 3 files changed, 24 insertions(+), 25 deletions(-) diff --git a/libcontainer/container_linux.go b/libcontainer/container_linux.go index e4a1e6d7d..2f0a6c641 100644 --- a/libcontainer/container_linux.go +++ b/libcontainer/container_linux.go @@ -1419,10 +1419,7 @@ func (c *Container) prepareCriuRestoreMounts(mounts []*configs.Mount) error { // set up in the order they are configured. if m.Device == "bind" { if err := utils.WithProcfd(c.config.Rootfs, m.Destination, func(dstFD string) error { - if err := mountViaFDs(m.Source, "", m.Destination, dstFD, "", unix.MS_BIND|unix.MS_REC, ""); err != nil { - return err - } - return nil + return mountViaFDs(m.Source, nil, m.Destination, dstFD, "", unix.MS_BIND|unix.MS_REC, "") }); err != nil { return err } diff --git a/libcontainer/mount_linux.go b/libcontainer/mount_linux.go index 751262f31..b391dd849 100644 --- a/libcontainer/mount_linux.go +++ b/libcontainer/mount_linux.go @@ -10,7 +10,7 @@ import ( type mountError struct { op string source string - srcFD string + srcFD *int target string dstFD string flags uintptr @@ -24,8 +24,8 @@ func (e *mountError) Error() string { if e.source != "" { out += "src=" + e.source + ", " - if e.srcFD != "" { - out += "srcFD=" + e.srcFD + ", " + if e.srcFD != nil { + out += "srcFD=" + strconv.Itoa(*e.srcFD) + ", " } } out += "dst=" + e.target @@ -53,21 +53,23 @@ func (e *mountError) Unwrap() error { // mount is a simple unix.Mount wrapper, returning an error with more context // in case it failed. func mount(source, target, fstype string, flags uintptr, data string) error { - return mountViaFDs(source, "", target, "", fstype, flags, data) + return mountViaFDs(source, nil, target, "", fstype, flags, data) } // mountViaFDs is a unix.Mount wrapper which uses srcFD instead of source, -// and dstFD instead of target, unless those are empty. The *FD arguments, -// if non-empty, are expected to be in the form of a path to an opened file -// descriptor on procfs (i.e. "/proc/self/fd/NN"). +// and dstFD instead of target, unless those are empty. +// If srcFD is different than nil, its path (i.e. "/proc/self/fd/NN") will be +// constructed by this function. +// dstFD argument, if non-empty, is expected to be in the form of a path to an +// opened file descriptor on procfs (i.e. "/proc/self/fd/NN"). // // If case an FD is used instead of a source or a target path, the // corresponding path is only used to add context to an error in case // the mount operation has failed. -func mountViaFDs(source, srcFD, target, dstFD, fstype string, flags uintptr, data string) error { +func mountViaFDs(source string, srcFD *int, target, dstFD, fstype string, flags uintptr, data string) error { src := source - if srcFD != "" { - src = srcFD + if srcFD != nil { + src = "/proc/self/fd/" + strconv.Itoa(*srcFD) } dst := target if dstFD != "" { diff --git a/libcontainer/rootfs_linux.go b/libcontainer/rootfs_linux.go index edd3abd3c..0da300f99 100644 --- a/libcontainer/rootfs_linux.go +++ b/libcontainer/rootfs_linux.go @@ -40,13 +40,13 @@ type mountConfig struct { // mountEntry contains mount data specific to a mount point. type mountEntry struct { *configs.Mount - srcFD string + srcFD *int idmapFD int } func (m *mountEntry) src() string { - if m.srcFD != "" { - return m.srcFD + if m.srcFD != nil { + return "/proc/self/fd/" + strconv.Itoa(*m.srcFD) } return m.Source } @@ -90,7 +90,7 @@ func prepareRootfs(pipe io.ReadWriter, iConfig *initConfig, mountFds mountFds) ( // Just before the loop we checked that if not empty, len(mountFds) == len(config.Mounts). // Therefore, we can access mountFds[i] without any concerns. if mountFds.sourceFds != nil && mountFds.sourceFds[i] != -1 { - entry.srcFD = "/proc/self/fd/" + strconv.Itoa(mountFds.sourceFds[i]) + entry.srcFD = &mountFds.sourceFds[i] } // We validated before we can access idmapFds[i]. @@ -98,7 +98,7 @@ func prepareRootfs(pipe io.ReadWriter, iConfig *initConfig, mountFds mountFds) ( entry.idmapFD = mountFds.idmapFds[i] } - if entry.idmapFD != -1 && entry.srcFD != "" { + if entry.idmapFD != -1 && entry.srcFD != nil { return fmt.Errorf("malformed mountFds and idmapFds slice, entry: %v has fds in both slices", i) } @@ -297,7 +297,7 @@ func mountCgroupV1(m *configs.Mount, c *mountConfig) error { data = cgroups.CgroupNamePrefix + data source = "systemd" } - return mountViaFDs(source, "", b.Destination, dstFD, "cgroup", uintptr(flags), data) + return mountViaFDs(source, nil, b.Destination, dstFD, "cgroup", uintptr(flags), data) }); err != nil { return err } @@ -329,7 +329,7 @@ func mountCgroupV2(m *configs.Mount, c *mountConfig) error { return err } err = utils.WithProcfd(c.root, m.Destination, func(dstFD string) error { - return mountViaFDs(m.Source, "", m.Destination, dstFD, "cgroup2", uintptr(m.Flags), m.Data) + return mountViaFDs(m.Source, nil, m.Destination, dstFD, "cgroup2", uintptr(m.Flags), m.Data) }) if err == nil || !(errors.Is(err, unix.EPERM) || errors.Is(err, unix.EBUSY)) { return err @@ -403,7 +403,7 @@ func doTmpfsCopyUp(m mountEntry, rootfs, mountLabel string) (Err error) { return fmt.Errorf("tmpcopyup: failed to copy %s to %s (%s): %w", m.Destination, dstFD, tmpDir, err) } // Now move the mount into the container. - if err := mountViaFDs(tmpDir, "", m.Destination, dstFD, "", unix.MS_MOVE, ""); err != nil { + if err := mountViaFDs(tmpDir, nil, m.Destination, dstFD, "", unix.MS_MOVE, ""); err != nil { return fmt.Errorf("tmpcopyup: failed to move mount: %w", err) } return nil @@ -497,7 +497,7 @@ func mountToRootfs(c *mountConfig, m mountEntry) error { // system type and data arguments are ignored: // https://man7.org/linux/man-pages/man2/mount.2.html // We also ignore procfd because we want to act on dest. - if err := mountViaFDs("", "", dest, dstFD, "", uintptr(pflag), ""); err != nil { + if err := mountViaFDs("", nil, dest, dstFD, "", uintptr(pflag), ""); err != nil { return err } } @@ -733,7 +733,7 @@ func bindMountDeviceNode(rootfs, dest string, node *devices.Device) error { _ = f.Close() } return utils.WithProcfd(rootfs, dest, func(dstFD string) error { - return mountViaFDs(node.Path, "", dest, dstFD, "bind", unix.MS_BIND, "") + return mountViaFDs(node.Path, nil, dest, dstFD, "bind", unix.MS_BIND, "") }) } @@ -1154,7 +1154,7 @@ func mountPropagate(m mountEntry, rootfs string, mountLabel string) error { // target needs to be re-opened. if err := utils.WithProcfd(rootfs, m.Destination, func(dstFD string) error { for _, pflag := range m.PropagationFlags { - if err := mountViaFDs("", "", m.Destination, dstFD, "", uintptr(pflag), ""); err != nil { + if err := mountViaFDs("", nil, m.Destination, dstFD, "", uintptr(pflag), ""); err != nil { return err } } From a3785c88ec5b2b271e13ebb872cf5bd49bcedace Mon Sep 17 00:00:00 2001 From: Francis Laniel Date: Thu, 20 Jul 2023 12:04:08 +0200 Subject: [PATCH 3/3] Remove idmapFD field for mountEntry We cannot have both srcFD and idMapFD set at the same time. So, we can simplify this struct to only have one field which is used a srcFD most of the time and as idMapFD when we do an id map mount. Signed-off-by: Francis Laniel --- libcontainer/rootfs_linux.go | 24 +++++++++++------------- 1 file changed, 11 insertions(+), 13 deletions(-) diff --git a/libcontainer/rootfs_linux.go b/libcontainer/rootfs_linux.go index 0da300f99..4894400b6 100644 --- a/libcontainer/rootfs_linux.go +++ b/libcontainer/rootfs_linux.go @@ -40,8 +40,7 @@ type mountConfig struct { // mountEntry contains mount data specific to a mount point. type mountEntry struct { *configs.Mount - srcFD *int - idmapFD int + srcFD *int } func (m *mountEntry) src() string { @@ -86,20 +85,19 @@ func prepareRootfs(pipe io.ReadWriter, iConfig *initConfig, mountFds mountFds) ( cgroupns: config.Namespaces.Contains(configs.NEWCGROUP), } for i, m := range config.Mounts { - entry := mountEntry{Mount: m, idmapFD: -1} - // Just before the loop we checked that if not empty, len(mountFds) == len(config.Mounts). - // Therefore, we can access mountFds[i] without any concerns. + entry := mountEntry{Mount: m} + // Just before the loop we checked that if not empty, len(mountFds.sourceFds) == len(config.Mounts). + // Therefore, we can access mountFds.sourceFds[i] without any concerns. if mountFds.sourceFds != nil && mountFds.sourceFds[i] != -1 { entry.srcFD = &mountFds.sourceFds[i] } - // We validated before we can access idmapFds[i]. + // We validated before we can access mountFds.idmapFds[i]. if mountFds.idmapFds != nil && mountFds.idmapFds[i] != -1 { - entry.idmapFD = mountFds.idmapFds[i] - } - - if entry.idmapFD != -1 && entry.srcFD != nil { - return fmt.Errorf("malformed mountFds and idmapFds slice, entry: %v has fds in both slices", i) + if entry.srcFD != nil { + return fmt.Errorf("malformed mountFds and idmapFds slice, entry: %v has fds in both slices", i) + } + entry.srcFD = &mountFds.idmapFds[i] } if err := mountToRootfs(mountConfig, entry); err != nil { @@ -482,10 +480,10 @@ func mountToRootfs(c *mountConfig, m mountEntry) error { } if m.IsBind() && m.IsIDMapped() { - if m.idmapFD == -1 { + if m.srcFD == nil { return fmt.Errorf("error creating mount %+v: idmapFD is invalid, should point to a valid fd", m) } - if err := unix.MoveMount(m.idmapFD, "", -1, dest, unix.MOVE_MOUNT_F_EMPTY_PATH); err != nil { + if err := unix.MoveMount(*m.srcFD, "", -1, dest, unix.MOVE_MOUNT_F_EMPTY_PATH); err != nil { return fmt.Errorf("error on unix.MoveMount %+v: %w", m, err) }