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

[Daniel P. Berrange]

On Wed, May 26, 2010 at 03:56:17PM -0400, Cole Robinson wrote:
> 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.

None of the inet_* functions should be used as they are all obsolete.
Use one of the virSocket* functions in src/util/network.c - if there
isn't a suitable one add a new one, based on getnameinfo() + AI_NUMERIC
flag.

Regards,
Daniel
-- 
|: Red Hat, Engineering, London    -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :|
|: http://autobuild.org        -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|