This patch adds DHCP Snooping support to libvirt.

Signed-off-by: David L Stevens<dlstevens@us.ibm.com> --- examples/xml/nwfilter/no-ip-spoofing.xml | 5 + src/Makefile.am | 2 + src/nwfilter/nwfilter_dhcpsnoop.c | 705 ++++++++++++++++++++++++++++++ src/nwfilter/nwfilter_dhcpsnoop.h | 38 ++ src/nwfilter/nwfilter_driver.c | 5 + src/nwfilter/nwfilter_gentech_driver.c | 51 ++- 6 files changed, 793 insertions(+), 13 deletions(-) create mode 100644 src/nwfilter/nwfilter_dhcpsnoop.c create mode 100644 src/nwfilter/nwfilter_dhcpsnoop.h

diff --git a/examples/xml/nwfilter/no-ip-spoofing.xml b/examples/xml/nwfilter/no-ip-spoofing.xml index b8c94c8..e5969a0 100644 --- a/examples/xml/nwfilter/no-ip-spoofing.xml +++ b/examples/xml/nwfilter/no-ip-spoofing.xml @@ -4,4 +4,9 @@ <rule action='drop' direction='out'> <ip match='no' srcipaddr='$IP' /> </rule> +<!-- allow DHCP requests --> +<rule action='return' direction='out'> +<ip match='yes' srcipaddr='0.0.0.0' protocol='udp' srcportstart='68' + srcportend='68' /> +</rule> </filter> Is this change necessary? Is it needed for the first instantiation of

