On 11/10/2011 03:28 PM, David L Stevens wrote:

This patch adds DHCP Snooping support to libvirt.

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

diff --git a/docs/formatnwfilter.html.in b/docs/formatnwfilter.html.in index 8df4a93..8003320 100644 --- a/docs/formatnwfilter.html.in +++ b/docs/formatnwfilter.html.in @@ -1775,6 +1775,23 @@ <br/><br/> In case a VM is resumed after suspension or migrated, IP address detection will be restarted. +<br/><br/> + Variable<i>ip_learning</i> may be used to specify + the IP address learning method. Valid values are<i>any</i>,<i>dhcp</i>, + or<i>none</i>. The default value is<i>any</i>, meaning that libvirt + may use any packet to determine the address in use by a VM. A value of +<i>dhcp</i> specifies that libvirt should only honor DHCP server-assigned + addresses with valid leases. If<i>ip_learning</i> is set to<i>none</i>, + libvirt does not do address learning and referencing<i>IP</i> without + assigning it an explicit value is an error. +<br/><br/> + Use of<i>ip_learning=dhcp</i> (DHCP snooping) provides additional + anti-spoofing security, especially when combined with a filter allowing + only trusted DHCP servers to assign addresses. If the DHCP lease expires, + the VM will no longer be able to use the IP address until it acquires a + new, valid lease from a DHCP server. If the VM is migrated, it must get + a new valid DHCP lease to use an IP address (e.g., by + bringing the VM interface down and up again). </p>

Can you add a sentence that it must be used in combination with a filter that allows DHCP traffic to pass?

<h3><a name="nwflimitsmigr">VM Migration</a></h3> diff --git a/examples/xml/nwfilter/no-ip-spoofing.xml b/examples/xml/nwfilter/no-ip-spoofing.xml index b8c94c8..7c7fd46 100644 --- a/examples/xml/nwfilter/no-ip-spoofing.xml +++ b/examples/xml/nwfilter/no-ip-spoofing.xml @@ -1,5 +1,10 @@ <filter name='no-ip-spoofing' chain='ipv4'>

