From 630c0d7e8ca8affbf687988365b53a03bc6980e1 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Wed, 26 Jan 2022 20:50:20 -0800 Subject: [PATCH 1/6] libct: Container, Factory: rm InitPath, InitArgs Those are *always* /proc/self/exe init, and it does not make sense to ever change these. More to say, if InitArgs option func (removed by this commit) is used to change these parameters, it will break things, since "init" is hardcoded elsewhere. Remove this. Signed-off-by: Kir Kolyshkin --- libcontainer/README.md | 2 +- libcontainer/container_linux.go | 6 ++---- libcontainer/factory_linux.go | 34 --------------------------------- 3 files changed, 3 insertions(+), 39 deletions(-) diff --git a/libcontainer/README.md b/libcontainer/README.md index 658eb4313..79388eaf0 100644 --- a/libcontainer/README.md +++ b/libcontainer/README.md @@ -45,7 +45,7 @@ Then to create a container you first have to initialize an instance of a factory that will handle the creation and initialization for a container. ```go -factory, err := libcontainer.New("/var/lib/container", libcontainer.InitArgs(os.Args[0], "init")) +factory, err := libcontainer.New("/var/lib/container") if err != nil { logrus.Fatal(err) return diff --git a/libcontainer/container_linux.go b/libcontainer/container_linux.go index 91bea2a33..6e0a5cefe 100644 --- a/libcontainer/container_linux.go +++ b/libcontainer/container_linux.go @@ -41,8 +41,6 @@ type linuxContainer struct { config *configs.Config cgroupManager cgroups.Manager intelRdtManager intelrdt.Manager - initPath string - initArgs []string initProcess parentProcess initProcessStartTime uint64 newuidmapPath string @@ -481,8 +479,8 @@ func (c *linuxContainer) newParentProcess(p *Process) (parentProcess, error) { } func (c *linuxContainer) commandTemplate(p *Process, childInitPipe *os.File, childLogPipe *os.File) *exec.Cmd { - cmd := exec.Command(c.initPath, c.initArgs[1:]...) - cmd.Args[0] = c.initArgs[0] + cmd := exec.Command("/proc/self/exe", "init") + cmd.Args[0] = os.Args[0] cmd.Stdin = p.Stdin cmd.Stdout = p.Stdout cmd.Stderr = p.Stderr diff --git a/libcontainer/factory_linux.go b/libcontainer/factory_linux.go index d72a0532c..13dbeea67 100644 --- a/libcontainer/factory_linux.go +++ b/libcontainer/factory_linux.go @@ -5,7 +5,6 @@ import ( "errors" "fmt" "os" - "path/filepath" "regexp" "runtime/debug" "strconv" @@ -29,25 +28,6 @@ const ( var idRegex = regexp.MustCompile(`^[\w+-\.]+$`) -// InitArgs returns an options func to configure a LinuxFactory with the -// provided init binary path and arguments. -func InitArgs(args ...string) func(*LinuxFactory) error { - return func(l *LinuxFactory) (err error) { - if len(args) > 0 { - // Resolve relative paths to ensure that its available - // after directory changes. - if args[0], err = filepath.Abs(args[0]); err != nil { - // The only error returned from filepath.Abs is - // the one from os.Getwd, i.e. a system error. - return err - } - } - - l.InitArgs = args - return nil - } -} - // IntelRdtfs is an options func to configure a LinuxFactory to return // containers that use the Intel RDT "resource control" filesystem to // create and manage Intel RDT resources (e.g., L3 cache, memory bandwidth). @@ -86,8 +66,6 @@ func New(root string, options ...func(*LinuxFactory) error) (Factory, error) { } l := &LinuxFactory{ Root: root, - InitPath: "/proc/self/exe", - InitArgs: []string{os.Args[0], "init"}, Validator: validate.New(), } @@ -107,14 +85,6 @@ type LinuxFactory struct { // Root directory for the factory to store state. Root string - // InitPath is the path for calling the init responsibilities for spawning - // a container. - InitPath string - - // InitArgs are arguments for calling the init responsibilities for spawning - // a container. - InitArgs []string - // New{u,g}idmapPath is the path to the binaries used for mapping with // rootless containers. NewuidmapPath string @@ -188,8 +158,6 @@ func (l *LinuxFactory) Create(id string, config *configs.Config) (Container, err id: id, root: containerRoot, config: config, - initPath: l.InitPath, - initArgs: l.InitArgs, newuidmapPath: l.NewuidmapPath, newgidmapPath: l.NewgidmapPath, cgroupManager: cm, @@ -231,8 +199,6 @@ func (l *LinuxFactory) Load(id string) (Container, error) { initProcessStartTime: state.InitProcessStartTime, id: id, config: &state.Config, - initPath: l.InitPath, - initArgs: l.InitArgs, newuidmapPath: l.NewuidmapPath, newgidmapPath: l.NewgidmapPath, cgroupManager: cm, From 0d21515038ec0603417dc9f64c5e3e5e8b4d3991 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Thu, 20 Jan 2022 15:49:14 -0800 Subject: [PATCH 2/6] libct: remove Validator interface We only have one implementation of config validator, which is always used. It makes no sense to have Validator interface. Having validate.Validator field in Factory does not make sense for all the same reasons. Signed-off-by: Kir Kolyshkin --- libcontainer/configs/validate/rootless.go | 4 +- .../configs/validate/rootless_test.go | 40 +++++--------- libcontainer/configs/validate/validator.go | 55 +++++++------------ .../configs/validate/validator_test.go | 52 ++++++------------ libcontainer/factory_linux.go | 8 +-- libcontainer/specconv/spec_linux_test.go | 9 +-- 6 files changed, 59 insertions(+), 109 deletions(-) diff --git a/libcontainer/configs/validate/rootless.go b/libcontainer/configs/validate/rootless.go index 9a6e5eb32..7afdb4310 100644 --- a/libcontainer/configs/validate/rootless.go +++ b/libcontainer/configs/validate/rootless.go @@ -8,9 +8,9 @@ import ( "github.com/opencontainers/runc/libcontainer/configs" ) -// rootlessEUID makes sure that the config can be applied when runc +// rootlessEUIDCheck makes sure that the config can be applied when runc // is being executed as a non-root user (euid != 0) in the current user namespace. -func (v *ConfigValidator) rootlessEUID(config *configs.Config) error { +func rootlessEUIDCheck(config *configs.Config) error { if !config.RootlessEUID { return nil } diff --git a/libcontainer/configs/validate/rootless_test.go b/libcontainer/configs/validate/rootless_test.go index 59d15575d..0657abf48 100644 --- a/libcontainer/configs/validate/rootless_test.go +++ b/libcontainer/configs/validate/rootless_test.go @@ -34,10 +34,8 @@ func rootlessEUIDConfig() *configs.Config { } func TestValidateRootlessEUID(t *testing.T) { - validator := New() - config := rootlessEUIDConfig() - if err := validator.Validate(config); err != nil { + if err := Validate(config); err != nil { t.Errorf("Expected error to not occur: %+v", err) } } @@ -45,31 +43,25 @@ func TestValidateRootlessEUID(t *testing.T) { /* rootlessEUIDMappings */ func TestValidateRootlessEUIDUserns(t *testing.T) { - validator := New() - config := rootlessEUIDConfig() config.Namespaces = nil - if err := validator.Validate(config); err == nil { + if err := Validate(config); err == nil { t.Errorf("Expected error to occur if user namespaces not set") } } func TestValidateRootlessEUIDMappingUid(t *testing.T) { - validator := New() - config := rootlessEUIDConfig() config.UidMappings = nil - if err := validator.Validate(config); err == nil { + if err := Validate(config); err == nil { t.Errorf("Expected error to occur if no uid mappings provided") } } func TestValidateNonZeroEUIDMappingGid(t *testing.T) { - validator := New() - config := rootlessEUIDConfig() config.GidMappings = nil - if err := validator.Validate(config); err == nil { + if err := Validate(config); err == nil { t.Errorf("Expected error to occur if no gid mappings provided") } } @@ -78,8 +70,6 @@ func TestValidateNonZeroEUIDMappingGid(t *testing.T) { func TestValidateRootlessEUIDMountUid(t *testing.T) { config := rootlessEUIDConfig() - validator := New() - config.Mounts = []*configs.Mount{ { Source: "devpts", @@ -88,37 +78,35 @@ func TestValidateRootlessEUIDMountUid(t *testing.T) { }, } - if err := validator.Validate(config); err != nil { + if err := Validate(config); err != nil { t.Errorf("Expected error to not occur when uid= not set in mount options: %+v", err) } config.Mounts[0].Data = "uid=5" - if err := validator.Validate(config); err == nil { + if err := Validate(config); err == nil { t.Errorf("Expected error to occur when setting uid=5 in mount options") } config.Mounts[0].Data = "uid=0" - if err := validator.Validate(config); err != nil { + if err := Validate(config); err != nil { t.Errorf("Expected error to not occur when setting uid=0 in mount options: %+v", err) } config.Mounts[0].Data = "uid=2" config.UidMappings[0].Size = 10 - if err := validator.Validate(config); err != nil { + 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") } config.Mounts[0].Data = "uid=20" config.UidMappings[0].Size = 10 - if err := validator.Validate(config); err == nil { + 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") } } func TestValidateRootlessEUIDMountGid(t *testing.T) { config := rootlessEUIDConfig() - validator := New() - config.Mounts = []*configs.Mount{ { Source: "devpts", @@ -127,29 +115,29 @@ func TestValidateRootlessEUIDMountGid(t *testing.T) { }, } - if err := validator.Validate(config); err != nil { + if err := Validate(config); err != nil { t.Errorf("Expected error to not occur when gid= not set in mount options: %+v", err) } config.Mounts[0].Data = "gid=5" - if err := validator.Validate(config); err == nil { + if err := Validate(config); err == nil { t.Errorf("Expected error to occur when setting gid=5 in mount options") } config.Mounts[0].Data = "gid=0" - if err := validator.Validate(config); err != nil { + if err := Validate(config); err != nil { t.Errorf("Expected error to not occur when setting gid=0 in mount options: %+v", err) } config.Mounts[0].Data = "gid=5" config.GidMappings[0].Size = 10 - if err := validator.Validate(config); err != nil { + 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") } config.Mounts[0].Data = "gid=11" config.GidMappings[0].Size = 10 - if err := validator.Validate(config); err == nil { + 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") } } diff --git a/libcontainer/configs/validate/validator.go b/libcontainer/configs/validate/validator.go index 6493124a3..6972ec9c7 100644 --- a/libcontainer/configs/validate/validator.go +++ b/libcontainer/configs/validate/validator.go @@ -16,30 +16,19 @@ import ( "golang.org/x/sys/unix" ) -type Validator interface { - Validate(*configs.Config) error -} - -func New() Validator { - return &ConfigValidator{} -} - -type ConfigValidator struct{} - type check func(config *configs.Config) error -func (v *ConfigValidator) Validate(config *configs.Config) error { +func Validate(config *configs.Config) error { checks := []check{ - v.cgroups, - v.rootfs, - v.network, - v.hostname, - v.security, - v.usernamespace, - v.cgroupnamespace, - v.sysctl, - v.intelrdt, - v.rootlessEUID, + cgroupsCheck, + rootfs, + network, + hostname, + security, + namespaces, + sysctl, + intelrdtCheck, + rootlessEUIDCheck, } for _, c := range checks { if err := c(config); err != nil { @@ -48,7 +37,7 @@ func (v *ConfigValidator) Validate(config *configs.Config) error { } // Relaxed validation rules for backward compatibility warns := []check{ - v.mounts, // TODO (runc v1.x.x): make this an error instead of a warning + mounts, // TODO (runc v1.x.x): make this an error instead of a warning } for _, c := range warns { if err := c(config); err != nil { @@ -60,7 +49,7 @@ func (v *ConfigValidator) Validate(config *configs.Config) error { // rootfs validates if the rootfs is an absolute path and is not a symlink // to the container's root filesystem. -func (v *ConfigValidator) rootfs(config *configs.Config) error { +func rootfs(config *configs.Config) error { if _, err := os.Stat(config.Rootfs); err != nil { return fmt.Errorf("invalid rootfs: %w", err) } @@ -77,7 +66,7 @@ func (v *ConfigValidator) rootfs(config *configs.Config) error { return nil } -func (v *ConfigValidator) network(config *configs.Config) error { +func network(config *configs.Config) error { if !config.Namespaces.Contains(configs.NEWNET) { if len(config.Networks) > 0 || len(config.Routes) > 0 { return errors.New("unable to apply network settings without a private NET namespace") @@ -86,14 +75,14 @@ func (v *ConfigValidator) network(config *configs.Config) error { return nil } -func (v *ConfigValidator) hostname(config *configs.Config) error { +func hostname(config *configs.Config) error { if config.Hostname != "" && !config.Namespaces.Contains(configs.NEWUTS) { return errors.New("unable to set hostname without a private UTS namespace") } return nil } -func (v *ConfigValidator) security(config *configs.Config) error { +func security(config *configs.Config) error { // restrict sys without mount namespace if (len(config.MaskPaths) > 0 || len(config.ReadonlyPaths) > 0) && !config.Namespaces.Contains(configs.NEWNS) { @@ -106,7 +95,7 @@ func (v *ConfigValidator) security(config *configs.Config) error { return nil } -func (v *ConfigValidator) usernamespace(config *configs.Config) error { +func namespaces(config *configs.Config) error { if config.Namespaces.Contains(configs.NEWUSER) { if _, err := os.Stat("/proc/self/ns/user"); os.IsNotExist(err) { return errors.New("USER namespaces aren't enabled in the kernel") @@ -116,15 +105,13 @@ func (v *ConfigValidator) usernamespace(config *configs.Config) error { return errors.New("User namespace mappings specified, but USER namespace isn't enabled in the config") } } - return nil -} -func (v *ConfigValidator) cgroupnamespace(config *configs.Config) error { if config.Namespaces.Contains(configs.NEWCGROUP) { if _, err := os.Stat("/proc/self/ns/cgroup"); os.IsNotExist(err) { return errors.New("cgroup namespaces aren't enabled in the kernel") } } + return nil } @@ -161,7 +148,7 @@ func convertSysctlVariableToDotsSeparator(val string) string { // sysctl validates that the specified sysctl keys are valid or not. // /proc/sys isn't completely namespaced and depending on which namespaces // are specified, a subset of sysctls are permitted. -func (v *ConfigValidator) sysctl(config *configs.Config) error { +func sysctl(config *configs.Config) error { validSysctlMap := map[string]bool{ "kernel.msgmax": true, "kernel.msgmnb": true, @@ -227,7 +214,7 @@ func (v *ConfigValidator) sysctl(config *configs.Config) error { return nil } -func (v *ConfigValidator) intelrdt(config *configs.Config) error { +func intelrdtCheck(config *configs.Config) error { if config.IntelRdt != nil { if !intelrdt.IsCATEnabled() && !intelrdt.IsMBAEnabled() { return errors.New("intelRdt is specified in config, but Intel RDT is not supported or enabled") @@ -248,7 +235,7 @@ func (v *ConfigValidator) intelrdt(config *configs.Config) error { return nil } -func (v *ConfigValidator) cgroups(config *configs.Config) error { +func cgroupsCheck(config *configs.Config) error { c := config.Cgroups if c == nil { return nil @@ -277,7 +264,7 @@ func (v *ConfigValidator) cgroups(config *configs.Config) error { return nil } -func (v *ConfigValidator) mounts(config *configs.Config) error { +func mounts(config *configs.Config) error { for _, m := range config.Mounts { if !filepath.IsAbs(m.Destination) { return fmt.Errorf("invalid mount %+v: mount destination not absolute", m) diff --git a/libcontainer/configs/validate/validator_test.go b/libcontainer/configs/validate/validator_test.go index 5181333fb..59a403389 100644 --- a/libcontainer/configs/validate/validator_test.go +++ b/libcontainer/configs/validate/validator_test.go @@ -14,8 +14,7 @@ func TestValidate(t *testing.T) { Rootfs: "/var", } - validator := New() - err := validator.Validate(config) + err := Validate(config) if err != nil { t.Errorf("Expected error to not occur: %+v", err) } @@ -32,8 +31,7 @@ func TestValidateWithInvalidRootfs(t *testing.T) { Rootfs: dir, } - validator := New() - err := validator.Validate(config) + err := Validate(config) if err == nil { t.Error("Expected error to occur but it was nil") } @@ -47,8 +45,7 @@ func TestValidateNetworkWithoutNETNamespace(t *testing.T) { Networks: []*configs.Network{network}, } - validator := New() - err := validator.Validate(config) + err := Validate(config) if err == nil { t.Error("Expected error to occur but it was nil") } @@ -62,8 +59,7 @@ func TestValidateNetworkRoutesWithoutNETNamespace(t *testing.T) { Routes: []*configs.Route{route}, } - validator := New() - err := validator.Validate(config) + err := Validate(config) if err == nil { t.Error("Expected error to occur but it was nil") } @@ -80,8 +76,7 @@ func TestValidateHostname(t *testing.T) { ), } - validator := New() - err := validator.Validate(config) + err := Validate(config) if err != nil { t.Errorf("Expected error to not occur: %+v", err) } @@ -93,8 +88,7 @@ func TestValidateHostnameWithoutUTSNamespace(t *testing.T) { Hostname: "runc", } - validator := New() - err := validator.Validate(config) + err := Validate(config) if err == nil { t.Error("Expected error to occur but it was nil") } @@ -111,8 +105,7 @@ func TestValidateSecurityWithMaskPaths(t *testing.T) { ), } - validator := New() - err := validator.Validate(config) + err := Validate(config) if err != nil { t.Errorf("Expected error to not occur: %+v", err) } @@ -129,8 +122,7 @@ func TestValidateSecurityWithROPaths(t *testing.T) { ), } - validator := New() - err := validator.Validate(config) + err := Validate(config) if err != nil { t.Errorf("Expected error to not occur: %+v", err) } @@ -143,8 +135,7 @@ func TestValidateSecurityWithoutNEWNS(t *testing.T) { ReadonlyPaths: []string{"/proc/sys"}, } - validator := New() - err := validator.Validate(config) + err := Validate(config) if err == nil { t.Error("Expected error to occur but it was nil") } @@ -163,8 +154,7 @@ func TestValidateUsernamespace(t *testing.T) { ), } - validator := New() - err := validator.Validate(config) + err := Validate(config) if err != nil { t.Errorf("expected error to not occur %+v", err) } @@ -177,8 +167,7 @@ func TestValidateUsernamespaceWithoutUserNS(t *testing.T) { UidMappings: []configs.IDMap{uidMap}, } - validator := New() - err := validator.Validate(config) + err := Validate(config) if err == nil { t.Error("Expected error to occur but it was nil") } @@ -226,8 +215,7 @@ func TestValidateSysctl(t *testing.T) { Sysctl: map[string]string{k: v}, } - validator := New() - err := validator.Validate(config) + err := Validate(config) if err == nil { t.Error("Expected error to occur but it was nil") } @@ -259,8 +247,7 @@ func TestValidateValidSysctl(t *testing.T) { }, } - validator := New() - err := validator.Validate(config) + err := Validate(config) if err != nil { t.Errorf("Expected error to not occur with {%s=%s} but got: %q", k, v, err) } @@ -281,8 +268,7 @@ func TestValidateSysctlWithSameNs(t *testing.T) { ), } - validator := New() - err := validator.Validate(config) + err := Validate(config) if err == nil { t.Error("Expected error to occur but it was nil") } @@ -323,8 +309,7 @@ func TestValidateSysctlWithBindHostNetNS(t *testing.T) { ), } - validator := New() - if err := validator.Validate(config); err == nil { + if err := Validate(config); err == nil { t.Error("Expected error to occur but it was nil") } } @@ -336,8 +321,7 @@ func TestValidateSysctlWithoutNETNamespace(t *testing.T) { Namespaces: []configs.Namespace{}, } - validator := New() - err := validator.Validate(config) + err := Validate(config) if err == nil { t.Error("Expected error to occur but it was nil") } @@ -358,8 +342,6 @@ func TestValidateMounts(t *testing.T) { {isErr: false, dest: "/abs/but/../unclean"}, } - validator := New() - for _, tc := range testCases { config := &configs.Config{ Rootfs: "/var", @@ -368,7 +350,7 @@ func TestValidateMounts(t *testing.T) { }, } - err := validator.Validate(config) + err := Validate(config) if tc.isErr && err == nil { t.Errorf("mount dest: %s, expected error, got nil", tc.dest) } diff --git a/libcontainer/factory_linux.go b/libcontainer/factory_linux.go index 13dbeea67..32d008a2c 100644 --- a/libcontainer/factory_linux.go +++ b/libcontainer/factory_linux.go @@ -65,8 +65,7 @@ func New(root string, options ...func(*LinuxFactory) error) (Factory, error) { } } l := &LinuxFactory{ - Root: root, - Validator: validate.New(), + Root: root, } for _, opt := range options { @@ -90,9 +89,6 @@ type LinuxFactory struct { NewuidmapPath string NewgidmapPath string - // Validator provides validation to container configurations. - Validator validate.Validator - // NewIntelRdtManager returns an initialized Intel RDT manager for a single container. NewIntelRdtManager func(config *configs.Config, id string, path string) intelrdt.Manager } @@ -104,7 +100,7 @@ func (l *LinuxFactory) Create(id string, config *configs.Config) (Container, err if err := l.validateID(id); err != nil { return nil, err } - if err := l.Validator.Validate(config); err != nil { + if err := validate.Validate(config); err != nil { return nil, err } containerRoot, err := securejoin.SecureJoin(l.Root, id) diff --git a/libcontainer/specconv/spec_linux_test.go b/libcontainer/specconv/spec_linux_test.go index 56d808699..1068c55a8 100644 --- a/libcontainer/specconv/spec_linux_test.go +++ b/libcontainer/specconv/spec_linux_test.go @@ -540,8 +540,7 @@ func TestSpecconvExampleValidate(t *testing.T) { spec.Process.NoNewPrivileges, config.NoNewPrivileges) } - validator := validate.New() - if err := validator.Validate(config); err != nil { + if err := validate.Validate(config); err != nil { t.Errorf("Expected specconv to produce valid container config: %v", err) } } @@ -562,8 +561,7 @@ func TestSpecconvNoLinuxSection(t *testing.T) { t.Errorf("Couldn't create libcontainer config: %v", err) } - validator := validate.New() - if err := validator.Validate(config); err != nil { + if err := validate.Validate(config); err != nil { t.Errorf("Expected specconv to produce valid container config: %v", err) } } @@ -617,8 +615,7 @@ func TestNonZeroEUIDCompatibleSpecconvValidate(t *testing.T) { t.Errorf("Couldn't create libcontainer config: %v", err) } - validator := validate.New() - if err := validator.Validate(config); err != nil { + if err := validate.Validate(config); err != nil { t.Errorf("Expected specconv to produce valid rootless container config: %v", err) } } From 39bd7b7217db91a9b349738a03960ed7b9390327 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Thu, 27 Jan 2022 10:35:38 -0800 Subject: [PATCH 3/6] libct: Container, Factory: rm newuidmap/newgidmap These were introduced in commit d8b669400 back in 2017, with a TODO of "make binary names configurable". Apparently, everyone is happy with the hardcoded names. In fact, they *are* configurable (by prepending the PATH with a directory containing own version of newuidmap/newgidmap). Now, these binaries are only needed in a few specific cases (when rootless is set etc.), so let's look them up only when needed. Signed-off-by: Kir Kolyshkin --- libcontainer/container_linux.go | 29 +++++++++++++++++------------ libcontainer/factory_linux.go | 27 --------------------------- utils_linux.go | 17 +---------------- 3 files changed, 18 insertions(+), 55 deletions(-) diff --git a/libcontainer/container_linux.go b/libcontainer/container_linux.go index 6e0a5cefe..b285a3175 100644 --- a/libcontainer/container_linux.go +++ b/libcontainer/container_linux.go @@ -43,8 +43,6 @@ type linuxContainer struct { intelRdtManager intelrdt.Manager initProcess parentProcess initProcessStartTime uint64 - newuidmapPath string - newgidmapPath string m sync.Mutex criuVersion int state containerState @@ -2148,11 +2146,16 @@ func (c *linuxContainer) bootstrapData(cloneFlags uintptr, nsMaps map[configs.Na if !joinExistingUser { // write uid mappings if len(c.config.UidMappings) > 0 { - if c.config.RootlessEUID && c.newuidmapPath != "" { - r.AddData(&Bytemsg{ - Type: UidmapPathAttr, - Value: []byte(c.newuidmapPath), - }) + if c.config.RootlessEUID { + // We resolve the paths for new{u,g}idmap from + // the context of runc to avoid doing a path + // lookup in the nsexec context. + if path, err := exec.LookPath("newuidmap"); err == nil { + r.AddData(&Bytemsg{ + Type: UidmapPathAttr, + Value: []byte(path), + }) + } } b, err := encodeIDMapping(c.config.UidMappings) if err != nil { @@ -2174,11 +2177,13 @@ func (c *linuxContainer) bootstrapData(cloneFlags uintptr, nsMaps map[configs.Na Type: GidmapAttr, Value: b, }) - if c.config.RootlessEUID && c.newgidmapPath != "" { - r.AddData(&Bytemsg{ - Type: GidmapPathAttr, - Value: []byte(c.newgidmapPath), - }) + if c.config.RootlessEUID { + if path, err := exec.LookPath("newgidmap"); err == nil { + r.AddData(&Bytemsg{ + Type: GidmapPathAttr, + Value: []byte(path), + }) + } } if requiresRootOrMappingTool(c.config) { r.AddData(&Boolmsg{ diff --git a/libcontainer/factory_linux.go b/libcontainer/factory_linux.go index 32d008a2c..afb0a43fa 100644 --- a/libcontainer/factory_linux.go +++ b/libcontainer/factory_linux.go @@ -84,11 +84,6 @@ type LinuxFactory struct { // Root directory for the factory to store state. Root string - // New{u,g}idmapPath is the path to the binaries used for mapping with - // rootless containers. - NewuidmapPath string - NewgidmapPath string - // NewIntelRdtManager returns an initialized Intel RDT manager for a single container. NewIntelRdtManager func(config *configs.Config, id string, path string) intelrdt.Manager } @@ -154,8 +149,6 @@ func (l *LinuxFactory) Create(id string, config *configs.Config) (Container, err id: id, root: containerRoot, config: config, - newuidmapPath: l.NewuidmapPath, - newgidmapPath: l.NewgidmapPath, cgroupManager: cm, } if l.NewIntelRdtManager != nil { @@ -195,8 +188,6 @@ func (l *LinuxFactory) Load(id string) (Container, error) { initProcessStartTime: state.InitProcessStartTime, id: id, config: &state.Config, - newuidmapPath: l.NewuidmapPath, - newgidmapPath: l.NewgidmapPath, cgroupManager: cm, root: containerRoot, created: state.Created, @@ -327,24 +318,6 @@ func (l *LinuxFactory) validateID(id string) error { return nil } -// NewuidmapPath returns an option func to configure a LinuxFactory with the -// provided .. -func NewuidmapPath(newuidmapPath string) func(*LinuxFactory) error { - return func(l *LinuxFactory) error { - l.NewuidmapPath = newuidmapPath - return nil - } -} - -// NewgidmapPath returns an option func to configure a LinuxFactory with the -// provided .. -func NewgidmapPath(newgidmapPath string) func(*LinuxFactory) error { - return func(l *LinuxFactory) error { - l.NewgidmapPath = newgidmapPath - return nil - } -} - func parseMountFds() ([]int, error) { fdsJSON := os.Getenv("_LIBCONTAINER_MOUNT_FDS") if fdsJSON == "" { diff --git a/utils_linux.go b/utils_linux.go index 9f7619d65..d49b024ec 100644 --- a/utils_linux.go +++ b/utils_linux.go @@ -5,7 +5,6 @@ import ( "fmt" "net" "os" - "os/exec" "path/filepath" "strconv" @@ -34,21 +33,7 @@ func loadFactory(context *cli.Context) (libcontainer.Factory, error) { intelRdtManager := libcontainer.IntelRdtFs - // We resolve the paths for {newuidmap,newgidmap} from the context of runc, - // to avoid doing a path lookup in the nsexec context. TODO: The binary - // names are not currently configurable. - newuidmap, err := exec.LookPath("newuidmap") - if err != nil { - newuidmap = "" - } - newgidmap, err := exec.LookPath("newgidmap") - if err != nil { - newgidmap = "" - } - - return libcontainer.New(abs, intelRdtManager, - libcontainer.NewuidmapPath(newuidmap), - libcontainer.NewgidmapPath(newgidmap)) + return libcontainer.New(abs, intelRdtManager) } // getContainer returns the specified container instance by loading it from state From 9258eac0727f6c6323459c0b0d32924fc1091428 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Thu, 3 Feb 2022 17:23:13 -0800 Subject: [PATCH 4/6] libct/start: use execabs for newuidmap lookup Since we are looking up the path to newuidmap/newgidmap in one context, and executing those in another (libct/nsenter), it might make sense to use a stricter rules for looking up path to those binaries. Practically it means that if someone wants to use custom newuidmap and newgidmap binaries from $PATH, it would be impossible to use these from the current directory by means of PATH=.:$PATH; instead one would have to do something like PATH=$(pwd):$PATH. See https://go.dev/blog/path-security for background. Signed-off-by: Kir Kolyshkin --- libcontainer/container_linux.go | 5 +- vendor/golang.org/x/sys/execabs/execabs.go | 102 +++++++++++++++++++++ vendor/modules.txt | 1 + 3 files changed, 106 insertions(+), 2 deletions(-) create mode 100644 vendor/golang.org/x/sys/execabs/execabs.go diff --git a/libcontainer/container_linux.go b/libcontainer/container_linux.go index b285a3175..4fe133d00 100644 --- a/libcontainer/container_linux.go +++ b/libcontainer/container_linux.go @@ -23,6 +23,7 @@ import ( "github.com/opencontainers/runtime-spec/specs-go" "github.com/sirupsen/logrus" "github.com/vishvananda/netlink/nl" + "golang.org/x/sys/execabs" "golang.org/x/sys/unix" "google.golang.org/protobuf/proto" @@ -2150,7 +2151,7 @@ func (c *linuxContainer) bootstrapData(cloneFlags uintptr, nsMaps map[configs.Na // We resolve the paths for new{u,g}idmap from // the context of runc to avoid doing a path // lookup in the nsexec context. - if path, err := exec.LookPath("newuidmap"); err == nil { + if path, err := execabs.LookPath("newuidmap"); err == nil { r.AddData(&Bytemsg{ Type: UidmapPathAttr, Value: []byte(path), @@ -2178,7 +2179,7 @@ func (c *linuxContainer) bootstrapData(cloneFlags uintptr, nsMaps map[configs.Na Value: b, }) if c.config.RootlessEUID { - if path, err := exec.LookPath("newgidmap"); err == nil { + if path, err := execabs.LookPath("newgidmap"); err == nil { r.AddData(&Bytemsg{ Type: GidmapPathAttr, Value: []byte(path), diff --git a/vendor/golang.org/x/sys/execabs/execabs.go b/vendor/golang.org/x/sys/execabs/execabs.go new file mode 100644 index 000000000..78192498d --- /dev/null +++ b/vendor/golang.org/x/sys/execabs/execabs.go @@ -0,0 +1,102 @@ +// Copyright 2020 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +// Package execabs is a drop-in replacement for os/exec +// that requires PATH lookups to find absolute paths. +// That is, execabs.Command("cmd") runs the same PATH lookup +// as exec.Command("cmd"), but if the result is a path +// which is relative, the Run and Start methods will report +// an error instead of running the executable. +// +// See https://blog.golang.org/path-security for more information +// about when it may be necessary or appropriate to use this package. +package execabs + +import ( + "context" + "fmt" + "os/exec" + "path/filepath" + "reflect" + "unsafe" +) + +// ErrNotFound is the error resulting if a path search failed to find an executable file. +// It is an alias for exec.ErrNotFound. +var ErrNotFound = exec.ErrNotFound + +// Cmd represents an external command being prepared or run. +// It is an alias for exec.Cmd. +type Cmd = exec.Cmd + +// Error is returned by LookPath when it fails to classify a file as an executable. +// It is an alias for exec.Error. +type Error = exec.Error + +// An ExitError reports an unsuccessful exit by a command. +// It is an alias for exec.ExitError. +type ExitError = exec.ExitError + +func relError(file, path string) error { + return fmt.Errorf("%s resolves to executable in current directory (.%c%s)", file, filepath.Separator, path) +} + +// LookPath searches for an executable named file in the directories +// named by the PATH environment variable. If file contains a slash, +// it is tried directly and the PATH is not consulted. The result will be +// an absolute path. +// +// LookPath differs from exec.LookPath in its handling of PATH lookups, +// which are used for file names without slashes. If exec.LookPath's +// PATH lookup would have returned an executable from the current directory, +// LookPath instead returns an error. +func LookPath(file string) (string, error) { + path, err := exec.LookPath(file) + if err != nil { + return "", err + } + if filepath.Base(file) == file && !filepath.IsAbs(path) { + return "", relError(file, path) + } + return path, nil +} + +func fixCmd(name string, cmd *exec.Cmd) { + if filepath.Base(name) == name && !filepath.IsAbs(cmd.Path) { + // exec.Command was called with a bare binary name and + // exec.LookPath returned a path which is not absolute. + // Set cmd.lookPathErr and clear cmd.Path so that it + // cannot be run. + lookPathErr := (*error)(unsafe.Pointer(reflect.ValueOf(cmd).Elem().FieldByName("lookPathErr").Addr().Pointer())) + if *lookPathErr == nil { + *lookPathErr = relError(name, cmd.Path) + } + cmd.Path = "" + } +} + +// CommandContext is like Command but includes a context. +// +// The provided context is used to kill the process (by calling os.Process.Kill) +// if the context becomes done before the command completes on its own. +func CommandContext(ctx context.Context, name string, arg ...string) *exec.Cmd { + cmd := exec.CommandContext(ctx, name, arg...) + fixCmd(name, cmd) + return cmd + +} + +// Command returns the Cmd struct to execute the named program with the given arguments. +// See exec.Command for most details. +// +// Command differs from exec.Command in its handling of PATH lookups, +// which are used when the program name contains no slashes. +// If exec.Command would have returned an exec.Cmd configured to run an +// executable from the current directory, Command instead +// returns an exec.Cmd that will return an error from Start or Run. +func Command(name string, arg ...string) *exec.Cmd { + cmd := exec.Command(name, arg...) + fixCmd(name, cmd) + return cmd +} diff --git a/vendor/modules.txt b/vendor/modules.txt index e6cfb193e..17a6794f6 100644 --- a/vendor/modules.txt +++ b/vendor/modules.txt @@ -72,6 +72,7 @@ github.com/vishvananda/netns golang.org/x/net/bpf # golang.org/x/sys v0.0.0-20211116061358-0a5406a5449c ## explicit +golang.org/x/sys/execabs golang.org/x/sys/internal/unsafeheader golang.org/x/sys/unix golang.org/x/sys/windows From 85932850ec4062da696d93a8f16b0cb3a0ad50b6 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Thu, 9 Dec 2021 11:47:04 -0800 Subject: [PATCH 5/6] libct: rm TestGetContainerStats, mockIntelRdtManager TestGetContainerStats test a function that is smaller than the test itself, and only calls a couple of other functions (which are represented by mocks). It does not make sense to have it. mockIntelRdtManager is only needed for TestGetContainerStats and TestGetContainerState, which basically tests that Path is called. Also, it does not make much sense to have it. Signed-off-by: Kir Kolyshkin --- libcontainer/container_linux_test.go | 112 +-------------------------- 1 file changed, 4 insertions(+), 108 deletions(-) diff --git a/libcontainer/container_linux_test.go b/libcontainer/container_linux_test.go index 3eb6e5aff..27eee624c 100644 --- a/libcontainer/container_linux_test.go +++ b/libcontainer/container_linux_test.go @@ -7,22 +7,15 @@ import ( "github.com/opencontainers/runc/libcontainer/cgroups" "github.com/opencontainers/runc/libcontainer/configs" - "github.com/opencontainers/runc/libcontainer/intelrdt" "github.com/opencontainers/runc/libcontainer/system" ) type mockCgroupManager struct { pids []int allPids []int - stats *cgroups.Stats paths map[string]string } -type mockIntelRdtManager struct { - stats *intelrdt.Stats - path string -} - func (m *mockCgroupManager) GetPids() ([]int, error) { return m.pids, nil } @@ -32,7 +25,7 @@ func (m *mockCgroupManager) GetAllPids() ([]int, error) { } func (m *mockCgroupManager) GetStats() (*cgroups.Stats, error) { - return m.stats, nil + return nil, nil } func (m *mockCgroupManager) Apply(pid int) error { @@ -76,30 +69,6 @@ func (m *mockCgroupManager) GetFreezerState() (configs.FreezerState, error) { return configs.Thawed, nil } -func (m *mockIntelRdtManager) Apply(pid int) error { - return nil -} - -func (m *mockIntelRdtManager) GetStats() (*intelrdt.Stats, error) { - return m.stats, nil -} - -func (m *mockIntelRdtManager) Destroy() error { - return nil -} - -func (m *mockIntelRdtManager) GetPath() string { - return m.path -} - -func (m *mockIntelRdtManager) Set(container *configs.Config) error { - return nil -} - -func (m *mockIntelRdtManager) GetCgroups() (*configs.Cgroup, error) { - return nil, nil -} - type mockProcess struct { _pid int started uint64 @@ -173,61 +142,11 @@ func TestGetContainerPids(t *testing.T) { } } -func TestGetContainerStats(t *testing.T) { - container := &linuxContainer{ - id: "myid", - config: &configs.Config{}, - cgroupManager: &mockCgroupManager{ - pids: []int{1, 2, 3}, - stats: &cgroups.Stats{ - MemoryStats: cgroups.MemoryStats{ - Usage: cgroups.MemoryData{ - Usage: 1024, - }, - }, - }, - }, - intelRdtManager: &mockIntelRdtManager{ - stats: &intelrdt.Stats{ - L3CacheSchema: "L3:0=f;1=f0", - MemBwSchema: "MB:0=20;1=70", - }, - }, - } - stats, err := container.Stats() - if err != nil { - t.Fatal(err) - } - if stats.CgroupStats == nil { - t.Fatal("cgroup stats are nil") - } - if stats.CgroupStats.MemoryStats.Usage.Usage != 1024 { - t.Fatalf("expected memory usage 1024 but received %d", stats.CgroupStats.MemoryStats.Usage.Usage) - } - if intelrdt.IsCATEnabled() { - if stats.IntelRdtStats == nil { - t.Fatal("intel rdt stats are nil") - } - if stats.IntelRdtStats.L3CacheSchema != "L3:0=f;1=f0" { - t.Fatalf("expected L3CacheSchema L3:0=f;1=f0 but received %s", stats.IntelRdtStats.L3CacheSchema) - } - } - if intelrdt.IsMBAEnabled() { - if stats.IntelRdtStats == nil { - t.Fatal("intel rdt stats are nil") - } - if stats.IntelRdtStats.MemBwSchema != "MB:0=20;1=70" { - t.Fatalf("expected MemBwSchema MB:0=20;1=70 but received %s", stats.IntelRdtStats.MemBwSchema) - } - } -} - func TestGetContainerState(t *testing.T) { var ( - pid = os.Getpid() - expectedMemoryPath = "/sys/fs/cgroup/memory/myid" - expectedNetworkPath = fmt.Sprintf("/proc/%d/ns/net", pid) - expectedIntelRdtPath = "/sys/fs/resctrl/myid" + pid = os.Getpid() + expectedMemoryPath = "/sys/fs/cgroup/memory/myid" + expectedNetworkPath = fmt.Sprintf("/proc/%d/ns/net", pid) ) container := &linuxContainer{ id: "myid", @@ -248,24 +167,10 @@ func TestGetContainerState(t *testing.T) { }, cgroupManager: &mockCgroupManager{ pids: []int{1, 2, 3}, - stats: &cgroups.Stats{ - MemoryStats: cgroups.MemoryStats{ - Usage: cgroups.MemoryData{ - Usage: 1024, - }, - }, - }, paths: map[string]string{ "memory": expectedMemoryPath, }, }, - intelRdtManager: &mockIntelRdtManager{ - stats: &intelrdt.Stats{ - L3CacheSchema: "L3:0=f0;1=f", - MemBwSchema: "MB:0=70;1=20", - }, - path: expectedIntelRdtPath, - }, } container.state = &createdState{c: container} state, err := container.State() @@ -285,15 +190,6 @@ func TestGetContainerState(t *testing.T) { if memPath := paths["memory"]; memPath != expectedMemoryPath { t.Fatalf("expected memory path %q but received %q", expectedMemoryPath, memPath) } - if intelrdt.IsCATEnabled() || intelrdt.IsMBAEnabled() { - intelRdtPath := state.IntelRdtPath - if intelRdtPath == "" { - t.Fatal("intel rdt path should not be empty") - } - if intelRdtPath != expectedIntelRdtPath { - t.Fatalf("expected intel rdt path %q but received %q", expectedIntelRdtPath, intelRdtPath) - } - } for _, ns := range container.config.Namespaces { path := state.NamespacePaths[ns.Type] if path == "" { From dbd990d5559733bd0f33e2bc23facd2e866cac53 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Thu, 27 Jan 2022 10:58:18 -0800 Subject: [PATCH 6/6] libct: rm intelrtd.Manager interface, NewIntelRdtManager Remove intelrtd.Manager interface, since we only have a single implementation, and do not expect another one. Rename intelRdtManager to Manager, and modify its users accordingly. Remove NewIntelRdtManager from factory. Remove IntelRdtfs. Instead, make intelrdt.NewManager return nil if the feature is not available. Remove TestFactoryNewIntelRdt as it is now identical to TestFactoryNew. Add internal function newManager to be used for tests (to make sure some testing is done even when the feature is not available in kernel/hardware). Signed-off-by: Kir Kolyshkin --- libcontainer/container_linux.go | 2 +- libcontainer/factory_linux.go | 33 ++++------------- libcontainer/factory_linux_test.go | 24 +------------ libcontainer/intelrdt/intelrdt.go | 49 +++++++++++--------------- libcontainer/intelrdt/intelrdt_test.go | 10 +++--- libcontainer/process_linux.go | 2 +- utils_linux.go | 4 +-- 7 files changed, 36 insertions(+), 88 deletions(-) diff --git a/libcontainer/container_linux.go b/libcontainer/container_linux.go index 4fe133d00..8af9ac2c9 100644 --- a/libcontainer/container_linux.go +++ b/libcontainer/container_linux.go @@ -41,7 +41,7 @@ type linuxContainer struct { root string config *configs.Config cgroupManager cgroups.Manager - intelRdtManager intelrdt.Manager + intelRdtManager *intelrdt.Manager initProcess parentProcess initProcessStartTime uint64 m sync.Mutex diff --git a/libcontainer/factory_linux.go b/libcontainer/factory_linux.go index afb0a43fa..a0ea4fe3f 100644 --- a/libcontainer/factory_linux.go +++ b/libcontainer/factory_linux.go @@ -28,20 +28,6 @@ const ( var idRegex = regexp.MustCompile(`^[\w+-\.]+$`) -// IntelRdtfs is an options func to configure a LinuxFactory to return -// containers that use the Intel RDT "resource control" filesystem to -// create and manage Intel RDT resources (e.g., L3 cache, memory bandwidth). -func IntelRdtFs(l *LinuxFactory) error { - if !intelrdt.IsCATEnabled() && !intelrdt.IsMBAEnabled() { - l.NewIntelRdtManager = nil - } else { - l.NewIntelRdtManager = func(config *configs.Config, id string, path string) intelrdt.Manager { - return intelrdt.NewManager(config, id, path) - } - } - return nil -} - // TmpfsRoot is an option func to mount LinuxFactory.Root to tmpfs. func TmpfsRoot(l *LinuxFactory) error { mounted, err := mountinfo.Mounted(l.Root) @@ -83,9 +69,6 @@ func New(root string, options ...func(*LinuxFactory) error) (Factory, error) { type LinuxFactory struct { // Root directory for the factory to store state. Root string - - // NewIntelRdtManager returns an initialized Intel RDT manager for a single container. - NewIntelRdtManager func(config *configs.Config, id string, path string) intelrdt.Manager } func (l *LinuxFactory) Create(id string, config *configs.Config) (Container, error) { @@ -146,13 +129,11 @@ func (l *LinuxFactory) Create(id string, config *configs.Config) (Container, err return nil, err } c := &linuxContainer{ - id: id, - root: containerRoot, - config: config, - cgroupManager: cm, - } - if l.NewIntelRdtManager != nil { - c.intelRdtManager = l.NewIntelRdtManager(config, id, "") + id: id, + root: containerRoot, + config: config, + cgroupManager: cm, + intelRdtManager: intelrdt.NewManager(config, id, ""), } c.state = &stoppedState{c: c} return c, nil @@ -189,12 +170,10 @@ func (l *LinuxFactory) Load(id string) (Container, error) { id: id, config: &state.Config, cgroupManager: cm, + intelRdtManager: intelrdt.NewManager(&state.Config, id, state.IntelRdtPath), root: containerRoot, created: state.Created, } - if l.NewIntelRdtManager != nil { - c.intelRdtManager = l.NewIntelRdtManager(&state.Config, id, state.IntelRdtPath) - } c.state = &loadedState{c: c} if err := c.refreshState(); err != nil { return nil, err diff --git a/libcontainer/factory_linux_test.go b/libcontainer/factory_linux_test.go index d29c32e9d..47f306995 100644 --- a/libcontainer/factory_linux_test.go +++ b/libcontainer/factory_linux_test.go @@ -37,28 +37,6 @@ func TestFactoryNew(t *testing.T) { } } -func TestFactoryNewIntelRdt(t *testing.T) { - root := t.TempDir() - factory, err := New(root, IntelRdtFs) - if err != nil { - t.Fatal(err) - } - if factory == nil { - t.Fatal("factory should not be nil") - } - lfactory, ok := factory.(*LinuxFactory) - if !ok { - t.Fatal("expected linux factory returned on linux based systems") - } - if lfactory.Root != root { - t.Fatalf("expected factory root to be %q but received %q", root, lfactory.Root) - } - - if factory.Type() != "libcontainer" { - t.Fatalf("unexpected factory type: %q, expected %q", factory.Type(), "libcontainer") - } -} - func TestFactoryNewTmpfs(t *testing.T) { root := t.TempDir() factory, err := New(root, TmpfsRoot) @@ -157,7 +135,7 @@ func TestFactoryLoadContainer(t *testing.T) { if err := marshal(filepath.Join(root, id, stateFilename), expectedState); err != nil { t.Fatal(err) } - factory, err := New(root, IntelRdtFs) + factory, err := New(root) if err != nil { t.Fatal(err) } diff --git a/libcontainer/intelrdt/intelrdt.go b/libcontainer/intelrdt/intelrdt.go index 1fe1ec3e8..3953f930d 100644 --- a/libcontainer/intelrdt/intelrdt.go +++ b/libcontainer/intelrdt/intelrdt.go @@ -146,34 +146,27 @@ import ( * } */ -type Manager interface { - // Applies Intel RDT configuration to the process with the specified pid - Apply(pid int) error - - // Returns statistics for Intel RDT - GetStats() (*Stats, error) - - // Destroys the Intel RDT container-specific 'container_id' group - Destroy() error - - // Returns Intel RDT path to save in a state file and to be able to - // restore the object later - GetPath() string - - // Set Intel RDT "resource control" filesystem as configured. - Set(container *configs.Config) error -} - -// This implements interface Manager -type intelRdtManager struct { +type Manager struct { mu sync.Mutex config *configs.Config id string path string } -func NewManager(config *configs.Config, id string, path string) Manager { - return &intelRdtManager{ +// NewManager returns a new instance of Manager, or nil, if the Intel RDT +// functionality is not available from hardware or not enabled in the kernel. +func NewManager(config *configs.Config, id string, path string) *Manager { + if _, err := Root(); err != nil { + // Intel RDT is not available. + return nil + } + return newManager(config, id, path) +} + +// newManager is the same as NewManager, except it does not check if the feature +// is actually available. Used by unit tests that mock intelrdt paths. +func newManager(config *configs.Config, id string, path string) *Manager { + return &Manager{ config: config, id: id, path: path, @@ -507,7 +500,7 @@ func IsMBAScEnabled() bool { } // Get the path of the clos group in "resource control" filesystem that the container belongs to -func (m *intelRdtManager) getIntelRdtPath() (string, error) { +func (m *Manager) getIntelRdtPath() (string, error) { rootPath, err := Root() if err != nil { return "", err @@ -522,7 +515,7 @@ func (m *intelRdtManager) getIntelRdtPath() (string, error) { } // Applies Intel RDT configuration to the process with the specified pid -func (m *intelRdtManager) Apply(pid int) (err error) { +func (m *Manager) Apply(pid int) (err error) { // If intelRdt is not specified in config, we do nothing if m.config.IntelRdt == nil { return nil @@ -557,7 +550,7 @@ func (m *intelRdtManager) Apply(pid int) (err error) { } // Destroys the Intel RDT container-specific 'container_id' group -func (m *intelRdtManager) Destroy() error { +func (m *Manager) Destroy() error { // Don't remove resctrl group if closid has been explicitly specified. The // group is likely externally managed, i.e. by some other entity than us. // There are probably other containers/tasks sharing the same group. @@ -574,7 +567,7 @@ func (m *intelRdtManager) Destroy() error { // Returns Intel RDT path to save in a state file and to be able to // restore the object later -func (m *intelRdtManager) GetPath() string { +func (m *Manager) GetPath() string { if m.path == "" { m.path, _ = m.getIntelRdtPath() } @@ -582,7 +575,7 @@ func (m *intelRdtManager) GetPath() string { } // Returns statistics for Intel RDT -func (m *intelRdtManager) GetStats() (*Stats, error) { +func (m *Manager) GetStats() (*Stats, error) { // If intelRdt is not specified in config if m.config.IntelRdt == nil { return nil, nil @@ -668,7 +661,7 @@ func (m *intelRdtManager) GetStats() (*Stats, error) { } // Set Intel RDT "resource control" filesystem as configured. -func (m *intelRdtManager) Set(container *configs.Config) error { +func (m *Manager) Set(container *configs.Config) error { // About L3 cache schema: // It has allocation bitmasks/values for L3 cache on each socket, // which contains L3 cache id and capacity bitmask (CBM). diff --git a/libcontainer/intelrdt/intelrdt_test.go b/libcontainer/intelrdt/intelrdt_test.go index 2184a1468..c127cd8f7 100644 --- a/libcontainer/intelrdt/intelrdt_test.go +++ b/libcontainer/intelrdt/intelrdt_test.go @@ -20,7 +20,7 @@ func TestIntelRdtSetL3CacheSchema(t *testing.T) { }) helper.config.IntelRdt.L3CacheSchema = l3CacheSchemeAfter - intelrdt := NewManager(helper.config, "", helper.IntelRdtPath) + intelrdt := newManager(helper.config, "", helper.IntelRdtPath) if err := intelrdt.Set(helper.config); err != nil { t.Fatal(err) } @@ -50,7 +50,7 @@ func TestIntelRdtSetMemBwSchema(t *testing.T) { }) helper.config.IntelRdt.MemBwSchema = memBwSchemeAfter - intelrdt := NewManager(helper.config, "", helper.IntelRdtPath) + intelrdt := newManager(helper.config, "", helper.IntelRdtPath) if err := intelrdt.Set(helper.config); err != nil { t.Fatal(err) } @@ -80,7 +80,7 @@ func TestIntelRdtSetMemBwScSchema(t *testing.T) { }) helper.config.IntelRdt.MemBwSchema = memBwScSchemeAfter - intelrdt := NewManager(helper.config, "", helper.IntelRdtPath) + intelrdt := newManager(helper.config, "", helper.IntelRdtPath) if err := intelrdt.Set(helper.config); err != nil { t.Fatal(err) } @@ -103,7 +103,7 @@ func TestApply(t *testing.T) { const closID = "test-clos" helper.config.IntelRdt.ClosID = closID - intelrdt := NewManager(helper.config, "", helper.IntelRdtPath) + intelrdt := newManager(helper.config, "", helper.IntelRdtPath) if err := intelrdt.Apply(1234); err == nil { t.Fatal("unexpected success when applying pid") } @@ -112,7 +112,7 @@ func TestApply(t *testing.T) { } // Dir should be created if some schema has been specified - intelrdt.(*intelRdtManager).config.IntelRdt.L3CacheSchema = "L3:0=f" + intelrdt.config.IntelRdt.L3CacheSchema = "L3:0=f" if err := intelrdt.Apply(1235); err != nil { t.Fatalf("Apply() failed: %v", err) } diff --git a/libcontainer/process_linux.go b/libcontainer/process_linux.go index 97f28a1ab..ae91d5b90 100644 --- a/libcontainer/process_linux.go +++ b/libcontainer/process_linux.go @@ -303,7 +303,7 @@ type initProcess struct { logFilePair filePair config *initConfig manager cgroups.Manager - intelRdtManager intelrdt.Manager + intelRdtManager *intelrdt.Manager container *linuxContainer fds []string process *Process diff --git a/utils_linux.go b/utils_linux.go index d49b024ec..c2214a233 100644 --- a/utils_linux.go +++ b/utils_linux.go @@ -31,9 +31,7 @@ func loadFactory(context *cli.Context) (libcontainer.Factory, error) { return nil, err } - intelRdtManager := libcontainer.IntelRdtFs - - return libcontainer.New(abs, intelRdtManager) + return libcontainer.New(abs) } // getContainer returns the specified container instance by loading it from state