diff --git a/src/Makefile.am b/src/Makefile.am index 87d91ed..c948cbf 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -481,6 +481,8 @@ NWFILTER_DRIVER_SOURCES = \ nwfilter/nwfilter_driver.h nwfilter/nwfilter_driver.c \ nwfilter/nwfilter_gentech_driver.c \ nwfilter/nwfilter_gentech_driver.h \ + nwfilter/nwfilter_dhcpsnoop.c \ + nwfilter/nwfilter_dhcpsnoop.h \ nwfilter/nwfilter_ebiptables_driver.c \ nwfilter/nwfilter_ebiptables_driver.h \ nwfilter/nwfilter_learnipaddr.c \ diff --git a/src/nwfilter/nwfilter_dhcpsnoop.c b/src/nwfilter/nwfilter_dhcpsnoop.c new file mode 100644 index 0000000..b77e2b0 --- /dev/null +++ b/src/nwfilter/nwfilter_dhcpsnoop.c @@ -0,0 +1,705 @@ +/* + * nwfilter_dhcpsnoop.c: support for DHCP snooping used by a VM + * on an interface + * + * Copyright (C) 2011 IBM Corp. + * Copyright (C) 2011 David L Stevens + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + * + * Author: David L Stevens<dlstevens@us.ibm.com> + * Based in part on work by Stefan Berger<stefanb@us.ibm.com> + */ + +#include<config.h> + +#ifdef HAVE_LIBPCAP +#include<pcap.h> +#endif + +#include<fcntl.h> +#include<sys/ioctl.h> +#include<signal.h> + +#include<arpa/inet.h> +#include<net/ethernet.h> +#include<netinet/ip.h> +#include<netinet/udp.h> +#include<net/if.h> +#include<net/if_arp.h> +#include<intprops.h> + +#include "internal.h" + +#include "buf.h" +#include "memory.h" +#include "logging.h" +#include "datatypes.h" +#include "interface.h" +#include "virterror_internal.h" +#include "threads.h" +#include "conf/nwfilter_params.h" +#include "conf/domain_conf.h" +#include "nwfilter_gentech_driver.h" +#include "nwfilter_ebiptables_driver.h" +#include "nwfilter_dhcpsnoop.h" + +#define VIR_FROM_THIS VIR_FROM_NWFILTER + +#ifdef HAVE_LIBPCAP + +static virHashTablePtr SnoopReqs; +static pthread_mutex_t SnoopLock; + +#define snoop_lock() { pthread_mutex_lock(&SnoopLock); } +#define snoop_unlock() { pthread_mutex_unlock(&SnoopLock); } + +struct virNWFilterSnoopReq { + virNWFilterTechDriverPtr techdriver; + const char *ifname; + int ifindex; + const char *linkdev; + enum virDomainNetType nettype; + unsigned char macaddr[VIR_MAC_STRING_BUFLEN]; + const char *filtername; + virNWFilterHashTablePtr vars; + virNWFilterDriverStatePtr driver; + pthread_t thread; + /* start and end of lease list, ordered by lease time */ + struct iplease *start; + struct iplease *end; + bool die; +}; + +#define POLL_INTERVAL 10*1000 /* 10 secs */ + +struct iplease { + uint32_t ipl_ipaddr; + uint32_t ipl_server; + struct virNWFilterSnoopReq *ipl_req; + unsigned int ipl_timeout; + /* timer list */ + struct iplease *ipl_prev; + struct iplease *ipl_next; +}; + +static struct iplease *ipl_getbyip(struct iplease *start, uint32_t ipaddr); +static void ipl_update(struct iplease *pl, uint32_t timeout); + + +/* + * ipl_ladd - add an IP lease to a list + */ +static void +ipl_ladd(struct iplease *plnew, struct iplease **start, struct iplease **end) +{ + struct iplease *pl; + + plnew->ipl_next = plnew->ipl_prev = 0; + if (!*start) { + plnew->ipl_prev = plnew->ipl_next = 0; + *start = *end = plnew; + return; + } + for (pl = *end; pl&& plnew->ipl_timeout< pl->ipl_timeout; + pl = pl->ipl_prev) + /* empty */ ; + if (!pl) { + plnew->ipl_next = *start; + *start = plnew; + } else { + plnew->ipl_next = pl->ipl_next; + pl->ipl_next = plnew; + } + plnew->ipl_prev = pl; + if (plnew->ipl_next) + plnew->ipl_next->ipl_prev = plnew; + else + *end = plnew; +} + +/* + * ipl_tadd - add an IP lease to the timer list + */ +static void +ipl_tadd(struct iplease *plnew) +{ + struct virNWFilterSnoopReq *req = plnew->ipl_req; + + ipl_ladd(plnew,&req->start,&req->end); +} + +/* + * ipl_add - create or update an IP lease + */ +static void +ipl_add(struct iplease *plnew) +{ + struct iplease *pl; + int rc; + char ipbuf[20]; /* dotted decimal IP addr string */ + char *ipstr; + + pl = ipl_getbyip(plnew->ipl_req->start, plnew->ipl_ipaddr); + if (pl) { + ipl_update(pl, plnew->ipl_timeout); + return; + } + /* support for multiple addresses requires the ability to add filters + * to existing chains, or to instantiate address lists via + * virNWFilterInstantiateFilterLate(). Until one of those capabilities + * is added, don't allow a new address when one is already assigned to + * this interface. + */ + if (plnew->ipl_req->start) + return; /* silently ignore multiple addresses */ + + if (VIR_ALLOC(pl)< 0) { + virReportOOMError(); + return; + } + *pl = *plnew; + + if (!inet_ntop(AF_INET,&pl->ipl_ipaddr, ipbuf, sizeof(ipbuf))) { + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, + _("ipl_add inet_ntop " "failed (0x%08X)"), + pl->ipl_ipaddr); + VIR_FREE(pl); + return; + + } + + ipstr = strdup(ipbuf); + if (!ipstr) { + VIR_FREE(pl); + virReportOOMError(); + return; + } + if (virNWFilterHashTablePut(pl->ipl_req->vars, "IP", ipstr, 1)) { + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, + _("Could not add variable \"IP\" to hashmap")); + VIR_FREE(ipstr); + VIR_FREE(pl); + return; + } + rc = virNWFilterInstantiateFilterLate(NULL, + pl->ipl_req->ifname, + pl->ipl_req->ifindex, + pl->ipl_req->linkdev, + pl->ipl_req->nettype, + pl->ipl_req->macaddr, + pl->ipl_req->filtername, + pl->ipl_req->vars, + pl->ipl_req->driver); + if (rc) { + VIR_FREE(ipstr); + VIR_FREE(pl); + return; + } + ipl_tadd(pl); +} + +/* + * ipl_tdel - remove an IP lease from a list + */ +static void +ipl_ldel(struct iplease *ipl, struct iplease **start, struct iplease **end) +{ + if (ipl->ipl_prev) + ipl->ipl_prev->ipl_next = ipl->ipl_next; + else + *start = ipl->ipl_next; + if (ipl->ipl_next) + ipl->ipl_next->ipl_prev = ipl->ipl_prev; + else + *end = ipl->ipl_prev; + ipl->ipl_next = ipl->ipl_prev = 0; +} + +/* + * ipl_tdel - remove an IP lease from the timer list + */ +static void +ipl_tdel(struct iplease *ipl) +{ + struct virNWFilterSnoopReq *req = ipl->ipl_req; + + ipl_ldel(ipl,&req->start,&req->end); + ipl->ipl_timeout = 0; +} + +/* + * ipl_del - delete an IP lease + */ +static void +ipl_del(struct iplease *ipl) +{ + struct virNWFilterSnoopReq *req; + + if (!ipl) + return; + + req = ipl->ipl_req; + + ipl_tdel(ipl); + + /* for multiple address support, this needs to remove those rules + * referencing "IP" with ipl's ip value. + */ + if (req->techdriver->applyDHCPOnlyRules(req->ifname, req->macaddr, NULL)) { + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, "ipl_ldel failed"); + } + VIR_FREE(ipl); +} + +/* + * ipl_update - update the timeout on an IP lease + */ +static void +ipl_update(struct iplease *ipl, uint32_t timeout) +{ + ipl_tdel(ipl); + ipl->ipl_timeout = timeout; + ipl_tadd(ipl); + return; +} + +/* + * ipl_getbyip - lookup IP lease by IP address + */ +static struct iplease * +ipl_getbyip(struct iplease *start, uint32_t ipaddr) +{ + struct iplease *pl; + + for (pl = start; pl&& pl->ipl_ipaddr != ipaddr; pl = pl->ipl_next) + /* empty */ ; + return pl; +} + +#define GRACE 5 + +/* + * ipl_trun - run the IP lease timeout list + */ +static unsigned int +ipl_trun(struct virNWFilterSnoopReq *req) +{ + uint32_t now; + + now = time(0); + while (req->start&& req->start->ipl_timeout<= now) + ipl_del(req->start); + return 0; +} + +typedef unsigned char Eaddr[6]; + +struct eth { + Eaddr eh_dst; + Eaddr eh_src; + unsigned short eh_type; + unsigned char eh_data[0]; +}; The below algorithm also seems to assume that the VM does not send 802.1Q (VLAN) headers. I remember having had problems when trying to receive 802.1Q headers when the VM's interface is on a bridge and the remote traffic comes through the network. I wonder whether it generally doesn't work. I think at least in the code you should look at the

