[libvirt] [PATCH] network: bridge: Don't start network if it collides with host routing

[Laine Stump]

I'm stepping back, for now, on my suggestion of using libnl to do this. 
The proposed method works, and solves a serious problem. Using libnl 
would also work, but would create a new dependency (which can be a bit 
hairy with libnl - eg RHEL5 has libnl-1.0 which is API-incompatible with 
libnl-1.1, and can't be installed in parallel), and would take longer to 
get right.

I still have a couple nits, but will ACK conditional on those.

On 05/20/2010 07:41 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.
>
> Caveat: I'm not very networking savvy
>
> Signed-off-by: Cole Robinson<crobinso at redhat.com>
> ---
>   src/network/bridge_driver.c |   92 +++++++++++++++++++++++++++++++++++++++++++
>   1 files changed, 92 insertions(+), 0 deletions(-)
>
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index 5d7ef19..79da757 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,92 @@ cleanup:
>       return ret;
>   }
>
> +#define PROC_NET_ROUTE "/proc/net/route"
> +
> +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));
> +    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[17];
> +        char dest_buf[128];
> +        char *dest_ip;
> +        struct in_addr in;
> +        int num;
> +        unsigned int addr_val;
> +
> +        cur++;
> +        num = sscanf(cur, "%16s %127s", iface, dest_buf);
>    

sscanf is still allowed, but there has been an effort to eliminate it. 
How about replacing it with something like this? (it has the added side 
effect of not chopping off interface names that are > 16 characters long)


       ...
       char *dest_raw;
       char *iface;

       cur++;
       iface = dest_raw = cur;
       while (*dest_raw > ' ') {
             /* skip until we hit a space or control character (probably 
a tab) */
             dest_raw++l
       }
       *dest_raw++ = 0; /* null terminate interface name */

       ...

      if (virStrToLong_ui(dest_raw, NULL, 16, &addr_val) < 0) {
      ...

> +        if (num != 2) {
> +            networkReportError(VIR_ERR_INTERNAL_ERROR,
> +                               _("Failed to parse %s"), PROC_NET_ROUTE);
> +            goto error;
> +        }
> +
> +        if (virStrToLong_ui(dest_buf, NULL, 16,&addr_val)<  0) {
> +            networkReportError(VIR_ERR_INTERNAL_ERROR,
> +                               _("Failed not convert network address %s"),
> +                               dest_buf);
>    

Some bad English in the error message there! ;-)

> +            goto error;
> +        }
> +
> +        in.s_addr = addr_val;
> +        dest_ip = inet_ntoa(in);
> +
> +        if (STREQ(netaddr, dest_ip)) {
> +            networkReportError(VIR_ERR_INTERNAL_ERROR,
> +                              _("Network destination %s is already in use "
> +                                "by interface %s"), netaddr, iface);
> +            goto error;
> +        }
> +
> +        cur = strchr(cur, '\n');
> +    }
> +
> +    ret = 0;
> +error:
> +    VIR_FREE(buf);
> +    VIR_FREE(netaddr);
> +    return ret;
> +}
> +
>   static int networkStartNetworkDaemon(struct network_driver *driver,
>                                        virNetworkObjPtr network)
>   {
> @@ -919,6 +1007,10 @@ static int networkStartNetworkDaemon(struct network_driver *driver,
>           return -1;
>       }
>
> +    /* Check to see if network collides with an existing route */
> +    if (networkCheckRouteCollision(network)<  0)
> +        return -1;
> +
>       if ((err = brAddBridge(driver->brctl, network->def->bridge))) {
>           virReportSystemError(err,
>                                _("cannot create bridge '%s'"),
>    

ACK, modulo the error message typo and the use of sscanf, and with a 
note somewhere that we should refine the check later to possibly take 
into account route netmasks and gateways, and host interface addresses 
(we need to be careful not to put in too much restriction, and kill 
useful configurations, so I think we should take time to think about it 
before acting further).