On 7/29/24 15:31, Peter Krempa wrote: > On Thu, Jul 04, 2024 at 13:02:46 +0200, Michal Privoznik wrote: >> 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. Reject those values >> right away. >> >> And while at it, fix the schema which assumed speed values fit >> into uint. >> >> Resolves: https://issues.redhat.com/browse/RHEL-45200 >> Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx> >> --- >> src/conf/netdev_bandwidth_conf.c | 17 +++++++++++++++++ >> src/conf/schemas/networkcommon.rng | 2 +- >> 2 files changed, 18 insertions(+), 1 deletion(-) >> >> diff --git a/src/conf/netdev_bandwidth_conf.c b/src/conf/netdev_bandwidth_conf.c >> index 9faa46a27f..f3f0b2209a 100644 >> --- a/src/conf/netdev_bandwidth_conf.c >> +++ b/src/conf/netdev_bandwidth_conf.c >> @@ -24,6 +24,16 @@ >> >> #define VIR_FROM_THIS VIR_FROM_NONE >> >> +#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 -1; \ >> + } \ >> + } while (0) >> + >> static int >> virNetDevBandwidthParseRate(xmlNodePtr node, >> virNetDevBandwidthRate *rate, >> @@ -50,6 +60,11 @@ virNetDevBandwidthParseRate(xmlNodePtr node, >> &rate->floor)) < 0) >> return -1; >> >> + CHECK_LIMIT(rate->average, 1ULL << 54, "average"); >> + CHECK_LIMIT(rate->peak, 1ULL << 54, "peak"); >> + CHECK_LIMIT(rate->burst, UINT_MAX >> 10, "burst"); >> + CHECK_LIMIT(rate->floor, 1ULL << 54, "floor"); > > This is making the parser stricter than it used to be, which could > result into failures when parsing existing libvirt-stored configs. > > 'virNetDevBandwidthParseRate()' is called from > 'virNetDevBandwidthParse()' which in turn is used e.g. in > 'virDomainNetDefParseXML()' thus in worst case could lead into losing > defined VMs. > > As of such I don't think this validation can be in the parser. > One could argue that: a) these limits couldn't ever be set successfully, b) nobody really sets 16EB/s rates (the limit introduced here). But okay. Let me post v2. Michal