On 10/24/2011 07:12 PM, David L Stevens wrote: the rules or is it needed to support the case when all IP addresses have expired? I am asking because below I saw you calling + if (req->techdriver->applyDHCPOnlyRules(req->ifname, req->macaddr, NULL)) { + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, "ipl_ldel failed"); + } which does a similiar thing. If it was necessary, couldn't you just reference the allow-dhcp filter? header, check for ETHERTYPE_IPv4 and then use eh_data[0], otherwise either discard it or if ETHERTYPE_VLAN (0x8100) is used read the encapsulated protocol ID and make sure ETHERTYPE_IPv4 is found there and then use eh_data[4] for further processing.

+ +struct dhcp { + unsigned char d_op; + unsigned char d_htype; + unsigned char d_hlen; + unsigned char d_hops; + unsigned int d_xid; + unsigned short d_secs; + unsigned short d_flags; + unsigned int d_ciaddr; + unsigned int d_yiaddr; + unsigned int d_siaddr; + unsigned int d_giaddr; + unsigned char d_chaddr[16]; + char d_sname[64]; + char d_file[128]; + unsigned char d_opts[0]; +}; + +/* DHCP options */ + +#define DHCPO_PAD 0 +#define DHCPO_LEASE 51 /* lease time in secs */ +#define DHCPO_MTYPE 53 /* message type */ +#define DHCPO_END 255 /* end of options */ + +/* DHCP message types */ +#define DHCPDECLINE 4 +#define DHCPACK 5 +#define DHCPRELEASE 7 + +unsigned char dhcp_magic[4] = { 99, 130, 83, 99 }; + +static int +dhcp_getopt(struct dhcp *pd, int len, int *pmtype, int *pleasetime) +{ + int oind, olen; + int oend; + + olen = len - sizeof *pd; + oind = 0; + + if (olen< 4) /* bad magic */ + return -1; + for (oind = 0; oind< sizeof dhcp_magic; ++oind) + if (pd->d_opts[oind] != dhcp_magic[oind]) + return -1; /* bad magic */ + memcmp? + oend = 0; + + *pmtype = *pleasetime = 0; + + while (oind< olen) { + switch (pd->d_opts[oind]) { + case DHCPO_LEASE: + if (olen - oind< 6) + goto malformed; + if (*pleasetime) + return -1; /* duplicate lease time */ + *pleasetime = + ntohl(*(unsigned int *) (pd->d_opts + oind + 2)); + break; + case DHCPO_MTYPE: + if (olen - oind< 3) + goto malformed; + if (*pmtype) + return -1; /* duplicate message type */ + *pmtype = pd->d_opts[oind + 2]; + break; + case DHCPO_PAD: + oind++; + continue; + + case DHCPO_END: + oend = 1; + break; + default: + if (olen - oind< 2) + goto malformed; + } + if (oend) + break; + oind += pd->d_opts[oind + 1] + 2; + } + return 0; + malformed: + VIR_WARN("got lost in the options!"); + return -1; +} + +static void +dhcpdecode(struct virNWFilterSnoopReq *req, struct eth *pep, int len) +{ + struct iphdr *pip; + struct udphdr *pup; + struct dhcp *pd; + struct iplease ipl, *pipl; + int mtype, leasetime; + + /* go through the protocol headers */ + pip = (struct iphdr *) pep->eh_data; + len -= sizeof(*pep); + pup = (struct udphdr *) ((char *) pip + (pip->ihl<< 2)); + len -= pip->ihl<< 2; + pd = (struct dhcp *) ((char *) pup + sizeof(*pup)); + len -= sizeof(*pup); + if (len< 0) + return; /* dhcpdecode: invalid packet length */ + if (dhcp_getopt(pd, len,&mtype,&leasetime)< 0) + return; + + memset(&ipl, 0, sizeof(ipl)); + ipl.ipl_ipaddr = pd->d_yiaddr; + ipl.ipl_server = pd->d_siaddr; + if (leasetime == ~0) + ipl.ipl_timeout = ~0; + else + ipl.ipl_timeout = time(0) + leasetime; + ipl.ipl_req = req; + + switch (mtype) { + case DHCPACK: + ipl_add(&ipl); + break; + case DHCPDECLINE: + case DHCPRELEASE: + pipl = ipl_getbyip(req->start, ipl.ipl_ipaddr); + ipl_del(pipl); + break; + default: + break; + } +} + +#define PBUFSIZE 576 /*>= IP/TCP/DHCP headers */ + +static pcap_t * +dhcpopen(const char *intf) +{ + pcap_t *handle; + struct bpf_program fp; + char filter[64]; + char pcap_errbuf[PCAP_ERRBUF_SIZE]; + + handle = pcap_open_live(intf, PBUFSIZE, 0, POLL_INTERVAL, pcap_errbuf); + if (handle == NULL) { + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, + _("pcap_open_live: %s"), pcap_errbuf); + return 0; + } + + sprintf(filter, "port 67 or dst port 68"); + if (pcap_compile(handle,&fp, filter, 1, PCAP_NETMASK_UNKNOWN) != 0) { + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, + _("pcap_compile: %s"), pcap_geterr(handle)); + return 0; + } + if (pcap_setfilter(handle,&fp) != 0) { + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, + _("pcap_setfilter: %s"), pcap_geterr(handle)); + return 0; + } + pcap_freecode(&fp); + return handle; +} + +static void +snoopReqFree(struct virNWFilterSnoopReq *req) +{ + struct iplease *ipl; + + /* free all leases */ + snoop_lock(); + for (ipl = req->start; ipl; ipl = req->start) + ipl_del(ipl); + snoop_unlock(); + + /* free all req data */ + if (req->ifname) + VIR_FREE(req->ifname); + if (req->linkdev) + VIR_FREE(req->linkdev); + if (req->filtername) + VIR_FREE(req->filtername); + if (req->vars) + virNWFilterHashTableFree(req->vars); A 'make syntax-check' will probably flag those... don't check for NULL before free(). + VIR_FREE(req); +} + +static void * +virNWFilterDHCPSnoop(void *req0) +{ + struct virNWFilterSnoopReq *req = req0; + pcap_t *handle; + struct pcap_pkthdr hdr; + struct eth *packet; + int ifindex; + + handle = dhcpopen(req->ifname); + if (!handle) + return 0; + + ifindex = if_nametoindex(req->ifname); + + while (1) { + if (req->die) + break; + snoop_lock(); + ipl_trun(req); + snoop_unlock(); + + packet = (struct eth *) pcap_next(handle,&hdr); + + if (!packet) { + if (ifaceCheck(false, req->ifname, NULL, ifindex)) + virNWFilterDHCPSnoopEnd(req->ifname); + continue; + } + + snoop_lock(); + dhcpdecode(req, packet, hdr.caplen); + snoop_unlock(); + } + snoopReqFree(req); + return 0; +} + +int +virNWFilterDHCPSnoopReq(virNWFilterTechDriverPtr techdriver, + const char *ifname, + int ifindex, + const char *linkdev, + enum virDomainNetType nettype, + const unsigned char *macaddr, + const char *filtername, + virNWFilterHashTablePtr filterparams, + virNWFilterDriverStatePtr driver) +{ + struct virNWFilterSnoopReq *req; + + snoop_lock(); + req = virHashLookup(SnoopReqs, ifname); + snoop_unlock(); + if (req) + return 0; + + if (VIR_ALLOC(req)< 0) { + virReportOOMError(); + return 1; + } + + req->techdriver = techdriver; + req->ifname = strdup(ifname); + if (req->ifname == 0) { compare a string against NULL rather than 0. + snoopReqFree(req); + virReportOOMError(); + return 1; + } + req->ifindex = ifindex; + req->linkdev = linkdev ? strdup(linkdev) : 0; + req->nettype = nettype; + memcpy(req->macaddr, macaddr, sizeof(req->macaddr)); + req->filtername = strdup(filtername); + if (req->filtername == 0) { + snoopReqFree(req); + virReportOOMError(); + return 1; + } + + if (techdriver->applyDHCPOnlyRules(req->ifname, req->macaddr, NULL)) { + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, "applyDHCPOnlyRules " + "failed - spoofing not protected!"); + } + + req->vars = virNWFilterHashTableCreate(0); + if (!req->vars) { + snoopReqFree(req); + virReportOOMError(); + return 1; + } + if (virNWFilterHashTablePutAll(filterparams, req->vars)) { + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, + _("virNWFilterDHCPSnoopReq: can't copy variables" + " on if %s"), ifname); + snoopReqFree(req); + return 1; + } + req->driver = driver; + + snoop_lock(); + if (virHashAddEntry(SnoopReqs, ifname, req)) { + snoop_unlock(); + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, + _("virNWFilterDHCPSnoopReq req add failed on" + "interface \"%s\""), ifname); + snoopReqFree(req); + return 1; + } + snoop_unlock(); + if (pthread_create(&req->thread, NULL, virNWFilterDHCPSnoop, req) != 0) { + (void) virHashRemoveEntry(SnoopReqs, ifname); + snoopReqFree(req); + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, + _("virNWFilterDHCPSnoopReq pthread_create failed" + " oninterface \"%s\""), ifname); + return 1; + } + return 0; +} + +/* + * freeReq - hash table free function to kill a request + */ +static void +freeReq(void *req0, const void *name ATTRIBUTE_UNUSED) +{ + struct virNWFilterSnoopReq *req = (struct virNWFilterSnoopReq *) req0; + + if (!req) + return; + + req->die = 1; +} + +int +virNWFilterDHCPSnoopInit(void) +{ + if (SnoopReqs) + return 0; + + if (pthread_mutex_init(&SnoopLock, 0)< 0) + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, + _("pthread_mutex_init: %s"), + strerror(errno)); Return -1 in case of failure? + snoop_lock(); + SnoopReqs = virHashCreate(0, freeReq); + if (!SnoopReqs) { + snoop_unlock(); + virReportOOMError(); + return -1; + } + snoop_unlock(); + return 0; +} + +void +virNWFilterDHCPSnoopEnd(const char *ifname) +{ + snoop_lock(); + if (!SnoopReqs) { + snoop_unlock(); + return; + } + if (ifname) + virHashRemoveEntry(SnoopReqs, ifname); + else /* free all of them */ + virHashFree(SnoopReqs); + snoop_unlock(); +} + +#else /* HAVE_LIBPCAP */ +int +virNWFilterDHCPSnoopInit(void) +{ + return; Should return a value. Please test it. +} + +void +virNWFilterDHCPSnoopEnd(const char *ifname ATTRIBUTE_UNUSED) +{ + return; +} + +int +virNWFilterDHCPSnoopReq(virNWFilterTechDriverPtr techdriver ATTRIBUTE_UNUSED, + const char *ifname ATTRIBUTE_UNUSED, + int ifindex ATTRIBUTE_UNUSED, + const char *linkdev ATTRIBUTE_UNUSED, + enum virDomainNetType nettype ATTRIBUTE_UNUSED, + const unsigned char *macaddr ATTRIBUTE_UNUSED, + const char *filtername ATTRIBUTE_UNUSED, + virNWFilterHashTablePtr filterparams ATTRIBUTE_UNUSED, + virNWFilterDriverStatePtr driver ATTRIBUTE_UNUSED) +{ + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, _("libvirt was not compiled " + "with libpcap and \"ip_learning='dhcp'\" requires" + " it.")); + return 1; +} +#endif /* HAVE_LIBPCAP */ diff --git a/src/nwfilter/nwfilter_dhcpsnoop.h b/src/nwfilter/nwfilter_dhcpsnoop.h new file mode 100644 index 0000000..3393157 --- /dev/null +++ b/src/nwfilter/nwfilter_dhcpsnoop.h @@ -0,0 +1,38 @@ +/* + * nwfilter_dhcpsnoop.h: support DHCP snooping for a VM on an interface + * + * Copyright (C) 2010 IBM Corp. + * Copyright (C) 2010 David L Stevens + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + * + * Author: David L Stevens<dlstevens@us.ibm.com> + */ + +#ifndef __NWFILTER_DHCPSNOOP_H +# define __NWFILTER_DHCPSNOOP_H + +int virNWFilterDHCPSnoopInit(void); +int virNWFilterDHCPSnoopReq(virNWFilterTechDriverPtr techdriver, + const char *ifname, + int ifindex, + const char *linkdev, + enum virDomainNetType nettype, + const unsigned char *macaddr, + const char *filtername, + virNWFilterHashTablePtr filterparams, + virNWFilterDriverStatePtr driver); +void virNWFilterDHCPSnoopEnd(const char *ifname); +#endif /* __NWFILTER_DHCPSNOOP_H */ diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index a735059..57eb52c 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -39,6 +39,7 @@ #include "nwfilter_gentech_driver.h" #include "configmake.h"

