[libvirt] [PATCH 0/2] Fix domiftune bandwidth bounds checking
For https://bugzilla.redhat.com/show_bug.cgi?id=1043735 Ján Tomko (2): virsh: check if domiftune parameters fit into UINT Don't overwrite errors from virNetDevBandwidthSet src/lxc/lxc_process.c | 6 +----- src/network/bridge_driver.c | 6 +----- src/qemu/qemu_command.c | 6 +----- src/qemu/qemu_driver.c | 6 +----- src/qemu/qemu_hotplug.c | 6 +----- src/util/virnetdevmacvlan.c | 3 --- tools/virsh-domain.c | 14 ++++++++++++++ 7 files changed, 19 insertions(+), 28 deletions(-) -- 1.8.5.5
We parse the bandwidth rates as unsinged long long, then try to fit them in VIR_TYPED_PARAM_UINT. Report an error if they exceed UINT_MAX instead of quietly using wrong values. https://bugzilla.redhat.com/show_bug.cgi?id=1043735 --- tools/virsh-domain.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index ad68aab..f7193cb 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -2686,6 +2686,14 @@ cmdDomIftune(vshControl *ctl, const vshCmd *cmd) vshError(ctl, _("inbound format is incorrect")); goto cleanup; } + /* we parse the rate as unsigned long long, but the API + * only accepts UINT */ + if (inbound.average > UINT_MAX || inbound.peak > UINT_MAX || + inbound.burst > UINT_MAX) { + vshError(ctl, _("inbound rate larger than maximum %u"), + UINT_MAX); + goto cleanup; + } if (inbound.average == 0 && (inbound.burst || inbound.peak)) { vshError(ctl, _("inbound average is mandatory")); goto cleanup; @@ -2714,6 +2722,12 @@ cmdDomIftune(vshControl *ctl, const vshCmd *cmd) vshError(ctl, _("outbound format is incorrect")); goto cleanup; } + if (outbound.average > UINT_MAX || outbound.peak > UINT_MAX || + outbound.burst > UINT_MAX) { + vshError(ctl, _("outbound rate larger than maximum %u"), + UINT_MAX); + goto cleanup; + } if (outbound.average == 0 && (outbound.burst || outbound.peak)) { vshError(ctl, _("outbound average is mandatory")); goto cleanup; -- 1.8.5.5
On 07/28/2014 09:30 AM, Ján Tomko wrote:
We parse the bandwidth rates as unsinged long long, then try to fit them in VIR_TYPED_PARAM_UINT.
Report an error if they exceed UINT_MAX instead of quietly using wrong values.
https://bugzilla.redhat.com/show_bug.cgi?id=1043735 --- tools/virsh-domain.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index ad68aab..f7193cb 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -2686,6 +2686,14 @@ cmdDomIftune(vshControl *ctl, const vshCmd *cmd) vshError(ctl, _("inbound format is incorrect")); goto cleanup; }
I think the parseRateStr() should be modified that way the attach-interface can also make use of this range check as well... John
+ /* we parse the rate as unsigned long long, but the API + * only accepts UINT */ + if (inbound.average > UINT_MAX || inbound.peak > UINT_MAX || + inbound.burst > UINT_MAX) { + vshError(ctl, _("inbound rate larger than maximum %u"), + UINT_MAX); + goto cleanup; + } if (inbound.average == 0 && (inbound.burst || inbound.peak)) { vshError(ctl, _("inbound average is mandatory")); goto cleanup; @@ -2714,6 +2722,12 @@ cmdDomIftune(vshControl *ctl, const vshCmd *cmd) vshError(ctl, _("outbound format is incorrect")); goto cleanup; } + if (outbound.average > UINT_MAX || outbound.peak > UINT_MAX || + outbound.burst > UINT_MAX) { + vshError(ctl, _("outbound rate larger than maximum %u"), + UINT_MAX); + goto cleanup; + } if (outbound.average == 0 && (outbound.burst || outbound.peak)) { vshError(ctl, _("outbound average is mandatory")); goto cleanup;
On 07/30/2014 10:34 PM, John Ferlan wrote:
On 07/28/2014 09:30 AM, Ján Tomko wrote:
We parse the bandwidth rates as unsinged long long, then try to fit them in VIR_TYPED_PARAM_UINT.
Report an error if they exceed UINT_MAX instead of quietly using wrong values.
https://bugzilla.redhat.com/show_bug.cgi?id=1043735 --- tools/virsh-domain.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index ad68aab..f7193cb 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -2686,6 +2686,14 @@ cmdDomIftune(vshControl *ctl, const vshCmd *cmd) vshError(ctl, _("inbound format is incorrect")); goto cleanup; }
I think the parseRateStr() should be modified that way the attach-interface can also make use of this range check as well...
cmdAttachInterface is not limited by this range check - it transmits the value via XML as a string. But I could move the error messages there and use a different maximum for attach-interface. Jan
On 07/31/2014 06:00 AM, Ján Tomko wrote:
On 07/30/2014 10:34 PM, John Ferlan wrote:
On 07/28/2014 09:30 AM, Ján Tomko wrote:
We parse the bandwidth rates as unsinged long long, then try to fit them in VIR_TYPED_PARAM_UINT.
Just noticed this too - I see your fingers do the same as mine and typed "unsinged" - that'd need to change to "unsigned"
Report an error if they exceed UINT_MAX instead of quietly using wrong values.
https://bugzilla.redhat.com/show_bug.cgi?id=1043735 --- tools/virsh-domain.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index ad68aab..f7193cb 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -2686,6 +2686,14 @@ cmdDomIftune(vshControl *ctl, const vshCmd *cmd) vshError(ctl, _("inbound format is incorrect")); goto cleanup; }
I think the parseRateStr() should be modified that way the attach-interface can also make use of this range check as well...
cmdAttachInterface is not limited by this range check - it transmits the value via XML as a string. But I could move the error messages there and use a different maximum for attach-interface.
After doing a bit of digging - it seems using attach-interface allows > UINT_MAX for these values, while using domiftune limits one to UINT_MAX. When first looking at the code I just figured since both use the same function to parse the values and both are using virNetDevBandwidthRate to store values, then both could have the same limits applied. Although I understood it was outside the scope of the original bz which was just for the domiftune. A side note - the man page or docs do not seem to list a "max" value, although one would have a tough time imagining today such a large value for a network especially in kb[/s]... The qemu API qemuDomainSetInterfaceParameters() has a virTypedParamsValidate() which should check values vs. VIR_TYPED_PARAM_UINT; however, virNetDevBandwidthSet() handles ULL values so one wonders if it's necessary to limit in domiftune. The latter API is where the final size decision is determined. Using ULL by virNetDevBandwidthRate was designed that way as the original data structure was added in commit id '7373188'. The attach-interface code was added in '7b2723c5'. The original domiftune code was added afterwards as commit id 'b2310b29' and it seemed to "limit" to VIR_TYPED_PARAM_UINT. Commit id '9b2d2446' adjusted things to use virTypedParamsAddUInt(), but still the UINT seems to be a "day 1" type difference. Whether domiftune should use ULL instead of UINT_MAX as the max is the question that needs to be answered. Perhaps the "right" solution is to modify the code to accept the ULL instead of limiting to UINT_MAX since the underlying algorithms manage the data in ULL. John
On 07/31/2014 02:44 PM, John Ferlan wrote:
After doing a bit of digging - it seems using attach-interface allows > UINT_MAX for these values, while using domiftune limits one to UINT_MAX.
When first looking at the code I just figured since both use the same function to parse the values and both are using virNetDevBandwidthRate to store values, then both could have the same limits applied. Although I understood it was outside the scope of the original bz which was just for the domiftune. A side note - the man page or docs do not seem to list a "max" value, although one would have a tough time imagining today such a large value for a network especially in kb[/s]...
Well, even a value of UINT_MAX is too large for current 'tc' to handle, hence the second patch in this series.
The qemu API qemuDomainSetInterfaceParameters() has a virTypedParamsValidate() which should check values vs. VIR_TYPED_PARAM_UINT; however, virNetDevBandwidthSet() handles ULL values so one wonders if it's necessary to limit in domiftune. The latter API is where the final size decision is determined.
Using ULL by virNetDevBandwidthRate was designed that way as the original data structure was added in commit id '7373188'. The attach-interface code was added in '7b2723c5'. The original domiftune code was added afterwards as commit id 'b2310b29' and it seemed to "limit" to VIR_TYPED_PARAM_UINT. Commit id '9b2d2446' adjusted things to use virTypedParamsAddUInt(), but still the UINT seems to be a "day 1" type difference.
The problem is in the PARAM_UINT limit. We could add new parameters with PARAM_ULLONG types, but virsh would still need to handle older daemons which support UINT and newer daemons should be able to handle older clients. This seemed like too much work for values that can't be handled by 'tc' and I was hoping to postpone it for a decade or two :)
Whether domiftune should use ULL instead of UINT_MAX as the max is the question that needs to be answered. Perhaps the "right" solution is to modify the code to accept the ULL instead of limiting to UINT_MAX since the underlying algorithms manage the data in ULL.
Jan
On 07/31/2014 09:51 AM, Ján Tomko wrote:
On 07/31/2014 02:44 PM, John Ferlan wrote:
After doing a bit of digging - it seems using attach-interface allows > UINT_MAX for these values, while using domiftune limits one to UINT_MAX.
When first looking at the code I just figured since both use the same function to parse the values and both are using virNetDevBandwidthRate to store values, then both could have the same limits applied. Although I understood it was outside the scope of the original bz which was just for the domiftune. A side note - the man page or docs do not seem to list a "max" value, although one would have a tough time imagining today such a large value for a network especially in kb[/s]...
Well, even a value of UINT_MAX is too large for current 'tc' to handle, hence the second patch in this series.
Understood, but even "some day" that could change... like you point out below hopefully not any time soon! So back to the original patch - I'm fine to ACK as is. It'd be "nice" to have the attach-interface path do the same checks, but it's going to fail anyway and the message from tc should point out the reason why as of the second patch. John
The qemu API qemuDomainSetInterfaceParameters() has a virTypedParamsValidate() which should check values vs. VIR_TYPED_PARAM_UINT; however, virNetDevBandwidthSet() handles ULL values so one wonders if it's necessary to limit in domiftune. The latter API is where the final size decision is determined.
Using ULL by virNetDevBandwidthRate was designed that way as the original data structure was added in commit id '7373188'. The attach-interface code was added in '7b2723c5'. The original domiftune code was added afterwards as commit id 'b2310b29' and it seemed to "limit" to VIR_TYPED_PARAM_UINT. Commit id '9b2d2446' adjusted things to use virTypedParamsAddUInt(), but still the UINT seems to be a "day 1" type difference.
The problem is in the PARAM_UINT limit. We could add new parameters with PARAM_ULLONG types, but virsh would still need to handle older daemons which support UINT and newer daemons should be able to handle older clients. This seemed like too much work for values that can't be handled by 'tc' and I was hoping to postpone it for a decade or two :)
Whether domiftune should use ULL instead of UINT_MAX as the max is the question that needs to be answered. Perhaps the "right" solution is to modify the code to accept the ULL instead of limiting to UINT_MAX since the underlying algorithms manage the data in ULL.
Jan
Otherwise this beautiful error would be overwritten when the function is called with a really high rate number: 2014-07-28 12:51:47.920+0000: 2304: error : virCommandWait:2399 : internal error: Child process (/sbin/tc class add dev vnet0 parent 1: classid 1:1 htb rate 4294968kbps) unexpected exit status 1: Illegal "rate" Usage: ... qdisc add ... htb [default N] [r2q N] default minor id of class to which unclassified packets are sent {0} r2q DRR quantums are computed as rate in Bps/r2q {10} debug string of 16 numbers each 0-3 {0} ... class add ... htb rate R1 [burst B1] [mpu B] [overhead O] [prio P] [slot S] [pslot PS] [ceil R2] [cburst B2] [mtu MTU] [quantum Q] rate rate allocated to this class (class can still borrow) burst max bytes burst which can be accumulated during idle period {computed} mpu minimum packet size used in rate computations overhead per-packet size overhead used in rate computations linklay adapting to a linklayer e.g. atm ceil definite upper class rate (no borrows) {rate} cburst burst but for ceil {computed} mtu max packet size we create rate map for {1600} prio priority of leaf; lowe https://bugzilla.redhat.com/show_bug.cgi?id=1043735 --- src/lxc/lxc_process.c | 6 +----- src/network/bridge_driver.c | 6 +----- src/qemu/qemu_command.c | 6 +----- src/qemu/qemu_driver.c | 6 +----- src/qemu/qemu_hotplug.c | 6 +----- src/util/virnetdevmacvlan.c | 3 --- 6 files changed, 5 insertions(+), 28 deletions(-) diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c index 4912035..3353dc1 100644 --- a/src/lxc/lxc_process.c +++ b/src/lxc/lxc_process.c @@ -276,12 +276,8 @@ char *virLXCProcessSetupInterfaceBridged(virConnectPtr conn, if (virNetDevBandwidthSet(net->ifname, virDomainNetGetActualBandwidth(net), - false) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("cannot set bandwidth limits on %s"), - net->ifname); + false) < 0) goto cleanup; - } if (net->filter && virDomainConfNWFilterInstantiate(conn, vm->uuid, net) < 0) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 6ccc6e2..4a53f8a 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -2080,12 +2080,8 @@ networkStartNetworkVirtual(virNetworkDriverStatePtr driver, } if (virNetDevBandwidthSet(network->def->bridge, - network->def->bandwidth, true) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("cannot set bandwidth limits on %s"), - network->def->bridge); + network->def->bandwidth, true) < 0) goto err5; - } VIR_FREE(macTapIfName); diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index beb8ca8..fb8b75f 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -382,12 +382,8 @@ qemuNetworkIfaceConnect(virDomainDefPtr def, if (virNetDevBandwidthSet(net->ifname, virDomainNetGetActualBandwidth(net), - false) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("cannot set bandwidth limits on %s"), - net->ifname); + false) < 0) goto cleanup; - } if (net->filter && net->ifname && virDomainConfNWFilterInstantiate(conn, def->uuid, net) < 0) { diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 704ba39..199858e 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -9966,12 +9966,8 @@ qemuDomainSetInterfaceParameters(virDomainPtr dom, sizeof(*newBandwidth->out)); } - if (virNetDevBandwidthSet(net->ifname, newBandwidth, false) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("cannot set bandwidth limits on %s"), - device); + if (virNetDevBandwidthSet(net->ifname, newBandwidth, false) < 0) goto cleanup; - } virNetDevBandwidthFree(net->bandwidth); if (newBandwidth->in || newBandwidth->out) { diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 729744c..004b6a4 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -2181,12 +2181,8 @@ qemuDomainChangeNet(virQEMUDriverPtr driver, if (needBandwidthSet) { if (virNetDevBandwidthSet(newdev->ifname, virDomainNetGetActualBandwidth(newdev), - false) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("cannot set bandwidth limits on %s"), - newdev->ifname); + false) < 0) goto cleanup; - } needReplaceDevDef = true; } diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c index cb85b74..054194b 100644 --- a/src/util/virnetdevmacvlan.c +++ b/src/util/virnetdevmacvlan.c @@ -922,9 +922,6 @@ int virNetDevMacVLanCreateWithVPortProfile(const char *tgifname, } if (virNetDevBandwidthSet(cr_ifname, bandwidth, false) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("cannot set bandwidth limits on %s"), - cr_ifname); if (withTap) VIR_FORCE_CLOSE(rc); /* sets rc to -1 */ else -- 1.8.5.5
On 07/28/2014 09:30 AM, Ján Tomko wrote:
Otherwise this beautiful error would be overwritten when the function is called with a really high rate number:
2014-07-28 12:51:47.920+0000: 2304: error : virCommandWait:2399 : internal error: Child process (/sbin/tc class add dev vnet0 parent 1: classid 1:1 htb rate 4294968kbps) unexpected exit status 1: Illegal "rate" Usage: ... qdisc add ... htb [default N] [r2q N] default minor id of class to which unclassified packets are sent {0} r2q DRR quantums are computed as rate in Bps/r2q {10} debug string of 16 numbers each 0-3 {0}
... class add ... htb rate R1 [burst B1] [mpu B] [overhead O] [prio P] [slot S] [pslot PS] [ceil R2] [cburst B2] [mtu MTU] [quantum Q] rate rate allocated to this class (class can still borrow) burst max bytes burst which can be accumulated during idle period {computed} mpu minimum packet size used in rate computations overhead per-packet size overhead used in rate computations linklay adapting to a linklayer e.g. atm ceil definite upper class rate (no borrows) {rate} cburst burst but for ceil {computed} mtu max packet size we create rate map for {1600} prio priority of leaf; lowe
https://bugzilla.redhat.com/show_bug.cgi?id=1043735 --- src/lxc/lxc_process.c | 6 +----- src/network/bridge_driver.c | 6 +----- src/qemu/qemu_command.c | 6 +----- src/qemu/qemu_driver.c | 6 +----- src/qemu/qemu_hotplug.c | 6 +----- src/util/virnetdevmacvlan.c | 3 --- 6 files changed, 5 insertions(+), 28 deletions(-)
ACK - Although TBH the concept of losing/overwriting the last error message still confounds me :-) John
participants (2)
-
John Ferlan -
Ján Tomko