[libvirt] [PATCH 0/2] check for null ifname when setting/clearing bandwidth
When John Ferlan ran the Coverity static checker on my patches to fix the problem with nicindexes, it began complaining about calling the virNetDevBandwidth* functions with a null ifname. Investigation pointed out some real problems with this that are fixed by these two patches (which will be pushed *before* the nicindex fix patch). Laine Stump (2): network: only clear bandwidth if it has been set util: check for null ifname inside virNetDevBandwidthSet() src/conf/netdev_bandwidth_conf.c | 6 ++++-- src/lxc/lxc_driver.c | 7 ++++++- src/network/bridge_driver.c | 6 ++++-- src/qemu/qemu_hotplug.c | 4 +++- src/util/virnetdevbandwidth.c | 9 ++++++++- src/util/virnetdevbandwidth.h | 4 ++-- 6 files changed, 27 insertions(+), 9 deletions(-) -- 2.1.0
libvirt was unconditionally calling virNetDevBandwidthClear() for every interface (and network bridge) of a type that supported bandwidth, whether it actually had anything set or not. This doesn't hurt anything (unless ifname == NULL!), but is wasteful. This patch makes sure that all calls to virNetDevBandwidthClear() are qualified by checking that ifname != NULL and that it really had some bandwidth setup done. (NB: an sa_assert(detach->ifname) was added in lxcDomainDetachDeviceNetLive() because the preceding new check of detach->ifname before calling virNetDevBandwidthClear() caused Coverity to complain about a possible null dereference.) --- src/conf/netdev_bandwidth_conf.c | 6 ++++-- src/lxc/lxc_driver.c | 7 ++++++- src/network/bridge_driver.c | 6 ++++-- src/qemu/qemu_hotplug.c | 4 +++- 4 files changed, 17 insertions(+), 6 deletions(-) diff --git a/src/conf/netdev_bandwidth_conf.c b/src/conf/netdev_bandwidth_conf.c index c3e0f9f..42d42d6 100644 --- a/src/conf/netdev_bandwidth_conf.c +++ b/src/conf/netdev_bandwidth_conf.c @@ -1,5 +1,5 @@ /* - * Copyright (C) 2009-2014 Red Hat, Inc. + * Copyright (C) 2009-2015 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -285,7 +285,9 @@ virDomainClearNetBandwidth(virDomainObjPtr vm) for (i = 0; i < vm->def->nnets; i++) { type = virDomainNetGetActualType(vm->def->nets[i]); - if (virNetDevSupportBandwidth(type)) + if (vm->def->nets[i]->ifname && + virDomainNetGetActualBandwidth(vm->def->nets[i]) && + virNetDevSupportBandwidth(type)) virNetDevBandwidthClear(vm->def->nets[i]->ifname); } } diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 3adb21d..87fcd98 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -4650,13 +4650,18 @@ lxcDomainDetachDeviceNetLive(virDomainObjPtr vm, actualType = virDomainNetGetActualType(detach); /* clear network bandwidth */ - if (virNetDevSupportBandwidth(actualType) && + if (detach->ifname && + virDomainNetGetActualBandwidth(detach) && + virNetDevSupportBandwidth(actualType) && virNetDevBandwidthClear(detach->ifname)) goto cleanup; switch (actualType) { case VIR_DOMAIN_NET_TYPE_BRIDGE: case VIR_DOMAIN_NET_TYPE_NETWORK: + /* make sure Coverity knows that detach->ifname is valid */ + sa_assert(detach->ifname); + if (virNetDevVethDelete(detach->ifname) < 0) { virDomainAuditNet(vm, detach, NULL, "detach", false); goto cleanup; diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index f240d3b..1209609 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -2096,7 +2096,8 @@ networkStartNetworkVirtual(virNetworkObjPtr network) return 0; err5: - virNetDevBandwidthClear(network->def->bridge); + if (network->def->bandwidth) + virNetDevBandwidthClear(network->def->bridge); err4: if (!save_err) @@ -2142,7 +2143,8 @@ networkStartNetworkVirtual(virNetworkObjPtr network) static int networkShutdownNetworkVirtual(virNetworkObjPtr network) { - virNetDevBandwidthClear(network->def->bridge); + if (network->def->bandwidth) + virNetDevBandwidthClear(network->def->bridge); if (network->radvdPid > 0) { char *radvdpidbase; diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 8691c7e..9518abd 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -3750,7 +3750,9 @@ qemuDomainDetachNetDevice(virQEMUDriverPtr driver, goto cleanup; } - if (virNetDevSupportBandwidth(virDomainNetGetActualType(detach)) && + if (detach->ifname && + virDomainNetGetActualBandwidth(detach) && + virNetDevSupportBandwidth(virDomainNetGetActualType(detach)) && virNetDevBandwidthClear(detach->ifname) < 0) VIR_WARN("cannot clear bandwidth setting for device : %s", detach->ifname); -- 2.1.0
On 24.02.2015 20:39, Laine Stump wrote:
libvirt was unconditionally calling virNetDevBandwidthClear() for every interface (and network bridge) of a type that supported bandwidth, whether it actually had anything set or not. This doesn't hurt anything (unless ifname == NULL!), but is wasteful.
This patch makes sure that all calls to virNetDevBandwidthClear() are qualified by checking that ifname != NULL and that it really had some bandwidth setup done.
(NB: an sa_assert(detach->ifname) was added in lxcDomainDetachDeviceNetLive() because the preceding new check of detach->ifname before calling virNetDevBandwidthClear() caused Coverity to complain about a possible null dereference.) --- src/conf/netdev_bandwidth_conf.c | 6 ++++-- src/lxc/lxc_driver.c | 7 ++++++- src/network/bridge_driver.c | 6 ++++-- src/qemu/qemu_hotplug.c | 4 +++- 4 files changed, 17 insertions(+), 6 deletions(-)
How about moving the check for non-NULL ifname into virNetDevBandwidthClear()? Its counterpart virNetDevBandwidthSet() does something similar over @bandwidth argument. And after 2/2 it'll done the same over @ifname. Michal
On 02/25/2015 06:12 AM, Michal Privoznik wrote:
On 24.02.2015 20:39, Laine Stump wrote:
libvirt was unconditionally calling virNetDevBandwidthClear() for every interface (and network bridge) of a type that supported bandwidth, whether it actually had anything set or not. This doesn't hurt anything (unless ifname == NULL!), but is wasteful.
This patch makes sure that all calls to virNetDevBandwidthClear() are qualified by checking that ifname != NULL and that it really had some bandwidth setup done.
(NB: an sa_assert(detach->ifname) was added in lxcDomainDetachDeviceNetLive() because the preceding new check of detach->ifname before calling virNetDevBandwidthClear() caused Coverity to complain about a possible null dereference.) --- src/conf/netdev_bandwidth_conf.c | 6 ++++-- src/lxc/lxc_driver.c | 7 ++++++- src/network/bridge_driver.c | 6 ++++-- src/qemu/qemu_hotplug.c | 4 +++- 4 files changed, 17 insertions(+), 6 deletions(-)
How about moving the check for non-NULL ifname into virNetDevBandwidthClear()? Its counterpart virNetDevBandwidthSet() does something similar over @bandwidth argument. And after 2/2 it'll done the same over @ifname.
Yeah, I had some reason for doing it externally originally, but then forgot the reason (or it went away), then after I fixed virNetDevBandwidthSet() I thought about changing this one, and decided against it, mostly because I was tired and couldn't be certain that I *didn't* have a valid reason in the first place. I'll do another spin that way.
On 25.02.2015 17:29, Laine Stump wrote:
On 02/25/2015 06:12 AM, Michal Privoznik wrote:
On 24.02.2015 20:39, Laine Stump wrote:
libvirt was unconditionally calling virNetDevBandwidthClear() for every interface (and network bridge) of a type that supported bandwidth, whether it actually had anything set or not. This doesn't hurt anything (unless ifname == NULL!), but is wasteful.
This patch makes sure that all calls to virNetDevBandwidthClear() are qualified by checking that ifname != NULL and that it really had some bandwidth setup done.
(NB: an sa_assert(detach->ifname) was added in lxcDomainDetachDeviceNetLive() because the preceding new check of detach->ifname before calling virNetDevBandwidthClear() caused Coverity to complain about a possible null dereference.) --- src/conf/netdev_bandwidth_conf.c | 6 ++++-- src/lxc/lxc_driver.c | 7 ++++++- src/network/bridge_driver.c | 6 ++++-- src/qemu/qemu_hotplug.c | 4 +++- 4 files changed, 17 insertions(+), 6 deletions(-)
How about moving the check for non-NULL ifname into virNetDevBandwidthClear()? Its counterpart virNetDevBandwidthSet() does something similar over @bandwidth argument. And after 2/2 it'll done the same over @ifname.
Yeah, I had some reason for doing it externally originally, but then forgot the reason (or it went away), then after I fixed virNetDevBandwidthSet() I thought about changing this one, and decided against it, mostly because I was tired and couldn't be certain that I *didn't* have a valid reason in the first place. I'll do another spin that way.
I don't want to discourage you to post patches, but I believe they are easy enough to be fixed locally and pushed. I don't want to enforce v2 just because of this small nit. Michal
On 02/25/2015 11:37 AM, Michal Privoznik wrote:
On 25.02.2015 17:29, Laine Stump wrote:
On 24.02.2015 20:39, Laine Stump wrote:
libvirt was unconditionally calling virNetDevBandwidthClear() for every interface (and network bridge) of a type that supported bandwidth, whether it actually had anything set or not. This doesn't hurt anything (unless ifname == NULL!), but is wasteful.
This patch makes sure that all calls to virNetDevBandwidthClear() are qualified by checking that ifname != NULL and that it really had some bandwidth setup done.
(NB: an sa_assert(detach->ifname) was added in lxcDomainDetachDeviceNetLive() because the preceding new check of detach->ifname before calling virNetDevBandwidthClear() caused Coverity to complain about a possible null dereference.) --- src/conf/netdev_bandwidth_conf.c | 6 ++++-- src/lxc/lxc_driver.c | 7 ++++++- src/network/bridge_driver.c | 6 ++++-- src/qemu/qemu_hotplug.c | 4 +++- 4 files changed, 17 insertions(+), 6 deletions(-)
How about moving the check for non-NULL ifname into virNetDevBandwidthClear()? Its counterpart virNetDevBandwidthSet() does something similar over @bandwidth argument. And after 2/2 it'll done the same over @ifname. Yeah, I had some reason for doing it externally originally, but then forgot the reason (or it went away), then after I fixed virNetDevBandwidthSet() I thought about changing this one, and decided against it, mostly because I was tired and couldn't be certain that I *didn't* have a valid reason in the first place. I'll do another spin
On 02/25/2015 06:12 AM, Michal Privoznik wrote: that way.
I don't want to discourage you to post patches, but I believe they are easy enough to be fixed locally and pushed. I don't want to enforce v2 just because of this small nit.
In the past I have done seemingly trivial things in last minute "minor" tweaks to patches that ended up breaking the build, so I tend to err on the side of caution (especially when nobody actually says "ACK with these nits fixed" :-)). This time I have triple-checked that I did make check and make syntax-check with the tweaked patches, and have even built an rpm, installed it, and tested it just to make sure it's okay. So combined with assuming that your above statement is an unspoken ACK, (and because macvtap and hostdev are busted until this gets fixed) I'm pushing the patches. Thanks to everyone for the reviews and suggestions.
Previously this function relied on having ATTRIBUTE_NONNULL(1) in its prototype rather than explicitly checking for a null ifname. Unfortunately, ATTRIBUTE_NONNULL is just a hint to the optimizer and code analyzers like Coverity, it doesn't actually check anything at execution time, so the result was possible warnings from Coverity, along with the possibility of null dereferences when ifname wasn't available. This patch removes the ATTRIBUTE_NONNULL from the prototype, and checks ifname inside the function, logging an error if it's NULL (once we've determined that the user really is trying to set a bandwidth). --- src/util/virnetdevbandwidth.c | 9 ++++++++- src/util/virnetdevbandwidth.h | 4 ++-- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/src/util/virnetdevbandwidth.c b/src/util/virnetdevbandwidth.c index fbd2a8d..5de84a9 100644 --- a/src/util/virnetdevbandwidth.c +++ b/src/util/virnetdevbandwidth.c @@ -1,5 +1,5 @@ /* - * Copyright (C) 2009-2014 Red Hat, Inc. + * Copyright (C) 2009-2015 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -83,6 +83,13 @@ virNetDevBandwidthSet(const char *ifname, return -1; } + if (!ifname) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("Unable to set bandwidth for interface because " + "device name is unknown")); + return -1; + } + virNetDevBandwidthClear(ifname); if (bandwidth->in && bandwidth->in->average) { diff --git a/src/util/virnetdevbandwidth.h b/src/util/virnetdevbandwidth.h index 4606a07..9b8f514 100644 --- a/src/util/virnetdevbandwidth.h +++ b/src/util/virnetdevbandwidth.h @@ -1,5 +1,5 @@ /* - * Copyright (C) 2009-2013 Red Hat, Inc. + * Copyright (C) 2009-2015 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -46,7 +46,7 @@ void virNetDevBandwidthFree(virNetDevBandwidthPtr def); int virNetDevBandwidthSet(const char *ifname, virNetDevBandwidthPtr bandwidth, bool hierarchical_class) - ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; + ATTRIBUTE_RETURN_CHECK; int virNetDevBandwidthClear(const char *ifname) ATTRIBUTE_NONNULL(1); int virNetDevBandwidthCopy(virNetDevBandwidthPtr *dest, -- 2.1.0
On Tue, Feb 24, 2015 at 02:39:52PM -0500, Laine Stump wrote:
When John Ferlan ran the Coverity static checker on my patches to fix the problem with nicindexes, it began complaining about calling the virNetDevBandwidth* functions with a null ifname. Investigation pointed out some real problems with this that are fixed by these two patches (which will be pushed *before* the nicindex fix patch).
Makes sense and works for me, ACK series after release.
Laine Stump (2): network: only clear bandwidth if it has been set util: check for null ifname inside virNetDevBandwidthSet()
src/conf/netdev_bandwidth_conf.c | 6 ++++-- src/lxc/lxc_driver.c | 7 ++++++- src/network/bridge_driver.c | 6 ++++-- src/qemu/qemu_hotplug.c | 4 +++- src/util/virnetdevbandwidth.c | 9 ++++++++- src/util/virnetdevbandwidth.h | 4 ++-- 6 files changed, 27 insertions(+), 9 deletions(-)
-- 2.1.0
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 02/25/2015 04:40 AM, Martin Kletzander wrote:
On Tue, Feb 24, 2015 at 02:39:52PM -0500, Laine Stump wrote:
When John Ferlan ran the Coverity static checker on my patches to fix the problem with nicindexes, it began complaining about calling the virNetDevBandwidth* functions with a null ifname. Investigation pointed out some real problems with this that are fixed by these two patches (which will be pushed *before* the nicindex fix patch).
Makes sense and works for me, ACK series after release.
No, these need to go in *before* the release, as does the patch for nicindexes - otherwise macvtap and hostdev interfaces will not work.
On 02/25/2015 11:30 AM, Laine Stump wrote:
On 02/25/2015 04:40 AM, Martin Kletzander wrote:
On Tue, Feb 24, 2015 at 02:39:52PM -0500, Laine Stump wrote:
When John Ferlan ran the Coverity static checker on my patches to fix the problem with nicindexes, it began complaining about calling the virNetDevBandwidth* functions with a null ifname. Investigation pointed out some real problems with this that are fixed by these two patches (which will be pushed *before* the nicindex fix patch).
Makes sense and works for me, ACK series after release.
No, these need to go in *before* the release, as does the patch for nicindexes - otherwise macvtap and hostdev interfaces will not work.
I agree - this release... For "reference" you have to see Laine's other patch series which references commit id 'f7afeddc' which was a change made during this release cycle... John
On Wed, Feb 25, 2015 at 11:57:47AM -0500, John Ferlan wrote:
On 02/25/2015 11:30 AM, Laine Stump wrote:
On 02/25/2015 04:40 AM, Martin Kletzander wrote:
On Tue, Feb 24, 2015 at 02:39:52PM -0500, Laine Stump wrote:
When John Ferlan ran the Coverity static checker on my patches to fix the problem with nicindexes, it began complaining about calling the virNetDevBandwidth* functions with a null ifname. Investigation pointed out some real problems with this that are fixed by these two patches (which will be pushed *before* the nicindex fix patch).
Makes sense and works for me, ACK series after release.
No, these need to go in *before* the release, as does the patch for nicindexes - otherwise macvtap and hostdev interfaces will not work.
I agree - this release... For "reference" you have to see Laine's other patch series which references commit id 'f7afeddc' which was a change made during this release cycle...
I was under the impression that the series Laine referred to has not gone in and I jumped on this one since it can be reviewed without that series I couldn't find. Anyway, sorry for my mistake then and if John agree, then I see no point in postponing it.
participants (4)
-
John Ferlan -
Laine Stump -
Martin Kletzander -
Michal Privoznik