2:56 p.m.
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(a)redhat.com>
> ---
> 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