[PATCH] virnetdevbandwidth: Don't generate burst outside of boundaries

Michal Privoznik posted 1 patch 3 years ago
Test syntax-check failed
Failed in applying to current master (apply log)
src/util/virnetdevbandwidth.c  | 12 +++++++++++-
tests/virnetdevbandwidthtest.c | 15 +++++++++++++++
2 files changed, 26 insertions(+), 1 deletion(-)
[PATCH] virnetdevbandwidth: Don't generate burst outside of boundaries
Posted by Michal Privoznik 3 years ago
When generating TC rules for domain's outbound traffic, Libvirt
will use the 'average' as the default for 'burst' - it's been
this way since the feature introduction in v0.9.4-rc1~22. The
reason is that 'average' considers 'burst' for policing. However,
when parsing its command line TC uses an unsigned int (with
overflow detection) to store the 'burst' size. This means, that
the upper limit for the value is UINT_MAX, well UINT_MAX / 1024
because we are putting the value in KiB onto the command line.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1912210
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/util/virnetdevbandwidth.c  | 12 +++++++++++-
 tests/virnetdevbandwidthtest.c | 15 +++++++++++++++
 2 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/src/util/virnetdevbandwidth.c b/src/util/virnetdevbandwidth.c
index 612e8f985c..77a97176b0 100644
--- a/src/util/virnetdevbandwidth.c
+++ b/src/util/virnetdevbandwidth.c
@@ -358,7 +358,17 @@ virNetDevBandwidthSet(const char *ifname,
 
     if (rx) {
         average = g_strdup_printf("%llukbps", rx->average);
-        burst = g_strdup_printf("%llukb", rx->burst ? rx->burst : rx->average);
+
+        if (rx->burst) {
+            burst = g_strdup_printf("%llukb", rx->burst);
+        } else {
+            /* Internally, tc uses uint to store burst size (in bytes).
+             * Therefore, the largest value we can set is UINT_MAX bytes.
+             * We're outputting the vale in KiB though. */
+            unsigned long long avg = MIN(rx->average, UINT_MAX / 1024);
+
+            burst = g_strdup_printf("%llukb", avg);
+        }
 
         virCommandFree(cmd);
         cmd = virCommandNew(TC);
diff --git a/tests/virnetdevbandwidthtest.c b/tests/virnetdevbandwidthtest.c
index 2e76af3d0c..7c236f50f3 100644
--- a/tests/virnetdevbandwidthtest.c
+++ b/tests/virnetdevbandwidthtest.c
@@ -156,6 +156,21 @@ mymain(void)
                  TC " filter add dev eth0 parent ffff: protocol all u32 match u32 0 0 "
                  "police rate 5kbps burst 7kb mtu 64kb drop flowid :1\n"));
 
+    DO_TEST_SET(("<bandwidth>"
+                 "  <inbound average='4294967295'/>"
+                 "  <outbound average='4294967295'/>"
+                 "</bandwidth>"),
+                (TC " qdisc del dev eth0 root\n"
+                 TC " qdisc del dev eth0 ingress\n"
+                 TC " qdisc add dev eth0 root handle 1: htb default 1\n"
+                 TC " class add dev eth0 parent 1: classid 1:1 htb rate 4294967295kbps quantum 366503875\n"
+                 TC " qdisc add dev eth0 parent 1:1 handle 2: sfq perturb 10\n"
+                 TC " filter add dev eth0 parent 1:0 protocol all prio 1 handle 1 fw flowid 1\n"
+                 TC " qdisc add dev eth0 ingress\n"
+                 TC " filter add dev eth0 parent ffff: protocol all u32 match "
+                 "u32 0 0 police rate 4294967295kbps burst 4194303kb mtu 64kb "
+                 "drop flowid :1\n"));
+
     return ret;
 }
 
-- 
2.26.2

Re: [PATCH] virnetdevbandwidth: Don't generate burst outside of boundaries
Posted by Laine Stump 3 years ago
On 3/5/21 6:11 AM, Michal Privoznik wrote:
> When generating TC rules for domain's outbound traffic, Libvirt
> will use the 'average' as the default for 'burst' - it's been
> this way since the feature introduction in v0.9.4-rc1~22. The
> reason is that 'average' considers 'burst' for policing. However,
> when parsing its command line TC uses an unsigned int (with
> overflow detection) to store the 'burst' size. This means, that
> the upper limit for the value is UINT_MAX, well UINT_MAX / 1024
> because we are putting the value in KiB onto the command line.
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1912210
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>

Reviewed-by: Laine Stump <laine@redhat.com>