[libvirt] [PATCH 0/2] Fix a pair of network-related crashes
Ján Tomko (2): netdev: accept NULL in virNetDevSetupControl bridge: don't crash on bandwidth unplug with no bandwidth src/network/bridge_driver.c | 5 +++++ src/util/virnetdev.c | 14 ++++++++------ 2 files changed, 13 insertions(+), 6 deletions(-) -- 1.8.1.5
Commit b9c6b073 dropped the version of virNetDevSetupControl that didn't check for NULL arguments, but we call it like that in virNetDevBridgeDelete. --- src/util/virnetdev.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 7aba515..ebe20d0 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -62,13 +62,15 @@ static int virNetDevSetupControlFull(const char *ifname, { int fd; - memset(ifr, 0, sizeof(*ifr)); + if (ifr && ifname) { + memset(ifr, 0, sizeof(*ifr)); - if (virStrcpyStatic(ifr->ifr_name, ifname) == NULL) { - virReportSystemError(ERANGE, - _("Network interface name '%s' is too long"), - ifname); - return -1; + if (virStrcpyStatic(ifr->ifr_name, ifname) == NULL) { + virReportSystemError(ERANGE, + _("Network interface name '%s' is too long"), + ifname); + return -1; + } } if ((fd = socket(domain, type, 0)) < 0) { -- 1.8.1.5
On 06/21/2013 07:30 PM, Ján Tomko wrote:
Commit b9c6b073 dropped the version of virNetDevSetupControl that didn't check for NULL arguments, but we call it like that in virNetDevBridgeDelete. --- src/util/virnetdev.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 7aba515..ebe20d0 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -62,13 +62,15 @@ static int virNetDevSetupControlFull(const char *ifname, { int fd;
- memset(ifr, 0, sizeof(*ifr)); + if (ifr && ifname) { + memset(ifr, 0, sizeof(*ifr));
- if (virStrcpyStatic(ifr->ifr_name, ifname) == NULL) { - virReportSystemError(ERANGE, - _("Network interface name '%s' is too long"), - ifname); - return -1; + if (virStrcpyStatic(ifr->ifr_name, ifname) == NULL) { + virReportSystemError(ERANGE, + _("Network interface name '%s' is too long"), + ifname); + return -1; + } }
if ((fd = socket(domain, type, 0)) < 0) {
I missed this: diff --git a/src/util/virnetdev.h b/src/util/virnetdev.h index 933a9b3..44a37ca 100644 --- a/src/util/virnetdev.h +++ b/src/util/virnetdev.h @@ -38,7 +38,7 @@ typedef void virIfreq; int virNetDevSetupControl(const char *ifname, virIfreq *ifr) - ATTRIBUTE_RETURN_CHECK ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); + ATTRIBUTE_RETURN_CHECK; int virNetDevExists(const char *brname) ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; Jan
On 06/21/2013 01:45 PM, Ján Tomko wrote:
On 06/21/2013 07:30 PM, Ján Tomko wrote:
Commit b9c6b073 dropped the version of virNetDevSetupControl that didn't check for NULL arguments, but we call it like that in virNetDevBridgeDelete. --- src/util/virnetdev.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)
ACK w/ the squashed change to virnetdev.h. John
On 06/21/2013 07:57 PM, John Ferlan wrote:
On 06/21/2013 01:45 PM, Ján Tomko wrote:
On 06/21/2013 07:30 PM, Ján Tomko wrote:
Commit b9c6b073 dropped the version of virNetDevSetupControl that didn't check for NULL arguments, but we call it like that in virNetDevBridgeDelete. --- src/util/virnetdev.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)
ACK w/ the squashed change to virnetdev.h.
Thanks, pushed now. Jan
If networkUnplugBandwidth is called on a network which has no bandwidth defined, print a warning instead of crashing. This can happen when destroying a domain with bandwith if bandwidth was removed from the network after the domain was started. https://bugzilla.redhat.com/show_bug.cgi?id=975359 --- src/network/bridge_driver.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index f7c2470..72a3f70 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -4808,6 +4808,11 @@ networkUnplugBandwidth(virNetworkObjPtr net, if (iface->data.network.actual && iface->data.network.actual->class_id) { + if (!net->def->bandwidth || !net->def->bandwidth->in) { + VIR_WARN("Network %s has no bandwidth but unplug requested", + net->def->name); + goto cleanup; + } /* we must remove class from bridge */ new_rate = net->def->bandwidth->in->average; -- 1.8.1.5
On 06/21/2013 01:30 PM, Ján Tomko wrote:
If networkUnplugBandwidth is called on a network which has no bandwidth defined, print a warning instead of crashing.
This can happen when destroying a domain with bandwith if
s/bandwith/bandwidth
bandwidth was removed from the network after the domain was started.
https://bugzilla.redhat.com/show_bug.cgi?id=975359 --- src/network/bridge_driver.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index f7c2470..72a3f70 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -4808,6 +4808,11 @@ networkUnplugBandwidth(virNetworkObjPtr net,
if (iface->data.network.actual && iface->data.network.actual->class_id) { + if (!net->def->bandwidth || !net->def->bandwidth->in) { + VIR_WARN("Network %s has no bandwidth but unplug requested", + net->def->name); + goto cleanup; + } /* we must remove class from bridge */ new_rate = net->def->bandwidth->in->average;
I don't know the code well enough - I can understand why there's a crash if bandwidth == NULL; however, it wasn't clear to me whether or not it might be necessary to make the other calls that didn't rely on bandwidth != NULL but do pass/use class_id. John
On 06/21/2013 09:23 PM, John Ferlan wrote:
On 06/21/2013 01:30 PM, Ján Tomko wrote:
If networkUnplugBandwidth is called on a network which has no bandwidth defined, print a warning instead of crashing.
This can happen when destroying a domain with bandwith if
s/bandwith/bandwidth
bandwidth was removed from the network after the domain was started.
https://bugzilla.redhat.com/show_bug.cgi?id=975359 --- src/network/bridge_driver.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index f7c2470..72a3f70 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -4808,6 +4808,11 @@ networkUnplugBandwidth(virNetworkObjPtr net,
if (iface->data.network.actual && iface->data.network.actual->class_id) { + if (!net->def->bandwidth || !net->def->bandwidth->in) { + VIR_WARN("Network %s has no bandwidth but unplug requested", + net->def->name); + goto cleanup; + } /* we must remove class from bridge */ new_rate = net->def->bandwidth->in->average;
I don't know the code well enough - I can understand why there's a crash if bandwidth == NULL; however, it wasn't clear to me whether or not it might be necessary to make the other calls that didn't rely on bandwidth != NULL but do pass/use class_id.
John
If I understand it correctly: if the network has no bandwidth, its bridge shouldn't have any class_ids set and there should be nothing to clean up. Jan
On 21.06.2013 19:30, Ján Tomko wrote:
If networkUnplugBandwidth is called on a network which has no bandwidth defined, print a warning instead of crashing.
This can happen when destroying a domain with bandwith if bandwidth was removed from the network after the domain was started.
https://bugzilla.redhat.com/show_bug.cgi?id=975359 --- src/network/bridge_driver.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index f7c2470..72a3f70 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -4808,6 +4808,11 @@ networkUnplugBandwidth(virNetworkObjPtr net,
if (iface->data.network.actual && iface->data.network.actual->class_id) { + if (!net->def->bandwidth || !net->def->bandwidth->in) { + VIR_WARN("Network %s has no bandwidth but unplug requested", + net->def->name); + goto cleanup; + } /* we must remove class from bridge */ new_rate = net->def->bandwidth->in->average;
So the problem is, if user starts a domain with interface which has @floor set. The @floor requires a bandwidth to be set on the network the interface is to be plugged into. Then he destroys the network, clear <bandwidth/> from the network definition and starts the network again. I think this is the place which should be fixed. We should deny removing <bandwidth/> in case there's at least one interface attached with @floor. Similarly, we refuse to start domain if the corresponding network doesn't have any <bandwidth/> configured. Michal
On 06/27/2013 09:54 AM, Michal Privoznik wrote:
On 21.06.2013 19:30, Ján Tomko wrote:
If networkUnplugBandwidth is called on a network which has no bandwidth defined, print a warning instead of crashing.
This can happen when destroying a domain with bandwith if bandwidth was removed from the network after the domain was started.
https://bugzilla.redhat.com/show_bug.cgi?id=975359 --- src/network/bridge_driver.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index f7c2470..72a3f70 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -4808,6 +4808,11 @@ networkUnplugBandwidth(virNetworkObjPtr net,
if (iface->data.network.actual && iface->data.network.actual->class_id) { + if (!net->def->bandwidth || !net->def->bandwidth->in) { + VIR_WARN("Network %s has no bandwidth but unplug requested", + net->def->name); + goto cleanup; + } /* we must remove class from bridge */ new_rate = net->def->bandwidth->in->average;
So the problem is, if user starts a domain with interface which has @floor set. The @floor requires a bandwidth to be set on the network the interface is to be plugged into. Then he destroys the network, clear <bandwidth/> from the network definition and starts the network again. I think this is the place which should be fixed. We should deny removing <bandwidth/> in case there's at least one interface attached with @floor. Similarly, we refuse to start domain if the corresponding network doesn't have any <bandwidth/> configured.
We refuse to start domains when the network isn't active too, even if they don't have bandwidth, yet we allow the networks to be shut down afterwards. I don't think we want to forbid shutting networks with bandwidth down (which essentially removes all the bandwidth from it). Jan
On 27.06.2013 11:56, Ján Tomko wrote:
On 06/27/2013 09:54 AM, Michal Privoznik wrote:
On 21.06.2013 19:30, Ján Tomko wrote:
If networkUnplugBandwidth is called on a network which has no bandwidth defined, print a warning instead of crashing.
This can happen when destroying a domain with bandwith if bandwidth was removed from the network after the domain was started.
https://bugzilla.redhat.com/show_bug.cgi?id=975359 --- src/network/bridge_driver.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index f7c2470..72a3f70 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -4808,6 +4808,11 @@ networkUnplugBandwidth(virNetworkObjPtr net,
if (iface->data.network.actual && iface->data.network.actual->class_id) { + if (!net->def->bandwidth || !net->def->bandwidth->in) { + VIR_WARN("Network %s has no bandwidth but unplug requested", + net->def->name); + goto cleanup; + } /* we must remove class from bridge */ new_rate = net->def->bandwidth->in->average;
So the problem is, if user starts a domain with interface which has @floor set. The @floor requires a bandwidth to be set on the network the interface is to be plugged into. Then he destroys the network, clear <bandwidth/> from the network definition and starts the network again. I think this is the place which should be fixed. We should deny removing <bandwidth/> in case there's at least one interface attached with @floor. Similarly, we refuse to start domain if the corresponding network doesn't have any <bandwidth/> configured.
We refuse to start domains when the network isn't active too, even if they don't have bandwidth, yet we allow the networks to be shut down afterwards.
I don't think we want to forbid shutting networks with bandwidth down (which essentially removes all the bandwidth from it).
Jan
Right. Good point, if user wants to shoot himself in the foot, we shouldn't disallow him to do that (there's just too much of cases which we would have to check). Libvirt's not foolproof. What we should do - or rather not do is SIGSEGV-ing if user pull the trigger. ACK then Michal
On 06/27/2013 12:11 PM, Michal Privoznik wrote:
On 27.06.2013 11:56, Ján Tomko wrote:
On 06/27/2013 09:54 AM, Michal Privoznik wrote:
On 21.06.2013 19:30, Ján Tomko wrote:
If networkUnplugBandwidth is called on a network which has no bandwidth defined, print a warning instead of crashing.
This can happen when destroying a domain with bandwith if bandwidth was removed from the network after the domain was started.
https://bugzilla.redhat.com/show_bug.cgi?id=975359 --- src/network/bridge_driver.c | 5 +++++ 1 file changed, 5 insertions(+)
So the problem is, if user starts a domain with interface which has @floor set. The @floor requires a bandwidth to be set on the network the interface is to be plugged into. Then he destroys the network, clear <bandwidth/> from the network definition and starts the network again. I think this is the place which should be fixed. We should deny removing <bandwidth/> in case there's at least one interface attached with @floor. Similarly, we refuse to start domain if the corresponding network doesn't have any <bandwidth/> configured.
We refuse to start domains when the network isn't active too, even if they don't have bandwidth, yet we allow the networks to be shut down afterwards.
I don't think we want to forbid shutting networks with bandwidth down (which essentially removes all the bandwidth from it).
Jan
Right. Good point, if user wants to shoot himself in the foot, we shouldn't disallow him to do that (there's just too much of cases which we would have to check). Libvirt's not foolproof. What we should do - or rather not do is SIGSEGV-ing if user pull the trigger.
ACK then
Thanks, pushed now. Jan
participants (3)
-
John Ferlan -
Ján Tomko -
Michal Privoznik