From c82c32f60579d148f37064e5156e857fa3c84c2f Mon Sep 17 00:00:00 2001
Message-Id: <c82c32f60579d148f37064e5156e857fa3c84c2f@dist-git>
From: Pavel Hrdina <phrdina@redhat.com>
Date: Thu, 4 Mar 2021 12:57:57 +0100
Subject: [PATCH] vircgroup: enforce range limit for cpu.shares
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Before the conversion to using systemd DBus API to set the cpu.shares
there was some magic conversion done by kernel which was documented in
virsh manpage as well. Now systemd errors out if the value is out of
range.

Since we enforce the range for other cpu cgroup attributes 'quota' and
'period' it makes sense to do the same for 'shares' as well.

Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
(cherry picked from commit 1d9d9961ada6c2d0b9facae0ef8be4f459cf7fc9)

Conflicts:
    docs/formatdomain.rst
    src/conf/domain_validate.c
        - both are not present in downstream

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1798463

Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Message-Id: <79b9ef9f98b3ab35061f8c4e4acf7b6861d28055.1614858616.git.phrdina@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
---
 docs/formatdomain.html.in |  1 +
 docs/manpages/virsh.rst   |  5 +----
 src/conf/domain_conf.c    | 10 ++++++++++
 src/util/vircgroup.h      |  2 ++
 src/util/vircgroupv1.c    | 10 ++++++++++
 src/util/vircgroupv2.c    | 10 ++++++++++
 6 files changed, 34 insertions(+), 4 deletions(-)

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 4341e256a8..7ac9523684 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -854,6 +854,7 @@
         it's a relative measure based on the setting of other VM,
         e.g. A VM configured with value
         2048 will get twice as much CPU time as a VM configured with value 1024.
+        The value should be in range [2, 262144].
         <span class="since">Since 0.9.0</span>
       </dd>
       <dt><code>period</code></dt>
diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst
index a5b95c1123..01e1c01912 100644
--- a/docs/manpages/virsh.rst
+++ b/docs/manpages/virsh.rst
@@ -3704,10 +3704,7 @@ If *--live* is specified, set scheduler information of a running guest.
 If *--config* is specified, affect the next boot of a persistent guest.
 If *--current* is specified, affect the current guest state.
 
-``Note``: The cpu_shares parameter has a valid value range of 0-262144; Negative
-values are wrapped to positive, and larger values are capped at the maximum.
-Therefore, -1 is a useful shorthand for 262144. On the Linux kernel, the
-values 0 and 1 are automatically converted to a minimal value of 2.
+``Note``: The cpu_shares parameter has a valid value range of 2-262144.
 
 ``Note``: The weight and cap parameters are defined only for the
 XEN_CREDIT scheduler.
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 9f6cdb0de8..444657c9a1 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -7026,6 +7026,16 @@ virDomainDefLifecycleActionValidate(const virDomainDef *def)
 static int
 virDomainDefCputuneValidate(const virDomainDef *def)
 {
+    if (def->cputune.shares > 0 &&
+        (def->cputune.shares < VIR_CGROUP_CPU_SHARES_MIN ||
+         def->cputune.shares > VIR_CGROUP_CPU_SHARES_MAX)) {
+        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+                       _("Value of cputune 'shares' must be in range [%llu, %llu]"),
+                         VIR_CGROUP_CPU_SHARES_MIN,
+                         VIR_CGROUP_CPU_SHARES_MAX);
+        return -1;
+    }
+
     CPUTUNE_VALIDATE_PERIOD(period);
     CPUTUNE_VALIDATE_PERIOD(global_period);
     CPUTUNE_VALIDATE_PERIOD(emulator_period);
diff --git a/src/util/vircgroup.h b/src/util/vircgroup.h
index 1c6edea0be..938cfdfbe3 100644
--- a/src/util/vircgroup.h
+++ b/src/util/vircgroup.h
@@ -243,6 +243,8 @@ virCgroupGetDomainTotalCpuStats(virCgroupPtr group,
 int virCgroupSetCpuShares(virCgroupPtr group, unsigned long long shares);
 int virCgroupGetCpuShares(virCgroupPtr group, unsigned long long *shares);
 
+#define VIR_CGROUP_CPU_SHARES_MIN 2LL
+#define VIR_CGROUP_CPU_SHARES_MAX 262144LL
 #define VIR_CGROUP_CPU_PERIOD_MIN 1000LL
 #define VIR_CGROUP_CPU_PERIOD_MAX 1000000LL
 #define VIR_CGROUP_CPU_QUOTA_MIN 1000LL
diff --git a/src/util/vircgroupv1.c b/src/util/vircgroupv1.c
index 49a2cb023e..d417446447 100644
--- a/src/util/vircgroupv1.c
+++ b/src/util/vircgroupv1.c
@@ -1901,6 +1901,16 @@ static int
 virCgroupV1SetCpuShares(virCgroupPtr group,
                         unsigned long long shares)
 {
+    if (shares < VIR_CGROUP_CPU_SHARES_MIN ||
+        shares > VIR_CGROUP_CPU_SHARES_MAX) {
+        virReportError(VIR_ERR_INVALID_ARG,
+                       _("shares '%llu' must be in range [%llu, %llu]"),
+                       shares,
+                       VIR_CGROUP_CPU_SHARES_MIN,
+                       VIR_CGROUP_CPU_SHARES_MAX);
+        return -1;
+    }
+
     if (group->unitName) {
         return virCgroupSetValueDBus(group->unitName, "CPUShares",
                                      "t", shares);
diff --git a/src/util/vircgroupv2.c b/src/util/vircgroupv2.c
index a14fc669fb..079fe6a8ec 100644
--- a/src/util/vircgroupv2.c
+++ b/src/util/vircgroupv2.c
@@ -1499,6 +1499,16 @@ static int
 virCgroupV2SetCpuShares(virCgroupPtr group,
                         unsigned long long shares)
 {
+    if (shares < VIR_CGROUP_CPU_SHARES_MIN ||
+        shares > VIR_CGROUP_CPU_SHARES_MAX) {
+        virReportError(VIR_ERR_INVALID_ARG,
+                       _("shares '%llu' must be in range [%llu, %llu]"),
+                       shares,
+                       VIR_CGROUP_CPU_SHARES_MIN,
+                       VIR_CGROUP_CPU_SHARES_MAX);
+        return -1;
+    }
+
     if (group->unitName) {
         return virCgroupSetValueDBus(group->unitName, "CPUWeight",
                                      "t", shares);
-- 
2.30.0