With support for multiple IP addresses per interface in place, this patch now adds support for multiple IP addresses per interface for the DHCP snooping code. Testing: Since the infrastructure I tested this with does not provide multiple IP addresses per MAC address (anymore), I either had to plug the VM's interface from the virtual bride connected directly to the infrastructure to virbr0 to get a 2nd IP address from dnsmasq (kill and run dhclient inside the VM) or changed the lease file (/var/run/libvirt/network/nwfilter.leases) and restart libvirtd to have a 2nd IP address on an existing interface. Note that dnsmasq can take a lease timeout parameter as part of the --dhcp-range command line parameter, so that timeouts can be tested that way (--dhcp-range 192.168.122.2,192.168.122.254,120). So, terminating and restarting dnsmasq with that parameter is another choice to watch an IP address disappear after 120 seconds. Regards, Stefan --- src/nwfilter/nwfilter_dhcpsnoop.c | 102 ++++++++++++++++++++++++++------------ 1 file changed, 70 insertions(+), 32 deletions(-) Index: libvirt-acl/src/nwfilter/nwfilter_dhcpsnoop.c =================================================================== --- libvirt-acl.orig/src/nwfilter/nwfilter_dhcpsnoop.c +++ libvirt-acl/src/nwfilter/nwfilter_dhcpsnoop.c @@ -59,6 +59,7 @@ #include "conf/domain_conf.h" #include "nwfilter_gentech_driver.h" #include "nwfilter_dhcpsnoop.h" +#include "nwfilter_ipaddrmap.h" #include "virnetdev.h" #include "virfile.h" #include "viratomic.h" @@ -222,7 +223,8 @@ struct _virNWFilterSnoopDHCPHdr { /* local function prototypes */ static int virNWFilterSnoopReqLeaseDel(virNWFilterSnoopReqPtr req, - uint32_t ipaddr, bool update_leasefile); + uint32_t ipaddr, bool update_leasefile, + bool instantiate); static void virNWFilterSnoopReqLock(virNWFilterSnoopReqPtr req); static void virNWFilterSnoopReqUnlock(virNWFilterSnoopReqPtr req); @@ -398,8 +400,8 @@ virNWFilterSnoopIPLeaseInstallRule(virNW { char ipbuf[INET_ADDRSTRLEN]; int rc = -1; - virNWFilterVarValuePtr ipVar; virNWFilterSnoopReqPtr req; + char *ipaddr; if (!inet_ntop(AF_INET, &ipl->IPAddress, ipbuf, sizeof(ipbuf))) { virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, @@ -408,18 +410,19 @@ virNWFilterSnoopIPLeaseInstallRule(virNW __func__, ipl->IPAddress); return -1; } - ipVar = virNWFilterVarValueCreateSimpleCopyValue(ipbuf); - if (!ipVar) { + + ipaddr = strdup(ipbuf); + if (ipaddr == NULL) { virReportOOMError(); return -1; } - if (virNWFilterHashTablePut(ipl->SnoopReq->vars, "IP", ipVar, 1) < 0) { - virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, - _("Could not add variable \"IP\" to hashmap")); - virNWFilterVarValueFree(ipVar); + + if (virNWFilterIPAddrMapAddIPAddr(ipl->SnoopReq->ifname, ipaddr) < 0) { + VIR_FREE(ipaddr); return -1; } + if (!instantiate) return 0; @@ -480,12 +483,18 @@ static unsigned int virNWFilterSnoopReqLeaseTimerRun(virNWFilterSnoopReqPtr req) { time_t now = time(0); + bool is_last = false; /* protect req->start */ virNWFilterSnoopReqLock(req); - while (req->start && req->start->Timeout <= now) - virNWFilterSnoopReqLeaseDel(req, req->start->IPAddress, true); + while (req->start && req->start->Timeout <= now) { + if (req->start->next == NULL || + req->start->next->Timeout > now) + is_last = true; + virNWFilterSnoopReqLeaseDel(req, req->start->IPAddress, true, + is_last); + } virNWFilterSnoopReqUnlock(req); @@ -552,7 +561,7 @@ virNWFilterSnoopReqFree(virNWFilterSnoop /* free all leases */ for (ipl = req->start; ipl; ipl = req->start) - virNWFilterSnoopReqLeaseDel(req, ipl->IPAddress, false); + virNWFilterSnoopReqLeaseDel(req, ipl->IPAddress, false, false); /* free all req data */ VIR_FREE(req->ifname); @@ -685,15 +694,6 @@ virNWFilterSnoopReqLeaseAdd(virNWFilterS virNWFilterSnoopReqUnlock(req); - /* 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 0; /* silently ignore multiple addresses */ - if (VIR_ALLOC(pl) < 0) { virReportOOMError(); return -1; @@ -761,34 +761,64 @@ virNWFilterSnoopReqRestore(virNWFilterSn * memory or when calling this function while reading * leases from the file. * + * @instantiate: when calling this function in a loop, indicate + * the last call with 'true' here so that the + * rules all get instantiated + * Always calling this with 'true' is fine, but less + * efficient. + * * Returns 0 on success, -1 if the instantiation of the rules failed */ static int virNWFilterSnoopReqLeaseDel(virNWFilterSnoopReqPtr req, - uint32_t ipaddr, bool update_leasefile) + uint32_t ipaddr, bool update_leasefile, + bool instantiate) { int ret = 0; virNWFilterSnoopIPLeasePtr ipl; + char ipstr[INET_ADDRSTRLEN]; + int ipAddrLeft; - /* protect req->start & req->ifname */ + /* protect req->start, req->ifname and the lease */ virNWFilterSnoopReqLock(req); ipl = virNWFilterSnoopIPLeaseGetByIP(req->start, ipaddr); if (ipl == NULL) goto lease_not_found; + if (!inet_ntop(AF_INET, &ipl->IPAddress, ipstr, sizeof(ipstr))) { + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, + _("%s: inet_ntop failed (0x%08X)"), + __func__, ipl->IPAddress); + ret = -1; + goto lease_not_found; + } + virNWFilterSnoopIPLeaseTimerDel(ipl); + /* lease is off the list now */ + + if (update_leasefile) + virNWFilterSnoopLeaseFileSave(ipl); - if (update_leasefile) { + ipAddrLeft = virNWFilterIPAddrMapDelIPAddr(req->ifname, ipstr); + + if (!req->threadkey || !instantiate) + goto skip_instantiate; + + if (ipAddrLeft) { + ret = virNWFilterInstantiateFilterLate(NULL, + req->ifname, + req->ifindex, + req->linkdev, + req->nettype, + req->macaddr, + req->filtername, + req->vars, + req->driver); + } else { const virNWFilterVarValuePtr dhcpsrvrs = virHashLookup(req->vars->hashTable, "DHCPSERVER"); - virNWFilterSnoopLeaseFileSave(ipl); - - /* - * for multiple address support, this needs to remove those rules - * referencing "IP" with ipl's ip value. - */ if (req->techdriver && req->techdriver->applyDHCPOnlyRules(req->ifname, req->macaddr, dhcpsrvrs, false) < 0) { @@ -798,6 +828,8 @@ virNWFilterSnoopReqLeaseDel(virNWFilterS } } + +skip_instantiate: VIR_FREE(ipl); virAtomicIntSub(&virNWFilterSnoopState.nLeases, 1); @@ -808,7 +840,6 @@ lease_not_found: return ret; } - static int virNWFilterSnoopDHCPGetOpt(virNWFilterSnoopDHCPHdrPtr pd, int len, int *pmtype, int *pleasetime) @@ -949,7 +980,7 @@ virNWFilterSnoopDHCPDecode(virNWFilterSn break; case DHCPDECLINE: case DHCPRELEASE: - if (virNWFilterSnoopReqLeaseDel(req, ipl.IPAddress, true) < 0) + if (virNWFilterSnoopReqLeaseDel(req, ipl.IPAddress, true, true) < 0) return -1; break; default: @@ -1490,7 +1521,7 @@ virNWFilterSnoopLeaseFileLoad(void) if (ipl.Timeout) virNWFilterSnoopReqLeaseAdd(req, &ipl, false); else - virNWFilterSnoopReqLeaseDel(req, ipl.IPAddress, false); + virNWFilterSnoopReqLeaseDel(req, ipl.IPAddress, false, false); virNWFilterSnoopReqPut(req); } @@ -1536,6 +1567,13 @@ virNWFilterSnoopRemAllReqIter(const void (void) virHashRemoveEntry(virNWFilterSnoopState.IfnameToKey, req->ifname); + /* + * Remove all IP addresses known to be associated with this + * interface so that a new thread will be started on this + * interface + */ + virNWFilterIPAddrMapDelIPAddr(req->ifname, NULL); + VIR_FREE(req->ifname); }