On 05/26/2010 02:05 PM, Laine Stump wrote: > A couple items I didn't notice before: > > On 05/24/2010 02:52 PM, Cole Robinson wrote: >> Fedora bug https://bugzilla.redhat.com/show_bug.cgi?id=235961 >> >> If using the default virtual network, an easy way to lose guest network >> connectivity is to install libvirt inside the VM. The autostarted >> default network inside the guest collides with host virtual network >> routing. This is a long standing issue that has caused users quite a >> bit of pain and confusion. >> >> On network startup, parse /proc/net/route and compare the requested >> IP+netmask against host routing destinations: if any matches are found, >> refuse to start the network. >> >> v2: Drop sscanf, fix a comment typo, comment that function could use >> libnl instead of /proc >> >> Signed-off-by: Cole Robinson<crobinso@xxxxxxxxxx> >> --- >> src/network/bridge_driver.c | 102 +++++++++++++++++++++++++++++++++++++++++++ >> 1 files changed, 102 insertions(+), 0 deletions(-) >> >> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c >> index 5d7ef19..090bed7 100644 >> --- a/src/network/bridge_driver.c >> +++ b/src/network/bridge_driver.c >> @@ -42,6 +42,8 @@ >> #include<stdio.h> >> #include<sys/wait.h> >> #include<sys/ioctl.h> >> +#include<netinet/in.h> >> +#include<arpa/inet.h> >> >> #include "virterror_internal.h" >> #include "datatypes.h" >> @@ -908,6 +910,102 @@ cleanup: >> return ret; >> } >> >> +#define PROC_NET_ROUTE "/proc/net/route" >> + >> +/* XXX: This function can be a lot more exhaustive, there are certainly >> + * other scenarios where we can ruin host network connectivity. >> + * XXX: Using a proper library is preferred over parsing /proc >> + */ >> +static int networkCheckRouteCollision(virNetworkObjPtr network) >> +{ >> + int ret = -1, len; >> + char *cur, *buf = NULL; >> + enum {MAX_ROUTE_SIZE = 1024*64}; >> + struct in_addr inaddress, innetmask; >> + char *netaddr = NULL; >> + >> + if (!network->def->ipAddress || !network->def->netmask) >> + return 0; >> + >> + if (inet_pton(AF_INET, network->def->ipAddress,&inaddress)<= 0) { >> + networkReportError(VIR_ERR_INTERNAL_ERROR, >> + _("cannot parse IP address '%s'"), >> + network->def->ipAddress); >> + goto error; >> + } >> + if (inet_pton(AF_INET, network->def->netmask,&innetmask)<= 0) { >> + networkReportError(VIR_ERR_INTERNAL_ERROR, >> + _("cannot parse netmask '%s'"), >> + network->def->netmask); >> + goto error; >> + } >> + >> + inaddress.s_addr&= innetmask.s_addr; >> + netaddr = strdup(inet_ntoa(inaddress)); >> > > inet_ntoa() isn't threadsafe (it re-uses the same static buffer). strdup > reduces the window, but the potential for a bad result is still there. > Better to use inet_ntop(), or possibly use virSocketFormatAddr() > (although that would have a bit of setup involved). > Will do. > But since the string you're comparing it to below was also originally a > binary value (and also converted with inet_ntoa(), maybe the best thing > would be to just compare the binary values directly and avoid the > conversion. > Doh, good point. I'll keep one of the inet_ntop conversions though, so it can be used in error messages. >> + if (!netaddr) { >> + virReportOOMError(); >> + goto error; >> + } >> + >> + /* Read whole routing table into memory */ >> + if ((len = virFileReadAll(PROC_NET_ROUTE, MAX_ROUTE_SIZE,&buf))< 0) >> + goto error; >> + >> + /* Dropping the last character shouldn't hurt */ >> + buf[len-1] = '\0'; >> + >> + /* First line is just headings, skip it */ >> + cur = strchr(buf, '\n'); >> + >> + while (cur) { >> + char *iface, *dest_raw; >> + char *dest_ip; >> + struct in_addr in; >> + unsigned int addr_val; >> + >> + cur++; >> + >> + /* Delimit interface field */ >> + iface = cur; >> + while(*cur> ' ') { >> + cur++; >> + } >> + *cur++ = '\0'; >> + >> + /* Delimit destination field */ >> + dest_raw = cur; >> + while(*cur> ' ') { >> + cur++; >> + } >> + *cur++ = '\0'; >> + >> + if (virStrToLong_ui(dest_raw, NULL, 16,&addr_val)< 0) { >> + networkReportError(VIR_ERR_INTERNAL_ERROR, >> + _("Failed to convert network address %s"), >> + dest_raw); >> + goto error; >> + } >> > > After seeing the exchange with nDuff in #virt a couple days ago, it > occurred to me that, in order to really check for an *exact* match of > networks (and thus allow one to be a subset of the other, which can be a > valid configuration), you should also parse further on in the line and > get the netmask. Then compare netmasks of the two (as well as ANDing > addr_val with the netmask to eliminate possibility of a network address > that has extra bits set in the bottom). > Done, updated patch sent now. Thanks, Cole -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list