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

[Cole Robinson]

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 at 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