This patch adds support for "continue" and "return" actions in filter rules. Signed-off-by: David L Stevens <dlstevens(a)us.ibm.com&gt; --- src/conf/nwfilter_conf.c | 8 ++++++-- src/conf/nwfilter_conf.h | 2 ++ 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c index 08ede48..e0c2fb6 100644 --- a/src/conf/nwfilter_conf.c +++ b/src/conf/nwfilter_conf.c @@ -55,12 +55,16 @@ VIR_ENUM_IMPL(virNWFilterRuleAction, VIR_NWFILTER_RULE_ACTION_LAST, "drop", "accept", - "reject"); + "reject", + "return", + "continue"); VIR_ENUM_IMPL(virNWFilterJumpTarget, VIR_NWFILTER_RULE_ACTION_LAST, "DROP", "ACCEPT", - "REJECT"); + "REJECT", + "RETURN", + "CONTINUE"); VIR_ENUM_IMPL(virNWFilterRuleDirection, VIR_NWFILTER_RULE_DIRECTION_LAST, "in", diff --git a/src/conf/nwfilter_conf.h b/src/conf/nwfilter_conf.h index 5306403..c96851a 100644 --- a/src/conf/nwfilter_conf.h +++ b/src/conf/nwfilter_conf.h @@ -299,6 +299,8 @@ enum virNWFilterRuleActionType { VIR_NWFILTER_RULE_ACTION_DROP = 0, VIR_NWFILTER_RULE_ACTION_ACCEPT, VIR_NWFILTER_RULE_ACTION_REJECT, + VIR_NWFILTER_RULE_ACTION_RETURN, + VIR_NWFILTER_RULE_ACTION_CONTINUE, VIR_NWFILTER_RULE_ACTION_LAST, }; -- 1.7.6.4

The ARP protocol requires processing of packets that may not be explicitly addressed to a host and only defines request and reply. This patch removes the filtering of ARP requests not explicitly addressed to a VM to allow for proper ARP cache updates for entries based on any traffic and removes the unnecessary check for arpop of request or reply. Signed-off-by: David L Stevens <dlstevens(a)us.ibm.com&gt; --- examples/xml/nwfilter/no-arp-spoofing.xml | 23 ++--------------------- 1 files changed, 2 insertions(+), 21 deletions(-) diff --git a/examples/xml/nwfilter/no-arp-spoofing.xml b/examples/xml/nwfilter/no-arp-spoofing.xml index 96c58c1..3c83acd 100644 --- a/examples/xml/nwfilter/no-arp-spoofing.xml +++ b/examples/xml/nwfilter/no-arp-spoofing.xml @@ -12,25 +12,6 @@ <rule action='drop' direction='out' priority='400' > <arp match='no' arpsrcipaddr='$IP' /> </rule> - <!-- allow gratuitous arp --> - <rule action='accept' direction='in' priority='425'> - <arp gratuitous='true'/> - </rule> - <!-- drop if ipaddr or macaddr does not belong to guest --> - <rule action='drop' direction='in' priority='450' > - <arp match='no' arpdstmacaddr='$MAC'/> - <arp opcode='reply'/> - </rule> - <rule action='drop' direction='in' priority='500' > - <arp match='no' arpdstipaddr='$IP' /> - </rule> - <!-- accept only request or reply packets --> - <rule action='accept' direction='inout' priority='600' > - <arp opcode='request'/> - </rule> - <rule action='accept' direction='inout' priority='650' > - <arp opcode='reply'/> - </rule> - <!-- drop everything else --> - <rule action='drop' direction='inout' priority='1000' /> + <!-- allow everything else --> + <rule action='accept' direction='in' priority='425' /> </filter> -- 1.7.6.4

On 10/12/2011 03:50 PM, David L Stevens wrote: The hex numbers can be replaced with their string representation. Afaics the parser/formatter consults the macProtoMap in nwfilter_conf.c and translates it into the protocol number for safer (internal) use. The ordering here was meant to create the chains used to evaluating the ARP protocol after IPv4 for faster processing of IPv4, which would be the bulk and efficiency would matter most for IPv4. MAC can be first. Also don't just remove the ARP table. It can stay, even if example filters were to be changed. Others may have written filters accessing the arp chain and still need it. I believe this and the preceding hunk would be necessary with the patches I posted today. these two could go.

Stefan Berger <stefanb(a)linux.vnet.ibm.com&gt; wrote on 10/25/2011 12:01:05 PM: The intention was to separate MAC and IP address matching to be their own chains to make adding and removing addresses dynamically easy. If you do this in one chain with "IP=X and MAC=Y -j ACCEPT", then you need every combination of IP and MAC address as a rule. If you separate them into MAC matching and protocol matching chains, you need one rule per MAC and IP address. After doing that, the ARP chain was empty, since that's all the standard chain had. I don't understand this comment. I didn't say it was the only way to support multiple addresses. :-) It uses nMAC + nIP rules, instead of nMAC X nIP rules to match every combination of MAC and IP addresses in the original ARP chain. This isn't a requirement, but in general, the filters are placing restrictions on packets that have nothing to do with spoofing and are not part of the underlying protocols. A host without an IP address can use source address "0.0.0.0" legally by the protocols in cases besides sending a DHCP request, DHCP requests need not be MAC broadcast or sent to the all-hosts broadcast address (e.g., if firmware configured a particular preferred server (unicast MAC and/or IP), or knows the subnet and uses a directed broadcast). ARP requests received from other machines need not be addressed to this VM to be processed and update the cache. By restricting these cases, the filters prevent VMs from doing protocol-legal things that have nothing to do with spoofing. It simply breaks unusual configurations unnecessarily. I was planning to submit clean-up patches to remove all these restrictions I found after my primary concern (DHCP snooping) is done. I threw in some of them along with the multiple address support in the version that did that, but they really are separate fixes that have nothing to do with either multiple address support or DHCP snooping. In the end, base anti-spoofing rules should prevent a VM from masquerading as someone else, and allow everything else. +-DLS

This patch simplifies the table rules by setting the protocol chains policy to be "DROP" and removes the explicit "-j DROP" entries that the protocol rules had previously. It also makes "no-other-rarp-traffic.xml" obsolete. Signed-off-by: David L Stevens <dlstevens(a)us.ibm.com&gt; --- examples/xml/nwfilter/Makefile.am | 1 - examples/xml/nwfilter/no-arpip-spoofing.xml | 2 -- examples/xml/nwfilter/no-arpmac-spoofing.xml | 2 -- examples/xml/nwfilter/no-ip-spoofing.xml | 2 -- examples/xml/nwfilter/no-mac-spoofing.xml | 2 -- examples/xml/nwfilter/no-other-rarp-traffic.xml | 3 --- examples/xml/nwfilter/qemu-announce-self.xml | 1 - src/nwfilter/nwfilter_ebiptables_driver.c | 11 +---------- 8 files changed, 1 insertions(+), 23 deletions(-) delete mode 100644 examples/xml/nwfilter/no-other-rarp-traffic.xml diff --git a/examples/xml/nwfilter/Makefile.am b/examples/xml/nwfilter/Makefile.am index 84aaa3c..67085fa 100644 --- a/examples/xml/nwfilter/Makefile.am +++ b/examples/xml/nwfilter/Makefile.am @@ -18,7 +18,6 @@ FILTERS = \ no-mac-broadcast.xml \ no-mac-spoofing.xml \ no-other-l2-traffic.xml \ - no-other-rarp-traffic.xml \ qemu-announce-self.xml \ qemu-announce-self-rarp.xml diff --git a/examples/xml/nwfilter/no-arpip-spoofing.xml b/examples/xml/nwfilter/no-arpip-spoofing.xml index ee42d40..7ef6f0f 100644 --- a/examples/xml/nwfilter/no-arpip-spoofing.xml +++ b/examples/xml/nwfilter/no-arpip-spoofing.xml @@ -7,6 +7,4 @@ <rule action='return' direction='out' priority='410' > <arp match='yes' arpsrcipaddr='0.0.0.0' /> </rule> - <!-- drop everything else --> - <rule action='drop' direction='out' priority='1000' /> </filter> diff --git a/examples/xml/nwfilter/no-arpmac-spoofing.xml b/examples/xml/nwfilter/no-arpmac-spoofing.xml index 90499d3..3834047 100644 --- a/examples/xml/nwfilter/no-arpmac-spoofing.xml +++ b/examples/xml/nwfilter/no-arpmac-spoofing.xml @@ -2,6 +2,4 @@ <rule action='return' direction='out' priority='350' > <arp match='yes' arpsrcmacaddr='$MAC'/> </rule> - <!-- drop everything else --> - <rule action='drop' direction='out' priority='1000' /> </filter> diff --git a/examples/xml/nwfilter/no-ip-spoofing.xml b/examples/xml/nwfilter/no-ip-spoofing.xml index 84e8a5e..2fccd12 100644 --- a/examples/xml/nwfilter/no-ip-spoofing.xml +++ b/examples/xml/nwfilter/no-ip-spoofing.xml @@ -4,6 +4,4 @@ <rule action='return' direction='out'> <ip match='yes' srcipaddr='$IP' /> </rule> - <!-- drop any that don't match the source IP list --> - <rule action='drop' direction='out' /> </filter> diff --git a/examples/xml/nwfilter/no-mac-spoofing.xml b/examples/xml/nwfilter/no-mac-spoofing.xml index aee56c7..e2e8c03 100644 --- a/examples/xml/nwfilter/no-mac-spoofing.xml +++ b/examples/xml/nwfilter/no-mac-spoofing.xml @@ -4,6 +4,4 @@ <rule action='return' direction='out' priority='350' > <mac match='yes' srcmacaddr='$MAC'/> </rule> - <!-- drop everything else --> - <rule action='drop' direction='out' priority='1000' /> </filter> diff --git a/examples/xml/nwfilter/no-other-rarp-traffic.xml b/examples/xml/nwfilter/no-other-rarp-traffic.xml deleted file mode 100644 index 7729996..0000000 --- a/examples/xml/nwfilter/no-other-rarp-traffic.xml +++ /dev/null @@ -1,3 +0,0 @@ -<filter name='no-other-rarp-traffic' chain='rarp'> - <rule action='drop' direction='inout' priority='1000'/> -</filter> diff --git a/examples/xml/nwfilter/qemu-announce-self.xml b/examples/xml/nwfilter/qemu-announce-self.xml index 352db50..12957b5 100644 --- a/examples/xml/nwfilter/qemu-announce-self.xml +++ b/examples/xml/nwfilter/qemu-announce-self.xml @@ -8,6 +8,5 @@ <!-- accept if it was changed to rarp --> <filterref filter='qemu-announce-self-rarp'/> - <filterref filter='no-other-rarp-traffic'/> </filter> diff --git a/src/nwfilter/nwfilter_ebiptables_driver.c b/src/nwfilter/nwfilter_ebiptables_driver.c index 3c6fca7..e6a4880 100644 --- a/src/nwfilter/nwfilter_ebiptables_driver.c +++ b/src/nwfilter/nwfilter_ebiptables_driver.c @@ -2791,7 +2791,7 @@ ebtablesCreateTmpSubChain(virBufferPtr buf, protostr[0] = '\0'; virBufferAsprintf(buf, - CMD_DEF("%s -t %s -N %s") CMD_SEPARATOR + CMD_DEF("%s -t %s -N %s -P DROP") CMD_SEPARATOR CMD_EXEC "%s" CMD_DEF("%s -t %s -A %s %s -j %s") CMD_SEPARATOR @@ -3015,15 +3015,6 @@ ebtablesApplyBasicRules(const char *ifname, PRINT_ROOT_CHAIN(chain, chainPrefix, ifname); virBufferAsprintf(&buf, - CMD_DEF("%s -t %s -A %s -s ! %s -j DROP") CMD_SEPARATOR - CMD_EXEC - "%s", - - ebtables_cmd_path, EBTABLES_DEFAULT_TABLE, - chain, macaddr_str, - CMD_STOPONERR(1)); - - virBufferAsprintf(&buf, CMD_DEF("%s -t %s -A %s -p IPv4 -j ACCEPT") CMD_SEPARATOR CMD_EXEC "%s", -- 1.7.6.4

This patch adds the internal capability to add rules to existing chains instead of using temporary chains and to generate placeholders for chains that are referenced without generating a rule for them immediately. Finally, it includes variable matching for filter instantiation (i.e., instantiate only when a given variable is present in a filter, or only when it is not). Signed-off-by: David L Stevens <dlstevens(a)us.ibm.com&gt; --- src/conf/nwfilter_conf.h | 4 +- src/nwfilter/nwfilter_ebiptables_driver.c | 93 +++++++++++++++++++++-------- src/nwfilter/nwfilter_gentech_driver.c | 32 +++++++++- 3 files changed, 100 insertions(+), 29 deletions(-) diff --git a/src/conf/nwfilter_conf.h b/src/conf/nwfilter_conf.h index 17e954e..4348378 100644 --- a/src/conf/nwfilter_conf.h +++ b/src/conf/nwfilter_conf.h @@ -525,7 +525,9 @@ typedef int (*virNWFilterRuleCreateInstance)(virConnectPtr conn, virNWFilterRuleDefPtr rule, const char *ifname, virNWFilterHashTablePtr vars, - virNWFilterRuleInstPtr res); + virNWFilterRuleInstPtr res, + bool usetemp, + bool dummy); typedef int (*virNWFilterRuleApplyNewRules)(virConnectPtr conn, const char *ifname, diff --git a/src/nwfilter/nwfilter_ebiptables_driver.c b/src/nwfilter/nwfilter_ebiptables_driver.c index e6a4880..918625c 100644 --- a/src/nwfilter/nwfilter_ebiptables_driver.c +++ b/src/nwfilter/nwfilter_ebiptables_driver.c @@ -1136,6 +1136,7 @@ iptablesEnforceDirection(int directionIn, * @isIPv6 : Whether this is an IPv6 rule * @maySkipICMP : whether this rule may under certain circumstances skip * the ICMP rule from being created + * @dummy : generate rule placeholder without installing * * Convert a single rule into its representation for later instantiation * @@ -1154,7 +1155,8 @@ _iptablesCreateRuleInstance(int directionIn, const char *match, bool defMatch, const char *accept_target, bool isIPv6, - bool maySkipICMP) + bool maySkipICMP, + bool dummy) { char chain[MAX_CHAINNAME_LENGTH]; char number[20]; @@ -1181,6 +1183,13 @@ _iptablesCreateRuleInstance(int directionIn, PRINT_IPT_ROOT_CHAIN(chain, chainPrefix, ifname); + if (dummy) { + virBufferAsprintf(&buf, CMD_DEF_PRE "%s -- %s -%%c %s %%s", + "echo", iptables_cmd, chain); + bufUsed = virBufferUse(&buf); + goto prskip; + } + switch (rule->prtclType) { case VIR_NWFILTER_RULE_PROTOCOL_TCP: case VIR_NWFILTER_RULE_PROTOCOL_TCPoIPV6: @@ -1521,6 +1530,8 @@ _iptablesCreateRuleInstance(int directionIn, return -1; } +prskip: + if ((srcMacSkipped && bufUsed == virBufferUse(&buf)) || skipRule) { virBufferFreeAndReset(&buf); @@ -1636,7 +1647,9 @@ iptablesCreateRuleInstanceStateCtrl(virNWFilterDefPtr nwfilter, const char *ifname, virNWFilterHashTablePtr vars, virNWFilterRuleInstPtr res, - bool isIPv6) + bool isIPv6, + bool usetemp, + bool dummy) { int rc; int directionIn = 0; @@ -1668,7 +1681,7 @@ iptablesCreateRuleInstanceStateCtrl(virNWFilterDefPtr nwfilter, return 1; } - chainPrefix[1] = CHAINPREFIX_HOST_IN_TEMP; + chainPrefix[1] = usetemp ? CHAINPREFIX_HOST_IN_TEMP : CHAINPREFIX_HOST_IN; if (create) { rc = _iptablesCreateRuleInstance(directionIn, chainPrefix, @@ -1680,7 +1693,8 @@ iptablesCreateRuleInstanceStateCtrl(virNWFilterDefPtr nwfilter, matchState, false, "RETURN", isIPv6, - maySkipICMP); + maySkipICMP, + dummy); VIR_FREE(matchState); if (rc) @@ -1700,7 +1714,8 @@ iptablesCreateRuleInstanceStateCtrl(virNWFilterDefPtr nwfilter, return 1; } - chainPrefix[1] = CHAINPREFIX_HOST_OUT_TEMP; + chainPrefix[1] = usetemp ? CHAINPREFIX_HOST_OUT_TEMP : + CHAINPREFIX_HOST_OUT; if (create) { rc = _iptablesCreateRuleInstance(!directionIn, chainPrefix, @@ -1712,7 +1727,8 @@ iptablesCreateRuleInstanceStateCtrl(virNWFilterDefPtr nwfilter, matchState, false, "ACCEPT", isIPv6, - maySkipICMP); + maySkipICMP, + dummy); VIR_FREE(matchState); @@ -1736,7 +1752,8 @@ iptablesCreateRuleInstanceStateCtrl(virNWFilterDefPtr nwfilter, if (create) { chainPrefix[0] = 'H'; - chainPrefix[1] = CHAINPREFIX_HOST_IN_TEMP; + chainPrefix[1] = usetemp ? CHAINPREFIX_HOST_IN_TEMP : + CHAINPREFIX_HOST_IN; rc = _iptablesCreateRuleInstance(directionIn, chainPrefix, nwfilter, @@ -1747,7 +1764,8 @@ iptablesCreateRuleInstanceStateCtrl(virNWFilterDefPtr nwfilter, matchState, false, "RETURN", isIPv6, - maySkipICMP); + maySkipICMP, + dummy); VIR_FREE(matchState); } @@ -1761,7 +1779,9 @@ iptablesCreateRuleInstance(virNWFilterDefPtr nwfilter, const char *ifname, virNWFilterHashTablePtr vars, virNWFilterRuleInstPtr res, - bool isIPv6) + bool isIPv6, + bool usetemp, + bool dummy) { int rc; int directionIn = 0; @@ -1777,7 +1797,9 @@ iptablesCreateRuleInstance(virNWFilterDefPtr nwfilter, ifname, vars, res, - isIPv6); + isIPv6, + usetemp, + dummy); } if ((rule->tt == VIR_NWFILTER_RULE_DIRECTION_IN) || @@ -1800,7 +1822,7 @@ iptablesCreateRuleInstance(virNWFilterDefPtr nwfilter, else matchState = NULL; - chainPrefix[1] = CHAINPREFIX_HOST_IN_TEMP; + chainPrefix[1] = usetemp ? CHAINPREFIX_HOST_IN_TEMP : CHAINPREFIX_HOST_IN; rc = _iptablesCreateRuleInstance(directionIn, chainPrefix, nwfilter, @@ -1811,7 +1833,8 @@ iptablesCreateRuleInstance(virNWFilterDefPtr nwfilter, matchState, true, "RETURN", isIPv6, - maySkipICMP); + maySkipICMP, + dummy); if (rc) return rc; @@ -1822,7 +1845,7 @@ iptablesCreateRuleInstance(virNWFilterDefPtr nwfilter, else matchState = NULL; - chainPrefix[1] = CHAINPREFIX_HOST_OUT_TEMP; + chainPrefix[1] = usetemp ? CHAINPREFIX_HOST_OUT_TEMP : CHAINPREFIX_HOST_OUT; rc = _iptablesCreateRuleInstance(!directionIn, chainPrefix, nwfilter, @@ -1833,7 +1856,8 @@ iptablesCreateRuleInstance(virNWFilterDefPtr nwfilter, matchState, true, "ACCEPT", isIPv6, - maySkipICMP); + maySkipICMP, + dummy); if (rc) return rc; @@ -1844,7 +1868,7 @@ iptablesCreateRuleInstance(virNWFilterDefPtr nwfilter, matchState = NULL; chainPrefix[0] = 'H'; - chainPrefix[1] = CHAINPREFIX_HOST_IN_TEMP; + chainPrefix[1] = usetemp ? CHAINPREFIX_HOST_IN_TEMP : CHAINPREFIX_HOST_IN; rc = _iptablesCreateRuleInstance(directionIn, chainPrefix, nwfilter, @@ -1855,7 +1879,8 @@ iptablesCreateRuleInstance(virNWFilterDefPtr nwfilter, matchState, true, "RETURN", isIPv6, - maySkipICMP); + maySkipICMP, + dummy); return rc; } @@ -1886,7 +1911,8 @@ ebtablesCreateRuleInstance(char chainPrefix, const char *ifname, virNWFilterHashTablePtr vars, virNWFilterRuleInstPtr res, - bool reverse) + bool reverse, + bool dummy) { char macaddr[VIR_MAC_STRING_BUFLEN], ipaddr[INET_ADDRSTRLEN], @@ -1909,6 +1935,11 @@ ebtablesCreateRuleInstance(char chainPrefix, PRINT_CHAIN(chain, chainPrefix, ifname, virNWFilterChainSuffixTypeToString(nwfilter->chainsuffix)); + if (dummy) { + virBufferAsprintf(&buf, CMD_DEF_PRE "%s -- -t %s -%%c %s %%s", + "echo", EBTABLES_DEFAULT_TABLE, chain); + goto prskip; + } switch (rule->prtclType) { case VIR_NWFILTER_RULE_PROTOCOL_MAC: @@ -2317,6 +2348,8 @@ ebtablesCreateRuleInstance(char chainPrefix, return -1; } +prskip: + switch (rule->action) { case VIR_NWFILTER_RULE_ACTION_REJECT: /* REJECT not supported */ @@ -2374,11 +2407,15 @@ ebiptablesCreateRuleInstance(virConnectPtr conn ATTRIBUTE_UNUSED, virNWFilterRuleDefPtr rule, const char *ifname, virNWFilterHashTablePtr vars, - virNWFilterRuleInstPtr res) + virNWFilterRuleInstPtr res, + bool usetemp, + bool dummy) { int rc = 0; bool isIPv6; + char chainPrefix; + chainPrefix = usetemp ? CHAINPREFIX_HOST_IN_TEMP : CHAINPREFIX_HOST_IN; switch (rule->prtclType) { case VIR_NWFILTER_RULE_PROTOCOL_IP: case VIR_NWFILTER_RULE_PROTOCOL_MAC: @@ -2389,26 +2426,30 @@ ebiptablesCreateRuleInstance(virConnectPtr conn ATTRIBUTE_UNUSED, if (rule->tt == VIR_NWFILTER_RULE_DIRECTION_OUT || rule->tt == VIR_NWFILTER_RULE_DIRECTION_INOUT) { - rc = ebtablesCreateRuleInstance(CHAINPREFIX_HOST_IN_TEMP, + rc = ebtablesCreateRuleInstance(chainPrefix, nwfilter, rule, ifname, vars, res, - rule->tt == VIR_NWFILTER_RULE_DIRECTION_INOUT); + rule->tt == VIR_NWFILTER_RULE_DIRECTION_INOUT, + dummy); if (rc) return rc; } + chainPrefix = usetemp ? CHAINPREFIX_HOST_OUT_TEMP : + CHAINPREFIX_HOST_OUT; if (rule->tt == VIR_NWFILTER_RULE_DIRECTION_IN || rule->tt == VIR_NWFILTER_RULE_DIRECTION_INOUT) { - rc = ebtablesCreateRuleInstance(CHAINPREFIX_HOST_OUT_TEMP, + rc = ebtablesCreateRuleInstance(chainPrefix, nwfilter, rule, ifname, vars, res, - false); + false, + dummy); } break; @@ -2427,7 +2468,9 @@ ebiptablesCreateRuleInstance(virConnectPtr conn ATTRIBUTE_UNUSED, ifname, vars, res, - isIPv6); + isIPv6, + usetemp, + dummy); break; case VIR_NWFILTER_RULE_PROTOCOL_TCPoIPV6: @@ -2444,7 +2487,9 @@ ebiptablesCreateRuleInstance(virConnectPtr conn ATTRIBUTE_UNUSED, ifname, vars, res, - isIPv6); + isIPv6, + usetemp, + dummy); break; case VIR_NWFILTER_RULE_PROTOCOL_LAST: diff --git a/src/nwfilter/nwfilter_gentech_driver.c b/src/nwfilter/nwfilter_gentech_driver.c index 7891983..79350ac 100644 --- a/src/nwfilter/nwfilter_gentech_driver.c +++ b/src/nwfilter/nwfilter_gentech_driver.c @@ -284,7 +284,9 @@ virNWFilterRuleInstantiate(virConnectPtr conn, virNWFilterDefPtr filter, virNWFilterRuleDefPtr rule, const char *ifname, - virNWFilterHashTablePtr vars) + virNWFilterHashTablePtr vars, + bool usetemp, + bool dummy) { int rc; int i; @@ -297,8 +299,8 @@ virNWFilterRuleInstantiate(virConnectPtr conn, ret->techdriver = techdriver; - rc = techdriver->createRuleInstance(conn, nettype, filter, - rule, ifname, vars, ret); + rc = techdriver->createRuleInstance(conn, nettype, filter, rule, ifname, + vars, ret, usetemp, dummy); if (rc) { for (i = 0; i < ret->ndata; i++) @@ -354,6 +356,8 @@ err_exit: * @ifname: The name of the interface to apply the rules to * @vars: A map holding variable names and values used for instantiating * the filter and its subfilters. + * @matchvar: if non-null, variable name to match + * @notmatch: if matchvar set, match filters that do not reference matchvar * @nEntries: number of virNWFilterInst objects collected * @insts: pointer to array for virNWFilterIns object pointers * @useNewFilter: instruct whether to use a newDef pointer rather than a @@ -377,6 +381,8 @@ _virNWFilterInstantiateRec(virConnectPtr conn, virNWFilterDefPtr filter, const char *ifname, virNWFilterHashTablePtr vars, + const char *matchvar, + bool notmatch, int *nEntries, virNWFilterRuleInstPtr **insts, enum instCase useNewFilter, bool *foundNewFilter, @@ -387,18 +393,34 @@ _virNWFilterInstantiateRec(virConnectPtr conn, int i; virNWFilterRuleInstPtr inst; virNWFilterDefPtr next_filter; + bool usetemp, dummy; for (i = 0; i < filter->nentries; i++) { virNWFilterRuleDefPtr rule = filter->filterEntries[i]->rule; virNWFilterIncludeDefPtr inc = filter->filterEntries[i]->include; if (rule) { + usetemp = 1; + dummy = 0; + if (matchvar) { + int j; + + for (j = 0; j < rule->nvars; ++j) + if (strcmp(rule->vars[j], matchvar) == 0) + break; + /* use temp chains only on base rule install */ + usetemp = notmatch; + /* skip if not found; notmatch reverses the sense */ + dummy = (j == rule->nvars) ^ notmatch; + } inst = virNWFilterRuleInstantiate(conn, techdriver, nettype, filter, rule, ifname, - vars); + vars, + usetemp, + dummy); if (!inst) { rc = 1; break; @@ -456,6 +478,7 @@ _virNWFilterInstantiateRec(virConnectPtr conn, next_filter, ifname, tmpvars, + matchvar, notmatch, nEntries, insts, useNewFilter, foundNewFilter, @@ -684,6 +707,7 @@ virNWFilterInstantiate(virConnectPtr conn, filter, ifname, vars, + 0, 0, &nEntries, &insts, useNewFilter, foundNewFilter, driver); -- 1.7.6.4

This patch adds the capability of adding individual rules to existing chains. Signed-off-by: David L Stevens <dlstevens(a)us.ibm.com&gt; --- src/conf/nwfilter_conf.h | 6 ++ src/nwfilter/nwfilter_ebiptables_driver.c | 73 +++++++++++++++++++++++++++++ 2 files changed, 79 insertions(+), 0 deletions(-) diff --git a/src/conf/nwfilter_conf.h b/src/conf/nwfilter_conf.h index 4348378..12d1e0f 100644 --- a/src/conf/nwfilter_conf.h +++ b/src/conf/nwfilter_conf.h @@ -540,6 +540,11 @@ typedef int (*virNWFilterRuleTeardownNewRules)(virConnectPtr conn, typedef int (*virNWFilterRuleTeardownOldRules)(virConnectPtr conn, const char *ifname); +typedef int (*virNWFilterRuleAddRules)(virConnectPtr conn, + const char *ifname, + int nruleInstances, + void **_inst); + typedef int (*virNWFilterRuleRemoveRules)(virConnectPtr conn, const char *ifname, int nruleInstances, @@ -580,6 +585,7 @@ struct _virNWFilterTechDriver { virNWFilterRuleApplyNewRules applyNewRules; virNWFilterRuleTeardownNewRules tearNewRules; virNWFilterRuleTeardownOldRules tearOldRules; + virNWFilterRuleAddRules addRules; virNWFilterRuleRemoveRules removeRules; virNWFilterRuleAllTeardown allTeardown; virNWFilterRuleFreeInstanceData freeRuleInstance; diff --git a/src/nwfilter/nwfilter_ebiptables_driver.c b/src/nwfilter/nwfilter_ebiptables_driver.c index 918625c..1169e5a 100644 --- a/src/nwfilter/nwfilter_ebiptables_driver.c +++ b/src/nwfilter/nwfilter_ebiptables_driver.c @@ -3695,6 +3695,78 @@ err_exit: return rc; } +/** + * ebiptablesAddRules: + * @conn : pointer to virConnect object + * @ifname : the name of the interface to which the rules apply + * @nRuleInstance : the number of given rules + * @_inst : array of rule instantiation data + * + * Add all rules one after the other + * + * Return 0 on success, 1 if execution of one or more cleanup + * commands failed. + */ +static int +ebiptablesAddRules(virConnectPtr conn, + const char *ifname, + int nruleInstances, + void **_inst) +{ + int i; + int cli_status; + ebiptablesRuleInstPtr *inst = (ebiptablesRuleInstPtr *)_inst; + virBuffer buf = VIR_BUFFER_INITIALIZER; + bool haveIptables = false; + bool haveIp6tables = false; + + for (i = 0; i < nruleInstances; i++) { + sa_assert (inst); + switch (inst[i]->ruleType) { + case RT_EBTABLES: + ebiptablesInstCommand(&buf, + inst[i]->commandTemplate, + 'A', -1, 1); + break; + case RT_IPTABLES: + if (inst[i]->ruleType == RT_IPTABLES) + iptablesInstCommand(&buf, + inst[i]->commandTemplate, + 'A', -1, 1); + haveIptables = true; + break; + case RT_IP6TABLES: + if (inst[i]->ruleType == RT_IP6TABLES) + iptablesInstCommand(&buf, + inst[i]->commandTemplate, + 'A', -1, 1); + haveIp6tables = true; + break; + } + } + + if (ebiptablesExecCLI(&buf, &cli_status) || cli_status != 0) + goto err_exit; + + if (haveIptables) + iptablesCheckBridgeNFCallEnabled(false); + + if (haveIp6tables) + iptablesCheckBridgeNFCallEnabled(true); + + return 0; + +err_exit: + (void) ebiptablesRemoveRules(conn, ifname, nruleInstances, _inst); + + virNWFilterReportError(VIR_ERR_BUILD_FIREWALL, + _("Some rules could not be created for " + "interface %s."), + ifname); + + return 1; +} + /** * ebiptablesAllTeardown: @@ -3751,6 +3823,7 @@ virNWFilterTechDriver ebiptables_driver = { .tearNewRules = ebiptablesTearNewRules, .tearOldRules = ebiptablesTearOldRules, .allTeardown = ebiptablesAllTeardown, + .addRules = ebiptablesAddRules, .removeRules = ebiptablesRemoveRules, .freeRuleInstance = ebiptablesFreeRuleInstance, .displayRuleInstance = ebiptablesDisplayRuleInstance, -- 1.7.6.4

On 10/12/2011 03:50 PM, David L Stevens wrote: Given the NWFILTER_STD_VAR_IP parameter, what does it give us at this point? I am wondering about this addRules() and whether the rules are being added to the end of a chain and thus the rules' assumed priority would have to be such that these rules can actually always be the last ones?

This patch adds DHCP Snooping support to libvirt. Signed-off-by: David L Stevens <dlstevens(a)us.ibm.com&gt; --- examples/xml/nwfilter/no-ip-spoofing.xml | 5 + src/Makefile.am | 2 + src/nwfilter/nwfilter_dhcpsnoop.c | 602 ++++++++++++++++++++++++++++++ src/nwfilter/nwfilter_dhcpsnoop.h | 36 ++ src/nwfilter/nwfilter_driver.c | 5 + src/nwfilter/nwfilter_gentech_driver.c | 83 +++-- 6 files changed, 708 insertions(+), 25 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 2fccd12..2ae9500 100644 --- a/examples/xml/nwfilter/no-ip-spoofing.xml +++ b/examples/xml/nwfilter/no-ip-spoofing.xml @@ -4,4 +4,9 @@ <rule action='return' direction='out'> <ip match='yes' 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> diff --git a/src/Makefile.am b/src/Makefile.am index 738ee91..f6d3fdd 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -473,6 +473,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..f784a29 --- /dev/null +++ b/src/nwfilter/nwfilter_dhcpsnoop.c @@ -0,0 +1,602 @@ + +/* + * 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(a)us.ibm.com&gt; + * Based in part on work by Stefan Berger <stefanb(a)us.ibm.com&gt; + */ + +#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 + +static virHashTablePtr SnoopReqs; + +struct virNWFilterSnoopReq { + virConnectPtr conn; + virNWFilterTechDriverPtr techdriver; + enum virDomainNetType nettype; + virNWFilterDefPtr filter; + const char *ifname; + 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; + } + 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; + } + rc = virNWFilterChangeVar(pl->ipl_req->conn, + pl->ipl_req->techdriver, + pl->ipl_req->nettype, + pl->ipl_req->filter, + pl->ipl_req->ifname, + pl->ipl_req->vars, + pl->ipl_req->driver, "IP", ipstr, 0); + 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; + char ipbuf[20]; /* dotted decimal IP addr string */ + char *ipstr; + + if (!ipl) + return; + + req = ipl->ipl_req; + + ipl_tdel(ipl); + + if (inet_ntop(AF_INET, &ipl->ipl_ipaddr, ipbuf, sizeof(ipbuf))) { + ipstr = strdup(ipbuf); + if (virNWFilterChangeVar(req->conn, + req->techdriver, + req->nettype, + req->filter, + req->ifname, + req->vars, req->driver, "IP", ipstr, 1)) + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, + _("ipl_del virNWFilterChangeVar failed; " + "filter not deleted")); + } else + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, + _("ipl_del inet_ntop " "failed (0x%08X)"), + ipl->ipl_ipaddr); + 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]; +}; + +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 */ + + 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 * +virNWFilterDHCPSnoop(void *req0) +{ + struct virNWFilterSnoopReq *req = req0; + pcap_t *handle; + struct pcap_pkthdr hdr; + struct eth *packet; + struct iplease *ipl; + int ifindex; + + handle = dhcpopen(req->ifname); + if (!handle) + return 0; + + ifindex = if_nametoindex(req->ifname); + + while (1) { + if (req->die) + break; + ipl_trun(req); + + packet = (struct eth *) pcap_next(handle, &hdr); + + if (!packet) { + if (ifaceCheck(false, req->ifname, NULL, ifindex)) + virNWFilterDHCPSnoopEnd(req->ifname); + continue; + } + + dhcpdecode(req, packet, hdr.caplen); + } + /* free all leases */ + for (ipl = req->start; ipl; ipl = req->start) + ipl_del(ipl); + + /* free all req data */ + VIR_FREE(req->ifname); + virNWFilterHashTableFree(req->vars); + VIR_FREE(req); + return 0; +} + +int +virNWFilterDHCPSnoopReq(virConnectPtr conn, + virNWFilterTechDriverPtr techdriver ATTRIBUTE_UNUSED, + enum virDomainNetType nettype ATTRIBUTE_UNUSED, + virNWFilterDefPtr filter ATTRIBUTE_UNUSED, + const char *ifname ATTRIBUTE_UNUSED, + virNWFilterHashTablePtr vars ATTRIBUTE_UNUSED, + virNWFilterDriverStatePtr driver ATTRIBUTE_UNUSED) +{ + struct virNWFilterSnoopReq *req; + + req = virHashLookup(SnoopReqs, ifname); + if (req) + return 0; + if (VIR_ALLOC(req) < 0) { + virReportOOMError(); + return 1; + } + + req->conn = conn; + req->techdriver = techdriver; + req->nettype = nettype; + req->filter = filter; + req->ifname = strdup(ifname); + req->vars = virNWFilterHashTableCreate(0); + if (!req->vars) { + virReportOOMError(); + return 1; + } + if (virNWFilterHashTablePutAll(vars, req->vars)) { + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, + _("virNWFilterDHCPSnoopReq: can't copy variables" + " on if %s"), ifname); + return 1; + } + req->driver = driver; + req->start = req->end = 0; + + if (virHashAddEntry(SnoopReqs, ifname, req)) { + VIR_FREE(req); + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, + _("virNWFilterDHCPSnoopReq req add failed on" + "interface \"%s\""), ifname); + return 1; + } + if (pthread_create(&req->thread, NULL, virNWFilterDHCPSnoop, req) != 0) { + (void) virHashRemoveEntry(SnoopReqs, ifname); + VIR_FREE(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; + SnoopReqs = virHashCreate(0, freeReq); + if (!SnoopReqs) { + virReportOOMError(); + return -1; + } + return 0; +} + +void +virNWFilterDHCPSnoopEnd(const char *ifname) +{ + if (!SnoopReqs) + return; + if (ifname) + virHashRemoveEntry(SnoopReqs, ifname); + else /* free all of them */ + virHashFree(SnoopReqs); +} diff --git a/src/nwfilter/nwfilter_dhcpsnoop.h b/src/nwfilter/nwfilter_dhcpsnoop.h new file mode 100644 index 0000000..248b1a0 --- /dev/null +++ b/src/nwfilter/nwfilter_dhcpsnoop.h @@ -0,0 +1,36 @@ +/* + * 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(a)us.ibm.com&gt; + */ + +#ifndef __NWFILTER_DHCPSNOOP_H +# define __NWFILTER_DHCPSNOOP_H + +int virNWFilterDHCPSnoopInit(void); +int virNWFilterDHCPSnoopReq(virConnectPtr conn, + virNWFilterTechDriverPtr techdriver, + enum virDomainNetType, + virNWFilterDefPtr filter, + const char *ifname, + virNWFilterHashTablePtr vars, + 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 563a1f3..577b48d 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 "DHCP" + static int _virNWFilterTeardownFilter(const char *ifname); @@ -747,6 +750,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) { @@ -764,39 +769,65 @@ virNWFilterInstantiate(virConnectPtr conn, if (rc) goto err_exit; + learning = virHashLookup(vars->hashTable, "ip_learning"); + 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 = _virNWFilterInstantiateRec(conn, + techdriver, + nettype, + filter, + ifname, + vars, + NWFILTER_STD_VAR_IP, 1, + &nEntries, &insts, + useNewFilter, foundNewFilter, + driver); + if (!rc) + rc = virNWFilterDHCPSnoopReq(conn, techdriver, nettype, + filter, ifname, vars, driver); + } 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 && virNWFilterLookupLearnReq(ifindex) != NULL) { goto err_exit; - } - - rc = _virNWFilterInstantiateRec(conn, - techdriver, - nettype, - filter, - ifname, - vars, - 0, 0, - &nEntries, &insts, - useNewFilter, foundNewFilter, - driver); + } else + rc = _virNWFilterInstantiateRec(conn, + techdriver, + nettype, + filter, + ifname, + vars, + 0, 0, + &nEntries, &insts, + useNewFilter, foundNewFilter, + driver); if (rc) goto err_exit; @@ -848,7 +879,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 " @@ -1180,6 +1211,8 @@ _virNWFilterTeardownFilter(const char *ifname) return 1; } + virNWFilterDHCPSnoopEnd(ifname); + virNWFilterTerminateLearnReq(ifname); if (virNWFilterLockIface(ifname)) -- 1.7.6.4

On 10/12/2011 03:50 PM, David L Stevens wrote: Following the lookup into the hashtable above you cannot just free the request. I suppose you'd first have to remove it from the hash table -- or did this maybe happen in the lines in between? This seems more like fix to the previous patch? The snoopReqFree() should really be introduced in the previous patch... no need to void the result since it doesn't return anything Replace with VIR_FORCE_CLOSE(). Since locking is done below I'd test for SnoopReq with the snoop lock being held otherwise you could (theoretically) have two threads running into the code below, at least the code looks that way. Move this lock to the top. What if this call fails? In case of error, shouldn't it unlink the file and return and error ? VIR_CLOSE() and handle error case ? Didn't find the fclose().. For every lease I guess you'd have to store the VLAN ID as well since VMs on different VLANs can have the same IP address. I am not sure what effect this has on the whole code. Maybe your snooping would have to look at the MAC header and extract VLAN identifier(s) [nested!] as well and store them here. I wonder about the MAC address that an IP address is associated and whether that should be recorded as well and used for determining whether a lease has expired. Stefan

On 10/12/2011 03:50 PM, David L Stevens wrote: Check for str_ipaddr or ipaddrlist being NULL. Test for NULL result. Also here.