From 82498e3d7706cb45e09bfc8a38e2a10aadc26dd3 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Thu, 24 Jun 2021 17:06:56 -0700 Subject: [PATCH 1/6] libct/specconf: remove unneeded checks In cases we have something like if y != "" { x = y } where both x and y are strings, and x was not set before, it makes no sense to have a condition, as such code is equivalent to mere x = y Simplify such cases by removing "if". Signed-off-by: Kir Kolyshkin --- libcontainer/specconv/spec_linux.go | 21 ++++++--------------- 1 file changed, 6 insertions(+), 15 deletions(-) diff --git a/libcontainer/specconv/spec_linux.go b/libcontainer/specconv/spec_linux.go index cbcb7d14059..89172316358 100644 --- a/libcontainer/specconv/spec_linux.go +++ b/libcontainer/specconv/spec_linux.go @@ -300,12 +300,9 @@ func CreateLibcontainerConfig(opts *CreateOpts) (*configs.Config, error) { config.Seccomp = seccomp } if spec.Linux.IntelRdt != nil { - config.IntelRdt = &configs.IntelRdt{} - if spec.Linux.IntelRdt.L3CacheSchema != "" { - config.IntelRdt.L3CacheSchema = spec.Linux.IntelRdt.L3CacheSchema - } - if spec.Linux.IntelRdt.MemBwSchema != "" { - config.IntelRdt.MemBwSchema = spec.Linux.IntelRdt.MemBwSchema + config.IntelRdt = &configs.IntelRdt{ + L3CacheSchema: spec.Linux.IntelRdt.L3CacheSchema, + MemBwSchema: spec.Linux.IntelRdt.MemBwSchema, } } } @@ -313,9 +310,7 @@ func CreateLibcontainerConfig(opts *CreateOpts) (*configs.Config, error) { config.OomScoreAdj = spec.Process.OOMScoreAdj config.NoNewPrivileges = spec.Process.NoNewPrivileges config.Umask = spec.Process.User.Umask - if spec.Process.SelinuxLabel != "" { - config.ProcessLabel = spec.Process.SelinuxLabel - } + config.ProcessLabel = spec.Process.SelinuxLabel if spec.Process.Capabilities != nil { config.Capabilities = &configs.Capabilities{ Bounding: spec.Process.Capabilities.Bounding, @@ -551,12 +546,8 @@ func CreateCgroupConfig(opts *CreateOpts, defaultDevs []*devices.Device) (*confi if r.CPU.RealtimePeriod != nil { c.Resources.CpuRtPeriod = *r.CPU.RealtimePeriod } - if r.CPU.Cpus != "" { - c.Resources.CpusetCpus = r.CPU.Cpus - } - if r.CPU.Mems != "" { - c.Resources.CpusetMems = r.CPU.Mems - } + c.Resources.CpusetCpus = r.CPU.Cpus + c.Resources.CpusetMems = r.CPU.Mems } if r.Pids != nil { c.Resources.PidsLimit = r.Pids.Limit From feff2c451ecd2d27ac176da522380a472848bd2e Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Thu, 24 Jun 2021 17:14:06 -0700 Subject: [PATCH 2/6] libct/intelrdt: fix potential nil dereference In case getIntelRdtData() returns an error, d is set to nil. In case the error returned is of NotFoundError type (which happens if resctlr mount is not found in /proc/self/mountinfo), the function proceeds to call d.join(), resulting in a nil deref and a panic. In practice, this never happens in runc because of the checks in intelrdt() function in libcontainer/configs/validate, which raises an error in case any of the parameters are set in config but the IntelRTD itself is not available (that includes checking that the mount point is there). Nevertheless, the code is wrong, and can result in nil dereference if some external users uses Apply on a system without resctrl mount. Fix this by removing the exclusion. Signed-off-by: Kir Kolyshkin --- libcontainer/intelrdt/intelrdt.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libcontainer/intelrdt/intelrdt.go b/libcontainer/intelrdt/intelrdt.go index bc01310e789..0ab762300f8 100644 --- a/libcontainer/intelrdt/intelrdt.go +++ b/libcontainer/intelrdt/intelrdt.go @@ -547,7 +547,7 @@ func (m *intelRdtManager) Apply(pid int) (err error) { return nil } d, err := getIntelRdtData(m.config, pid) - if err != nil && !IsNotFound(err) { + if err != nil { return err } From e0ce428bced7c96265a57373e5c6002d394a0679 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Thu, 24 Jun 2021 17:39:32 -0700 Subject: [PATCH 3/6] libct/intelrdt: remove NotFoundError type Initially, this was copied over from libcontainer/cgroups, where it made sense as for cgroup v1 we have multiple controllers and mount points. Here, we only have a single mount, so there's no need for the whole type. Replace all that with a simple error (which is currently internal since the only user is our own test case). Signed-off-by: Kir Kolyshkin --- libcontainer/intelrdt/intelrdt.go | 23 +++-------------------- libcontainer/intelrdt/intelrdt_test.go | 5 +++-- 2 files changed, 6 insertions(+), 22 deletions(-) diff --git a/libcontainer/intelrdt/intelrdt.go b/libcontainer/intelrdt/intelrdt.go index 0ab762300f8..8900983c99d 100644 --- a/libcontainer/intelrdt/intelrdt.go +++ b/libcontainer/intelrdt/intelrdt.go @@ -200,6 +200,8 @@ var ( // For Intel RDT initialization initOnce sync.Once + + errNotFound = errors.New("Intel RDT resctrl mount point not found") ) type intelRdtData struct { @@ -273,7 +275,7 @@ func findIntelRdtMountpointDir(f io.Reader) (string, error) { return "", err } if len(mi) < 1 { - return "", NewNotFoundError("Intel RDT") + return "", errNotFound } // Check if MBA Software Controller is enabled through mount option "-o mba_MBps" @@ -757,25 +759,6 @@ func (raw *intelRdtData) join(id string) (string, error) { return path, nil } -type NotFoundError struct { - ResourceControl string -} - -func (e *NotFoundError) Error() string { - return fmt.Sprintf("mountpoint for %s not found", e.ResourceControl) -} - -func NewNotFoundError(res string) error { - return &NotFoundError{ - ResourceControl: res, - } -} - -func IsNotFound(err error) bool { - var nfErr *NotFoundError - return errors.As(err, &nfErr) -} - type LastCmdError struct { LastCmdStatus string Err error diff --git a/libcontainer/intelrdt/intelrdt_test.go b/libcontainer/intelrdt/intelrdt_test.go index ef7abd33f5c..b3f6ed97c97 100644 --- a/libcontainer/intelrdt/intelrdt_test.go +++ b/libcontainer/intelrdt/intelrdt_test.go @@ -3,6 +3,7 @@ package intelrdt import ( + "errors" "io" "strings" "testing" @@ -223,8 +224,8 @@ func TestFindIntelRdtMountpointDir(t *testing.T) { mbaScEnabled = false mp, err := findIntelRdtMountpointDir(tc.input) if tc.isNotFoundError { - if !IsNotFound(err) { - t.Errorf("expected IsNotFound error, got %+v", err) + if !errors.Is(err, errNotFound) { + t.Errorf("expected errNotFound error, got %+v", err) } return } From 00d156296718500f8c307e3dcb77cb37dd8e9ed2 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Thu, 24 Jun 2021 19:05:36 -0700 Subject: [PATCH 4/6] libct/intelrdt: simplify NewLastCmdError For errors that only have a string and an underlying error, using fmt.Errorf with %w to wrap an error is sufficient. In this particular case, the code is simplified, and now we have unwrappable errors as a bonus (same could be achieved by adding (*LastCmdError).Unwrap() method, but that's adding more code). Signed-off-by: Kir Kolyshkin --- libcontainer/intelrdt/intelrdt.go | 16 ++-------------- 1 file changed, 2 insertions(+), 14 deletions(-) diff --git a/libcontainer/intelrdt/intelrdt.go b/libcontainer/intelrdt/intelrdt.go index 8900983c99d..3b61a70c373 100644 --- a/libcontainer/intelrdt/intelrdt.go +++ b/libcontainer/intelrdt/intelrdt.go @@ -759,22 +759,10 @@ func (raw *intelRdtData) join(id string) (string, error) { return path, nil } -type LastCmdError struct { - LastCmdStatus string - Err error -} - -func (e *LastCmdError) Error() string { - return e.Err.Error() + ", last_cmd_status: " + e.LastCmdStatus -} - func NewLastCmdError(err error) error { - lastCmdStatus, err1 := getLastCmdStatus() + status, err1 := getLastCmdStatus() if err1 == nil { - return &LastCmdError{ - LastCmdStatus: lastCmdStatus, - Err: err, - } + return fmt.Errorf("%w, last_cmd_status: %s", err, status) } return err } From 8f8dfc498a39e4b91711abfa9de43585828bca70 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Thu, 24 Jun 2021 19:12:28 -0700 Subject: [PATCH 5/6] libcontainer/intelrdt: move NewLastCmdError down ... the stack, so every caller will automatically benefit from it. The only change that it causes is the user in libcontainer/process_linux.go will get a better error message. [v2: typo fix] Signed-off-by: Kir Kolyshkin --- libcontainer/intelrdt/intelrdt.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/libcontainer/intelrdt/intelrdt.go b/libcontainer/intelrdt/intelrdt.go index 3b61a70c373..bca72db8701 100644 --- a/libcontainer/intelrdt/intelrdt.go +++ b/libcontainer/intelrdt/intelrdt.go @@ -400,7 +400,7 @@ func writeFile(dir, file, data string) error { return fmt.Errorf("no such directory for %s", file) } if err := ioutil.WriteFile(filepath.Join(dir, file), []byte(data+"\n"), 0o600); err != nil { - return fmt.Errorf("failed to write %v: %w", data, err) + return NewLastCmdError(fmt.Errorf("intelrdt: unable to write %v: %w", data, err)) } return nil } @@ -507,7 +507,7 @@ func WriteIntelRdtTasks(dir string, pid int) error { // Don't attach any pid if -1 is specified as a pid if pid != -1 { if err := ioutil.WriteFile(filepath.Join(dir, IntelRdtTasks), []byte(strconv.Itoa(pid)), 0o600); err != nil { - return fmt.Errorf("failed to write %v: %w", pid, err) + return NewLastCmdError(fmt.Errorf("intelrdt: unable to add pid %d: %w", pid, err)) } } return nil @@ -725,21 +725,21 @@ func (m *intelRdtManager) Set(container *configs.Config) error { // Write a single joint schema string to schemata file if l3CacheSchema != "" && memBwSchema != "" { if err := writeFile(path, "schemata", l3CacheSchema+"\n"+memBwSchema); err != nil { - return NewLastCmdError(err) + return err } } // Write only L3 cache schema string to schemata file if l3CacheSchema != "" && memBwSchema == "" { if err := writeFile(path, "schemata", l3CacheSchema); err != nil { - return NewLastCmdError(err) + return err } } // Write only memory bandwidth schema string to schemata file if l3CacheSchema == "" && memBwSchema != "" { if err := writeFile(path, "schemata", memBwSchema); err != nil { - return NewLastCmdError(err) + return err } } } @@ -754,7 +754,7 @@ func (raw *intelRdtData) join(id string) (string, error) { } if err := WriteIntelRdtTasks(path, raw.pid); err != nil { - return "", NewLastCmdError(err) + return "", err } return path, nil } From 0229a77a801cd5d92b90b775735160114b5f21ff Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Thu, 24 Jun 2021 19:17:31 -0700 Subject: [PATCH 6/6] libcontainer/intelrdt: privatize some ids These are not used anywhere outside of the package (I have also checked the only external user of the package (github.com/google/cadvisor). No changes other than changing the case. The following identifiers are now private: * IntelRdtTasks * NewLastCmdError * NewStats Brought to you by gorename. Signed-off-by: Kir Kolyshkin --- libcontainer/intelrdt/intelrdt.go | 16 ++++++++-------- libcontainer/intelrdt/stats.go | 2 +- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/libcontainer/intelrdt/intelrdt.go b/libcontainer/intelrdt/intelrdt.go index bca72db8701..9c32aabd337 100644 --- a/libcontainer/intelrdt/intelrdt.go +++ b/libcontainer/intelrdt/intelrdt.go @@ -183,7 +183,7 @@ func NewManager(config *configs.Config, id string, path string) Manager { } const ( - IntelRdtTasks = "tasks" + intelRdtTasks = "tasks" ) var ( @@ -400,7 +400,7 @@ func writeFile(dir, file, data string) error { return fmt.Errorf("no such directory for %s", file) } if err := ioutil.WriteFile(filepath.Join(dir, file), []byte(data+"\n"), 0o600); err != nil { - return NewLastCmdError(fmt.Errorf("intelrdt: unable to write %v: %w", data, err)) + return newLastCmdError(fmt.Errorf("intelrdt: unable to write %v: %w", data, err)) } return nil } @@ -501,13 +501,13 @@ func getLastCmdStatus() (string, error) { // WriteIntelRdtTasks writes the specified pid into the "tasks" file func WriteIntelRdtTasks(dir string, pid int) error { if dir == "" { - return fmt.Errorf("no such directory for %s", IntelRdtTasks) + return fmt.Errorf("no such directory for %s", intelRdtTasks) } // Don't attach any pid if -1 is specified as a pid if pid != -1 { - if err := ioutil.WriteFile(filepath.Join(dir, IntelRdtTasks), []byte(strconv.Itoa(pid)), 0o600); err != nil { - return NewLastCmdError(fmt.Errorf("intelrdt: unable to add pid %d: %w", pid, err)) + if err := ioutil.WriteFile(filepath.Join(dir, intelRdtTasks), []byte(strconv.Itoa(pid)), 0o600); err != nil { + return newLastCmdError(fmt.Errorf("intelrdt: unable to add pid %d: %w", pid, err)) } } return nil @@ -593,7 +593,7 @@ func (m *intelRdtManager) GetStats() (*Stats, error) { m.mu.Lock() defer m.mu.Unlock() - stats := NewStats() + stats := newStats() rootPath, err := getIntelRdtRoot() if err != nil { @@ -750,7 +750,7 @@ func (m *intelRdtManager) Set(container *configs.Config) error { func (raw *intelRdtData) join(id string) (string, error) { path := filepath.Join(raw.root, id) if err := os.MkdirAll(path, 0o755); err != nil { - return "", NewLastCmdError(err) + return "", newLastCmdError(err) } if err := WriteIntelRdtTasks(path, raw.pid); err != nil { @@ -759,7 +759,7 @@ func (raw *intelRdtData) join(id string) (string, error) { return path, nil } -func NewLastCmdError(err error) error { +func newLastCmdError(err error) error { status, err1 := getLastCmdStatus() if err1 == nil { return fmt.Errorf("%w, last_cmd_status: %s", err, status) diff --git a/libcontainer/intelrdt/stats.go b/libcontainer/intelrdt/stats.go index 70df0d14e6c..4f3bd84eaf1 100644 --- a/libcontainer/intelrdt/stats.go +++ b/libcontainer/intelrdt/stats.go @@ -54,6 +54,6 @@ type Stats struct { CMTStats *[]CMTNumaNodeStats `json:"cmt_stats,omitempty"` } -func NewStats() *Stats { +func newStats() *Stats { return &Stats{} }