+#include "nwfilter_dhcpsnoop.h" #include "nwfilter_learnipaddr.h"

#define VIR_FROM_THIS VIR_FROM_NWFILTER @@ -66,6 +67,8 @@ static int nwfilterDriverStartup(int privileged) { char *base = NULL;

+ if (virNWFilterDHCPSnoopInit()< 0) + return -1; if (virNWFilterLearnInit()< 0) return -1;

@@ -127,6 +130,7 @@ alloc_err_exit:

conf_init_err: virNWFilterTechDriversShutdown(); + virNWFilterDHCPSnoopEnd(0); virNWFilterLearnShutdown();

return -1; @@ -201,6 +205,7 @@ nwfilterDriverShutdown(void) {

virNWFilterConfLayerShutdown(); virNWFilterTechDriversShutdown(); + virNWFilterDHCPSnoopEnd(0); virNWFilterLearnShutdown();

nwfilterDriverLock(driverState); diff --git a/src/nwfilter/nwfilter_gentech_driver.c b/src/nwfilter/nwfilter_gentech_driver.c index 7891983..2a9773c 100644 --- a/src/nwfilter/nwfilter_gentech_driver.c +++ b/src/nwfilter/nwfilter_gentech_driver.c @@ -33,6 +33,7 @@ #include "virterror_internal.h" #include "nwfilter_gentech_driver.h" #include "nwfilter_ebiptables_driver.h" +#include "nwfilter_dhcpsnoop.h" #include "nwfilter_learnipaddr.h"

@@ -42,6 +43,8 @@ #define NWFILTER_STD_VAR_MAC "MAC" #define NWFILTER_STD_VAR_IP "IP"

+#define NWFILTER_DFLT_LEARN "any" + static int _virNWFilterTeardownFilter(const char *ifname);

@@ -638,6 +641,8 @@ virNWFilterInstantiate(virConnectPtr conn, void **ptrs = NULL; int instantiate = 1; char *buf; + const char *learning = NWFILTER_DFLT_LEARN; + bool reportIP = false;

virNWFilterHashTablePtr missing_vars = virNWFilterHashTableCreate(0); if (!missing_vars) { @@ -655,22 +660,40 @@ virNWFilterInstantiate(virConnectPtr conn, if (rc) goto err_exit;

+ learning = virHashLookup(vars->hashTable, "ip_learning"); +

Can you add this as documentation to the nwfilter documentation page?

if (virHashSize(missing_vars->hashTable) == 1) { if (virHashLookup(missing_vars->hashTable, NWFILTER_STD_VAR_IP) != NULL) { - if (virNWFilterLookupLearnReq(ifindex) == NULL) { - rc = virNWFilterLearnIPAddress(techdriver, - ifname, - ifindex, - linkdev, - nettype, macaddr, - filter->name, - vars, driver, - DETECT_DHCP|DETECT_STATIC); + if (strcasecmp(learning, "none") == 0) { /* no learning */ + reportIP = true; + goto err_unresolvable_vars; } - goto err_exit; - } - goto err_unresolvable_vars; + if (strcasecmp(learning, "dhcp") == 0) { + rc = virNWFilterDHCPSnoopReq(techdriver, ifname, ifindex, + linkdev, nettype, macaddr, + filter->name, vars, driver); + goto err_exit; + } else if (strcasecmp(learning, "any") == 0) { + if (virNWFilterLookupLearnReq(ifindex) == NULL) { + rc = virNWFilterLearnIPAddress(techdriver, + ifname, + ifindex, + linkdev, + nettype, macaddr, + filter->name, + vars, driver, + DETECT_DHCP|DETECT_STATIC); + } + goto err_exit; + } else { + rc = 1; + virNWFilterReportError(VIR_ERR_PARSE_FAILED, _("filter '%s' " + "learning value '%s' invalid."), + filter->name, learning); + } + } else + goto err_unresolvable_vars; } else if (virHashSize(missing_vars->hashTable)> 1) { goto err_unresolvable_vars; } else if (!forceWithPendingReq&& @@ -738,7 +761,7 @@ err_exit:

err_unresolvable_vars:

- buf = virNWFilterPrintVars(missing_vars->hashTable, ", ", false, false); + buf = virNWFilterPrintVars(missing_vars->hashTable, ", ", false, reportIP); if (buf) { virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, _("Cannot instantiate filter due to unresolvable " @@ -1070,6 +1093,8 @@ _virNWFilterTeardownFilter(const char *ifname) return 1; }

+ virNWFilterDHCPSnoopEnd(ifname); + virNWFilterTerminateLearnReq(ifname);

if (virNWFilterLockIface(ifname))

I'll try it. Stefan

Stefan Berger <stefanb@linux.vnet.ibm.com> wrote on 10/26/2011 11:32:25 AM:

...

...

diff --git a/examples/xml/nwfilter/no-ip-spoofing.xml b/examples/xml/nwfilter/no-ip-spoofing.xml index b8c94c8..e5969a0 100644 --- a/examples/xml/nwfilter/no-ip-spoofing.xml +++ b/examples/xml/nwfilter/no-ip-spoofing.xml @@ -4,4 +4,9 @@ <rule action='drop' direction='out'> <ip match='no' srcipaddr='$IP' /> </rule> +<!-- allow DHCP requests --> +<rule action='return' direction='out'> +<ip match='yes' srcipaddr='0.0.0.0' protocol='udp' srcportstart='68' + srcportend='68' /> +</rule> </filter> Is this change necessary? Is it needed for the first instantiation of the rules or is it needed to support the case when all IP addresses have expired? I am asking because below I saw you calling The order of the two rules as given here is wrong. As long as the IP variable is not set, it will not generate the filter and once the variable is set it will never get to the 2nd rule.

I've removed this in the version I'm testing now. The DHCP-only rules cover this without multiple address support. In the original, I generated all chains and added and removed rules from those chains dynamically with address addition and removal (ie, it did not use virNWFilterInstantiateLFilterLate()). I didn't see any ordering issues in either version -- don't know what you mean there, but as you pointed out, this change is unnecessary in the stripped-down single-address version.

Also, in case learning is NULL, you may need to set it to 'any' (for backwards-compatibility) to avoid a NULL pointer crash later on.

I didn't try it with NULL -- if it is not set (ie, the variable is not present at all), it defaults to "any" already. If it's set to garbage, that's an error case which I handle, unless I I've broken it in later versions -- I'll retest this in v5, where I'm incorporating your other comments now.

In the other file there's an fclose(fp) that causes a crash (fp=NULL) in case the lease file was not available -> move the fclose() two lines up.

Once the 2nd patch is applied and libvirt started, I see this here

2011-09-26 14:26:06.206: 21084: warning : networkAddGeneralIptablesRules:1270 : May need to update iptables package & kernel to support CHECKSUM rule. *** glibc detected *** /usr/sbin/libvirtd: malloc(): memory corruption: 0x00007fc5b433e010 ***

This does not occur with only the first patch applied and only occurs if

a lease file was found. So something in the leasefile support code is corrupting memory.

I didn't see any problems of this sort, with no pre-existing lease file or an existing one with various leases, active, deleted and expired. I'll look at it some more and see if I can figure out what you're seeing.

Also it probably should re-create the filters in case libvirt is restarted. Somehow you should be able to use the lease files to get the IP address to rebuild the filters. The 'IP learning' subsytem just did the static IP address detection in this case but with DHCP snooping one shouldn't have to cycle the interfaces to cause the DHCP protocol to run.

Yes, that's the point of the lease file. I did only simple tests (but including libvirtd restart) for this version (v4) since I don't think anything I changed affected this from previous revisions, but I'll stress it a little more and see if I can reproduce what you're seeing. I did have some issues with restarting libvirtd (primarily VM's exiting when I restarted multiple times), but those were there before I applied any of these patches; no log messages when that happened. +-DLS