+<!-- allow DHCP requests --> +<rule action='accept' direction='out'> +<ip match='yes' srcipaddr='0.0.0.0' protocol='udp' srcportstart='68' + srcportend='68' /> +</rule> <!-- drop if srcipaddr is not the IP address of the guest --> <rule action='drop' direction='out'> <ip match='no' srcipaddr='$IP' /> 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..8a37a6f --- /dev/null +++ b/src/nwfilter/nwfilter_dhcpsnoop.c @@ -0,0 +1,745 @@ +/* + * 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; + /* 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) { + *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_install - install rule for a lease + */ +static int +ipl_install(struct iplease *ipl) +{ + char ipbuf[20]; /* dotted decimal IP addr string */ + char *ipstr; + int rc; + + if (!inet_ntop(AF_INET,&ipl->ipl_ipaddr, ipbuf, sizeof(ipbuf))) { + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, + _("ipl_install inet_ntop " "failed (0x%08X)"), + ipl->ipl_ipaddr); + return -1; + } + + ipstr = strdup(ipbuf); + if (!ipstr) { + virReportOOMError(); + return -1; + } + if (virNWFilterHashTablePut(ipl->ipl_req->vars, "IP", ipstr, 1)) { + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, + _("Could not add variable \"IP\" to hashmap")); + VIR_FREE(ipstr); + return -1; + } + rc = virNWFilterInstantiateFilterLate(NULL, + ipl->ipl_req->ifname, + ipl->ipl_req->ifindex, + ipl->ipl_req->linkdev, + ipl->ipl_req->nettype, + ipl->ipl_req->macaddr, + ipl->ipl_req->filtername, + ipl->ipl_req->vars, + ipl->ipl_req->driver); + if (rc) { + VIR_FREE(ipstr); + return -1; + } + return 0; +} + +/* + * ipl_add - create or update an IP lease + */ +static void +ipl_add(struct iplease *plnew) +{ + struct iplease *pl; + struct virNWFilterSnoopReq *req = plnew->ipl_req; + + pl = ipl_getbyip(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 (req->start) + return; /* silently ignore multiple addresses */ + + if (VIR_ALLOC(pl)< 0) { + virReportOOMError(); + return; + } + *pl = *plnew; + + if (ipl_install(pl)< 0) { + 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 virNWFilterSnoopReq *req, uint32_t ipaddr) +{ + struct iplease *ipl; + + ipl = ipl_getbyip(req->start, ipaddr); + if (ipl == NULL) + return; + + 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) +{ + if (timeout< ipl->ipl_timeout) + return; /* no take-backs */ + 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; +} + +/* + * 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, req->start->ipl_ipaddr); + return 0; +} + +typedef unsigned char Eaddr[6]; + +struct eth { + Eaddr eh_dst; + Eaddr eh_src; + unsigned short eh_type; + union { + unsigned char eu_data[0]; + struct vlan_hdr { + unsigned short ev_flags; + unsigned short ev_type; + unsigned char ev_data[0]; + } eu_vlh; + } eth_un; +} ATTRIBUTE_PACKED; + +#define eh_data eth_un.eu_data +#define ehv_data eth_un.eu_vlh.ev_data +#define ehv_type eth_un.eu_vlh.ev_type + +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; + if (memcmp(dhcp_magic, pd->d_opts, sizeof dhcp_magic) != 0) + return -1; /* bad magic */ + oind += sizeof dhcp_magic; + + 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; + int mtype, leasetime; + + /* go through the protocol headers */ + switch (ntohs(pep->eh_type)) { + case ETHERTYPE_IP: + pip = (struct iphdr *) pep->eh_data; + len -= offsetof(struct eth, eh_data); + break; + case ETHERTYPE_VLAN: + if (ntohs(pep->ehv_type) != ETHERTYPE_IP) + return; + pip = (struct iphdr *) pep->ehv_data; + len -= offsetof(struct eth, ehv_data); + break;

Probably the previous discussions were a bit misleading. ebtables cannot filter ARP,RARP,ipv4 and IPv6 traffic when the VM is sending VLAN traffic. So we really have to ignore VLAN traffic and NOT instantiate any filters due to packets with VLAN headers since we won't be able to filter that later. I have a patch for that removing similar code in the ip learning code and have a section in limitations for the documentation -- all pending patches. I guess treat it like the 'default' case.

+ default: + return; + } + 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: + ipl_del(req, ipl.ipl_ipaddr); + 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"); Preferring snprintf. + 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; + + if (!req) + return; + + /* free all leases */ + snoop_lock(); + for (ipl = req->start; ipl; ipl = req->start) + ipl_del(req, ipl->ipl_ipaddr); + snoop_unlock(); + + /* free all req data */ + VIR_FREE(req->ifname); + VIR_FREE(req->linkdev); + VIR_FREE(req->filtername); + virNWFilterHashTableFree(req->vars); + 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; + pthread_t thread; + + snoop_lock(); + req = virHashLookup(SnoopReqs, ifname); + snoop_unlock(); + if (req) + return 0; + + if (VIR_ALLOC(req)< 0) { + virReportOOMError(); + return 1; Ok, I'll convert these to a '-1' in a later cleanup patch. + } + + req->ifname = strdup(ifname); + if (req->ifname == NULL) { + snoopReqFree(req); + virReportOOMError(); + return 1; + } + + req->techdriver = techdriver; + req->ifindex = ifindex; + req->linkdev = linkdev ? strdup(linkdev) : NULL; + req->nettype = nettype; + memcpy(req->macaddr, macaddr, sizeof(req->macaddr)); + req->filtername = strdup(filtername); + if (req->filtername == NULL) { + 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(&thread, NULL, virNWFilterDHCPSnoop, req) != 0) { + (void) virHashRemoveEntry(SnoopReqs, ifname); + snoopReqFree(req); + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, + _("virNWFilterDHCPSnoopReq pthread_create failed" + " on interface \"%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; + } + 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 */ Following coding style, the if branch should have a '{'. + virHashFree(SnoopReqs); + SnoopReqs = virHashCreate(0, freeReq); + if (!SnoopReqs) { + snoop_unlock(); + virReportOOMError(); + return; + } + } + snoop_unlock(); +} + +#else /* HAVE_LIBPCAP */ +int +virNWFilterDHCPSnoopInit(void) +{ + return -1; +} + +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..94762d1 --- /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..b877d9d 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; @@ -149,6 +153,7 @@ nwfilterDriverReload(void) { conn = virConnectOpen("qemu:///system");

if (conn) { + virNWFilterDHCPSnoopEnd(0); /* shut down all threads -- they will be restarted if necessary */ virNWFilterLearnThreadsTerminate(true);

@@ -201,6 +206,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..2b10f9b 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; + bool reportIP = false;

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

+ learning = virHashLookup(vars->hashTable, "ip_learning"); + if (learning == NULL) + learning = NWFILTER_DFLT_LEARN; + 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 (c_strcasecmp(learning, "none") == 0) { /* no learning */ + reportIP = true; + goto err_unresolvable_vars; } - goto err_exit; - } - goto err_unresolvable_vars; + if (c_strcasecmp(learning, "dhcp") == 0) { + rc = virNWFilterDHCPSnoopReq(techdriver, ifname, ifindex, + linkdev, nettype, macaddr, + filter->name, vars, driver); + goto err_exit; + } else if (c_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 +763,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 +1095,8 @@ _virNWFilterTeardownFilter(const char *ifname) return 1; }

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

if (virNWFilterLockIface(ifname)) I have tried it now and ended up with this leasefile:

cat run/libvirt/network/nwfilter.leases 1321458854 vnet0 9.59.241.232 9.59.241.10 1321458915 vnet0 9.59.241.232 9.59.241.10 For some reason two entries ended up in the leasefile for the same IP address. Any idea why 'kill -SIGHUP `pidof libvirtd`' doesn't restore the filtering rules? It cleans the ebtables tables but then nothing gets rebuilt. When restarting libvirtd it works as expected. Stefan

On 11/10/2011 03:28 PM, David L Stevens wrote:

This patch adds support for saving DHCP snooping leases to an on-disk file and restoring saved leases that are still active on restart.

Signed-off-by: David L Stevens<dlstevens@us.ibm.com> --- src/nwfilter/nwfilter_dhcpsnoop.c | 270 +++++++++++++++++++++++++++++++++---- 1 files changed, 243 insertions(+), 27 deletions(-)

diff --git a/src/nwfilter/nwfilter_dhcpsnoop.c b/src/nwfilter/nwfilter_dhcpsnoop.c index 8a37a6f..918ad7b 100644 --- a/src/nwfilter/nwfilter_dhcpsnoop.c +++ b/src/nwfilter/nwfilter_dhcpsnoop.c @@ -55,10 +55,18 @@ #include "nwfilter_gentech_driver.h" #include "nwfilter_ebiptables_driver.h" #include "nwfilter_dhcpsnoop.h" +#include "virfile.h" +#include "configmake.h"

#define VIR_FROM_THIS VIR_FROM_NWFILTER

#ifdef HAVE_LIBPCAP + +#define LEASEFILE LOCALSTATEDIR "/run/libvirt/network/nwfilter.leases" +#define TMPLEASEFILE LOCALSTATEDIR "/run/libvirt/network/nwfilter.ltmp" +static int lease_fd = -1; +static int nleases = 0; /* number of active leases */ +static int wleases = 0; /* number of written leases */

static virHashTablePtr SnoopReqs; static pthread_mutex_t SnoopLock; @@ -76,6 +84,7 @@ struct virNWFilterSnoopReq { const char *filtername; virNWFilterHashTablePtr vars; virNWFilterDriverStatePtr driver; + bool running; /* start and end of lease list, ordered by lease time */ struct iplease *start; struct iplease *end; @@ -96,7 +105,15 @@ struct iplease {

static struct iplease *ipl_getbyip(struct iplease *start, uint32_t ipaddr); static void ipl_update(struct iplease *pl, uint32_t timeout); + +static struct virNWFilterSnoopReq *newreq(const char *ifname);

+static void lease_open(void); +static void lease_close(void); +static void lease_load(void); +static void lease_save(struct iplease *ipl); +static void lease_restore(struct virNWFilterSnoopReq *req); +static void lease_refresh(void);

/* * ipl_ladd - add an IP lease to a list @@ -187,7 +204,7 @@ ipl_install(struct iplease *ipl) * ipl_add - create or update an IP lease */ static void -ipl_add(struct iplease *plnew) +ipl_add(struct iplease *plnew, bool update_leasefile) { struct iplease *pl; struct virNWFilterSnoopReq *req = plnew->ipl_req; @@ -195,6 +212,8 @@ ipl_add(struct iplease *plnew) pl = ipl_getbyip(req->start, plnew->ipl_ipaddr); if (pl) { ipl_update(pl, plnew->ipl_timeout); + if (update_leasefile) + lease_save(pl); return; } /* support for multiple addresses requires the ability to add filters @@ -212,11 +231,14 @@ ipl_add(struct iplease *plnew) } *pl = *plnew;

- if (ipl_install(pl)< 0) { + if (req->running&& ipl_install(pl)< 0) { VIR_FREE(pl); return; } ipl_tadd(pl); + nleases++; + if (update_leasefile) + lease_save(pl); }

/* @@ -252,7 +274,7 @@ ipl_tdel(struct iplease *ipl) * ipl_del - delete an IP lease */ static void -ipl_del(struct virNWFilterSnoopReq *req, uint32_t ipaddr) +ipl_del(struct virNWFilterSnoopReq *req, uint32_t ipaddr, bool update_leasefile) { struct iplease *ipl;

@@ -262,13 +284,18 @@ ipl_del(struct virNWFilterSnoopReq *req, uint32_t ipaddr)

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"); + if (update_leasefile) { + lease_save(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); + nleases--; }

/* @@ -308,7 +335,7 @@ ipl_trun(struct virNWFilterSnoopReq *req)

now = time(0); while (req->start&& req->start->ipl_timeout<= now) - ipl_del(req, req->start->ipl_ipaddr); + ipl_del(req, req->start->ipl_ipaddr, 1); return 0; }

@@ -467,11 +494,11 @@ dhcpdecode(struct virNWFilterSnoopReq *req, struct eth *pep, int len)

switch (mtype) { case DHCPACK: - ipl_add(&ipl); + ipl_add(&ipl, 1); break; case DHCPDECLINE: case DHCPRELEASE: - ipl_del(req, ipl.ipl_ipaddr); + ipl_del(req, ipl.ipl_ipaddr, 1); break; default: break; @@ -521,7 +548,7 @@ snoopReqFree(struct virNWFilterSnoopReq *req) /* free all leases */ snoop_lock(); for (ipl = req->start; ipl; ipl = req->start) - ipl_del(req, ipl->ipl_ipaddr); + ipl_del(req, ipl->ipl_ipaddr, 0); snoop_unlock();

/* free all req data */ @@ -547,6 +574,8 @@ virNWFilterDHCPSnoop(void *req0)

ifindex = if_nametoindex(req->ifname);

+ lease_restore(req); + while (1) { if (req->die) break; @@ -583,23 +612,22 @@ virNWFilterDHCPSnoopReq(virNWFilterTechDriverPtr techdriver, { struct virNWFilterSnoopReq *req; pthread_t thread; + bool isnewreq;

snoop_lock(); req = virHashLookup(SnoopReqs, ifname); - snoop_unlock(); - if (req) - return 0; - - if (VIR_ALLOC(req)< 0) { - virReportOOMError(); - return 1; - } - - req->ifname = strdup(ifname); - if (req->ifname == NULL) { - snoopReqFree(req); - virReportOOMError(); - return 1; + isnewreq = req == NULL; + if (!isnewreq) { + if (req->running) { + snoop_unlock(); + return 0; + } + snoop_unlock(); + } else { + snoop_unlock(); + req = newreq(ifname); + if (!req) + return 1; }

req->techdriver = techdriver; @@ -634,8 +662,10 @@ virNWFilterDHCPSnoopReq(virNWFilterTechDriverPtr techdriver, } req->driver = driver;

+ req->running = 1; + snoop_lock(); - if (virHashAddEntry(SnoopReqs, ifname, req)) { + if (isnewreq&& virHashAddEntry(SnoopReqs, ifname, req)) { snoop_unlock(); virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, _("virNWFilterDHCPSnoopReq req add failed on " @@ -669,6 +699,20 @@ freeReq(void *req0, const void *name ATTRIBUTE_UNUSED) req->die = 1; }

+static void +lease_close(void) +{ + VIR_FORCE_CLOSE(lease_fd); +} + +static void +lease_open(void) +{ + lease_close(); + + lease_fd = open(LEASEFILE, O_CREAT|O_RDWR|O_APPEND, 0644); +} + int virNWFilterDHCPSnoopInit(void) { @@ -687,6 +731,8 @@ virNWFilterDHCPSnoopInit(void) virReportOOMError(); return -1; } + lease_load(); + lease_open(); snoop_unlock(); return 0; } @@ -709,10 +755,180 @@ virNWFilterDHCPSnoopEnd(const char *ifname) virReportOOMError(); return; } + lease_load(); } + lease_close(); snoop_unlock(); }

+static int +lease_write(int lfd, const char *ifname, struct iplease *ipl) +{ + char lbuf[256],ipstr[16],dhcpstr[16]; + + if (inet_ntop(AF_INET,&ipl->ipl_ipaddr, ipstr, sizeof ipstr) == 0) { + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, + _("inet_ntop(0x%08X) failed"), ipl->ipl_ipaddr); + return -1; + } + if (inet_ntop(AF_INET,&ipl->ipl_server, dhcpstr, sizeof dhcpstr) == 0) { + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, + _("inet_ntop(0x%08X) failed"), ipl->ipl_server); + return -1; + } + /* time intf ip dhcpserver */ + snprintf(lbuf, sizeof(lbuf), "%u %s %s %s\n", ipl->ipl_timeout, + ifname, ipstr, dhcpstr); + if (write(lfd, lbuf, strlen(lbuf))< 0) { + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, + _("lease file write failed: %s"), + strerror(errno)); + return -1; + } + (void) fsync(lfd); + return 0; +} + +static void +lease_save(struct iplease *ipl) +{ + struct virNWFilterSnoopReq *req = ipl->ipl_req; + + if (lease_fd< 0) + lease_open(); + if (lease_write(lease_fd, req->ifname, ipl)< 0) + return; + /* keep dead leases at< ~95% of file size */ + if (++wleases>= nleases*20) + lease_load(); /* load& refresh lease file */ +} + +static struct virNWFilterSnoopReq * +newreq(const char *ifname) +{ + struct virNWFilterSnoopReq *req; + + if (VIR_ALLOC(req)< 0) { + virReportOOMError(); + return NULL; + } + req->ifname = strdup(ifname); + + return req; +} + +static void +SaveSnoopReqIter(void *payload, + const void *name ATTRIBUTE_UNUSED, + void *data) +{ + struct virNWFilterSnoopReq *req = payload; + int tfd = (int)data; + struct iplease *ipl; + + for (ipl = req->start; ipl; ipl = ipl->ipl_next) + (void) lease_write(tfd, req->ifname, ipl); +} + +static void +lease_refresh(void) +{ + int tfd; + + (void) unlink(TMPLEASEFILE); + /* lease file loaded, delete old one */ + tfd = open(TMPLEASEFILE, O_CREAT|O_RDWR|O_TRUNC|O_EXCL, 0644); + if (tfd< 0) { + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, + _("open(\"%s\"): %s"), + TMPLEASEFILE, strerror(errno)); + return; + } + if (SnoopReqs) + virHashForEach(SnoopReqs, SaveSnoopReqIter, (void *)tfd); To avoid the warnings pass '&tfd' and adjust the iterator accordingly.

nwfilter/nwfilter_dhcpsnoop.c: In function 'SaveSnoopReqIter': nwfilter/nwfilter_dhcpsnoop.c:826:15: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast] nwfilter/nwfilter_dhcpsnoop.c: In function 'lease_refresh': nwfilter/nwfilter_dhcpsnoop.c:848:53: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]

+ (void) close(tfd); Use VIR_FORCE_CLOSE(). + if (rename(TMPLEASEFILE, LEASEFILE)< 0) { + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, + _("rename(\"%s\", \"%s\"): %s"), + TMPLEASEFILE, LEASEFILE, strerror(errno)); + (void) unlink(TMPLEASEFILE); + } + wleases = 0; + lease_open(); +} + + +static void +lease_load(void) +{ + char line[256], ifname[16], ipstr[16], srvstr[16]; + struct iplease ipl; + struct virNWFilterSnoopReq *req; + time_t now; + FILE *fp; + int ln = 0; + + fp = fopen(LEASEFILE, "r"); + time(&now); + while (fp&& fgets(line, sizeof(line), fp)) { + if (line[strlen(line)-1] != '\n') { + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, + _("lease_load lease file line %d corrupt"), + ln); + break; + } + ln++; + if (sscanf(line, "%u %16s %16s %16s",&ipl.ipl_timeout, + ifname, ipstr, srvstr)< 4) { + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, + _("lease_load lease file line %d corrupt"), + ln); + break;; + } + if (ipl.ipl_timeout&& ipl.ipl_timeout< now) + continue; + req = virHashLookup(SnoopReqs, ifname); + if (!req) { + req = newreq(ifname); + if (!req) + break; + if (virHashAddEntry(SnoopReqs, ifname, req)) { + snoopReqFree(req); + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, + _("lease_load req add failed on " + "interface \"%s\""), ifname); + continue; + } + } + + if (inet_pton(AF_INET, ipstr,&ipl.ipl_ipaddr)<= 0) { + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, + _("line %d corrupt ipaddr \"%s\""), + ln, ipstr); + continue; + } + (void) inet_pton(AF_INET, srvstr,&ipl.ipl_server); + ipl.ipl_req = req; + + if (ipl.ipl_timeout) + ipl_add(&ipl, 0); + else + ipl_del(req, ipl.ipl_ipaddr, 0); + } + if (fp != NULL) + (void) fclose(fp); Use VIR_FORCE_FCLOSE(). No checking for fp != NULL necessary then. + lease_refresh(); +} + +static void +lease_restore(struct virNWFilterSnoopReq *req) +{ + struct iplease *ipl; + + for (ipl=req->start; ipl; ipl=ipl->ipl_next) + (void) ipl_install(ipl); +} + #else /* HAVE_LIBPCAP */ int virNWFilterDHCPSnoopInit(void) The code looks good now, but please fix the nits. I'll test it more later today.

When I start a VM that was previously running and thus still has a lease and restart libvirt while it is booting, libvirtd seems to automatically rebuilds the filters to the previous leased IP address. Can we invalidate the previous leases once new detection starts? Also, I would rather NOT use the name of the interface as a criterion to disable previous leases but the MAC address of the interface since the name of the interface can change (and another VM can now have 'vnet0'). I think the MAC address that an IP address is associated with should go into the lease file rather than the interface name. Would it be possible to change it in that direction? Stefan