[PATCH v2 0/2] conf: Validate QoS values
v2 of: https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/EDR7I... diff to v1: - Validate in validation phase of XML parsing, or since network XML doesn't have validation phase, do it during network start (just like other stuff) Michal Prívozník (2): conf: Introduce virNetDevBandwidthValidate() conf: Validate QoS values src/conf/domain_validate.c | 9 +++++++ src/conf/netdev_bandwidth_conf.c | 42 ++++++++++++++++++++++++++++++ src/conf/netdev_bandwidth_conf.h | 2 ++ src/conf/schemas/networkcommon.rng | 3 ++- src/libvirt_private.syms | 1 + src/network/bridge_driver.c | 4 +++ 6 files changed, 60 insertions(+), 1 deletion(-) -- 2.44.2
This function validates whether parsed limits are within range as defined by 'tc' sources (since we use tc to set QoS; or OVS which then uses tc too). The 'tc' program stores speeds in 64bit integers (unit is bytes per second) and sizes in uints (unit is bytes). We use different units: kilobytes per second and kibibytes and therefore we can parse values larger than 'tc' can handle and thus need a function to check if values still fit. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/netdev_bandwidth_conf.c | 42 ++++++++++++++++++++++++++++++++ src/conf/netdev_bandwidth_conf.h | 2 ++ src/libvirt_private.syms | 1 + 3 files changed, 45 insertions(+) diff --git a/src/conf/netdev_bandwidth_conf.c b/src/conf/netdev_bandwidth_conf.c index 9faa46a27f..43a2c62240 100644 --- a/src/conf/netdev_bandwidth_conf.c +++ b/src/conf/netdev_bandwidth_conf.c @@ -200,6 +200,48 @@ virNetDevBandwidthFormat(const virNetDevBandwidth *def, } +#define CHECK_LIMIT(val, limit, name) \ + do { \ + if ((val) > (limit)) { \ + virReportError(VIR_ERR_OVERFLOW, \ + _("value '%1$llu' is too big for '%2$s' parameter, maximum is '%3$llu'"), \ + val, name, (unsigned long long) limit); \ + return false; \ + } \ + } while (0) + +static bool +virNetDevBandwidthRateValidate(const virNetDevBandwidthRate *rate) +{ + const unsigned long long speedLimit = 1ULL << 54; + const unsigned int sizeLimit = UINT_MAX >> 10; + + /* These limits are taken straight from 'tc' sources. */ + + if (!rate) + return true; + + CHECK_LIMIT(rate->average, speedLimit, "average"); + CHECK_LIMIT(rate->peak, speedLimit, "peak"); + CHECK_LIMIT(rate->burst, sizeLimit, "burst"); + CHECK_LIMIT(rate->floor, speedLimit, "floor"); + + return true; +} + +#undef CHECK_LIMIT + +bool +virNetDevBandwidthValidate(const virNetDevBandwidth *def) +{ + if (!def) + return true; + + return virNetDevBandwidthRateValidate(def->in) && + virNetDevBandwidthRateValidate(def->out); +} + + bool virNetDevSupportsBandwidth(virDomainNetType type) { switch ((virDomainNetType) type) { diff --git a/src/conf/netdev_bandwidth_conf.h b/src/conf/netdev_bandwidth_conf.h index b679b0f51f..6dbe0298f6 100644 --- a/src/conf/netdev_bandwidth_conf.h +++ b/src/conf/netdev_bandwidth_conf.h @@ -34,6 +34,8 @@ int virNetDevBandwidthFormat(const virNetDevBandwidth *def, unsigned int class_id, virBuffer *buf); +bool virNetDevBandwidthValidate(const virNetDevBandwidth *def); + bool virNetDevSupportsBandwidth(virDomainNetType type); bool virNetDevBandwidthHasFloor(const virNetDevBandwidth *b); bool virNetDevBandwidthSupportsFloor(virNetworkForwardType type); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 0accca442a..7a5df5a6a4 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -825,6 +825,7 @@ virNetDevBandwidthFormat; virNetDevBandwidthHasFloor; virNetDevBandwidthParse; virNetDevBandwidthSupportsFloor; +virNetDevBandwidthValidate; virNetDevSupportsBandwidth; -- 2.44.2
Since we use 'tc' to set QoS, or we instruct OVS which then uses 'tc', we have to make sure values are within range acceptable to 'tc'. Resolves: https://issues.redhat.com/browse/RHEL-45200 Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_validate.c | 9 +++++++++ src/conf/schemas/networkcommon.rng | 3 ++- src/network/bridge_driver.c | 4 ++++ 3 files changed, 15 insertions(+), 1 deletion(-) diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c index 39b8d67928..ab1caadc7a 100644 --- a/src/conf/domain_validate.c +++ b/src/conf/domain_validate.c @@ -22,6 +22,7 @@ #include "domain_validate.h" #include "domain_conf.h" +#include "netdev_bandwidth_conf.h" #include "vircgroup.h" #include "virconftypes.h" #include "virlog.h" @@ -2068,6 +2069,10 @@ virDomainActualNetDefValidate(const virDomainNetDef *net) return -1; } + if (!virNetDevBandwidthValidate(bandwidth)) { + return -1; + } + if (virDomainNetDefValidatePortOptions(macstr, actualType, vport, virDomainNetGetActualPortOptionsIsolated(net)) < 0) { return -1; @@ -2143,6 +2148,10 @@ virDomainNetDefValidate(const virDomainNetDef *net) return -1; } + if (!virNetDevBandwidthValidate(net->bandwidth)) { + return -1; + } + switch (net->type) { case VIR_DOMAIN_NET_TYPE_VHOSTUSER: if (!virDomainNetIsVirtioModel(net)) { diff --git a/src/conf/schemas/networkcommon.rng b/src/conf/schemas/networkcommon.rng index 6df6d43f54..28424f9abd 100644 --- a/src/conf/schemas/networkcommon.rng +++ b/src/conf/schemas/networkcommon.rng @@ -180,9 +180,10 @@ </define> <define name="speed"> - <data type="unsignedInt"> + <data type="unsignedLong"> <param name="pattern">[0-9]+</param> <param name="minInclusive">1</param> + <param name="maxInclusive">18014398509481984</param> </data> </define> <define name="BurstSize"> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 32572c755f..915211d1b5 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -2838,6 +2838,10 @@ networkValidate(virNetworkDriverState *driver, return -1; } + if (!virNetDevBandwidthValidate(def->bandwidth)) { + return -1; + } + /* we support configs with a single PF defined: * <pf dev='eth0'/> * or with a list of netdev names: -- 2.44.2
On Tue, Aug 13, 2024 at 14:58:58 +0200, Michal Privoznik wrote:
v2 of:
https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/EDR7I...
diff to v1: - Validate in validation phase of XML parsing, or since network XML doesn't have validation phase, do it during network start (just like other stuff)
The latter makes sense. We even re-validate the definition of VMs when starting up. Reviewed-by: Peter Krempa <pkrempa@redhat.com>
participants (2)
-
Michal Privoznik -
Peter Krempa