From 3249e2379c1ba8c74cd65dad75378d1cf76c1ab4 Mon Sep 17 00:00:00 2001
From: Kir Kolyshkin <kolyshkin@gmail.com>
Date: Fri, 29 May 2020 14:28:16 -0700
Subject: [PATCH] cgroupv1: check cpu shares in place

Commit 4e65e0e90a5884 added a check for cpu shares. Apparently, the
kernel allows to set a value higher than max or lower than min without
an error, but the value read back is always within the limits.

The check (which was later moved out to a separate CheckCpushares()
function) is always performed after setting the cpu shares, so let's
move it to the very place where it is set.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
---
 libcontainer/cgroups/fs/apply_raw.go | 33 ----------------------------
 libcontainer/cgroups/fs/cpu.go       | 15 ++++++++++++-
 libcontainer/cgroups/systemd/v1.go   |  5 -----
 3 files changed, 14 insertions(+), 39 deletions(-)

diff --git a/libcontainer/cgroups/fs/apply_raw.go b/libcontainer/cgroups/fs/apply_raw.go
index f51dbc3d928..b5bace36959 100644
--- a/libcontainer/cgroups/fs/apply_raw.go
+++ b/libcontainer/cgroups/fs/apply_raw.go
@@ -4,7 +4,6 @@ package fs
 
 import (
 	"fmt"
-	"io"
 	"os"
 	"path/filepath"
 	"sync"
@@ -259,11 +258,6 @@ func (m *manager) Set(container *configs.Config) error {
 		}
 	}
 
-	if m.paths["cpu"] != "" {
-		if err := CheckCpushares(m.paths["cpu"], container.Cgroups.Resources.CpuShares); err != nil {
-			return err
-		}
-	}
 	return nil
 }
 
@@ -377,33 +371,6 @@ func removePath(p string, err error) error {
 	return nil
 }
 
-func CheckCpushares(path string, c uint64) error {
-	var cpuShares uint64
-
-	if c == 0 {
-		return nil
-	}
-
-	fd, err := os.Open(filepath.Join(path, "cpu.shares"))
-	if err != nil {
-		return err
-	}
-	defer fd.Close()
-
-	_, err = fmt.Fscanf(fd, "%d", &cpuShares)
-	if err != nil && err != io.EOF {
-		return err
-	}
-
-	if c > cpuShares {
-		return fmt.Errorf("The maximum allowed cpu-shares is %d", cpuShares)
-	} else if c < cpuShares {
-		return fmt.Errorf("The minimum allowed cpu-shares is %d", cpuShares)
-	}
-
-	return nil
-}
-
 func (m *manager) GetPaths() map[string]string {
 	m.mu.Lock()
 	defer m.mu.Unlock()
diff --git a/libcontainer/cgroups/fs/cpu.go b/libcontainer/cgroups/fs/cpu.go
index 4db7b647cf0..7386aace14f 100644
--- a/libcontainer/cgroups/fs/cpu.go
+++ b/libcontainer/cgroups/fs/cpu.go
@@ -4,6 +4,7 @@ package fs
 
 import (
 	"bufio"
+	"fmt"
 	"os"
 	"path/filepath"
 	"strconv"
@@ -66,9 +67,21 @@ func (s *CpuGroup) SetRtSched(path string, cgroup *configs.Cgroup) error {
 
 func (s *CpuGroup) Set(path string, cgroup *configs.Cgroup) error {
 	if cgroup.Resources.CpuShares != 0 {
-		if err := fscommon.WriteFile(path, "cpu.shares", strconv.FormatUint(cgroup.Resources.CpuShares, 10)); err != nil {
+		shares := cgroup.Resources.CpuShares
+		if err := fscommon.WriteFile(path, "cpu.shares", strconv.FormatUint(shares, 10)); err != nil {
 			return err
 		}
+		// read it back
+		sharesRead, err := fscommon.GetCgroupParamUint(path, "cpu.shares")
+		if err != nil {
+			return err
+		}
+		// ... and check
+		if shares > sharesRead {
+			return fmt.Errorf("the maximum allowed cpu-shares is %d", sharesRead)
+		} else if shares < sharesRead {
+			return fmt.Errorf("the minimum allowed cpu-shares is %d", sharesRead)
+		}
 	}
 	if cgroup.Resources.CpuPeriod != 0 {
 		if err := fscommon.WriteFile(path, "cpu.cfs_period_us", strconv.FormatUint(cgroup.Resources.CpuPeriod, 10)); err != nil {
diff --git a/libcontainer/cgroups/systemd/v1.go b/libcontainer/cgroups/systemd/v1.go
index dda271a107a..7d583803edc 100644
--- a/libcontainer/cgroups/systemd/v1.go
+++ b/libcontainer/cgroups/systemd/v1.go
@@ -435,11 +435,6 @@ func (m *legacyManager) Set(container *configs.Config) error {
 		}
 	}
 
-	if m.paths["cpu"] != "" {
-		if err := fs.CheckCpushares(m.paths["cpu"], container.Cgroups.Resources.CpuShares); err != nil {
-			return err
-		}
-	}
 	return nil
 }