Changes from V1 to V2: - using INT_BUFSIZE_BOUND() to determine the length of the buffersize for printing and integer into - not explicitly initializing static var threadsTerminate to false anymore, since that's done automatically - there are 2 calls in the virNWFilterLockIface function that can fail due to OOM error; in case that happens, loop until the memory is available to avoid a failing lock. The call initializing the mutex should never fail with pthreads, otherwise we'd have a compile-time problem (wrong parameters for recursive lock) and may fail due to OOM error on win-32, so I am also looping there. The interface name may cause a failure, but callers should be certain at this point that the string is suitable. I am adding ATTRIBUTE_RETURN_CHECK to force the caller to check for failure and adapting the code where the function is called. Since the name of an interface can be the same between stops and starts of different VMs I have to switch the IP address learning thread to use the index of the interface to determine whether an interface is still available or not - in the case of macvtap the thread needs to listen for traffic on the physical interface, thus having to time out periodically to check whether the VM's macvtap device is still there as an indication that the VM is still alive. Previously the following sequence of 2 VMs with macvtap device virsh start testvm1; virsh destroy testvm1 ; virsh start testvm2 would not terminate the thread upon testvm1's destroy since the name of the interface on the host could be the same (i.e, macvtap0) on testvm1 and testvm2, thus it was easily race-able. The thread would then determine the IP address parameter for testvm2 but apply the rule set for testvm1. :-( I am also introducing a lock for the interface (by name) that the thread must hold while it listens for the traffic and releases when it terminates upon VM termination or 0.5 second thereafter. Thus, the new thread for a newly started VM with the same interface name will not start while the old one still holds the lock. The only other code that I see that also needs to grab the lock to serialize operation is the one that tears down the firewall that were established on behalf of an interface. I am moving the code applying the 'basic' firewall rules during the IP address learning phase inside the thread but won't start the thread unless it is ensured that the firewall driver has the ability to apply the 'basic' firewall rules. Signed-off-by: Stefan Berger <stefanb(a)us.ibm.com&gt; --- src/nwfilter/nwfilter_gentech_driver.c | 24 +++ src/nwfilter/nwfilter_gentech_driver.h | 2 src/nwfilter/nwfilter_learnipaddr.c | 239 +++++++++++++++++++++++++++------ src/nwfilter/nwfilter_learnipaddr.h | 7 4 files changed, 229 insertions(+), 43 deletions(-) Index: libvirt-acl/src/nwfilter/nwfilter_learnipaddr.c =================================================================== --- libvirt-acl.orig/src/nwfilter/nwfilter_learnipaddr.c +++ libvirt-acl/src/nwfilter/nwfilter_learnipaddr.c @@ -36,6 +36,7 @@ #include <netinet/ip.h> #include <netinet/udp.h> #include <net/if_arp.h> +#include <intprops.h> #include "internal.h" @@ -54,6 +55,11 @@ #define VIR_FROM_THIS VIR_FROM_NWFILTER +#define IFINDEX2STR(VARNAME, ifindex) \ + char VARNAME[INT_BUFSIZE_BOUND(ifindex)]; \ + snprintf(VARNAME, sizeof(VARNAME), "%d", ifindex); + +#define PKT_TIMEOUT_MS 500 /* ms */ /* structure of an ARP request/reply message */ struct f_arphdr { @@ -109,6 +115,96 @@ static virHashTablePtr pendingLearnReq; static virMutex ipAddressMapLock; static virNWFilterHashTablePtr ipAddressMap; +static virMutex ifaceMapLock; +static virHashTablePtr ifaceLockMap; + +typedef struct _virNWFilterIfaceLock virNWFilterIfaceLock; +typedef virNWFilterIfaceLock *virNWFilterIfaceLockPtr; +struct _virNWFilterIfaceLock { + char ifname[IF_NAMESIZE]; + virMutex lock; + int refctr; +}; + + +static bool threadsTerminate = false; + + +int +virNWFilterLockIface(const char *ifname) { + virNWFilterIfaceLockPtr ifaceLock; + + virMutexLock(&ifaceMapLock); + + ifaceLock = virHashLookup(ifaceLockMap, ifname); + if (!ifaceLock) { + while (VIR_ALLOC(ifaceLock) < 0) { + /* wait for memory */ + usleep(50); + } + + if (virMutexInitRecursive(&ifaceLock->lock)) { + /* win32 - wait for memory */ + usleep(50); + } + + if (virStrcpyStatic(ifaceLock->ifname, ifname) == NULL) { + VIR_FREE(ifaceLock); + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, + _("%s: interface name %s does not fit into " + "buffer "), + __FUNCTION__, ifaceLock->ifname); + goto err_exit; + } + + while (virHashAddEntry(ifaceLockMap, ifname, ifaceLock)) { + /* wait for memory */ + usleep(50); + } + + ifaceLock->refctr = 0; + } + + ifaceLock->refctr++; + + virMutexUnlock(&ifaceMapLock); + + virMutexLock(&ifaceLock->lock); + + return 0; + + err_exit: + virMutexUnlock(&ifaceMapLock); + + return 1; +} + + +static void +freeIfaceLock(void *payload, const char *name ATTRIBUTE_UNUSED) { + VIR_FREE(payload); +} + + +void +virNWFilterUnlockIface(const char *ifname) { + virNWFilterIfaceLockPtr ifaceLock; + + virMutexLock(&ifaceMapLock); + + ifaceLock = virHashLookup(ifaceLockMap, ifname); + + if (ifaceLock) { + virMutexUnlock(&ifaceLock->lock); + + ifaceLock->refctr--; + if (ifaceLock->refctr == 0) + virHashRemoveEntry(ifaceLockMap, ifname, freeIfaceLock); + } + + virMutexUnlock(&ifaceMapLock); +} + static void virNWFilterIPAddrLearnReqFree(virNWFilterIPAddrLearnReqPtr req) { @@ -127,10 +223,12 @@ virNWFilterIPAddrLearnReqFree(virNWFilte static int virNWFilterRegisterLearnReq(virNWFilterIPAddrLearnReqPtr req) { int res = -1; + IFINDEX2STR(ifindex_str, req->ifindex); + virMutexLock(&pendingLearnReqLock); - if (!virHashLookup(pendingLearnReq, req->ifname)) - res = virHashAddEntry(pendingLearnReq, req->ifname, req); + if (!virHashLookup(pendingLearnReq, ifindex_str)) + res = virHashAddEntry(pendingLearnReq, ifindex_str, req); virMutexUnlock(&pendingLearnReqLock); @@ -141,12 +239,13 @@ virNWFilterRegisterLearnReq(virNWFilterI virNWFilterIPAddrLearnReqPtr -virNWFilterLookupLearnReq(const char *ifname) { +virNWFilterLookupLearnReq(int ifindex) { void *res; + IFINDEX2STR(ifindex_str, ifindex); virMutexLock(&pendingLearnReqLock); - res = virHashLookup(pendingLearnReq, ifname); + res = virHashLookup(pendingLearnReq, ifindex_str); virMutexUnlock(&pendingLearnReqLock); @@ -163,15 +262,16 @@ freeLearnReqEntry(void *payload, const c #ifdef HAVE_LIBPCAP static virNWFilterIPAddrLearnReqPtr -virNWFilterDeregisterLearnReq(const char *ifname) { +virNWFilterDeregisterLearnReq(int ifindex) { virNWFilterIPAddrLearnReqPtr res; + IFINDEX2STR(ifindex_str, ifindex); virMutexLock(&pendingLearnReqLock); - res = virHashLookup(pendingLearnReq, ifname); + res = virHashLookup(pendingLearnReq, ifindex_str); if (res) - virHashRemoveEntry(pendingLearnReq, ifname, NULL); + virHashRemoveEntry(pendingLearnReq, ifindex_str, NULL); virMutexUnlock(&pendingLearnReqLock); @@ -274,7 +374,7 @@ static void * learnIPAddressThread(void *arg) { char errbuf[PCAP_ERRBUF_SIZE] = {0}; - pcap_t *handle; + pcap_t *handle = NULL; struct bpf_program fp; struct pcap_pkthdr header; const u_char *packet; @@ -285,19 +385,27 @@ learnIPAddressThread(void *arg) unsigned int ethHdrSize; char *listen_if = (strlen(req->linkdev) != 0) ? req->linkdev : req->ifname; - int to_ms = (strlen(req->linkdev) != 0) ? 1000 - : 0; int dhcp_opts_len; char macaddr[VIR_MAC_STRING_BUFLEN]; virBuffer buf = VIR_BUFFER_INITIALIZER; - char *filter= NULL; + char *filter = NULL; uint16_t etherType; + bool showError = true; enum howDetect howDetected = 0; virNWFilterTechDriverPtr techdriver = req->techdriver; + if (virNWFilterLockIface(req->ifname)) + goto err_no_lock; + req->status = 0; - handle = pcap_open_live(listen_if, BUFSIZ, 0, to_ms, errbuf); + /* anything change to the VM's interface -- check at least once */ + if (ifaceCheck(false, req->ifname, NULL, req->ifindex)) { + req->status = ENODEV; + goto done; + } + + handle = pcap_open_live(listen_if, BUFSIZ, 0, PKT_TIMEOUT_MS, errbuf); if (handle == NULL) { VIR_DEBUG("Couldn't open device %s: %s\n", listen_if, errbuf); @@ -309,11 +417,22 @@ learnIPAddressThread(void *arg) switch (req->howDetect) { case DETECT_DHCP: + if (techdriver->applyDHCPOnlyRules(req->ifname, + req->macaddr, + NULL)) { + req->status = EINVAL; + goto done; + } virBufferVSprintf(&buf, " ether dst %s" " and src port 67 and dst port 68", macaddr); break; default: + if (techdriver->applyBasicRules(req->ifname, + req->macaddr)) { + req->status = EINVAL; + goto done; + } virBufferVSprintf(&buf, "ether host %s", macaddr); } @@ -324,25 +443,36 @@ learnIPAddressThread(void *arg) filter = virBufferContentAndReset(&buf); - if (pcap_compile(handle, &fp, filter, 1, 0) != 0 || - pcap_setfilter(handle, &fp) != 0) { - VIR_DEBUG("Couldn't compile or set filter '%s'.\n", filter); + if (pcap_compile(handle, &fp, filter, 1, 0) != 0) { + VIR_DEBUG("Couldn't compile filter '%s'.\n", filter); req->status = EINVAL; goto done; } + if (pcap_setfilter(handle, &fp) != 0) { + VIR_DEBUG("Couldn't set filter '%s'.\n", filter); + req->status = EINVAL; + pcap_freecode(&fp); + goto done; + } + + pcap_freecode(&fp); + while (req->status == 0 && vmaddr == 0) { packet = pcap_next(handle, &header); if (!packet) { - if (to_ms == 0) { - /* assuming IF disappeared */ - req->status = ENODEV; + + if (threadsTerminate) { + req->status = ECANCELED; + showError = false; break; } - /* listening on linkdev, check whether VM's dev is still there */ - if (ifaceCheck(false, req->ifname, req->macaddr, -1)) { + + /* check whether VM's dev is still there */ + if (ifaceCheck(false, req->ifname, NULL, req->ifindex)) { req->status = ENODEV; + showError = false; break; } continue; @@ -470,6 +600,7 @@ learnIPAddressThread(void *arg) ret = virNWFilterInstantiateFilterLate(NULL, req->ifname, + req->ifindex, req->linkdev, req->nettype, req->macaddr, @@ -478,13 +609,23 @@ learnIPAddressThread(void *arg) req->driver); VIR_DEBUG("Result from applying firewall rules on " "%s with IP addr %s : %d\n", req->ifname, inetaddr, ret); + } else { + if (showError) + virReportSystemError(req->status, + "%s encountered an error. Shutting down " + "interface %s", + __FUNCTION__, req->ifname); + ifaceDown(req->ifname); } memset(&req->thread, 0x0, sizeof(req->thread)); VIR_DEBUG("pcap thread terminating for interface %s\n",req->ifname); - virNWFilterDeregisterLearnReq(req->ifname); + virNWFilterUnlockIface(req->ifname); + + err_no_lock: + virNWFilterDeregisterLearnReq(req->ifindex); virNWFilterIPAddrLearnReqFree(req); @@ -496,6 +637,7 @@ learnIPAddressThread(void *arg) * virNWFilterLearnIPAddress * @techdriver : driver to build firewalls * @ifname: the name of the interface + * @ifindex: the index of the interface * @linkdev : the name of the link device; currently only used in case of a * macvtap device * @nettype : the type of interface @@ -516,6 +658,7 @@ learnIPAddressThread(void *arg) int virNWFilterLearnIPAddress(virNWFilterTechDriverPtr techdriver, const char *ifname, + int ifindex, const char *linkdev, enum virDomainNetType nettype, const unsigned char *macaddr, @@ -530,6 +673,14 @@ virNWFilterLearnIPAddress(virNWFilterTec if (howDetect == 0) return 1; + if ( !techdriver->canApplyBasicRules()) { + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("IP parameter must be provided since " + "snooping the IP address does not work " + "possibly due to missing tools")); + return 1; + } + if (VIR_ALLOC(req) < 0) { virReportOOMError(); goto err_no_req; @@ -538,7 +689,7 @@ virNWFilterLearnIPAddress(virNWFilterTec ht = virNWFilterHashTableCreate(0); if (ht == NULL) { virReportOOMError(); - goto err_no_ht; + goto err_free_req; } if (virNWFilterHashTablePutAll(filterparams, ht)) @@ -565,6 +716,8 @@ virNWFilterLearnIPAddress(virNWFilterTec goto err_free_ht; } } + + req->ifindex = ifindex; req->nettype = nettype; memcpy(req->macaddr, macaddr, sizeof(req->macaddr)); req->driver = driver; @@ -576,35 +729,21 @@ virNWFilterLearnIPAddress(virNWFilterTec rc = virNWFilterRegisterLearnReq(req); if (rc) - goto err_free_ht; - - switch (howDetect) { - case DETECT_DHCP: - if (techdriver->applyDHCPOnlyRules(ifname, - macaddr, - NULL)) - goto err_free_ht; - break; - default: - if (techdriver->applyBasicRules(ifname, - macaddr)) - goto err_free_ht; - } - + goto err_free_req; if (pthread_create(&req->thread, NULL, learnIPAddressThread, req) != 0) - goto err_remove_rules; + goto err_dereg_req; return 0; -err_remove_rules: - techdriver->removeBasicRules(ifname); +err_dereg_req: + virNWFilterDeregisterLearnReq(ifindex); err_free_ht: virNWFilterHashTableFree(ht); -err_no_ht: +err_free_req: virNWFilterIPAddrLearnReqFree(req); err_no_req: return 1; @@ -615,6 +754,7 @@ err_no_req: int virNWFilterLearnIPAddress(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, @@ -637,6 +777,12 @@ virNWFilterLearnIPAddress(virNWFilterTec */ int virNWFilterLearnInit(void) { + + if (pendingLearnReq) + return 0; + + threadsTerminate = false; + pendingLearnReq = virHashCreate(0); if (!pendingLearnReq) { virReportOOMError(); @@ -660,6 +806,18 @@ virNWFilterLearnInit(void) { return 1; } + ifaceLockMap = virHashCreate(0); + if (!ifaceLockMap) { + virReportOOMError(); + virNWFilterLearnShutdown(); + return 1; + } + + if (virMutexInit(&ifaceMapLock)) { + virNWFilterLearnShutdown(); + return 1; + } + return 0; } @@ -670,9 +828,18 @@ virNWFilterLearnInit(void) { */ void virNWFilterLearnShutdown(void) { + + threadsTerminate = true; + + while (virHashSize(pendingLearnReq) != 0) + usleep((PKT_TIMEOUT_MS * 1000) / 3); + virHashFree(pendingLearnReq, freeLearnReqEntry); pendingLearnReq = NULL; virNWFilterHashTableFree(ipAddressMap); ipAddressMap = NULL; + + virHashFree(ifaceLockMap, freeIfaceLock); + ifaceLockMap = NULL; } Index: libvirt-acl/src/nwfilter/nwfilter_learnipaddr.h =================================================================== --- libvirt-acl.orig/src/nwfilter/nwfilter_learnipaddr.h +++ libvirt-acl/src/nwfilter/nwfilter_learnipaddr.h @@ -35,6 +35,7 @@ typedef virNWFilterIPAddrLearnReq *virNW struct _virNWFilterIPAddrLearnReq { virNWFilterTechDriverPtr techdriver; char ifname[IF_NAMESIZE]; + int ifindex; char linkdev[IF_NAMESIZE]; enum virDomainNetType nettype; unsigned char macaddr[VIR_MAC_BUFLEN]; @@ -49,6 +50,7 @@ struct _virNWFilterIPAddrLearnReq { int virNWFilterLearnIPAddress(virNWFilterTechDriverPtr techdriver, const char *ifname, + int ifindex, const char *linkdev, enum virDomainNetType nettype, const unsigned char *macaddr, @@ -57,12 +59,15 @@ int virNWFilterLearnIPAddress(virNWFilte virNWFilterDriverStatePtr driver, enum howDetect howDetect); -virNWFilterIPAddrLearnReqPtr virNWFilterLookupLearnReq(const char *ifname); +virNWFilterIPAddrLearnReqPtr virNWFilterLookupLearnReq(int ifindex); void virNWFilterDelIpAddrForIfname(const char *ifname); const char *virNWFilterGetIpAddrForIfname(const char *ifname); +int virNWFilterLockIface(const char *ifname) ATTRIBUTE_RETURN_CHECK; +void virNWFilterUnlockIface(const char *ifname); + int virNWFilterLearnInit(void); void virNWFilterLearnShutdown(void); Index: libvirt-acl/src/nwfilter/nwfilter_gentech_driver.c =================================================================== --- libvirt-acl.orig/src/nwfilter/nwfilter_gentech_driver.c +++ libvirt-acl/src/nwfilter/nwfilter_gentech_driver.c @@ -557,6 +557,7 @@ virNWFilterInstantiate(virConnectPtr con enum virDomainNetType nettype, virNWFilterDefPtr filter, const char *ifname, + int ifindex, const char *linkdev, virNWFilterHashTablePtr vars, enum instCase useNewFilter, int *foundNewFilter, @@ -592,9 +593,10 @@ virNWFilterInstantiate(virConnectPtr con if (virHashSize(missing_vars->hashTable) == 1) { if (virHashLookup(missing_vars->hashTable, NWFILTER_STD_VAR_IP) != NULL) { - if (virNWFilterLookupLearnReq(ifname) == NULL) { + if (virNWFilterLookupLearnReq(ifindex) == NULL) { rc = virNWFilterLearnIPAddress(techdriver, ifname, + ifindex, linkdev, nettype, macaddr, filter->name, @@ -639,11 +641,22 @@ virNWFilterInstantiate(virConnectPtr con if (rc) goto err_exit; + if (virNWFilterLockIface(ifname)) + goto err_exit; + rc = techdriver->applyNewRules(conn, ifname, nptrs, ptrs); if (teardownOld && rc == 0) techdriver->tearOldRules(conn, ifname); + if (rc == 0 && ifaceCheck(false, ifname, NULL, ifindex)) { + /* interface changed/disppeared */ + techdriver->allTeardown(ifname); + rc = 1; + } + + virNWFilterUnlockIface(ifname); + VIR_FREE(ptrs); } @@ -666,6 +679,7 @@ static int __virNWFilterInstantiateFilter(virConnectPtr conn, bool teardownOld, const char *ifname, + int ifindex, const char *linkdev, enum virDomainNetType nettype, const unsigned char *macaddr, @@ -767,6 +781,7 @@ __virNWFilterInstantiateFilter(virConnec nettype, filter, ifname, + ifindex, linkdev, vars, useNewFilter, &foundNewFilter, @@ -798,9 +813,15 @@ _virNWFilterInstantiateFilter(virConnect const char *linkdev = (net->type == VIR_DOMAIN_NET_TYPE_DIRECT) ? net->data.direct.linkdev : NULL; + int ifindex; + + if (ifaceGetIndex(true, net->ifname, &ifindex)) + return 1; + return __virNWFilterInstantiateFilter(conn, teardownOld, net->ifname, + ifindex, linkdev, net->type, net->mac, @@ -814,6 +835,7 @@ _virNWFilterInstantiateFilter(virConnect int virNWFilterInstantiateFilterLate(virConnectPtr conn, const char *ifname, + int ifindex, const char *linkdev, enum virDomainNetType nettype, const unsigned char *macaddr, @@ -825,6 +847,7 @@ virNWFilterInstantiateFilterLate(virConn rc = __virNWFilterInstantiateFilter(conn, 1, ifname, + ifindex, linkdev, nettype, macaddr, Index: libvirt-acl/src/nwfilter/nwfilter_gentech_driver.h =================================================================== --- libvirt-acl.orig/src/nwfilter/nwfilter_gentech_driver.h +++ libvirt-acl/src/nwfilter/nwfilter_gentech_driver.h @@ -49,6 +49,7 @@ int virNWFilterTearOldFilter(virConnectP int virNWFilterInstantiateFilterLate(virConnectPtr conn, const char *ifname, + int ifindex, const char *linkdev, enum virDomainNetType nettype, const unsigned char *macaddr, @@ -77,6 +78,7 @@ virNWFilterTearNWFilter(virDomainNetDefP static inline void virNWFilterTearVMNWFilters(virDomainObjPtr vm) { int i; + for (i = 0; i < vm->def->nnets; i++) virNWFilterTearNWFilter(vm->def->nets[i]); }