[libvirt] [PATCH V1 0/9] NWFilter: Filter more protocols and other extensions
This patch series adds: - filtering support for VLAN traffic - support for a 'mac' chain - filtering support for STP (spanning tree protocol) - new filters that enable filtering of multiple IP addresses per interface - better error reporting if ebtables/ip(6)table commands fail Regards, Stefan
This patch adds support for filtering of VLAN (802.1Q) traffic to the parser and makes us of the ebtables support for VLAN filtering. This code now enables the filtering of traffic in chains with prefix 'vlan'. Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> --- docs/schemas/nwfilter.rng | 47 +++++++++++++ src/conf/nwfilter_conf.c | 102 ++++++++++++++++++++++++++++++ src/conf/nwfilter_conf.h | 17 +++++ src/nwfilter/nwfilter_ebiptables_driver.c | 35 ++++++++++ 4 files changed, 201 insertions(+) Index: libvirt-acl/src/conf/nwfilter_conf.c =================================================================== --- libvirt-acl.orig/src/conf/nwfilter_conf.c +++ libvirt-acl/src/conf/nwfilter_conf.c @@ -82,6 +82,7 @@ VIR_ENUM_IMPL(virNWFilterEbtablesTable, VIR_ENUM_IMPL(virNWFilterChainSuffix, VIR_NWFILTER_CHAINSUFFIX_LAST, "root", + "vlan", "arp", "rarp", "ipv4", @@ -90,6 +91,7 @@ VIR_ENUM_IMPL(virNWFilterChainSuffix, VI VIR_ENUM_IMPL(virNWFilterRuleProtocol, VIR_NWFILTER_RULE_PROTOCOL_LAST, "none", "mac", + "vlan", "arp", "rarp", "ip", @@ -126,6 +128,7 @@ struct int_map { static const struct int_map chain_priorities[] = { INTMAP_ENTRY(NWFILTER_ROOT_FILTER_PRI, "root"), + INTMAP_ENTRY(NWFILTER_VLAN_FILTER_PRI, "vlan"), INTMAP_ENTRY(NWFILTER_IPV4_FILTER_PRI, "ipv4"), INTMAP_ENTRY(NWFILTER_IPV6_FILTER_PRI, "ipv6"), INTMAP_ENTRY(NWFILTER_ARP_FILTER_PRI , "arp" ), @@ -459,6 +462,7 @@ static const struct int_map macProtoMap[ INTMAP_ENTRY(ETHERTYPE_REVARP, "rarp"), INTMAP_ENTRY(ETHERTYPE_IP , "ipv4"), INTMAP_ENTRY(ETHERTYPE_IPV6 , "ipv6"), + INTMAP_ENTRY(ETHERTYPE_VLAN , "vlan"), INTMAP_ENTRY_LAST }; @@ -513,6 +517,75 @@ macProtocolIDFormatter(virBufferPtr buf, } +static bool +checkVlanVlanID(enum attrDatatype datatype, union data *value, + virNWFilterRuleDefPtr nwf ATTRIBUTE_UNUSED, + nwItemDesc *item ATTRIBUTE_UNUSED) +{ + int32_t res; + + res = value->ui; + if (res < 0 || res > 4095) { + res = -1; + } + + if (res != -1) { + nwf->p.vlanHdrFilter.dataVlanID.u.u16 = res; + nwf->p.vlanHdrFilter.dataVlanID.datatype = datatype; + return true; + } + + return false; +} + +static bool +checkVlanProtocolID(enum attrDatatype datatype, union data *value, + virNWFilterRuleDefPtr nwf ATTRIBUTE_UNUSED, + nwItemDesc *item ATTRIBUTE_UNUSED) +{ + int32_t res = -1; + + if (datatype == DATATYPE_STRING) { + if (intMapGetByString(macProtoMap, value->c, 1, &res) == 0) + res = -1; + datatype = DATATYPE_UINT16; + } else if (datatype == DATATYPE_UINT16 || + datatype == DATATYPE_UINT16_HEX) { + res = value->ui; + if (res < 0x3c) + res = -1; + } + + if (res != -1) { + nwf->p.vlanHdrFilter.dataVlanEncap.u.u16 = res; + nwf->p.vlanHdrFilter.dataVlanEncap.datatype = datatype; + return true; + } + + return false; +} + +static bool +vlanProtocolIDFormatter(virBufferPtr buf, + virNWFilterRuleDefPtr nwf, + nwItemDesc *item ATTRIBUTE_UNUSED) +{ + const char *str = NULL; + bool asHex = true; + + if (intMapGetByInt(macProtoMap, + nwf->p.vlanHdrFilter.dataVlanEncap.u.u16, + &str)) { + virBufferAdd(buf, str, -1); + } else { + if (nwf->p.vlanHdrFilter.dataVlanEncap.datatype == DATATYPE_UINT16) + asHex = false; + virBufferAsprintf(buf, asHex ? "0x%x" : "%d", + nwf->p.vlanHdrFilter.dataVlanEncap.u.u16); + } + return true; +} + /* generic function to check for a valid (ipv4,ipv6, mac) mask * A mask is valid of there is a sequence of 1's followed by a sequence * of 0s or only 1s or only 0s @@ -951,6 +1024,27 @@ static const virXMLAttr2Struct macAttrib } }; +static const virXMLAttr2Struct vlanAttributes[] = { + COMMON_MAC_PROPS(ethHdrFilter), + { + .name = "vlanid", + .datatype = DATATYPE_UINT16 | DATATYPE_UINT16_HEX, + .dataIdx = offsetof(virNWFilterRuleDef, p.vlanHdrFilter.dataVlanID), + .validator = checkVlanVlanID, + }, + { + .name = "encap-protocol", + .datatype = DATATYPE_UINT16 | DATATYPE_UINT16_HEX | DATATYPE_STRING, + .dataIdx = offsetof(virNWFilterRuleDef, p.vlanHdrFilter.dataVlanEncap), + .validator = checkVlanProtocolID, + .formatter = vlanProtocolIDFormatter, + }, + COMMENT_PROP(vlanHdrFilter), + { + .name = NULL, + } +}; + static const virXMLAttr2Struct arpAttributes[] = { COMMON_MAC_PROPS(arpHdrFilter), { @@ -1408,6 +1502,7 @@ static const virAttributes virAttr[] = { PROTOCOL_ENTRY("arp" , arpAttributes , VIR_NWFILTER_RULE_PROTOCOL_ARP), PROTOCOL_ENTRY("rarp" , arpAttributes , VIR_NWFILTER_RULE_PROTOCOL_RARP), PROTOCOL_ENTRY("mac" , macAttributes , VIR_NWFILTER_RULE_PROTOCOL_MAC), + PROTOCOL_ENTRY("vlan" , vlanAttributes , VIR_NWFILTER_RULE_PROTOCOL_VLAN), PROTOCOL_ENTRY("ip" , ipAttributes , VIR_NWFILTER_RULE_PROTOCOL_IP), PROTOCOL_ENTRY("ipv6" , ipv6Attributes , VIR_NWFILTER_RULE_PROTOCOL_IPV6), PROTOCOL_ENTRY("tcp" , tcpAttributes , VIR_NWFILTER_RULE_PROTOCOL_TCP), @@ -1737,6 +1832,13 @@ virNWFilterRuleDefFixup(virNWFilterRuleD rule->p.ethHdrFilter.ethHdr.dataDstMACAddr); break; + case VIR_NWFILTER_RULE_PROTOCOL_VLAN: + COPY_NEG_SIGN(rule->p.vlanHdrFilter.ethHdr.dataSrcMACMask, + rule->p.vlanHdrFilter.ethHdr.dataSrcMACAddr); + COPY_NEG_SIGN(rule->p.vlanHdrFilter.ethHdr.dataDstMACMask, + rule->p.vlanHdrFilter.ethHdr.dataDstMACAddr); + break; + case VIR_NWFILTER_RULE_PROTOCOL_IP: COPY_NEG_SIGN(rule->p.ipHdrFilter.ipHdr.dataSrcIPMask, rule->p.ipHdrFilter.ipHdr.dataSrcIPAddr); Index: libvirt-acl/src/conf/nwfilter_conf.h =================================================================== --- libvirt-acl.orig/src/conf/nwfilter_conf.h +++ libvirt-acl/src/conf/nwfilter_conf.h @@ -55,6 +55,9 @@ # ifndef ETHERTYPE_IPV6 # define ETHERTYPE_IPV6 0x86dd # endif +# ifndef ETHERTYPE_VLAN +# define ETHERTYPE_VLAN 0x8100 +# endif /** * Chain suffix size is: @@ -151,6 +154,16 @@ struct _ethHdrFilterDef { }; +typedef struct _vlanHdrFilterDef vlanHdrFilterDef; +typedef vlanHdrFilterDef *vlanHdrFilterDefPtr; +struct _vlanHdrFilterDef { + ethHdrDataDef ethHdr; + nwItemDesc dataVlanID; + nwItemDesc dataVlanEncap; + nwItemDesc dataComment; +}; + + typedef struct _arpHdrFilterDef arpHdrFilterDef; typedef arpHdrFilterDef *arpHdrFilterDefPtr; struct _arpHdrFilterDef { @@ -323,6 +336,7 @@ enum virNWFilterChainPolicyType { enum virNWFilterRuleProtocolType { VIR_NWFILTER_RULE_PROTOCOL_NONE = 0, VIR_NWFILTER_RULE_PROTOCOL_MAC, + VIR_NWFILTER_RULE_PROTOCOL_VLAN, VIR_NWFILTER_RULE_PROTOCOL_ARP, VIR_NWFILTER_RULE_PROTOCOL_RARP, VIR_NWFILTER_RULE_PROTOCOL_IP, @@ -364,6 +378,7 @@ enum virNWFilterEbtablesTableType { # define NWFILTER_MAX_FILTER_PRIORITY MAX_RULE_PRIORITY # define NWFILTER_ROOT_FILTER_PRI 0 +# define NWFILTER_VLAN_FILTER_PRI -750 # define NWFILTER_IPV4_FILTER_PRI -700 # define NWFILTER_IPV6_FILTER_PRI -600 # define NWFILTER_ARP_FILTER_PRI -500 @@ -401,6 +416,7 @@ struct _virNWFilterRuleDef { enum virNWFilterRuleProtocolType prtclType; union { ethHdrFilterDef ethHdrFilter; + vlanHdrFilterDef vlanHdrFilter; arpHdrFilterDef arpHdrFilter; /* also used for rarp */ ipHdrFilterDef ipHdrFilter; ipv6HdrFilterDef ipv6HdrFilter; @@ -440,6 +456,7 @@ struct _virNWFilterEntry { enum virNWFilterChainSuffixType { VIR_NWFILTER_CHAINSUFFIX_ROOT = 0, + VIR_NWFILTER_CHAINSUFFIX_VLAN, VIR_NWFILTER_CHAINSUFFIX_ARP, VIR_NWFILTER_CHAINSUFFIX_RARP, VIR_NWFILTER_CHAINSUFFIX_IPv4, Index: libvirt-acl/src/nwfilter/nwfilter_ebiptables_driver.c =================================================================== --- libvirt-acl.orig/src/nwfilter/nwfilter_ebiptables_driver.c +++ libvirt-acl/src/nwfilter/nwfilter_ebiptables_driver.c @@ -177,6 +177,7 @@ enum l3_proto_idx { L3_PROTO_IPV6_IDX, L3_PROTO_ARP_IDX, L3_PROTO_RARP_IDX, + L2_PROTO_VLAN_IDX, L3_PROTO_LAST_IDX }; @@ -187,6 +188,7 @@ static const struct ushort_map l3_protoc USHORTMAP_ENTRY_IDX(L3_PROTO_IPV6_IDX, ETHERTYPE_IPV6 , "ipv6"), USHORTMAP_ENTRY_IDX(L3_PROTO_ARP_IDX , ETHERTYPE_ARP , "arp"), USHORTMAP_ENTRY_IDX(L3_PROTO_RARP_IDX, ETHERTYPE_REVARP, "rarp"), + USHORTMAP_ENTRY_IDX(L2_PROTO_VLAN_IDX, ETHERTYPE_VLAN , "vlan"), USHORTMAP_ENTRY_IDX(L3_PROTO_LAST_IDX, 0 , NULL), }; @@ -1980,6 +1982,38 @@ ebtablesCreateRuleInstance(char chainPre } break; + case VIR_NWFILTER_RULE_PROTOCOL_VLAN: + + virBufferAsprintf(&buf, + CMD_DEF_PRE "%s -t %s -%%c %s %%s", + ebtables_cmd_path, EBTABLES_DEFAULT_TABLE, chain); + + + if (ebtablesHandleEthHdr(&buf, + vars, + &rule->p.vlanHdrFilter.ethHdr, + reverse)) + goto err_exit; + + virBufferAddLit(&buf, + " -p 0x8100"); + +#define INST_ITEM(STRUCT, ITEM, CLI) \ + if (HAS_ENTRY_ITEM(&rule->p.STRUCT.ITEM)) { \ + if (printDataType(vars, \ + number, sizeof(number), \ + &rule->p.STRUCT.ITEM)) \ + goto err_exit; \ + virBufferAsprintf(&buf, \ + " " CLI " %s %s", \ + ENTRY_GET_NEG_SIGN(&rule->p.STRUCT.ITEM), \ + number); \ + } + + INST_ITEM(vlanHdrFilter, dataVlanID, "--vlan-id") + INST_ITEM(vlanHdrFilter, dataVlanEncap, "--vlan-encap") + break; + case VIR_NWFILTER_RULE_PROTOCOL_ARP: case VIR_NWFILTER_RULE_PROTOCOL_RARP: @@ -2427,6 +2461,7 @@ ebiptablesCreateRuleInstance(virConnectP switch (rule->prtclType) { case VIR_NWFILTER_RULE_PROTOCOL_IP: case VIR_NWFILTER_RULE_PROTOCOL_MAC: + case VIR_NWFILTER_RULE_PROTOCOL_VLAN: case VIR_NWFILTER_RULE_PROTOCOL_ARP: case VIR_NWFILTER_RULE_PROTOCOL_RARP: case VIR_NWFILTER_RULE_PROTOCOL_NONE: Index: libvirt-acl/docs/schemas/nwfilter.rng =================================================================== --- libvirt-acl.orig/docs/schemas/nwfilter.rng +++ libvirt-acl/docs/schemas/nwfilter.rng @@ -30,6 +30,16 @@ </optional> <optional> <zeroOrMore> + <element name="vlan"> + <ref name="match-attribute"/> + <ref name="common-l2-attributes"/> + <ref name="vlan-attributes"/> + <ref name="comment-attribute"/> + </element> + </zeroOrMore> + </optional> + <optional> + <zeroOrMore> <element name="arp"> <ref name="match-attribute"/> <ref name="common-l2-attributes"/> @@ -287,6 +297,9 @@ <choice> <value>root</value> <data type="string"> + <param name="pattern">vlan[a-zA-Z0-9_\.:-]*</param> + </data> + <data type="string"> <param name="pattern">arp[a-zA-Z0-9_\.:-]*</param> </data> <data type="string"> @@ -559,6 +572,21 @@ </interleave> </define> + <define name="vlan-attributes"> + <interleave> + <optional> + <attribute name="vlanid"> + <ref name="vlan-vlanid"/> + </attribute> + </optional> + <optional> + <attribute name="encap-protocol"> + <ref name="mac-protocolid"/> + </attribute> + </optional> + </interleave> + </define> + <define name="arp-attributes"> <interleave> <optional> @@ -764,10 +792,29 @@ <value>rarp</value> <value>ipv4</value> <value>ipv6</value> + <value>vlan</value> </choice> </choice> </define> + <define name="vlan-vlanid"> + <choice> + <!-- variable --> + <data type="string"> + <param name="pattern">$[a-zA-Z0-9_]+</param> + </data> + + <data type="string"> + <param name="pattern">0x([0-9a-fA-F]{1,3})</param> + </data> + + <data type="int"> + <param name="minInclusive">0</param> + <param name="maxInclusive">4095</param> + </data> + </choice> + </define> + <define name="uint8range"> <choice> <!-- variable -->
On 10/26/2011 09:11 AM, Stefan Berger wrote: Apologies for delaying so long on reviewing this.
This patch adds support for filtering of VLAN (802.1Q) traffic to the parser and makes us of the ebtables support for VLAN filtering. This code now enables the filtering of traffic in chains with prefix 'vlan'.
Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
--- docs/schemas/nwfilter.rng | 47 +++++++++++++ src/conf/nwfilter_conf.c | 102 ++++++++++++++++++++++++++++++ src/conf/nwfilter_conf.h | 17 +++++ src/nwfilter/nwfilter_ebiptables_driver.c | 35 ++++++++++ 4 files changed, 201 insertions(+)
This didn't apply cleanly for me. Am I missing the review of another pre-requisite series? Or is it just something where you need to rebase and post a v2 for easier review?
+static bool +checkVlanVlanID(enum attrDatatype datatype, union data *value, + virNWFilterRuleDefPtr nwf ATTRIBUTE_UNUSED,
ATTRIBUTE_UNUSED not necessary here, since...
+ nwItemDesc *item ATTRIBUTE_UNUSED) +{ + int32_t res; + + res = value->ui; + if (res < 0 || res > 4095) { + res = -1; + } + + if (res != -1) { + nwf->p.vlanHdrFilter.dataVlanID.u.u16 = res;
...this uses nwf. Similar comment applies elsewhere in the patch. Overall, it looks sane, but I didn't compile test it. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Tue, Nov 15, 2011 at 05:46:21PM -0700, Eric Blake wrote:
On 10/26/2011 09:11 AM, Stefan Berger wrote:
Apologies for delaying so long on reviewing this.
This patch adds support for filtering of VLAN (802.1Q) traffic to the parser and makes us of the ebtables support for VLAN filtering. This code now enables the filtering of traffic in chains with prefix 'vlan'.
Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
--- docs/schemas/nwfilter.rng | 47 +++++++++++++ src/conf/nwfilter_conf.c | 102 ++++++++++++++++++++++++++++++ src/conf/nwfilter_conf.h | 17 +++++ src/nwfilter/nwfilter_ebiptables_driver.c | 35 ++++++++++ 4 files changed, 201 insertions(+)
This didn't apply cleanly for me. Am I missing the review of another pre-requisite series? Or is it just something where you need to rebase and post a v2 for easier review?
It could be some of my netdev API renaming invalidated it ? Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 11/15/2011 07:46 PM, Eric Blake wrote:
On 10/26/2011 09:11 AM, Stefan Berger wrote:
Apologies for delaying so long on reviewing this.
This patch adds support for filtering of VLAN (802.1Q) traffic to the parser and makes us of the ebtables support for VLAN filtering. This code now enables the filtering of traffic in chains with prefix 'vlan'.
Signed-off-by: Stefan Berger<stefanb@linux.vnet.ibm.com>
--- docs/schemas/nwfilter.rng | 47 +++++++++++++ src/conf/nwfilter_conf.c | 102 ++++++++++++++++++++++++++++++ src/conf/nwfilter_conf.h | 17 +++++ src/nwfilter/nwfilter_ebiptables_driver.c | 35 ++++++++++ 4 files changed, 201 insertions(+)
This didn't apply cleanly for me. Am I missing the review of another pre-requisite series? Or is it just something where you need to rebase and post a v2 for easier review?
Yes, sorry for the confusion. This builds on top of the following two series: https://www.redhat.com/archives/libvir-list/2011-October/msg01227.html then came this one (already ACKed): https://www.redhat.com/archives/libvir-list/2011-October/msg01360.html
+static bool +checkVlanVlanID(enum attrDatatype datatype, union data *value, + virNWFilterRuleDefPtr nwf ATTRIBUTE_UNUSED,
ATTRIBUTE_UNUSED not necessary here, since...
+ nwItemDesc *item ATTRIBUTE_UNUSED) +{ + int32_t res; + + res = value->ui; + if (res< 0 || res> 4095) { + res = -1; + } + + if (res != -1) { + nwf->p.vlanHdrFilter.dataVlanID.u.u16 = res; ...this uses nwf. Similar comment applies elsewhere in the patch.
Thanks. Fixing it.
Overall, it looks sane, but I didn't compile test it.
Stefan
This patch adds a few test cases for the XML parsing of VLAN filtering nodes. Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> --- tests/nwfilterxml2xmlin/vlan-test.xml | 45 +++++++++++++++++++++++++++++++++ tests/nwfilterxml2xmlout/vlan-test.xml | 21 +++++++++++++++ tests/nwfilterxml2xmltest.c | 1 3 files changed, 67 insertions(+) Index: libvirt-acl/tests/nwfilterxml2xmlin/vlan-test.xml =================================================================== --- /dev/null +++ libvirt-acl/tests/nwfilterxml2xmlin/vlan-test.xml @@ -0,0 +1,45 @@ +<filter name='testcase'> + <uuid>5c6d49af-b071-6127-b4ec-6f8ed4b55335</uuid> + <rule action='continue' direction='inout'> + <vlan srcmacaddr='1:2:3:4:5:6' srcmacmask='ff:ff:ff:ff:ff:ff' + dstmacaddr='aa:bb:cc:dd:ee:ff' dstmacmask='ff:ff:ff:ff:ff:ff' + vlanid='0x123' + /> + </rule> + + <rule action='return' direction='inout'> + <vlan srcmacaddr='1:2:3:4:5:6' srcmacmask='ff:ff:ff:ff:ff:ff' + dstmacaddr='aa:bb:cc:dd:ee:ff' dstmacmask='ff:ff:ff:ff:ff:ff' + vlanid='1234' + /> + </rule> + + <rule action='reject' direction='in'> + <vlan srcmacaddr='1:2:3:4:5:6' srcmacmask='ff:ff:ff:ff:ff:ff' + dstmacaddr='aa:bb:cc:dd:ee:ff' dstmacmask='ff:ff:ff:ff:ff:ff' + vlanid='0x123' + /> + </rule> + + <rule action='accept' direction='in'> + <vlan srcmacaddr='1:2:3:4:5:6' srcmacmask='ff:ff:ff:ff:ff:ff' + dstmacaddr='aa:bb:cc:dd:ee:ff' dstmacmask='ff:ff:ff:ff:ff:ff' + vlanid='0xffff' + /> + </rule> + + <rule action='drop' direction='out'> + <vlan srcmacaddr='1:2:3:4:5:6' srcmacmask='ff:ff:ff:ff:ff:ff' + dstmacaddr='aa:bb:cc:dd:ee:ff' dstmacmask='ff:ff:ff:ff:ff:ff' + encap-protocol='arp' + /> + </rule> + + <rule action='accept' direction='out'> + <vlan srcmacaddr='1:2:3:4:5:6' srcmacmask='ff:ff:ff:ff:ff:ff' + dstmacaddr='aa:bb:cc:dd:ee:ff' dstmacmask='ff:ff:ff:ff:ff:ff' + encap-protocol='0x1234' + /> + </rule> + +</filter> Index: libvirt-acl/tests/nwfilterxml2xmltest.c =================================================================== --- libvirt-acl.orig/tests/nwfilterxml2xmltest.c +++ libvirt-acl/tests/nwfilterxml2xmltest.c @@ -108,6 +108,7 @@ mymain(void) } while (0) DO_TEST("mac-test", true); + DO_TEST("vlan-test", true); DO_TEST("arp-test", true); DO_TEST("rarp-test", true); DO_TEST("ip-test", true); Index: libvirt-acl/tests/nwfilterxml2xmlout/vlan-test.xml =================================================================== --- /dev/null +++ libvirt-acl/tests/nwfilterxml2xmlout/vlan-test.xml @@ -0,0 +1,21 @@ +<filter name='testcase' chain='root'> + <uuid>5c6d49af-b071-6127-b4ec-6f8ed4b55335</uuid> + <rule action='continue' direction='inout' priority='500'> + <vlan srcmacaddr='01:02:03:04:05:06' srcmacmask='ff:ff:ff:ff:ff:ff' dstmacaddr='aa:bb:cc:dd:ee:ff' dstmacmask='ff:ff:ff:ff:ff:ff' vlanid='0x123'/> + </rule> + <rule action='return' direction='inout' priority='500'> + <vlan srcmacaddr='01:02:03:04:05:06' srcmacmask='ff:ff:ff:ff:ff:ff' dstmacaddr='aa:bb:cc:dd:ee:ff' dstmacmask='ff:ff:ff:ff:ff:ff' vlanid='1234'/> + </rule> + <rule action='reject' direction='in' priority='500'> + <vlan srcmacaddr='01:02:03:04:05:06' srcmacmask='ff:ff:ff:ff:ff:ff' dstmacaddr='aa:bb:cc:dd:ee:ff' dstmacmask='ff:ff:ff:ff:ff:ff' vlanid='0x123'/> + </rule> + <rule action='accept' direction='in' priority='500'> + <vlan srcmacaddr='01:02:03:04:05:06' srcmacmask='ff:ff:ff:ff:ff:ff' dstmacaddr='aa:bb:cc:dd:ee:ff' dstmacmask='ff:ff:ff:ff:ff:ff'/> + </rule> + <rule action='drop' direction='out' priority='500'> + <vlan srcmacaddr='01:02:03:04:05:06' srcmacmask='ff:ff:ff:ff:ff:ff' dstmacaddr='aa:bb:cc:dd:ee:ff' dstmacmask='ff:ff:ff:ff:ff:ff' encap-protocol='arp'/> + </rule> + <rule action='accept' direction='out' priority='500'> + <vlan srcmacaddr='01:02:03:04:05:06' srcmacmask='ff:ff:ff:ff:ff:ff' dstmacaddr='aa:bb:cc:dd:ee:ff' dstmacmask='ff:ff:ff:ff:ff:ff' encap-protocol='0x1234'/> + </rule> +</filter>
On 10/26/2011 09:11 AM, Stefan Berger wrote:
This patch adds a few test cases for the XML parsing of VLAN filtering nodes.
Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
--- tests/nwfilterxml2xmlin/vlan-test.xml | 45 +++++++++++++++++++++++++++++++++ tests/nwfilterxml2xmlout/vlan-test.xml | 21 +++++++++++++++ tests/nwfilterxml2xmltest.c | 1 3 files changed, 67 insertions(+)
ACK. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Add documentation for the VLAN filtering support. Describe the XML attributes that are supported. Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> --- docs/formatnwfilter.html.in | 55 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 55 insertions(+) Index: libvirt-acl/docs/formatnwfilter.html.in =================================================================== --- libvirt-acl.orig/docs/formatnwfilter.html.in +++ libvirt-acl/docs/formatnwfilter.html.in @@ -412,6 +412,61 @@ [...] </pre> + <h5><a name="nwfelemsRulesProtoVLAN">VLAN (802.1Q)</a> + <span class="since">(Since 0.9.7)</span> + </h5> + <p> + Protocol ID: <code>vlan</code> + <br/> + Note: Rules of this type should go eitherinto the <code>root</code> or + <code>vlan</code> chain. + </p> + <table class="top_table"> + <tr> + <th> Attribute </th> + <th> Datatype </th> + <th> Semantics </th> + </tr> + <tr> + <td>srcmacaddr</td> + <td>MAC_ADDR</td> + <td>MAC address of sender</td> + </tr> + <tr> + <td>srcmacmask</td> + <td>MAC_MASK</td> + <td>Mask applied to MAC address of sender</td> + </tr> + <tr> + <td>dstmacaddr</td> + <td>MAC_ADDR</td> + <td>MAC address of destination</td> + </tr> + <tr> + <td>dstmacmask</td> + <td>MAC_MASK</td> + <td>Mask applied to MAC address of destination</td> + </tr> + <tr> + <td>vlan-id</td> + <td>UINT16 (0x0-0xfff, 0 - 4095)</td> + <td>VLAN ID</td> + </tr> + <tr> + <td>encap-protocol</td> + <td>UINT16 (0x03c-0xfff), String</td> + <td>Encapsulated layer 3 protocol ID</td> + </tr> + <tr> + <td>comment </td> + <td>STRING</td> + <td>text with max. 256 characters</td> + </tr> + </table> + <p> + Valid Strings for <code>encap-protocol</code> are: arp, ipv4, ipv6 + </p> + <h5><a name="nwfelemsRulesProtoARP">ARP/RARP</a></h5> <p> Protocol ID: <code>arp</code> or <code>rarp</code>
On 10/26/2011 09:12 AM, Stefan Berger wrote:
Add documentation for the VLAN filtering support. Describe the XML attributes that are supported.
Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
Not sure if this overlaps with any of the stuff you pulled in earlier (it will be Monday before I get my development tree back into a state where I can apply this to actually test it). But in general, adding docs is good.
+ <br/> + Note: Rules of this type should go eitherinto the <code>root</code> or
s/eitherinto/either into/ ACK with the typo fixed, assuming it still applies fairly easily. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 11/18/2011 07:14 PM, Eric Blake wrote:
On 10/26/2011 09:12 AM, Stefan Berger wrote:
Add documentation for the VLAN filtering support. Describe the XML attributes that are supported.
Signed-off-by: Stefan Berger<stefanb@linux.vnet.ibm.com> Not sure if this overlaps with any of the stuff you pulled in earlier (it will be Monday before I get my development tree back into a state where I can apply this to actually test it). But in general, adding docs is good.
+<br/> + Note: Rules of this type should go eitherinto the<code>root</code> or s/eitherinto/either into/
ACK with the typo fixed, assuming it still applies fairly easily.
Was not a problem... I now pushed the 3 VLAN related patches. Thanks.
With hunks borrowed from one of David Steven's previous patches, we now add the capability of having a 'mac' chain which is useful to filter for multiple valid MAC addresses. Signed-off-by: David L Stevens <dlstevens@us.ibm.com> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> --- docs/schemas/nwfilter.rng | 3 +++ src/conf/nwfilter_conf.c | 2 ++ src/conf/nwfilter_conf.h | 2 ++ src/nwfilter/nwfilter_ebiptables_driver.c | 16 ++++++++++++++-- 4 files changed, 21 insertions(+), 2 deletions(-) Index: libvirt-acl/src/nwfilter/nwfilter_ebiptables_driver.c =================================================================== --- libvirt-acl.orig/src/nwfilter/nwfilter_ebiptables_driver.c +++ libvirt-acl/src/nwfilter/nwfilter_ebiptables_driver.c @@ -177,6 +177,7 @@ enum l3_proto_idx { L3_PROTO_IPV6_IDX, L3_PROTO_ARP_IDX, L3_PROTO_RARP_IDX, + L2_PROTO_MAC_IDX, L2_PROTO_VLAN_IDX, L3_PROTO_LAST_IDX }; @@ -189,6 +190,7 @@ static const struct ushort_map l3_protoc USHORTMAP_ENTRY_IDX(L3_PROTO_ARP_IDX , ETHERTYPE_ARP , "arp"), USHORTMAP_ENTRY_IDX(L3_PROTO_RARP_IDX, ETHERTYPE_REVARP, "rarp"), USHORTMAP_ENTRY_IDX(L2_PROTO_VLAN_IDX, ETHERTYPE_VLAN , "vlan"), + USHORTMAP_ENTRY_IDX(L2_PROTO_MAC_IDX, 0 , "mac"), USHORTMAP_ENTRY_IDX(L3_PROTO_LAST_IDX, 0 , NULL), }; @@ -2905,11 +2907,21 @@ ebtablesCreateTmpSubChain(ebiptablesRule char rootchain[MAX_CHAINNAME_LENGTH], chain[MAX_CHAINNAME_LENGTH]; char chainPrefix = (incoming) ? CHAINPREFIX_HOST_IN_TEMP : CHAINPREFIX_HOST_OUT_TEMP; + char protostr[16] = { 0, }; PRINT_ROOT_CHAIN(rootchain, chainPrefix, ifname); PRINT_CHAIN(chain, chainPrefix, ifname, (filtername) ? filtername : l3_protocols[protoidx].val); + switch (protoidx) { + case L2_PROTO_MAC_IDX: + break; + default: + snprintf(protostr, sizeof(protostr), "-p 0x%04x ", + l3_protocols[protoidx].attr); + break; + } + virBufferAsprintf(&buf, CMD_DEF("%s -t %s -F %s") CMD_SEPARATOR CMD_EXEC @@ -2918,7 +2930,7 @@ ebtablesCreateTmpSubChain(ebiptablesRule CMD_DEF("%s -t %s -N %s") CMD_SEPARATOR CMD_EXEC "%s" - CMD_DEF("%s -t %s -%%c %s %%s -p 0x%x -j %s") + CMD_DEF("%s -t %s -%%c %s %%s %s -j %s") CMD_SEPARATOR CMD_EXEC "%s", @@ -2930,7 +2942,7 @@ ebtablesCreateTmpSubChain(ebiptablesRule CMD_STOPONERR(stopOnError), ebtables_cmd_path, EBTABLES_DEFAULT_TABLE, - rootchain, l3_protocols[protoidx].attr, chain, + rootchain, protostr, chain, CMD_STOPONERR(stopOnError)); Index: libvirt-acl/src/conf/nwfilter_conf.c =================================================================== --- libvirt-acl.orig/src/conf/nwfilter_conf.c +++ libvirt-acl/src/conf/nwfilter_conf.c @@ -82,6 +82,7 @@ VIR_ENUM_IMPL(virNWFilterEbtablesTable, VIR_ENUM_IMPL(virNWFilterChainSuffix, VIR_NWFILTER_CHAINSUFFIX_LAST, "root", + "mac", "vlan", "arp", "rarp", @@ -128,6 +129,7 @@ struct int_map { static const struct int_map chain_priorities[] = { INTMAP_ENTRY(NWFILTER_ROOT_FILTER_PRI, "root"), + INTMAP_ENTRY(NWFILTER_MAC_FILTER_PRI, "mac"), INTMAP_ENTRY(NWFILTER_VLAN_FILTER_PRI, "vlan"), INTMAP_ENTRY(NWFILTER_IPV4_FILTER_PRI, "ipv4"), INTMAP_ENTRY(NWFILTER_IPV6_FILTER_PRI, "ipv6"), Index: libvirt-acl/src/conf/nwfilter_conf.h =================================================================== --- libvirt-acl.orig/src/conf/nwfilter_conf.h +++ libvirt-acl/src/conf/nwfilter_conf.h @@ -378,6 +378,7 @@ enum virNWFilterEbtablesTableType { # define NWFILTER_MAX_FILTER_PRIORITY MAX_RULE_PRIORITY # define NWFILTER_ROOT_FILTER_PRI 0 +# define NWFILTER_MAC_FILTER_PRI -800 # define NWFILTER_VLAN_FILTER_PRI -750 # define NWFILTER_IPV4_FILTER_PRI -700 # define NWFILTER_IPV6_FILTER_PRI -600 @@ -456,6 +457,7 @@ struct _virNWFilterEntry { enum virNWFilterChainSuffixType { VIR_NWFILTER_CHAINSUFFIX_ROOT = 0, + VIR_NWFILTER_CHAINSUFFIX_MAC, VIR_NWFILTER_CHAINSUFFIX_VLAN, VIR_NWFILTER_CHAINSUFFIX_ARP, VIR_NWFILTER_CHAINSUFFIX_RARP, Index: libvirt-acl/docs/schemas/nwfilter.rng =================================================================== --- libvirt-acl.orig/docs/schemas/nwfilter.rng +++ libvirt-acl/docs/schemas/nwfilter.rng @@ -297,6 +297,9 @@ <choice> <value>root</value> <data type="string"> + <param name="pattern">mac[a-zA-Z0-9_\.:-]*</param> + </data> + <data type="string"> <param name="pattern">vlan[a-zA-Z0-9_\.:-]*</param> </data> <data type="string">
On 10/26/2011 09:12 AM, Stefan Berger wrote:
With hunks borrowed from one of David Steven's previous patches, we now add the capability of having a 'mac' chain which is useful to filter for multiple valid MAC addresses.
Signed-off-by: David L Stevens <dlstevens@us.ibm.com> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
+ char protostr[16] = { 0, };
A bit oversized...
PRINT_ROOT_CHAIN(rootchain, chainPrefix, ifname); PRINT_CHAIN(chain, chainPrefix, ifname, (filtername) ? filtername : l3_protocols[protoidx].val);
+ switch (protoidx) { + case L2_PROTO_MAC_IDX: + break; + default: + snprintf(protostr, sizeof(protostr), "-p 0x%04x ",
for a max of 11 bytes (including trailing NUL) ever printed into it, but not the end of the world. And since you didn't check snprintf results for failure, if we ever change the size of protostr or the length of what we print into it, it's a slight maintenance risk we are taking on, compared to dynamic allocation that always gets the right length. But I don't know if it is worth replacing this snprintf with virAsprintf/VIR_FREE overhead, so I can live with it as is.
@@ -2918,7 +2930,7 @@ ebtablesCreateTmpSubChain(ebiptablesRule CMD_DEF("%s -t %s -N %s") CMD_SEPARATOR CMD_EXEC "%s" - CMD_DEF("%s -t %s -%%c %s %%s -p 0x%x -j %s") + CMD_DEF("%s -t %s -%%c %s %%s %s -j %s")
This results in output with a double space, either: ...%s -j ... or ...%s -p 0xnnnn -j ... Also not the end of the world, but you may want to remove the extra space before the -j. ACK. There is some conflict resolution needed in nwfilter.rng, but that should be trivial. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 11/21/2011 05:13 PM, Eric Blake wrote:
On 10/26/2011 09:12 AM, Stefan Berger wrote:
With hunks borrowed from one of David Steven's previous patches, we now add the capability of having a 'mac' chain which is useful to filter for multiple valid MAC addresses.
Signed-off-by: David L Stevens<dlstevens@us.ibm.com> Signed-off-by: Stefan Berger<stefanb@linux.vnet.ibm.com>
+ char protostr[16] = { 0, }; A bit oversized...
PRINT_ROOT_CHAIN(rootchain, chainPrefix, ifname); PRINT_CHAIN(chain, chainPrefix, ifname, (filtername) ? filtername : l3_protocols[protoidx].val);
+ switch (protoidx) { + case L2_PROTO_MAC_IDX: + break; + default: + snprintf(protostr, sizeof(protostr), "-p 0x%04x ",
for a max of 11 bytes (including trailing NUL) ever printed into it, but not the end of the world. And since you didn't check snprintf results for failure, if we ever change the size of protostr or the length of what we print into it, it's a slight maintenance risk we are taking on, compared to dynamic allocation that always gets the right length. But I don't know if it is worth replacing this snprintf with virAsprintf/VIR_FREE overhead, so I can live with it as is. I converted it now to virAsprintf / VIR_FREE.
@@ -2918,7 +2930,7 @@ ebtablesCreateTmpSubChain(ebiptablesRule CMD_DEF("%s -t %s -N %s") CMD_SEPARATOR CMD_EXEC "%s" - CMD_DEF("%s -t %s -%%c %s %%s -p 0x%x -j %s") + CMD_DEF("%s -t %s -%%c %s %%s %s -j %s") This results in output with a double space, either:
...%s -j ...
or
...%s -p 0xnnnn -j ...
Also not the end of the world, but you may want to remove the extra space before the -j. Fixed. Will repost as v2. ACK. There is some conflict resolution needed in nwfilter.rng, but that should be trivial.
This patch adds support for filtering of STP (spanning tree protocol) traffic to the parser and makes us of the ebtables support for STP filtering. This code now enables the filtering of traffic in chains with prefix 'stp'. Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> --- docs/schemas/nwfilter.rng | 154 +++++++++++++++++++++++++ src/conf/nwfilter_conf.c | 178 ++++++++++++++++++++++++++++++ src/conf/nwfilter_conf.h | 41 ++++++ src/libvirt_private.syms | 1 src/nwfilter/nwfilter_ebiptables_driver.c | 90 +++++++++++++++ 5 files changed, 461 insertions(+), 3 deletions(-) Index: libvirt-acl/src/conf/nwfilter_conf.c =================================================================== --- libvirt-acl.orig/src/conf/nwfilter_conf.c +++ libvirt-acl/src/conf/nwfilter_conf.c @@ -84,6 +84,7 @@ VIR_ENUM_IMPL(virNWFilterChainSuffix, VI "root", "mac", "vlan", + "stp", "arp", "rarp", "ipv4", @@ -93,6 +94,7 @@ VIR_ENUM_IMPL(virNWFilterRuleProtocol, V "none", "mac", "vlan", + "stp", "arp", "rarp", "ip", @@ -1047,6 +1049,136 @@ static const virXMLAttr2Struct vlanAttri } }; +static const virXMLAttr2Struct stpAttributes[] = { + /* spanning tree uses a special destination MAC address */ + { + .name = SRCMACADDR, + .datatype = DATATYPE_MACADDR, + .dataIdx = offsetof(virNWFilterRuleDef, + p.stpHdrFilter.ethHdr.dataSrcMACAddr), + }, + { + .name = SRCMACMASK, + .datatype = DATATYPE_MACMASK, + .dataIdx = offsetof(virNWFilterRuleDef, + p.stpHdrFilter.ethHdr.dataSrcMACMask), + }, + { + .name = "type", + .datatype = DATATYPE_UINT8 | DATATYPE_UINT8_HEX, + .dataIdx = offsetof(virNWFilterRuleDef, p.stpHdrFilter.dataType), + }, + { + .name = "flags", + .datatype = DATATYPE_UINT8 | DATATYPE_UINT8_HEX, + .dataIdx = offsetof(virNWFilterRuleDef, p.stpHdrFilter.dataFlags), + }, + { + .name = "root-priority", + .datatype = DATATYPE_UINT16 | DATATYPE_UINT16_HEX, + .dataIdx = offsetof(virNWFilterRuleDef, p.stpHdrFilter.dataRootPri), + }, + { + .name = "root-priority-hi", + .datatype = DATATYPE_UINT16 | DATATYPE_UINT16_HEX, + .dataIdx = offsetof(virNWFilterRuleDef, p.stpHdrFilter.dataRootPriHi), + }, + { + .name = "root-address", + .datatype = DATATYPE_MACADDR, + .dataIdx = offsetof(virNWFilterRuleDef, p.stpHdrFilter.dataRootAddr), + }, + { + .name = "root-address-mask", + .datatype = DATATYPE_MACMASK, + .dataIdx = offsetof(virNWFilterRuleDef, p.stpHdrFilter.dataRootAddrMask), + }, + { + .name = "root-cost", + .datatype = DATATYPE_UINT32 | DATATYPE_UINT32_HEX, + .dataIdx = offsetof(virNWFilterRuleDef, p.stpHdrFilter.dataRootCost), + }, + { + .name = "root-cost-hi", + .datatype = DATATYPE_UINT32 | DATATYPE_UINT32_HEX, + .dataIdx = offsetof(virNWFilterRuleDef, p.stpHdrFilter.dataRootCostHi), + }, + { + .name = "sender-priority", + .datatype = DATATYPE_UINT16 | DATATYPE_UINT16_HEX, + .dataIdx = offsetof(virNWFilterRuleDef, p.stpHdrFilter.dataSndrPrio), + }, + { + .name = "sender-priority-hi", + .datatype = DATATYPE_UINT16 | DATATYPE_UINT16_HEX, + .dataIdx = offsetof(virNWFilterRuleDef, p.stpHdrFilter.dataSndrPrioHi), + }, + { + .name = "sender-address", + .datatype = DATATYPE_MACADDR, + .dataIdx = offsetof(virNWFilterRuleDef, p.stpHdrFilter.dataSndrAddr), + }, + { + .name = "sender-address-mask", + .datatype = DATATYPE_MACMASK, + .dataIdx = offsetof(virNWFilterRuleDef, p.stpHdrFilter.dataSndrAddrMask), + }, + { + .name = "port", + .datatype = DATATYPE_UINT16 | DATATYPE_UINT16_HEX, + .dataIdx = offsetof(virNWFilterRuleDef, p.stpHdrFilter.dataPort), + }, + { + .name = "port-hi", + .datatype = DATATYPE_UINT16 | DATATYPE_UINT16_HEX, + .dataIdx = offsetof(virNWFilterRuleDef, p.stpHdrFilter.dataPortHi), + }, + { + .name = "age", + .datatype = DATATYPE_UINT16 | DATATYPE_UINT16_HEX, + .dataIdx = offsetof(virNWFilterRuleDef, p.stpHdrFilter.dataAge), + }, + { + .name = "age-hi", + .datatype = DATATYPE_UINT16 | DATATYPE_UINT16_HEX, + .dataIdx = offsetof(virNWFilterRuleDef, p.stpHdrFilter.dataAgeHi), + }, + { + .name = "max-age", + .datatype = DATATYPE_UINT16 | DATATYPE_UINT16_HEX, + .dataIdx = offsetof(virNWFilterRuleDef, p.stpHdrFilter.dataMaxAge), + }, + { + .name = "max-age-hi", + .datatype = DATATYPE_UINT16 | DATATYPE_UINT16_HEX, + .dataIdx = offsetof(virNWFilterRuleDef, p.stpHdrFilter.dataMaxAgeHi), + }, + { + .name = "hello-time", + .datatype = DATATYPE_UINT16 | DATATYPE_UINT16_HEX, + .dataIdx = offsetof(virNWFilterRuleDef, p.stpHdrFilter.dataHelloTime), + }, + { + .name = "hello-time-hi", + .datatype = DATATYPE_UINT16 | DATATYPE_UINT16_HEX, + .dataIdx = offsetof(virNWFilterRuleDef, p.stpHdrFilter.dataHelloTimeHi), + }, + { + .name = "forward-delay", + .datatype = DATATYPE_UINT16 | DATATYPE_UINT16_HEX, + .dataIdx = offsetof(virNWFilterRuleDef, p.stpHdrFilter.dataFwdDelay), + }, + { + .name = "forward-delay-hi", + .datatype = DATATYPE_UINT16 | DATATYPE_UINT16_HEX, + .dataIdx = offsetof(virNWFilterRuleDef, p.stpHdrFilter.dataFwdDelayHi), + }, + COMMENT_PROP(stpHdrFilter), + { + .name = NULL, + } +}; + static const virXMLAttr2Struct arpAttributes[] = { COMMON_MAC_PROPS(arpHdrFilter), { @@ -1505,6 +1637,7 @@ static const virAttributes virAttr[] = { PROTOCOL_ENTRY("rarp" , arpAttributes , VIR_NWFILTER_RULE_PROTOCOL_RARP), PROTOCOL_ENTRY("mac" , macAttributes , VIR_NWFILTER_RULE_PROTOCOL_MAC), PROTOCOL_ENTRY("vlan" , vlanAttributes , VIR_NWFILTER_RULE_PROTOCOL_VLAN), + PROTOCOL_ENTRY("stp" , stpAttributes , VIR_NWFILTER_RULE_PROTOCOL_STP), PROTOCOL_ENTRY("ip" , ipAttributes , VIR_NWFILTER_RULE_PROTOCOL_IP), PROTOCOL_ENTRY("ipv6" , ipv6Attributes , VIR_NWFILTER_RULE_PROTOCOL_IPV6), PROTOCOL_ENTRY("tcp" , tcpAttributes , VIR_NWFILTER_RULE_PROTOCOL_TCP), @@ -1628,6 +1761,18 @@ virNWFilterRuleDetailsParse(xmlNodePtr n rc = -1; break; + case DATATYPE_UINT32_HEX: + base = 16; + /* fallthrough */ + case DATATYPE_UINT32: + if (virStrToLong_ui(prop, NULL, base, &uint_val) >= 0) { + item->u.u32 = uint_val; + found = 1; + data.ui = uint_val; + } else + rc = -1; + break; + case DATATYPE_IPADDR: if (virSocketParseIpv4Addr(prop, &item->u.ipaddr) < 0) @@ -1841,6 +1986,31 @@ virNWFilterRuleDefFixup(virNWFilterRuleD rule->p.vlanHdrFilter.ethHdr.dataDstMACAddr); break; + case VIR_NWFILTER_RULE_PROTOCOL_STP: + COPY_NEG_SIGN(rule->p.stpHdrFilter.ethHdr.dataSrcMACMask, + rule->p.stpHdrFilter.ethHdr.dataSrcMACAddr); + COPY_NEG_SIGN(rule->p.stpHdrFilter.dataRootPriHi, + rule->p.stpHdrFilter.dataRootPri); + COPY_NEG_SIGN(rule->p.stpHdrFilter.dataRootAddrMask, + rule->p.stpHdrFilter.dataRootAddr); + COPY_NEG_SIGN(rule->p.stpHdrFilter.dataRootCostHi, + rule->p.stpHdrFilter.dataRootCost); + COPY_NEG_SIGN(rule->p.stpHdrFilter.dataSndrPrioHi, + rule->p.stpHdrFilter.dataSndrPrio); + COPY_NEG_SIGN(rule->p.stpHdrFilter.dataSndrAddrMask, + rule->p.stpHdrFilter.dataSndrAddr); + COPY_NEG_SIGN(rule->p.stpHdrFilter.dataPortHi, + rule->p.stpHdrFilter.dataPort); + COPY_NEG_SIGN(rule->p.stpHdrFilter.dataAgeHi, + rule->p.stpHdrFilter.dataAge); + COPY_NEG_SIGN(rule->p.stpHdrFilter.dataMaxAgeHi, + rule->p.stpHdrFilter.dataMaxAge); + COPY_NEG_SIGN(rule->p.stpHdrFilter.dataHelloTimeHi, + rule->p.stpHdrFilter.dataHelloTime); + COPY_NEG_SIGN(rule->p.stpHdrFilter.dataFwdDelayHi, + rule->p.stpHdrFilter.dataFwdDelay); + break; + case VIR_NWFILTER_RULE_PROTOCOL_IP: COPY_NEG_SIGN(rule->p.ipHdrFilter.ipHdr.dataSrcIPMask, rule->p.ipHdrFilter.ipHdr.dataSrcIPAddr); @@ -2979,6 +3149,14 @@ virNWFilterRuleDefDetailsFormat(virBuffe item->u.u16); break; + case DATATYPE_UINT32_HEX: + asHex = true; + /* fallthrough */ + case DATATYPE_UINT32: + virBufferAsprintf(buf, asHex ? "0x%x" : "%d", + item->u.u32); + break; + case DATATYPE_IPADDR: case DATATYPE_IPV6ADDR: virNWIPAddressFormat(buf, Index: libvirt-acl/src/conf/nwfilter_conf.h =================================================================== --- libvirt-acl.orig/src/conf/nwfilter_conf.h +++ libvirt-acl/src/conf/nwfilter_conf.h @@ -101,10 +101,14 @@ enum attrDatatype { DATATYPE_IPV6MASK = (1 << 10), DATATYPE_STRINGCOPY = (1 << 11), DATATYPE_BOOLEAN = (1 << 12), + DATATYPE_UINT32 = (1 << 13), + DATATYPE_UINT32_HEX = (1 << 14), - DATATYPE_LAST = (1 << 13), + DATATYPE_LAST = (1 << 15), }; +# define NWFILTER_MAC_BGA "01:80:c2:00:00:00" + typedef struct _nwMACAddress nwMACAddress; typedef nwMACAddress *nwMACAddressPtr; @@ -125,6 +129,7 @@ struct _nwItemDesc { bool boolean; uint8_t u8; uint16_t u16; + uint32_t u32; char protocolID[10]; char *string; struct { @@ -164,6 +169,36 @@ struct _vlanHdrFilterDef { }; +typedef struct _stpHdrFilterDef stpHdrFilterDef; +typedef stpHdrFilterDef *stpHdrFilterDefPtr; +struct _stpHdrFilterDef { + ethHdrDataDef ethHdr; + nwItemDesc dataType; + nwItemDesc dataFlags; + nwItemDesc dataRootPri; + nwItemDesc dataRootPriHi; + nwItemDesc dataRootAddr; + nwItemDesc dataRootAddrMask; + nwItemDesc dataRootCost; + nwItemDesc dataRootCostHi; + nwItemDesc dataSndrPrio; + nwItemDesc dataSndrPrioHi; + nwItemDesc dataSndrAddr; + nwItemDesc dataSndrAddrMask; + nwItemDesc dataPort; + nwItemDesc dataPortHi; + nwItemDesc dataAge; + nwItemDesc dataAgeHi; + nwItemDesc dataMaxAge; + nwItemDesc dataMaxAgeHi; + nwItemDesc dataHelloTime; + nwItemDesc dataHelloTimeHi; + nwItemDesc dataFwdDelay; + nwItemDesc dataFwdDelayHi; + nwItemDesc dataComment; +}; + + typedef struct _arpHdrFilterDef arpHdrFilterDef; typedef arpHdrFilterDef *arpHdrFilterDefPtr; struct _arpHdrFilterDef { @@ -337,6 +372,7 @@ enum virNWFilterRuleProtocolType { VIR_NWFILTER_RULE_PROTOCOL_NONE = 0, VIR_NWFILTER_RULE_PROTOCOL_MAC, VIR_NWFILTER_RULE_PROTOCOL_VLAN, + VIR_NWFILTER_RULE_PROTOCOL_STP, VIR_NWFILTER_RULE_PROTOCOL_ARP, VIR_NWFILTER_RULE_PROTOCOL_RARP, VIR_NWFILTER_RULE_PROTOCOL_IP, @@ -378,6 +414,7 @@ enum virNWFilterEbtablesTableType { # define NWFILTER_MAX_FILTER_PRIORITY MAX_RULE_PRIORITY # define NWFILTER_ROOT_FILTER_PRI 0 +# define NWFILTER_STP_FILTER_PRI -810 # define NWFILTER_MAC_FILTER_PRI -800 # define NWFILTER_VLAN_FILTER_PRI -750 # define NWFILTER_IPV4_FILTER_PRI -700 @@ -418,6 +455,7 @@ struct _virNWFilterRuleDef { union { ethHdrFilterDef ethHdrFilter; vlanHdrFilterDef vlanHdrFilter; + stpHdrFilterDef stpHdrFilter; arpHdrFilterDef arpHdrFilter; /* also used for rarp */ ipHdrFilterDef ipHdrFilter; ipv6HdrFilterDef ipv6HdrFilter; @@ -459,6 +497,7 @@ enum virNWFilterChainSuffixType { VIR_NWFILTER_CHAINSUFFIX_ROOT = 0, VIR_NWFILTER_CHAINSUFFIX_MAC, VIR_NWFILTER_CHAINSUFFIX_VLAN, + VIR_NWFILTER_CHAINSUFFIX_STP, VIR_NWFILTER_CHAINSUFFIX_ARP, VIR_NWFILTER_CHAINSUFFIX_RARP, VIR_NWFILTER_CHAINSUFFIX_IPv4, Index: libvirt-acl/src/nwfilter/nwfilter_ebiptables_driver.c =================================================================== --- libvirt-acl.orig/src/nwfilter/nwfilter_ebiptables_driver.c +++ libvirt-acl/src/nwfilter/nwfilter_ebiptables_driver.c @@ -179,6 +179,7 @@ enum l3_proto_idx { L3_PROTO_RARP_IDX, L2_PROTO_MAC_IDX, L2_PROTO_VLAN_IDX, + L2_PROTO_STP_IDX, L3_PROTO_LAST_IDX }; @@ -190,6 +191,7 @@ static const struct ushort_map l3_protoc USHORTMAP_ENTRY_IDX(L3_PROTO_ARP_IDX , ETHERTYPE_ARP , "arp"), USHORTMAP_ENTRY_IDX(L3_PROTO_RARP_IDX, ETHERTYPE_REVARP, "rarp"), USHORTMAP_ENTRY_IDX(L2_PROTO_VLAN_IDX, ETHERTYPE_VLAN , "vlan"), + USHORTMAP_ENTRY_IDX(L2_PROTO_STP_IDX, 0 , "stp"), USHORTMAP_ENTRY_IDX(L2_PROTO_MAC_IDX, 0 , "mac"), USHORTMAP_ENTRY_IDX(L3_PROTO_LAST_IDX, 0 , NULL), }; @@ -290,6 +292,16 @@ _printDataType(virNWFilterVarCombIterPtr } break; + case DATATYPE_UINT32: + case DATATYPE_UINT32_HEX: + if (snprintf(buf, bufsize, asHex ? "0x%x" : "%d", + item->u.u32) >= bufsize) { + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Buffer too small for uint32 type")); + return 1; + } + break; + case DATATYPE_UINT16: case DATATYPE_UINT16_HEX: if (snprintf(buf, bufsize, asHex ? "0x%x" : "%d", @@ -2012,10 +2024,82 @@ ebtablesCreateRuleInstance(char chainPre number); \ } +#define INST_ITEM_2PARMS(STRUCT, ITEM, ITEM_HI, CLI, SEP) \ + if (HAS_ENTRY_ITEM(&rule->p.STRUCT.ITEM)) { \ + if (printDataType(vars, \ + number, sizeof(number), \ + &rule->p.STRUCT.ITEM)) \ + goto err_exit; \ + virBufferAsprintf(&buf, \ + " " CLI " %s %s", \ + ENTRY_GET_NEG_SIGN(&rule->p.STRUCT.ITEM), \ + number); \ + if (HAS_ENTRY_ITEM(&rule->p.STRUCT.ITEM_HI)) { \ + if (printDataType(vars, \ + number, sizeof(number), \ + &rule->p.STRUCT.ITEM_HI)) \ + goto err_exit; \ + virBufferAsprintf(&buf, SEP "%s", number); \ + } \ + } +#define INST_ITEM_RANGE(S, I, I_HI, C) \ + INST_ITEM_2PARMS(S, I, I_HI, C, ":") +#define INST_ITEM_MASK(S, I, MASK, C) \ + INST_ITEM_2PARMS(S, I, MASK, C, "/") + INST_ITEM(vlanHdrFilter, dataVlanID, "--vlan-id") INST_ITEM(vlanHdrFilter, dataVlanEncap, "--vlan-encap") break; + case VIR_NWFILTER_RULE_PROTOCOL_STP: + + /* cannot handle inout direction with srcmask set in reverse dir. + since this clashes with -d below... */ + if (reverse && + HAS_ENTRY_ITEM(&rule->p.stpHdrFilter.ethHdr.dataSrcMACAddr)) { + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, + _("STP filtering in %s direction with " + "source MAC address set is not supported"), + virNWFilterRuleDirectionTypeToString( + VIR_NWFILTER_RULE_DIRECTION_INOUT)); + return -1; + } + + virBufferAsprintf(&buf, + CMD_DEF_PRE "%s -t %s -%%c %s %%s", + ebtables_cmd_path, EBTABLES_DEFAULT_TABLE, chain); + + + if (ebtablesHandleEthHdr(&buf, + vars, + &rule->p.stpHdrFilter.ethHdr, + reverse)) + goto err_exit; + + virBufferAddLit(&buf, " -d " NWFILTER_MAC_BGA); + + INST_ITEM(stpHdrFilter, dataType, "--stp-type") + INST_ITEM(stpHdrFilter, dataFlags, "--stp-flags") + INST_ITEM_RANGE(stpHdrFilter, dataRootPri, dataRootPriHi, + "--stp-root-pri"); + INST_ITEM_MASK( stpHdrFilter, dataRootAddr, dataRootAddrMask, + "--stp-root-addr"); + INST_ITEM_RANGE(stpHdrFilter, dataRootCost, dataRootCostHi, + "--stp-root-cost"); + INST_ITEM_RANGE(stpHdrFilter, dataSndrPrio, dataSndrPrioHi, + "--stp-sender-prio"); + INST_ITEM_MASK( stpHdrFilter, dataSndrAddr, dataSndrAddrMask, + "--stp-sender-addr"); + INST_ITEM_RANGE(stpHdrFilter, dataPort, dataPortHi, "--stp-port"); + INST_ITEM_RANGE(stpHdrFilter, dataAge, dataAgeHi, "--stp-msg-age"); + INST_ITEM_RANGE(stpHdrFilter, dataMaxAge, dataMaxAgeHi, + "--stp-max-age"); + INST_ITEM_RANGE(stpHdrFilter, dataHelloTime, dataHelloTimeHi, + "--stp-hello-time"); + INST_ITEM_RANGE(stpHdrFilter, dataFwdDelay, dataFwdDelayHi, + "--stp-forward-delay"); + break; + case VIR_NWFILTER_RULE_PROTOCOL_ARP: case VIR_NWFILTER_RULE_PROTOCOL_RARP: @@ -2464,6 +2548,7 @@ ebiptablesCreateRuleInstance(virConnectP case VIR_NWFILTER_RULE_PROTOCOL_IP: case VIR_NWFILTER_RULE_PROTOCOL_MAC: case VIR_NWFILTER_RULE_PROTOCOL_VLAN: + case VIR_NWFILTER_RULE_PROTOCOL_STP: case VIR_NWFILTER_RULE_PROTOCOL_ARP: case VIR_NWFILTER_RULE_PROTOCOL_RARP: case VIR_NWFILTER_RULE_PROTOCOL_NONE: @@ -2907,7 +2992,7 @@ ebtablesCreateTmpSubChain(ebiptablesRule char rootchain[MAX_CHAINNAME_LENGTH], chain[MAX_CHAINNAME_LENGTH]; char chainPrefix = (incoming) ? CHAINPREFIX_HOST_IN_TEMP : CHAINPREFIX_HOST_OUT_TEMP; - char protostr[16] = { 0, }; + char protostr[32] = { 0, }; PRINT_ROOT_CHAIN(rootchain, chainPrefix, ifname); PRINT_CHAIN(chain, chainPrefix, ifname, @@ -2916,6 +3001,9 @@ ebtablesCreateTmpSubChain(ebiptablesRule switch (protoidx) { case L2_PROTO_MAC_IDX: break; + case L2_PROTO_STP_IDX: + snprintf(protostr, sizeof(protostr), "-d " NWFILTER_MAC_BGA); + break; default: snprintf(protostr, sizeof(protostr), "-p 0x%04x ", l3_protocols[protoidx].attr); Index: libvirt-acl/docs/schemas/nwfilter.rng =================================================================== --- libvirt-acl.orig/docs/schemas/nwfilter.rng +++ libvirt-acl/docs/schemas/nwfilter.rng @@ -40,6 +40,16 @@ </optional> <optional> <zeroOrMore> + <element name="stp"> + <ref name="match-attribute"/> + <ref name="srcmacandmask-attributes"/> + <ref name="stp-attributes"/> + <ref name="comment-attribute"/> + </element> + </zeroOrMore> + </optional> + <optional> + <zeroOrMore> <element name="arp"> <ref name="match-attribute"/> <ref name="common-l2-attributes"/> @@ -300,6 +310,9 @@ <param name="pattern">mac[a-zA-Z0-9_\.:-]*</param> </data> <data type="string"> + <param name="pattern">stp[a-zA-Z0-9_\.:-]*</param> + </data> + <data type="string"> <param name="pattern">vlan[a-zA-Z0-9_\.:-]*</param> </data> <data type="string"> @@ -384,7 +397,7 @@ </interleave> </define> - <define name="common-l2-attributes"> + <define name="srcmacandmask-attributes"> <interleave> <ref name="srcmac-attribute"/> <optional> @@ -392,6 +405,12 @@ <ref name="addrMAC"/> </attribute> </optional> + </interleave> + </define> + + <define name="common-l2-attributes"> + <interleave> + <ref name="srcmacandmask-attributes"/> <optional> <attribute name="dstmacaddr"> <ref name="addrMAC"/> @@ -590,6 +609,121 @@ </interleave> </define> + <define name="stp-attributes"> + <interleave> + <optional> + <attribute name="type"> + <ref name="uint8range"/> + </attribute> + </optional> + <optional> + <attribute name="flags"> + <ref name="uint8range"/> + </attribute> + </optional> + <optional> + <attribute name="root-priority"> + <ref name="uint16range"/> + </attribute> + </optional> + <optional> + <attribute name="root-priority-hi"> + <ref name="uint16range"/> + </attribute> + </optional> + <optional> + <attribute name="root-address"> + <ref name="addrMAC"/> + </attribute> + </optional> + <optional> + <attribute name="root-address-mask"> + <ref name="addrMAC"/> + </attribute> + </optional> + <optional> + <attribute name="root-cost"> + <ref name="uint32range"/> + </attribute> + </optional> + <optional> + <attribute name="root-cost-hi"> + <ref name="uint32range"/> + </attribute> + </optional> + <optional> + <attribute name="sender-priority"> + <ref name="uint16range"/> + </attribute> + </optional> + <optional> + <attribute name="sender-priority-hi"> + <ref name="uint16range"/> + </attribute> + </optional> + <optional> + <attribute name="sender-address"> + <ref name="addrMAC"/> + </attribute> + </optional> + <optional> + <attribute name="sender-address-mask"> + <ref name="addrMAC"/> + </attribute> + </optional> + <optional> + <attribute name="port"> + <ref name="uint16range"/> + </attribute> + </optional> + <optional> + <attribute name="port-hi"> + <ref name="uint16range"/> + </attribute> + </optional> + <optional> + <attribute name="age"> + <ref name="uint16range"/> + </attribute> + </optional> + <optional> + <attribute name="age-hi"> + <ref name="uint16range"/> + </attribute> + </optional> + <optional> + <attribute name="max-age"> + <ref name="uint16range"/> + </attribute> + </optional> + <optional> + <attribute name="max-age-hi"> + <ref name="uint16range"/> + </attribute> + </optional> + <optional> + <attribute name="hello-time"> + <ref name="uint16range"/> + </attribute> + </optional> + <optional> + <attribute name="hello-time-hi"> + <ref name="uint16range"/> + </attribute> + </optional> + <optional> + <attribute name="forward-delay"> + <ref name="uint16range"/> + </attribute> + </optional> + <optional> + <attribute name="forward-delay-hi"> + <ref name="uint16range"/> + </attribute> + </optional> + </interleave> + </define> + <define name="arp-attributes"> <interleave> <optional> @@ -854,6 +988,24 @@ </choice> </define> + <define name="uint32range"> + <choice> + <!-- variable --> + <data type="string"> + <param name="pattern">$[a-zA-Z0-9_]+</param> + </data> + + <data type="string"> + <param name="pattern">0x[0-9a-fA-F]{1,8}</param> + </data> + + <data type="int"> + <param name="minInclusive">0</param> + <param name="maxInclusive">4294967295</param> + </data> + </choice> + </define> + <define name="boolean"> <choice> <value>yes</value> Index: libvirt-acl/src/libvirt_private.syms =================================================================== --- libvirt-acl.orig/src/libvirt_private.syms +++ libvirt-acl/src/libvirt_private.syms @@ -874,6 +874,7 @@ virNWFilterPrintStateMatchFlags; virNWFilterPrintTCPFlags; virNWFilterRegisterCallbackDriver; virNWFilterRuleActionTypeToString; +virNWFilterRuleDirectionTypeToString; virNWFilterRuleProtocolTypeToString; virNWFilterTestUnassignDef; virNWFilterUnlockFilterUpdates;
On 10/26/2011 09:12 AM, Stefan Berger wrote:
This patch adds support for filtering of STP (spanning tree protocol) traffic to the parser and makes us of the ebtables support for STP filtering. This code now enables the filtering of traffic in chains with prefix 'stp'.
Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
--- docs/schemas/nwfilter.rng | 154 +++++++++++++++++++++++++ src/conf/nwfilter_conf.c | 178 ++++++++++++++++++++++++++++++ src/conf/nwfilter_conf.h | 41 ++++++ src/libvirt_private.syms | 1 src/nwfilter/nwfilter_ebiptables_driver.c | 90 +++++++++++++++ 5 files changed, 461 insertions(+), 3 deletions(-)
Some conflict resolution, again in the rng, and also in context for nwfilter_conf.c, but should be trivial. You already pre-emptively mentioned STP chains in an earlier patch, and I see the title of 7/9 mentions more about STP documentation, so I'll assume that between those two, the new .rng additions are properly documented.
@@ -1047,6 +1049,136 @@ static const virXMLAttr2Struct vlanAttri } };
+static const virXMLAttr2Struct stpAttributes[] = { + /* spanning tree uses a special destination MAC address */ + { + .name = SRCMACADDR, + .datatype = DATATYPE_MACADDR, + .dataIdx = offsetof(virNWFilterRuleDef, + p.stpHdrFilter.ethHdr.dataSrcMACAddr), + },
+ { + .name = "forward-delay-hi", + .datatype = DATATYPE_UINT16 | DATATYPE_UINT16_HEX, + .dataIdx = offsetof(virNWFilterRuleDef, p.stpHdrFilter.dataFwdDelayHi), + }, + COMMENT_PROP(stpHdrFilter),
I'm assuming this is an accurate layout mapping the on-the-wire struct to named fields for reference in XML attributes, although I didn't actually go hunt down an RFC to verify. Perhaps a comment pointing tot the STP RFC might prove handy.
@@ -2979,6 +3149,14 @@ virNWFilterRuleDefDetailsFormat(virBuffe item->u.u16); break;
+ case DATATYPE_UINT32_HEX: + asHex = true; + /* fallthrough */ + case DATATYPE_UINT32: + virBufferAsprintf(buf, asHex ? "0x%x" : "%d", + item->u.u32);
%u, not %d. Otherwise you introduce a spurious negative sign on values with the most-significant-bit set. Also, I'm not entirely sure whether %u and uint32_t always match, or if there are some 32-bit platforms where uint32_t is long and this would trigger a type mismatch warning from gcc. On the other hand, this code only compiles on Linux where we know uint32_t is always int; using <inttypes.h> for PRIu32 would be more portable, but that's a separate cleanup.
@@ -290,6 +292,16 @@ _printDataType(virNWFilterVarCombIterPtr } break;
+ case DATATYPE_UINT32: + case DATATYPE_UINT32_HEX: + if (snprintf(buf, bufsize, asHex ? "0x%x" : "%d", + item->u.u32) >= bufsize) { + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Buffer too small for uint32 type"));
Again, %u, not %d. Also, this code tends to be called with a hard-coded allocation of 'number[20];', which is sufficient for uint32_t, but not long enough if we ever expand to DATATYPE_UINT64. I'm wondering if we should use "intprops.h" from gnulib, for the INT_BUFSIZE_BOUND() macro, rather than a hard-coded 20. But at least this snprintf usage checked for error (I noticed in 4/9 that you used snprintf without error checking).
+ return 1;
Looks like this is code addition to an existing function with positive 1 return convention, so you can defer changing it to -1 until a later patch that cleans up the entire function (I'm only worried about completely new functions introduced by this patch).
+ case VIR_NWFILTER_RULE_PROTOCOL_STP: + + /* cannot handle inout direction with srcmask set in reverse dir. + since this clashes with -d below... */ + if (reverse && + HAS_ENTRY_ITEM(&rule->p.stpHdrFilter.ethHdr.dataSrcMACAddr)) { + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, + _("STP filtering in %s direction with " + "source MAC address set is not supported"), + virNWFilterRuleDirectionTypeToString( + VIR_NWFILTER_RULE_DIRECTION_INOUT)); + return -1; + } + + virBufferAsprintf(&buf, + CMD_DEF_PRE "%s -t %s -%%c %s %%s", + ebtables_cmd_path, EBTABLES_DEFAULT_TABLE, chain);
Looks like you've got some rebasing to do, depending on whether you push this or your env-var cleanup first.
@@ -2907,7 +2992,7 @@ ebtablesCreateTmpSubChain(ebiptablesRule char rootchain[MAX_CHAINNAME_LENGTH], chain[MAX_CHAINNAME_LENGTH]; char chainPrefix = (incoming) ? CHAINPREFIX_HOST_IN_TEMP : CHAINPREFIX_HOST_OUT_TEMP; - char protostr[16] = { 0, }; + char protostr[32] = { 0, };
PRINT_ROOT_CHAIN(rootchain, chainPrefix, ifname); PRINT_CHAIN(chain, chainPrefix, ifname, @@ -2916,6 +3001,9 @@ ebtablesCreateTmpSubChain(ebiptablesRule switch (protoidx) { case L2_PROTO_MAC_IDX: break; + case L2_PROTO_STP_IDX: + snprintf(protostr, sizeof(protostr), "-d " NWFILTER_MAC_BGA); + break;
Ah - here's an unchecked snprintf, which is probably worth tweaking for maintenance safety, especially since you just changed the size of protostr above.
@@ -590,6 +609,121 @@ </interleave> </define>
+ <define name="stp-attributes"> + <interleave> + <optional> + <attribute name="type"> + <ref name="uint8range"/> + </attribute> + </optional>
If I understand RNG correctly, <interleave> isn't required for attributes (only for sub-elements). We're starting to get enough comments and merge conflicts as I review this, that I think it will help if you post a v2 with all of the remaining patches from both this series and your $EBT patch rolled into a single commit series showing the final order you plan to push in. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 11/21/2011 05:50 PM, Eric Blake wrote:
On 10/26/2011 09:12 AM, Stefan Berger wrote:
This patch adds support for filtering of STP (spanning tree protocol) traffic to the parser and makes us of the ebtables support for STP filtering. This code now enables the filtering of traffic in chains with prefix 'stp'.
Signed-off-by: Stefan Berger<stefanb@linux.vnet.ibm.com>
--- docs/schemas/nwfilter.rng | 154 +++++++++++++++++++++++++ src/conf/nwfilter_conf.c | 178 ++++++++++++++++++++++++++++++ src/conf/nwfilter_conf.h | 41 ++++++ src/libvirt_private.syms | 1 src/nwfilter/nwfilter_ebiptables_driver.c | 90 +++++++++++++++ 5 files changed, 461 insertions(+), 3 deletions(-) Some conflict resolution, again in the rng, and also in context for nwfilter_conf.c, but should be trivial.
You already pre-emptively mentioned STP chains in an earlier patch, and I see the title of 7/9 mentions more about STP documentation, so I'll assume that between those two, the new .rng additions are properly documented.
@@ -1047,6 +1049,136 @@ static const virXMLAttr2Struct vlanAttri } };
+static const virXMLAttr2Struct stpAttributes[] = { + /* spanning tree uses a special destination MAC address */ + { + .name = SRCMACADDR, + .datatype = DATATYPE_MACADDR, + .dataIdx = offsetof(virNWFilterRuleDef, + p.stpHdrFilter.ethHdr.dataSrcMACAddr), + }, + { + .name = "forward-delay-hi", + .datatype = DATATYPE_UINT16 | DATATYPE_UINT16_HEX, + .dataIdx = offsetof(virNWFilterRuleDef, p.stpHdrFilter.dataFwdDelayHi), + }, + COMMENT_PROP(stpHdrFilter), I'm assuming this is an accurate layout mapping the on-the-wire struct to named fields for reference in XML attributes, although I didn't actually go hunt down an RFC to verify. Perhaps a comment pointing tot the STP RFC might prove handy.
I don't think it's an RFC, but an IEEE standard. I couldn't find a better page than this one, though: http://www.javvin.com/protocolSTP.html
@@ -2979,6 +3149,14 @@ virNWFilterRuleDefDetailsFormat(virBuffe item->u.u16); break;
+ case DATATYPE_UINT32_HEX: + asHex = true; + /* fallthrough */ + case DATATYPE_UINT32: + virBufferAsprintf(buf, asHex ? "0x%x" : "%d", + item->u.u32); %u, not %d. Otherwise you introduce a spurious negative sign on values with the most-significant-bit set. Fixed. Also, I'm not entirely sure whether %u and uint32_t always match, or if there are some 32-bit platforms where uint32_t is long and this would trigger a type mismatch warning from gcc. On the other hand, this code only compiles on Linux where we know uint32_t is always int; using <inttypes.h> for PRIu32 would be more portable, but that's a separate cleanup.
@@ -290,6 +292,16 @@ _printDataType(virNWFilterVarCombIterPtr } break;
+ case DATATYPE_UINT32: + case DATATYPE_UINT32_HEX: + if (snprintf(buf, bufsize, asHex ? "0x%x" : "%d", + item->u.u32)>= bufsize) { + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Buffer too small for uint32 type")); Again, %u, not %d.
Fixed.
Also, this code tends to be called with a hard-coded allocation of 'number[20];', which is sufficient for uint32_t, but not long enough if we ever expand to DATATYPE_UINT64. I'm wondering if we should use "intprops.h" from gnulib, for the INT_BUFSIZE_BOUND() macro, rather than a hard-coded 20. But at least this snprintf usage checked for error (I noticed in 4/9 that you used snprintf without error checking).
Using that now with all occurrences of number[].
+ return 1; Looks like this is code addition to an existing function with positive 1 return convention, so you can defer changing it to -1 until a later patch that cleans up the entire function (I'm only worried about completely new functions introduced by this patch).
+ case VIR_NWFILTER_RULE_PROTOCOL_STP: + + /* cannot handle inout direction with srcmask set in reverse dir. + since this clashes with -d below... */ + if (reverse&& + HAS_ENTRY_ITEM(&rule->p.stpHdrFilter.ethHdr.dataSrcMACAddr)) { + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, + _("STP filtering in %s direction with " + "source MAC address set is not supported"), + virNWFilterRuleDirectionTypeToString( + VIR_NWFILTER_RULE_DIRECTION_INOUT)); + return -1; + } + + virBufferAsprintf(&buf, + CMD_DEF_PRE "%s -t %s -%%c %s %%s", + ebtables_cmd_path, EBTABLES_DEFAULT_TABLE, chain);
Looks like you've got some rebasing to do, depending on whether you push this or your env-var cleanup first.
That shell var replacement stuff is scheduled for after this patch series...
@@ -2907,7 +2992,7 @@ ebtablesCreateTmpSubChain(ebiptablesRule char rootchain[MAX_CHAINNAME_LENGTH], chain[MAX_CHAINNAME_LENGTH]; char chainPrefix = (incoming) ? CHAINPREFIX_HOST_IN_TEMP : CHAINPREFIX_HOST_OUT_TEMP; - char protostr[16] = { 0, }; + char protostr[32] = { 0, };
PRINT_ROOT_CHAIN(rootchain, chainPrefix, ifname); PRINT_CHAIN(chain, chainPrefix, ifname, @@ -2916,6 +3001,9 @@ ebtablesCreateTmpSubChain(ebiptablesRule switch (protoidx) { case L2_PROTO_MAC_IDX: break; + case L2_PROTO_STP_IDX: + snprintf(protostr, sizeof(protostr), "-d " NWFILTER_MAC_BGA); + break; Ah - here's an unchecked snprintf, which is probably worth tweaking for maintenance safety, especially since you just changed the size of protostr above.
Fixed.
@@ -590,6 +609,121 @@ </interleave> </define>
+<define name="stp-attributes"> +<interleave> +<optional> +<attribute name="type"> +<ref name="uint8range"/> +</attribute> +</optional> If I understand RNG correctly,<interleave> isn't required for attributes (only for sub-elements). Fixed. I'll clean-up the rng at some point since I did this just about everywhere... We're starting to get enough comments and merge conflicts as I review this, that I think it will help if you post a v2 with all of the remaining patches from both this series and your $EBT patch rolled into a single commit series showing the final order you plan to push in.
On 11/22/2011 07:51 AM, Stefan Berger wrote:
On 11/21/2011 05:50 PM, Eric Blake wrote:
On 10/26/2011 09:12 AM, Stefan Berger wrote:
This patch adds support for filtering of STP (spanning tree protocol) traffic to the parser and makes us of the ebtables support for STP filtering. This code now enables the filtering of traffic in chains with prefix 'stp'.
I'm assuming this is an accurate layout mapping the on-the-wire struct to named fields for reference in XML attributes, although I didn't actually go hunt down an RFC to verify. Perhaps a comment pointing tot the STP RFC might prove handy.
I don't think it's an RFC, but an IEEE standard. I couldn't find a better page than this one, though:
Then how about this comment: /* STP is documented by IEEE 802.1D; for a synopsis, * see http://www.javvin.com/protocolSTP.html */ -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
This patch adds a few test cases for the XML parsing of STP filtering nodes. Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> --- tests/nwfilterxml2xmlin/stp-test.xml | 26 ++++++++++++++++++++++++++ tests/nwfilterxml2xmlout/stp-test.xml | 12 ++++++++++++ tests/nwfilterxml2xmltest.c | 1 + 3 files changed, 39 insertions(+) Index: libvirt-acl/tests/nwfilterxml2xmlin/stp-test.xml =================================================================== --- /dev/null +++ libvirt-acl/tests/nwfilterxml2xmlin/stp-test.xml @@ -0,0 +1,26 @@ +<filter name='testcase' chain='stp-xyz'> + <uuid>5c6d49af-b071-6127-b4ec-6f8ed4b55335</uuid> + <rule action='continue' direction='in'> + <stp srcmacaddr='1:2:3:4:5:6' srcmacmask='ff:ff:ff:ff:ff:ff' + type='0x12' flags='0x44'/> + </rule> + + <rule action='return' direction='out'> + <stp srcmacaddr='1:2:3:4:5:6' srcmacmask='ff:ff:ff:ff:ff:ff' + root-priority='0x1234' root-priority-hi='0x2345' + root-address="6:5:4:3:2:1" root-address-mask='ff:ff:ff:ff:ff:ff' + root-cost='0x11223344' root-cost-hi='0x22334455' /> + </rule> + + <rule action='reject' direction='in'> + <stp srcmacaddr='1:2:3:4:5:6' srcmacmask='ff:ff:ff:ff:ff:ff' + sender-priority='0x1234' + sender-address="6:5:4:3:2:1" + port='123' port-hi='234' + age='5544' age-hi='5555' + max-age='7777' max-age-hi='8888' + hello-time='12345' hello-time-hi='12346' + forward-delay='54321' forward-delay-hi='65432'/> + </rule> + +</filter> Index: libvirt-acl/tests/nwfilterxml2xmltest.c =================================================================== --- libvirt-acl.orig/tests/nwfilterxml2xmltest.c +++ libvirt-acl/tests/nwfilterxml2xmltest.c @@ -109,6 +109,7 @@ mymain(void) DO_TEST("mac-test", true); DO_TEST("vlan-test", true); + DO_TEST("stp-test", false); DO_TEST("arp-test", true); DO_TEST("rarp-test", true); DO_TEST("ip-test", true); Index: libvirt-acl/tests/nwfilterxml2xmlout/stp-test.xml =================================================================== --- /dev/null +++ libvirt-acl/tests/nwfilterxml2xmlout/stp-test.xml @@ -0,0 +1,12 @@ +<filter name='testcase' chain='stp-xyz'> + <uuid>5c6d49af-b071-6127-b4ec-6f8ed4b55335</uuid> + <rule action='continue' direction='in' priority='500'> + <stp srcmacaddr='01:02:03:04:05:06' srcmacmask='ff:ff:ff:ff:ff:ff' type='0x12' flags='0x44'/> + </rule> + <rule action='return' direction='out' priority='500'> + <stp srcmacaddr='01:02:03:04:05:06' srcmacmask='ff:ff:ff:ff:ff:ff' root-priority='0x1234' root-priority-hi='0x2345' root-address='06:05:04:03:02:01' root-address-mask='ff:ff:ff:ff:ff:ff' root-cost='0x11223344' root-cost-hi='0x22334455'/> + </rule> + <rule action='reject' direction='in' priority='500'> + <stp srcmacaddr='01:02:03:04:05:06' srcmacmask='ff:ff:ff:ff:ff:ff' sender-priority='0x1234' sender-address='06:05:04:03:02:01' port='123' port-hi='234' age='5544' age-hi='5555' max-age='7777' max-age-hi='8888' hello-time='12345' hello-time-hi='12346' forward-delay='54321' forward-delay-hi='65432'/> + </rule> +</filter>
On 10/26/2011 09:12 AM, Stefan Berger wrote:
This patch adds a few test cases for the XML parsing of STP filtering nodes.
Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
--- tests/nwfilterxml2xmlin/stp-test.xml | 26 ++++++++++++++++++++++++++ tests/nwfilterxml2xmlout/stp-test.xml | 12 ++++++++++++ tests/nwfilterxml2xmltest.c | 1 + 3 files changed, 39 insertions(+)
Looks good. ACK.
+ <rule action='reject' direction='in'> + <stp srcmacaddr='1:2:3:4:5:6' srcmacmask='ff:ff:ff:ff:ff:ff' + sender-priority='0x1234' + sender-address="6:5:4:3:2:1" + port='123' port-hi='234' + age='5544' age-hi='5555' + max-age='7777' max-age-hi='8888' + hello-time='12345' hello-time-hi='12346' + forward-delay='54321' forward-delay-hi='65432'/>
That's a lot of attributes for one element, thankfully we can split it over multiple lines. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Add documentation for the STP filtering support. Describe the XML attributes that are supported. Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> --- docs/formatnwfilter.html.in | 142 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 142 insertions(+) Index: libvirt-acl/docs/formatnwfilter.html.in =================================================================== --- libvirt-acl.orig/docs/formatnwfilter.html.in +++ libvirt-acl/docs/formatnwfilter.html.in @@ -467,6 +467,148 @@ Valid Strings for <code>encap-protocol</code> are: arp, ipv4, ipv6 </p> + <h5><a name="nwfelemsRulesProtoSTP">STP (Spanning Tree Protocol)</a> + <span class="since">(Since 0.9.7)</span> + </h5> + <p> + Protocol ID: <code>stp</code> + <br/> + Note: Rules of this type should go eitherinto the <code>root</code> or + <code>stp</code> chain. + </p> + <table class="top_table"> + <tr> + <th> Attribute </th> + <th> Datatype </th> + <th> Semantics </th> + </tr> + <tr> + <td>srcmacaddr</td> + <td>MAC_ADDR</td> + <td>MAC address of sender</td> + </tr> + <tr> + <td>srcmacmask</td> + <td>MAC_MASK</td> + <td>Mask applied to MAC address of sender</td> + </tr> + <tr> + <td>type</td> + <td>UINT8</td> + <td>Bridge Protcol Data Unit (BPDU) type</td> + </tr> + <tr> + <td>flags</td> + <td>UINT8</td> + <td>BPDU flag</td> + </tr> + <tr> + <td>root-priority</td> + <td>UINT16</td> + <td>Root priority (range start)</td> + </tr> + <tr> + <td>root-priority-hi</td> + <td>UINT16</td> + <td>Root priority range end</td> + </tr> + <tr> + <td>root-address</td> + <td>MAC_ADDRESS</td> + <td>Root MAC address</td> + </tr> + <tr> + <td>root-address-mask</td> + <td>MAC_MASK</td> + <td>Root MAC address mask</td> + </tr> + <tr> + <td>root-cost</td> + <td>UINT32</td> + <td>Root path cost (range start)</td> + </tr> + <tr> + <td>root-cost-hi</td> + <td>UINT32</td> + <td>Root path cost range end</td> + </tr> + <tr> + <td>sender-priority</td> + <td>UINT16</td> + <td>Sender priority (range start)</td> + </tr> + <tr> + <td>sender-priority-hi</td> + <td>UINT16</td> + <td>Sender priority range end</td> + </tr> + <tr> + <td>sender-address</td> + <td>MAC_ADDRESS</td> + <td>BPDU sender MAC address</td> + </tr> + <tr> + <td>sender-address-mask</td> + <td>MAC_MASK</td> + <td>BPDU sender MAC address mask</td> + </tr> + <tr> + <td>port</td> + <td>UINT16</td> + <td>Port identifier (range start)</td> + </tr> + <tr> + <td>port_hi</td> + <td>UINT16</td> + <td>Port identifier range end</td> + </tr> + <tr> + <td>msg-age</td> + <td>UINT16</td> + <td>Message age timer (range start)</td> + </tr> + <tr> + <td>msg-age-hi</td> + <td>UINT16</td> + <td>Message age timer range end</td> + </tr> + <tr> + <td>max-age</td> + <td>UINT16</td> + <td>Maximum age timer (range start)</td> + </tr> + <tr> + <td>max-age-hi</td> + <td>UINT16</td> + <td>Maximum age timer range end</td> + </tr> + <tr> + <td>hello-time</td> + <td>UINT16</td> + <td>Hello time timer (range start)</td> + </tr> + <tr> + <td>hello-time-hi</td> + <td>UINT16</td> + <td>Hello time timer range end</td> + </tr> + <tr> + <td>forward-delay</td> + <td>UINT16</td> + <td>Forward delay (range start)</td> + </tr> + <tr> + <td>forward-delay-hi</td> + <td>UINT16</td> + <td>Forward delay range end</td> + </tr> + <tr> + <td>comment</td> + <td>STRING</td> + <td>text with max. 256 characters</td> + </tr> + </table> + <h5><a name="nwfelemsRulesProtoARP">ARP/RARP</a></h5> <p> Protocol ID: <code>arp</code> or <code>rarp</code>
On 10/26/2011 09:12 AM, Stefan Berger wrote:
Add documentation for the STP filtering support. Describe the XML attributes that are supported.
Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
--- docs/formatnwfilter.html.in | 142 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 142 insertions(+)
Index: libvirt-acl/docs/formatnwfilter.html.in =================================================================== --- libvirt-acl.orig/docs/formatnwfilter.html.in +++ libvirt-acl/docs/formatnwfilter.html.in @@ -467,6 +467,148 @@ Valid Strings for <code>encap-protocol</code> are: arp, ipv4, ipv6 </p>
+ <h5><a name="nwfelemsRulesProtoSTP">STP (Spanning Tree Protocol)</a> + <span class="since">(Since 0.9.7)</span>
0.9.8, now.
+ </h5> + <p> + Protocol ID: <code>stp</code> + <br/> + Note: Rules of this type should go eitherinto the <code>root</code> or
s/eitherinto/either into/ ACK with those nits fixed. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
With new filters borrowed from David Steven's submission: A set of new filters to handle multiple IP addresses and MAC addresses per interface. The alternative would be to replace some of the existing ones with these here. Signed-off-by: David L Stevens <dlstevens@us.ibm.com> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> --- examples/xml/nwfilter/Makefile.am | 6 +++++ examples/xml/nwfilter/clean-traffic-new.xml | 29 ++++++++++++++++++++++++++ examples/xml/nwfilter/no-arp-spoofing-new.xml | 4 +++ examples/xml/nwfilter/no-arpip-spoofing.xml | 12 ++++++++++ examples/xml/nwfilter/no-arpmac-spoofing.xml | 8 +++++++ examples/xml/nwfilter/no-ip-spoofing-new.xml | 5 ++++ examples/xml/nwfilter/no-mac-spoofing-new.xml | 10 ++++++++ 7 files changed, 74 insertions(+) Index: libvirt-acl/examples/xml/nwfilter/Makefile.am =================================================================== --- libvirt-acl.orig/examples/xml/nwfilter/Makefile.am +++ libvirt-acl/examples/xml/nwfilter/Makefile.am @@ -8,11 +8,17 @@ FILTERS = \ allow-incoming-ipv4.xml \ allow-ipv4.xml \ clean-traffic.xml \ + clean-traffic-new.xml \ no-arp-spoofing.xml \ + no-arp-spoofing-new.xml \ + no-arpip-spoofing.xml \ + no-arpmac-spoofing.xml \ no-ip-multicast.xml \ no-ip-spoofing.xml \ + no-ip-spoofing-new.xml \ no-mac-broadcast.xml \ no-mac-spoofing.xml \ + no-mac-spoofing-new.xml \ no-other-l2-traffic.xml \ no-other-rarp-traffic.xml \ qemu-announce-self.xml \ Index: libvirt-acl/examples/xml/nwfilter/no-arpip-spoofing.xml =================================================================== --- /dev/null +++ libvirt-acl/examples/xml/nwfilter/no-arpip-spoofing.xml @@ -0,0 +1,12 @@ +<filter name='no-arpip-spoofing' chain='arpip' priority='-510'> + <!-- no arp spoofing --> + <!-- drop if ipaddr does not belong to guest --> + <rule action='return' direction='out' priority='400' > + <arp match='yes' arpsrcipaddr='$IP' /> + </rule> + <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> Index: libvirt-acl/examples/xml/nwfilter/no-arpmac-spoofing.xml =================================================================== --- /dev/null +++ libvirt-acl/examples/xml/nwfilter/no-arpmac-spoofing.xml @@ -0,0 +1,8 @@ +<filter name='no-arpmac-spoofing' chain='arpmac' priority='-510'> + <rule action='return' direction='out' priority='350' > + <arp match='yes' arpsrcmacaddr='$MAC'/> + </rule> + <!-- drop everything else --> + <rule action='drop' direction='out' priority='1000' /> +</filter> + Index: libvirt-acl/examples/xml/nwfilter/clean-traffic-new.xml =================================================================== --- /dev/null +++ libvirt-acl/examples/xml/nwfilter/clean-traffic-new.xml @@ -0,0 +1,29 @@ +<filter name='clean-traffic-new' chain='root'> + <!-- An example of a traffic filter enforcing clean traffic + from a VM by + - preventing MAC spoofing --> + <filterref filter='no-mac-spoofing-new'/> + + <!-- preventing IP spoofing on outgoing, allow all IPv4 in incoming --> + <filterref filter='no-ip-spoofing-new'/> + + <rule direction='out' action='accept' priority='-750'> + <mac protocolid='ipv4'/> + </rule> + + <filterref filter='allow-incoming-ipv4'/> + + <!-- preventing ARP spoofing/poisoning --> + <filterref filter='no-arp-spoofing-new'/> + + <rule direction='out' action='accept' priority='-550'> + <mac protocolid='arp'/> + </rule> + + <!-- preventing any other traffic than IPv4 and ARP --> + <filterref filter='no-other-l2-traffic'/> + + <!-- allow qemu to send a self-announce upon migration end --> + <filterref filter='qemu-announce-self'/> + +</filter> Index: libvirt-acl/examples/xml/nwfilter/no-arp-spoofing-new.xml =================================================================== --- /dev/null +++ libvirt-acl/examples/xml/nwfilter/no-arp-spoofing-new.xml @@ -0,0 +1,4 @@ +<filter name='no-arp-spoofing-new'> + <filterref filter='no-arpmac-spoofing'/> + <filterref filter='no-arpip-spoofing'/> +</filter> Index: libvirt-acl/examples/xml/nwfilter/no-mac-spoofing-new.xml =================================================================== --- /dev/null +++ libvirt-acl/examples/xml/nwfilter/no-mac-spoofing-new.xml @@ -0,0 +1,10 @@ +<filter name='no-mac-spoofing-new' chain='mac' priority='-800'> + <!-- return packets with VM's MAC address as source address --> + <rule direction='out' action='return'> + <mac srcmacaddr='$MAC'/> + </rule> + <!-- drop everything else --> + <rule direction='out' action='drop'> + <mac/> + </rule> +</filter> Index: libvirt-acl/examples/xml/nwfilter/no-ip-spoofing-new.xml =================================================================== --- /dev/null +++ libvirt-acl/examples/xml/nwfilter/no-ip-spoofing-new.xml @@ -0,0 +1,5 @@ +<filter name='no-ip-spoofing-new' chain='ipv4-ip' priority='-710'> + <rule direction='out' action='return'> + <ip match='yes' srcipaddr='$IP'/> + </rule> +</filter>
On 10/26/2011 09:12 AM, Stefan Berger wrote:
With new filters borrowed from David Steven's submission: A set of new filters to handle multiple IP addresses and MAC addresses per interface. The alternative would be to replace some of the existing ones with these here.
On IRC, you mentioned that you were dropping this, so I haven't bothered to review it. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Display the executed command and failure message if a command failed to execute. Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com> --- src/nwfilter/nwfilter_ebiptables_driver.c | 82 ++++++++++++++++++------------ 1 file changed, 50 insertions(+), 32 deletions(-) Index: libvirt-acl/src/nwfilter/nwfilter_ebiptables_driver.c =================================================================== --- libvirt-acl.orig/src/nwfilter/nwfilter_ebiptables_driver.c +++ libvirt-acl/src/nwfilter/nwfilter_ebiptables_driver.c @@ -63,10 +63,10 @@ #define CMD_DEF_PRE "cmd='" #define CMD_DEF_POST "'" #define CMD_DEF(X) CMD_DEF_PRE X CMD_DEF_POST -#define CMD_EXEC "eval res=\\$\\(\"${cmd}\"\\)" CMD_SEPARATOR +#define CMD_EXEC "eval res=\\$\\(\"${cmd} 2>&1 \"\\)" CMD_SEPARATOR #define CMD_STOPONERR(X) \ X ? "if [ $? -ne 0 ]; then" \ - " echo \"Failure to execute command '${cmd}'.\";" \ + " echo \"Failure to execute command '${cmd}' : '${res}'.\";" \ " exit 1;" \ "fi" CMD_SEPARATOR \ : "" @@ -2785,12 +2785,16 @@ err_exit: */ static int ebiptablesExecCLI(virBufferPtr buf, - int *status) + int *status, char **errbuf) { char *cmds; char *filename; int rc; const char *argv[] = {NULL, NULL}; + virCommandPtr cmd; + + if (errbuf) + VIR_FREE(*errbuf); if (virBufferError(buf)) { virReportOOMError(); @@ -2817,7 +2821,13 @@ ebiptablesExecCLI(virBufferPtr buf, virMutexLock(&execCLIMutex); - rc = virRun(argv, status); + cmd = virCommandNewArgs(argv); + if (errbuf) + virCommandSetOutputBuffer(cmd, errbuf); + + rc = virCommandRun(cmd, status); + + virCommandFree(cmd); virMutexUnlock(&execCLIMutex); @@ -2831,6 +2841,8 @@ ebiptablesExecCLI(virBufferPtr buf, } VIR_DEBUG("rc = %d, status = %d", rc, *status); + if (errbuf && *errbuf) + VIR_DEBUG("error message: %s", *errbuf); unlink(filename); @@ -3285,7 +3297,7 @@ ebtablesApplyBasicRules(const char *ifna ebtablesLinkTmpRootChain(&buf, 1, ifname, 1); ebtablesRenameTmpRootChain(&buf, 1, ifname); - if (ebiptablesExecCLI(&buf, &cli_status) || cli_status != 0) + if (ebiptablesExecCLI(&buf, &cli_status, NULL) || cli_status != 0) goto tear_down_tmpebchains; return 0; @@ -3401,7 +3413,7 @@ ebtablesApplyDHCPOnlyRules(const char *i ebtablesRenameTmpRootChain(&buf, 1, ifname); ebtablesRenameTmpRootChain(&buf, 0, ifname); - if (ebiptablesExecCLI(&buf, &cli_status) || cli_status != 0) + if (ebiptablesExecCLI(&buf, &cli_status, NULL) || cli_status != 0) goto tear_down_tmpebchains; VIR_FREE(srcIPParam); @@ -3474,7 +3486,7 @@ ebtablesApplyDropAllRules(const char *if ebtablesRenameTmpRootChain(&buf, 1, ifname); ebtablesRenameTmpRootChain(&buf, 0, ifname); - if (ebiptablesExecCLI(&buf, &cli_status) || cli_status != 0) + if (ebiptablesExecCLI(&buf, &cli_status, NULL) || cli_status != 0) goto tear_down_tmpebchains; return 0; @@ -3517,7 +3529,7 @@ static int ebtablesCleanAll(const char * ebtablesRemoveTmpRootChain(&buf, 1, ifname); ebtablesRemoveTmpRootChain(&buf, 0, ifname); - ebiptablesExecCLI(&buf, &cli_status); + ebiptablesExecCLI(&buf, &cli_status, NULL); return 0; } @@ -3677,6 +3689,7 @@ ebiptablesApplyNewRules(virConnectPtr co bool haveIp6tables = false; ebiptablesRuleInstPtr ebtChains = NULL; int nEbtChains = 0; + char *errmsg = NULL; if (!chains_in_set || !chains_out_set) { virReportOOMError(); @@ -3715,7 +3728,7 @@ ebiptablesApplyNewRules(virConnectPtr co ebtablesRemoveTmpSubChains(&buf, ifname); ebtablesRemoveTmpRootChain(&buf, 1, ifname); ebtablesRemoveTmpRootChain(&buf, 0, ifname); - ebiptablesExecCLI(&buf, &cli_status); + ebiptablesExecCLI(&buf, &cli_status, NULL); } /* create needed chains */ @@ -3730,7 +3743,7 @@ ebiptablesApplyNewRules(virConnectPtr co qsort(&ebtChains[0], nEbtChains, sizeof(ebtChains[0]), ebiptablesRuleOrderSort); - if (ebiptablesExecCLI(&buf, &cli_status) || cli_status != 0) + if (ebiptablesExecCLI(&buf, &cli_status, &errmsg) || cli_status != 0) goto tear_down_tmpebchains; /* process ebtables commands; interleave commands from filters with @@ -3764,7 +3777,7 @@ ebiptablesApplyNewRules(virConnectPtr co ebtChains[j++].commandTemplate, 'A', -1, 1); - if (ebiptablesExecCLI(&buf, &cli_status) || cli_status != 0) + if (ebiptablesExecCLI(&buf, &cli_status, &errmsg) || cli_status != 0) goto tear_down_tmpebchains; if (haveIptables) { @@ -3773,17 +3786,17 @@ ebiptablesApplyNewRules(virConnectPtr co iptablesCreateBaseChains(iptables_cmd_path, &buf); - if (ebiptablesExecCLI(&buf, &cli_status) || cli_status != 0) + if (ebiptablesExecCLI(&buf, &cli_status, &errmsg) || cli_status != 0) goto tear_down_tmpebchains; iptablesCreateTmpRootChains(iptables_cmd_path, &buf, ifname); - if (ebiptablesExecCLI(&buf, &cli_status) || cli_status != 0) + if (ebiptablesExecCLI(&buf, &cli_status, &errmsg) || cli_status != 0) goto tear_down_tmpiptchains; iptablesLinkTmpRootChains(iptables_cmd_path, &buf, ifname); iptablesSetupVirtInPost(iptables_cmd_path, &buf, ifname); - if (ebiptablesExecCLI(&buf, &cli_status) || cli_status != 0) + if (ebiptablesExecCLI(&buf, &cli_status, &errmsg) || cli_status != 0) goto tear_down_tmpiptchains; for (i = 0; i < nruleInstances; i++) { @@ -3794,7 +3807,7 @@ ebiptablesApplyNewRules(virConnectPtr co 'A', -1, 1); } - if (ebiptablesExecCLI(&buf, &cli_status) || cli_status != 0) + if (ebiptablesExecCLI(&buf, &cli_status, &errmsg) || cli_status != 0) goto tear_down_tmpiptchains; iptablesCheckBridgeNFCallEnabled(false); @@ -3806,17 +3819,17 @@ ebiptablesApplyNewRules(virConnectPtr co iptablesCreateBaseChains(ip6tables_cmd_path, &buf); - if (ebiptablesExecCLI(&buf, &cli_status) || cli_status != 0) + if (ebiptablesExecCLI(&buf, &cli_status, &errmsg) || cli_status != 0) goto tear_down_tmpiptchains; iptablesCreateTmpRootChains(ip6tables_cmd_path, &buf, ifname); - if (ebiptablesExecCLI(&buf, &cli_status) || cli_status != 0) + if (ebiptablesExecCLI(&buf, &cli_status, &errmsg) || cli_status != 0) goto tear_down_tmpip6tchains; iptablesLinkTmpRootChains(ip6tables_cmd_path, &buf, ifname); iptablesSetupVirtInPost(ip6tables_cmd_path, &buf, ifname); - if (ebiptablesExecCLI(&buf, &cli_status) || cli_status != 0) + if (ebiptablesExecCLI(&buf, &cli_status, &errmsg) || cli_status != 0) goto tear_down_tmpip6tchains; for (i = 0; i < nruleInstances; i++) { @@ -3826,7 +3839,7 @@ ebiptablesApplyNewRules(virConnectPtr co 'A', -1, 1); } - if (ebiptablesExecCLI(&buf, &cli_status) || cli_status != 0) + if (ebiptablesExecCLI(&buf, &cli_status, &errmsg) || cli_status != 0) goto tear_down_tmpip6tchains; iptablesCheckBridgeNFCallEnabled(true); @@ -3837,7 +3850,7 @@ ebiptablesApplyNewRules(virConnectPtr co if (virHashSize(chains_out_set) != 0) ebtablesLinkTmpRootChain(&buf, 0, ifname, 1); - if (ebiptablesExecCLI(&buf, &cli_status) || cli_status != 0) + if (ebiptablesExecCLI(&buf, &cli_status, &errmsg) || cli_status != 0) goto tear_down_ebsubchains_and_unlink; virHashFree(chains_in_set); @@ -3847,6 +3860,8 @@ ebiptablesApplyNewRules(virConnectPtr co VIR_FREE(ebtChains[i].commandTemplate); VIR_FREE(ebtChains); + VIR_FREE(errmsg); + return 0; tear_down_ebsubchains_and_unlink: @@ -3874,12 +3889,13 @@ tear_down_tmpebchains: ebtablesRemoveTmpRootChain(&buf, 0, ifname); } - ebiptablesExecCLI(&buf, &cli_status); + ebiptablesExecCLI(&buf, &cli_status, NULL); virNWFilterReportError(VIR_ERR_BUILD_FIREWALL, _("Some rules could not be created for " - "interface %s."), - ifname); + "interface %s : %s"), + ifname, + errmsg ? errmsg : ""); exit_free_sets: virHashFree(chains_in_set); @@ -3889,6 +3905,8 @@ exit_free_sets: VIR_FREE(ebtChains[i].commandTemplate); VIR_FREE(ebtChains); + VIR_FREE(errmsg); + return 1; } @@ -3919,7 +3937,7 @@ ebiptablesTearNewRules(virConnectPtr con ebtablesRemoveTmpRootChain(&buf, 0, ifname); } - ebiptablesExecCLI(&buf, &cli_status); + ebiptablesExecCLI(&buf, &cli_status, NULL); return 0; } @@ -3938,7 +3956,7 @@ ebiptablesTearOldRules(virConnectPtr con iptablesRemoveRootChains(iptables_cmd_path, &buf, ifname); iptablesRenameTmpRootChains(iptables_cmd_path, &buf, ifname); - ebiptablesExecCLI(&buf, &cli_status); + ebiptablesExecCLI(&buf, &cli_status, NULL); } if (ip6tables_cmd_path) { @@ -3946,7 +3964,7 @@ ebiptablesTearOldRules(virConnectPtr con iptablesRemoveRootChains(ip6tables_cmd_path, &buf, ifname); iptablesRenameTmpRootChains(ip6tables_cmd_path, &buf, ifname); - ebiptablesExecCLI(&buf, &cli_status); + ebiptablesExecCLI(&buf, &cli_status, NULL); } if (ebtables_cmd_path) { @@ -3960,7 +3978,7 @@ ebiptablesTearOldRules(virConnectPtr con ebtablesRenameTmpSubAndRootChains(&buf, ifname); - ebiptablesExecCLI(&buf, &cli_status); + ebiptablesExecCLI(&buf, &cli_status, NULL); } return 0; @@ -3997,7 +4015,7 @@ ebiptablesRemoveRules(virConnectPtr conn 'D', -1, 0); - if (ebiptablesExecCLI(&buf, &cli_status)) + if (ebiptablesExecCLI(&buf, &cli_status, NULL)) goto err_exit; if (cli_status) { @@ -4048,7 +4066,7 @@ ebiptablesAllTeardown(const char *ifname ebtablesRemoveRootChain(&buf, 1, ifname); ebtablesRemoveRootChain(&buf, 0, ifname); } - ebiptablesExecCLI(&buf, &cli_status); + ebiptablesExecCLI(&buf, &cli_status, NULL); return 0; } @@ -4103,7 +4121,7 @@ ebiptablesDriverInit(bool privileged) ebtables_cmd_path, EBTABLES_DEFAULT_TABLE, CMD_STOPONERR(1)); - if (ebiptablesExecCLI(&buf, &cli_status) || cli_status) + if (ebiptablesExecCLI(&buf, &cli_status, NULL) || cli_status) VIR_FREE(ebtables_cmd_path); } @@ -4116,7 +4134,7 @@ ebiptablesDriverInit(bool privileged) iptables_cmd_path, CMD_STOPONERR(1)); - if (ebiptablesExecCLI(&buf, &cli_status) || cli_status) + if (ebiptablesExecCLI(&buf, &cli_status, NULL) || cli_status) VIR_FREE(iptables_cmd_path); } @@ -4129,7 +4147,7 @@ ebiptablesDriverInit(bool privileged) ip6tables_cmd_path, CMD_STOPONERR(1)); - if (ebiptablesExecCLI(&buf, &cli_status) || cli_status) + if (ebiptablesExecCLI(&buf, &cli_status, NULL) || cli_status) VIR_FREE(ip6tables_cmd_path); }
On 10/26/2011 09:12 AM, Stefan Berger wrote:
Display the executed command and failure message if a command failed to execute.
Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
--- src/nwfilter/nwfilter_ebiptables_driver.c | 82 ++++++++++++++++++------------ 1 file changed, 50 insertions(+), 32 deletions(-)
Index: libvirt-acl/src/nwfilter/nwfilter_ebiptables_driver.c =================================================================== --- libvirt-acl.orig/src/nwfilter/nwfilter_ebiptables_driver.c +++ libvirt-acl/src/nwfilter/nwfilter_ebiptables_driver.c @@ -63,10 +63,10 @@ #define CMD_DEF_PRE "cmd='" #define CMD_DEF_POST "'" #define CMD_DEF(X) CMD_DEF_PRE X CMD_DEF_POST -#define CMD_EXEC "eval res=\\$\\(\"${cmd}\"\\)" CMD_SEPARATOR +#define CMD_EXEC "eval res=\\$\\(\"${cmd} 2>&1 \"\\)" CMD_SEPARATOR
Okay, after turning the C literal into shell, you have: eval res=\$\("${cmd} 2>&1 "\) and after the shell eval, you have: res=$(command 2>&1 ) That space after '2>&1' could be deleted, but it doesn't hurt to leave it in.
#define CMD_STOPONERR(X) \ X ? "if [ $? -ne 0 ]; then" \ - " echo \"Failure to execute command '${cmd}'.\";" \ + " echo \"Failure to execute command '${cmd}' : '${res}'.\";" \
Yes, this is a bit more informative.
" exit 1;" \ "fi" CMD_SEPARATOR \ : "" @@ -2785,12 +2785,16 @@ err_exit: */ static int ebiptablesExecCLI(virBufferPtr buf, - int *status) + int *status, char **errbuf) { char *cmds; char *filename; int rc; const char *argv[] = {NULL, NULL}; + virCommandPtr cmd; + + if (errbuf) + VIR_FREE(*errbuf);
This has merge conflicts with existing patches that went in during the meantime. I'd feel better seeing a v2 to verify proper rebasing.
if (virBufferError(buf)) { virReportOOMError(); @@ -2817,7 +2821,13 @@ ebiptablesExecCLI(virBufferPtr buf,
virMutexLock(&execCLIMutex);
- rc = virRun(argv, status); + cmd = virCommandNewArgs(argv); + if (errbuf) + virCommandSetOutputBuffer(cmd, errbuf);
It looks a bit odd calling it errbuf, when it is the output collected from stdout; perhaps calling it outbuf would make more sense.
@@ -3285,7 +3297,7 @@ ebtablesApplyBasicRules(const char *ifna ebtablesLinkTmpRootChain(&buf, 1, ifname, 1); ebtablesRenameTmpRootChain(&buf, 1, ifname);
- if (ebiptablesExecCLI(&buf, &cli_status) || cli_status != 0) + if (ebiptablesExecCLI(&buf, &cli_status, NULL) || cli_status != 0)
You know, as long as we are cleaning things up, you could pass NULL instead of &cli_status to enforce that the command has a 0 exit status, so that you trim lines like this to: if (ebiptablesExecCLI(&buf, NULL, NULL) < 0)
@@ -3874,12 +3889,13 @@ tear_down_tmpebchains: ebtablesRemoveTmpRootChain(&buf, 0, ifname); }
- ebiptablesExecCLI(&buf, &cli_status); + ebiptablesExecCLI(&buf, &cli_status, NULL);
virNWFilterReportError(VIR_ERR_BUILD_FIREWALL, _("Some rules could not be created for " - "interface %s."), - ifname); + "interface %s : %s"), + ifname, + errmsg ? errmsg : "");
That outputs a trailing space if there was no error message. Better might be: virNWFilterReportError(VIR_ERR_BUILD_FIREWALL, _("Some rules could not be created for " "interface %s%s%s"), ifname, errmsg ? ": " : "", errmsg ? errmsg : ""); Alas, we have an i18n nightmare. errmsg contains English text output by the shell script, which is not marked for translation by either the shell script (which itself is tricky - witness the libvirt-guests init script) nor for translation in the C code that generated the shell script. Meanwhile, since we didn't call virCommandAddEnvPassCommon() to set the virCommand to force LC_ALL=C, that means we passed the libvirtd setting of LC_MESSAGES on through to the child processes, such that sh and ebtables output might also be translated. For an example, that means someone using LC_MESSAGES=es_ES.UTF-8 could see: Algunas reglas no han podido crearse para la interfaz eth0 : Failure to execute command '/path/to/ebtables -t nat ...' : /bin/sh: /path/to/ebtables: no se encontró la orden That's a nasty mix of native/English/native output all in one very long message. Maybe we should be rethinking the desired output format a bit, for the sake of generating properly translated messages? Even breaking things into multiple messages, so that each message is either translated or English, rather than mixed, might help. Is it worth trying to fix part of the translation issue, by defining CMD_STOPONERROR to something that outputs the results of _("Failure to execute command '%s'"), then taking that translation and properly escaping it (via virBufferEscapeShell) so that the entire message will be output literally by shell (so that translators can't cause arbitrary shell execution by sneaking ';' into their translation), so that the generated script has a pre-translated message? But that sounds hairy. It also means that CMD_STOPONERROR would no longer be a string literal, but a complex function call to properly append stuff into each place you build up the command sequence into &buf. Maybe this just serves as yet another reason why using the shell as a script driver is prone to problems, and that anything we can do long-term to rewrite this into internal libvirt API that bypasses shell scripting will give better results, and we just live with the poor diagnostics in the meantime. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 11/21/2011 06:42 PM, Eric Blake wrote:
On 10/26/2011 09:12 AM, Stefan Berger wrote:
Display the executed command and failure message if a command failed to execute.
Signed-off-by: Stefan Berger<stefanb@linux.vnet.ibm.com>
--- src/nwfilter/nwfilter_ebiptables_driver.c | 82 ++++++++++++++++++------------ 1 file changed, 50 insertions(+), 32 deletions(-)
Index: libvirt-acl/src/nwfilter/nwfilter_ebiptables_driver.c =================================================================== --- libvirt-acl.orig/src/nwfilter/nwfilter_ebiptables_driver.c +++ libvirt-acl/src/nwfilter/nwfilter_ebiptables_driver.c @@ -63,10 +63,10 @@ #define CMD_DEF_PRE "cmd='" #define CMD_DEF_POST "'" #define CMD_DEF(X) CMD_DEF_PRE X CMD_DEF_POST -#define CMD_EXEC "eval res=\\$\\(\"${cmd}\"\\)" CMD_SEPARATOR +#define CMD_EXEC "eval res=\\$\\(\"${cmd} 2>&1 \"\\)" CMD_SEPARATOR Okay, after turning the C literal into shell, you have:
eval res=\$\("${cmd} 2>&1 "\)
and after the shell eval, you have:
res=$(command 2>&1 )
That space after '2>&1' could be deleted, but it doesn't hurt to leave it in.
I removed it now.
#define CMD_STOPONERR(X) \ X ? "if [ $? -ne 0 ]; then" \ - " echo \"Failure to execute command '${cmd}'.\";" \ + " echo \"Failure to execute command '${cmd}' : '${res}'.\";" \ Yes, this is a bit more informative.
" exit 1;" \ "fi" CMD_SEPARATOR \ : "" @@ -2785,12 +2785,16 @@ err_exit: */ static int ebiptablesExecCLI(virBufferPtr buf, - int *status) + int *status, char **errbuf) { char *cmds; char *filename; int rc; const char *argv[] = {NULL, NULL}; + virCommandPtr cmd; + + if (errbuf) + VIR_FREE(*errbuf);
This has merge conflicts with existing patches that went in during the meantime. I'd feel better seeing a v2 to verify proper rebasing.
ok
if (virBufferError(buf)) { virReportOOMError(); @@ -2817,7 +2821,13 @@ ebiptablesExecCLI(virBufferPtr buf,
virMutexLock(&execCLIMutex);
- rc = virRun(argv, status); + cmd = virCommandNewArgs(argv); + if (errbuf) + virCommandSetOutputBuffer(cmd, errbuf);
It looks a bit odd calling it errbuf, when it is the output collected from stdout; perhaps calling it outbuf would make more sense. Fixed.
@@ -3285,7 +3297,7 @@ ebtablesApplyBasicRules(const char *ifna ebtablesLinkTmpRootChain(&buf, 1, ifname, 1); ebtablesRenameTmpRootChain(&buf, 1, ifname);
- if (ebiptablesExecCLI(&buf,&cli_status) || cli_status != 0) + if (ebiptablesExecCLI(&buf,&cli_status, NULL) || cli_status != 0) You know, as long as we are cleaning things up, you could pass NULL instead of&cli_status to enforce that the command has a 0 exit status, so that you trim lines like this to:
if (ebiptablesExecCLI(&buf, NULL, NULL)< 0) I replace the occurrences of the above pattern with the line you are showing.
@@ -3874,12 +3889,13 @@ tear_down_tmpebchains: ebtablesRemoveTmpRootChain(&buf, 0, ifname); }
- ebiptablesExecCLI(&buf,&cli_status); + ebiptablesExecCLI(&buf,&cli_status, NULL);
virNWFilterReportError(VIR_ERR_BUILD_FIREWALL, _("Some rules could not be created for " - "interface %s."), - ifname); + "interface %s : %s"), + ifname, + errmsg ? errmsg : ""); That outputs a trailing space if there was no error message. Better might be:
virNWFilterReportError(VIR_ERR_BUILD_FIREWALL, _("Some rules could not be created for " "interface %s%s%s"), ifname, errmsg ? ": " : "", errmsg ? errmsg : "");
Fixed.
Alas, we have an i18n nightmare. errmsg contains English text output by the shell script, which is not marked for translation by either the shell script (which itself is tricky - witness the libvirt-guests init script) nor for translation in the C code that generated the shell script. Meanwhile, since we didn't call virCommandAddEnvPassCommon() to set the virCommand to force LC_ALL=C, that means we passed the libvirtd setting of LC_MESSAGES on through to the child processes, such that sh and ebtables output might also be translated. For an example, that means someone using LC_MESSAGES=es_ES.UTF-8 could see:
Algunas reglas no han podido crearse para la interfaz eth0 : Failure to execute command '/path/to/ebtables -t nat ...' : /bin/sh: /path/to/ebtables: no se encontró la orden
That's a nasty mix of native/English/native output all in one very long message. Maybe we should be rethinking the desired output format a bit, for the sake of generating properly translated messages? Even breaking things into multiple messages, so that each message is either translated or English, rather than mixed, might help.
Is it worth trying to fix part of the translation issue, by defining CMD_STOPONERROR to something that outputs the results of _("Failure to execute command '%s'"), then taking that translation and properly escaping it (via virBufferEscapeShell) so that the entire message will be output literally by shell (so that translators can't cause arbitrary shell execution by sneaking ';' into their translation), so that the generated script has a pre-translated message? But that sounds hairy. It also means that CMD_STOPONERROR would no longer be a string literal, but a complex function call to properly append stuff into each place you build up the command sequence into&buf.
Maybe this just serves as yet another reason why using the shell as a script driver is prone to problems, and that anything we can do long-term to rewrite this into internal libvirt API that bypasses shell scripting will give better results, and we just live with the poor diagnostics in the meantime.
Will leave it as-is.
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Stefan Berger