From badd60d3f6e9f68c812a932b022d2c072522c524 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Tue, 23 Mar 2021 11:00:24 +0100 Subject: [PATCH 1/6] sys: un-export runningPrivileged(), remove runningUnprivileged() These are currently only used inside this package, so we might as well un-export them until we need them elsewhere. Also updated SetOOMScore() to first check for privileged; check for privileged looks to be the "faster" path, and checking it first could (in case of non- privileged) save having to read and parse /proc/self/uid_map. Signed-off-by: Sebastiaan van Stijn --- sys/env.go | 33 --------------------------------- sys/mount_linux_test.go | 2 +- sys/oom_linux.go | 9 ++++++++- sys/oom_linux_test.go | 4 ++-- 4 files changed, 11 insertions(+), 37 deletions(-) delete mode 100644 sys/env.go diff --git a/sys/env.go b/sys/env.go deleted file mode 100644 index 8450d6275..000000000 --- a/sys/env.go +++ /dev/null @@ -1,33 +0,0 @@ -// +build !windows - -/* - Copyright The containerd Authors. - - Licensed under the Apache License, Version 2.0 (the "License"); - you may not use this file except in compliance with the License. - You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - - Unless required by applicable law or agreed to in writing, software - distributed under the License is distributed on an "AS IS" BASIS, - WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - See the License for the specific language governing permissions and - limitations under the License. -*/ - -package sys - -import "golang.org/x/sys/unix" - -// RunningPrivileged returns true if the effective user ID of the -// calling process is 0 -func RunningPrivileged() bool { - return unix.Geteuid() == 0 -} - -// RunningUnprivileged returns true if the effective user ID of the -// calling process is not 0 -func RunningUnprivileged() bool { - return !RunningPrivileged() -} diff --git a/sys/mount_linux_test.go b/sys/mount_linux_test.go index 8210c837e..38745f497 100644 --- a/sys/mount_linux_test.go +++ b/sys/mount_linux_test.go @@ -32,7 +32,7 @@ import ( type fMountatCaseFunc func(t *testing.T, root string) func TestFMountat(t *testing.T) { - if RunningUnprivileged() { + if !runningPrivileged() { t.Skip("Needs to be run as root") return } diff --git a/sys/oom_linux.go b/sys/oom_linux.go index f24e9af97..daa0c74d2 100644 --- a/sys/oom_linux.go +++ b/sys/oom_linux.go @@ -24,6 +24,7 @@ import ( "strings" "github.com/containerd/containerd/pkg/userns" + "golang.org/x/sys/unix" ) const ( @@ -42,7 +43,7 @@ func SetOOMScore(pid, score int) error { } defer f.Close() if _, err = f.WriteString(strconv.Itoa(score)); err != nil { - if os.IsPermission(err) && (userns.RunningInUserNS() || RunningUnprivileged()) { + if os.IsPermission(err) && (!runningPrivileged() || userns.RunningInUserNS()) { return nil } return err @@ -59,3 +60,9 @@ func GetOOMScoreAdj(pid int) (int, error) { } return strconv.Atoi(strings.TrimSpace(string(data))) } + +// runningPrivileged returns true if the effective user ID of the +// calling process is 0 +func runningPrivileged() bool { + return unix.Geteuid() == 0 +} diff --git a/sys/oom_linux_test.go b/sys/oom_linux_test.go index 2b824eb70..cc9d123ef 100644 --- a/sys/oom_linux_test.go +++ b/sys/oom_linux_test.go @@ -37,7 +37,7 @@ func TestSetPositiveOomScoreAdjustment(t *testing.T) { } func TestSetNegativeOomScoreAdjustmentWhenPrivileged(t *testing.T) { - if RunningUnprivileged() { + if !runningPrivileged() { t.Skip("Needs to be run as root") return } @@ -51,7 +51,7 @@ func TestSetNegativeOomScoreAdjustmentWhenPrivileged(t *testing.T) { } func TestSetNegativeOomScoreAdjustmentWhenUnprivilegedHasNoEffect(t *testing.T) { - if RunningPrivileged() { + if runningPrivileged() { t.Skip("Needs to be run as non-root") return } From 6e727152261bd22fbf7da87392cf2e3c7dc16df5 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Tue, 23 Mar 2021 11:20:23 +0100 Subject: [PATCH 2/6] sys: add missing pre-condition checks in tests SetOOMScore requires both privileged (root) and non-user namespace, for negative values, so adjust the pre-conditions accordingly. Signed-off-by: Sebastiaan van Stijn --- sys/oom_linux_test.go | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/sys/oom_linux_test.go b/sys/oom_linux_test.go index cc9d123ef..dcc0ceddd 100644 --- a/sys/oom_linux_test.go +++ b/sys/oom_linux_test.go @@ -23,11 +23,13 @@ import ( "testing" "time" + "github.com/containerd/containerd/pkg/userns" "gotest.tools/v3/assert" is "gotest.tools/v3/assert/cmp" ) func TestSetPositiveOomScoreAdjustment(t *testing.T) { + // Setting a *positive* OOM score adjust does not require privileged _, adjustment, err := adjustOom(123) if err != nil { t.Error(err) @@ -37,8 +39,8 @@ func TestSetPositiveOomScoreAdjustment(t *testing.T) { } func TestSetNegativeOomScoreAdjustmentWhenPrivileged(t *testing.T) { - if !runningPrivileged() { - t.Skip("Needs to be run as root") + if !runningPrivileged() || userns.RunningInUserNS() { + t.Skip("requires root and not running in user namespace") return } @@ -51,8 +53,8 @@ func TestSetNegativeOomScoreAdjustmentWhenPrivileged(t *testing.T) { } func TestSetNegativeOomScoreAdjustmentWhenUnprivilegedHasNoEffect(t *testing.T) { - if runningPrivileged() { - t.Skip("Needs to be run as non-root") + if runningPrivileged() && !userns.RunningInUserNS() { + t.Skip("needs to be run as non-root or in user namespace") return } From ace1912bbaf468c0b960df2191a375b2b48823a5 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Tue, 23 Mar 2021 11:35:05 +0100 Subject: [PATCH 3/6] sys: use assert for error checks in OOM tests Slightly easier to read, and we were already using assert in this file Signed-off-by: Sebastiaan van Stijn --- sys/oom_linux_test.go | 15 +++------------ 1 file changed, 3 insertions(+), 12 deletions(-) diff --git a/sys/oom_linux_test.go b/sys/oom_linux_test.go index dcc0ceddd..adb655320 100644 --- a/sys/oom_linux_test.go +++ b/sys/oom_linux_test.go @@ -31,10 +31,7 @@ import ( func TestSetPositiveOomScoreAdjustment(t *testing.T) { // Setting a *positive* OOM score adjust does not require privileged _, adjustment, err := adjustOom(123) - if err != nil { - t.Error(err) - return - } + assert.NilError(t, err) assert.Check(t, is.Equal(adjustment, 123)) } @@ -45,10 +42,7 @@ func TestSetNegativeOomScoreAdjustmentWhenPrivileged(t *testing.T) { } _, adjustment, err := adjustOom(-123) - if err != nil { - t.Error(err) - return - } + assert.NilError(t, err) assert.Check(t, is.Equal(adjustment, -123)) } @@ -59,10 +53,7 @@ func TestSetNegativeOomScoreAdjustmentWhenUnprivilegedHasNoEffect(t *testing.T) } initial, adjustment, err := adjustOom(-123) - if err != nil { - t.Error(err) - return - } + assert.NilError(t, err) assert.Check(t, is.Equal(adjustment, initial)) } From 44240116aa8b4d9e9846a916e94476aef37d60ae Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Tue, 23 Mar 2021 11:43:00 +0100 Subject: [PATCH 4/6] sys: add boundary checks to SetOOMScore() Produce a more user-friendly error in the odd case we would try to set a score out of range Signed-off-by: Sebastiaan van Stijn --- sys/oom_linux.go | 7 ++++++- sys/oom_linux_test.go | 23 +++++++++++++++++++++++ 2 files changed, 29 insertions(+), 1 deletion(-) diff --git a/sys/oom_linux.go b/sys/oom_linux.go index daa0c74d2..9f9586580 100644 --- a/sys/oom_linux.go +++ b/sys/oom_linux.go @@ -30,12 +30,17 @@ import ( const ( // OOMScoreMaxKillable is the maximum score keeping the process killable by the oom killer OOMScoreMaxKillable = -999 - // OOMScoreAdjMax is from OOM_SCORE_ADJ_MAX https://github.com/torvalds/linux/blob/master/include/uapi/linux/oom.h + // OOMScoreAdjMin is from OOM_SCORE_ADJ_MIN https://github.com/torvalds/linux/blob/v5.10/include/uapi/linux/oom.h#L9 + OOMScoreAdjMin = -1000 + // OOMScoreAdjMax is from OOM_SCORE_ADJ_MAX https://github.com/torvalds/linux/blob/v5.10/include/uapi/linux/oom.h#L10 OOMScoreAdjMax = 1000 ) // SetOOMScore sets the oom score for the provided pid func SetOOMScore(pid, score int) error { + if score > OOMScoreAdjMax || score < OOMScoreAdjMin { + return fmt.Errorf("value out of range (%d): OOM score must be between %d and %d", score, OOMScoreAdjMin, OOMScoreAdjMax) + } path := fmt.Sprintf("/proc/%d/oom_score_adj", pid) f, err := os.OpenFile(path, os.O_WRONLY, 0) if err != nil { diff --git a/sys/oom_linux_test.go b/sys/oom_linux_test.go index adb655320..bcbb1bc04 100644 --- a/sys/oom_linux_test.go +++ b/sys/oom_linux_test.go @@ -18,6 +18,7 @@ package sys import ( "errors" + "fmt" "os" "os/exec" "testing" @@ -57,6 +58,28 @@ func TestSetNegativeOomScoreAdjustmentWhenUnprivilegedHasNoEffect(t *testing.T) assert.Check(t, is.Equal(adjustment, initial)) } +func TestSetOOMScoreBoundaries(t *testing.T) { + err := SetOOMScore(0, OOMScoreAdjMax+1) + assert.ErrorContains(t, err, fmt.Sprintf("value out of range (%d): OOM score must be between", OOMScoreAdjMax+1)) + + err = SetOOMScore(0, OOMScoreAdjMin-1) + assert.ErrorContains(t, err, fmt.Sprintf("value out of range (%d): OOM score must be between", OOMScoreAdjMin-1)) + + _, adjustment, err := adjustOom(OOMScoreAdjMax) + assert.NilError(t, err) + assert.Check(t, is.Equal(adjustment, OOMScoreAdjMax)) + + score, err := GetOOMScoreAdj(os.Getpid()) + assert.NilError(t, err) + if score == 0 || score == OOMScoreAdjMin { + // we won't be able to set the score lower than the parent process, + // so only test if parent process does not have a oom-score-adj + _, adjustment, err = adjustOom(OOMScoreAdjMin) + assert.NilError(t, err) + assert.Check(t, is.Equal(adjustment, OOMScoreAdjMin)) + } +} + func adjustOom(adjustment int) (int, int, error) { cmd := exec.Command("sleep", "100") if err := cmd.Start(); err != nil { From 91e7d21ee825b6a2d04ef75d1a4a9783f3f4e9c2 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Tue, 23 Mar 2021 11:50:32 +0100 Subject: [PATCH 5/6] sys: add AdjustOOMScore() utility Handle the limits in this function so that consumers don't have to perform the boundary checks. Signed-off-by: Sebastiaan van Stijn --- runtime/v1/shim/client/client.go | 5 +---- runtime/v2/shim/util_unix.go | 5 +---- sys/oom_linux.go | 11 +++++++++++ sys/oom_unsupported.go | 8 ++++++++ 4 files changed, 21 insertions(+), 8 deletions(-) diff --git a/runtime/v1/shim/client/client.go b/runtime/v1/shim/client/client.go index 9211c0052..a8afb0e48 100644 --- a/runtime/v1/shim/client/client.go +++ b/runtime/v1/shim/client/client.go @@ -182,10 +182,7 @@ func setupOOMScore(shimPid int) error { return errors.Wrap(err, "get daemon OOM score") } shimScore := score + 1 - if shimScore > sys.OOMScoreAdjMax { - shimScore = sys.OOMScoreAdjMax - } - if err := sys.SetOOMScore(shimPid, shimScore); err != nil { + if err := sys.AdjustOOMScore(shimPid, shimScore); err != nil { return errors.Wrap(err, "set shim OOM score") } return nil diff --git a/runtime/v2/shim/util_unix.go b/runtime/v2/shim/util_unix.go index f4c7cfa55..9c505707e 100644 --- a/runtime/v2/shim/util_unix.go +++ b/runtime/v2/shim/util_unix.go @@ -61,10 +61,7 @@ func AdjustOOMScore(pid int) error { return errors.Wrap(err, "get parent OOM score") } shimScore := score + 1 - if shimScore > sys.OOMScoreAdjMax { - shimScore = sys.OOMScoreAdjMax - } - if err := sys.SetOOMScore(pid, shimScore); err != nil { + if err := sys.AdjustOOMScore(pid, shimScore); err != nil { return errors.Wrap(err, "set shim OOM score") } return nil diff --git a/sys/oom_linux.go b/sys/oom_linux.go index 9f9586580..8e1e3f2f9 100644 --- a/sys/oom_linux.go +++ b/sys/oom_linux.go @@ -36,6 +36,17 @@ const ( OOMScoreAdjMax = 1000 ) +// AdjustOOMScore sets the oom score for the provided pid. If the provided score +// is out of range (-1000 - 1000), it is clipped to the min/max value. +func AdjustOOMScore(pid, score int) error { + if score > OOMScoreAdjMax { + score = OOMScoreAdjMax + } else if score < OOMScoreAdjMin { + score = OOMScoreAdjMin + } + return SetOOMScore(pid, score) +} + // SetOOMScore sets the oom score for the provided pid func SetOOMScore(pid, score int) error { if score > OOMScoreAdjMax || score < OOMScoreAdjMin { diff --git a/sys/oom_unsupported.go b/sys/oom_unsupported.go index 558340790..f5d7e9786 100644 --- a/sys/oom_unsupported.go +++ b/sys/oom_unsupported.go @@ -25,6 +25,14 @@ const ( OOMScoreAdjMax = 0 ) +// AdjustOOMScore sets the oom score for the provided pid. If the provided score +// is out of range (-1000 - 1000), it is clipped to the min/max value. +// +// Not implemented on Windows +func AdjustOOMScore(pid, score int) error { + return nil +} + // SetOOMScore sets the oom score for the process // // Not implemented on Windows From 7bb73da6b9f19132d7d0747c1fca3b7ef8d8fde2 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Fri, 2 Apr 2021 12:26:13 +0200 Subject: [PATCH 6/6] runtime/v2/shim: remove unused SetScore() and remove sys.OOMScoreMaxKillable The shim.SetScore() utility was no longer used since 7dfc605fc6d36dd7327420b8956ac3f9ac71f190. Checking for uses outside of this repository, I found only one external use of this in gVisor; https://github.com/google/gvisor/blob/a9441aea2780da8c93da1c73da860219f98438de/pkg/shim/service.go#L262-L264 Signed-off-by: Sebastiaan van Stijn --- runtime/v2/shim/util_unix.go | 5 ----- sys/oom_linux.go | 2 -- 2 files changed, 7 deletions(-) diff --git a/runtime/v2/shim/util_unix.go b/runtime/v2/shim/util_unix.go index 9c505707e..f956b0986 100644 --- a/runtime/v2/shim/util_unix.go +++ b/runtime/v2/shim/util_unix.go @@ -46,11 +46,6 @@ func getSysProcAttr() *syscall.SysProcAttr { } } -// SetScore sets the oom score for a process -func SetScore(pid int) error { - return sys.SetOOMScore(pid, sys.OOMScoreMaxKillable) -} - // AdjustOOMScore sets the OOM score for the process to the parents OOM score +1 // to ensure that they parent has a lower* score than the shim // if not already at the maximum OOM Score diff --git a/sys/oom_linux.go b/sys/oom_linux.go index 8e1e3f2f9..6e759a0b1 100644 --- a/sys/oom_linux.go +++ b/sys/oom_linux.go @@ -28,8 +28,6 @@ import ( ) const ( - // OOMScoreMaxKillable is the maximum score keeping the process killable by the oom killer - OOMScoreMaxKillable = -999 // OOMScoreAdjMin is from OOM_SCORE_ADJ_MIN https://github.com/torvalds/linux/blob/v5.10/include/uapi/linux/oom.h#L9 OOMScoreAdjMin = -1000 // OOMScoreAdjMax is from OOM_SCORE_ADJ_MAX https://github.com/torvalds/linux/blob/v5.10/include/uapi/linux/oom.h#L10