From 6c222e55fa4222c6724094e1e7a0a69addf6b030 Mon Sep 17 00:00:00 2001 From: Justin Pettit Date: Thu, 9 Jun 2011 15:43:18 -0700 Subject: [PATCH] Remove NXAST_DROP_SPOOFED_ARP action. The NXAST_DROP_SPOOFED_ARP action has been deprecated in favor of defining flows using the NXM_NX_ARP_SHA flow match for a while. This commit removes it. Signed-off-by: Justin Pettit Acked-by: Jesse Gross --- datapath/actions.c | 35 +------------------------ datapath/datapath.c | 2 -- include/openflow/nicira-ext.h | 20 +------------- include/openvswitch/datapath-protocol.h | 1 - lib/dpif-netdev.c | 34 ------------------------ lib/odp-util.c | 4 --- lib/ofp-parse.c | 5 ---- lib/ofp-print.c | 8 ++---- lib/ofp-util.c | 2 +- ofproto/ofproto-dpif.c | 13 +-------- tests/ovs-ofctl.at | 6 ----- utilities/ovs-ofctl.8.in | 10 ------- 12 files changed, 6 insertions(+), 134 deletions(-) diff --git a/datapath/actions.c b/datapath/actions.c index de98d990..6c1ca49c 100644 --- a/datapath/actions.c +++ b/datapath/actions.c @@ -216,34 +216,6 @@ static struct sk_buff *set_tp_port(struct sk_buff *skb, const struct nlattr *a) return skb; } -/** - * is_spoofed_arp - check for invalid ARP packet - * - * @skb: skbuff containing an Ethernet packet, with network header pointing - * just past the Ethernet and optional 802.1Q header. - * - * Returns true if @skb is an invalid Ethernet+IPv4 ARP packet: one with screwy - * or truncated header fields or one whose inner and outer Ethernet address - * differ. - */ -static bool is_spoofed_arp(struct sk_buff *skb) -{ - struct arp_eth_header *arp; - - if (OVS_CB(skb)->flow->key.eth.type != htons(ETH_P_ARP)) - return false; - - if (skb_network_offset(skb) + sizeof(struct arp_eth_header) > skb->len) - return true; - - arp = (struct arp_eth_header *)skb_network_header(skb); - return (arp->ar_hrd != htons(ARPHRD_ETHER) || - arp->ar_pro != htons(ETH_P_IP) || - arp->ar_hln != ETH_ALEN || - arp->ar_pln != 4 || - compare_ether_addr(arp->ar_sha, eth_hdr(skb)->h_source)); -} - static void do_output(struct datapath *dp, struct sk_buff *skb, int out_port) { struct vport *p; @@ -359,16 +331,11 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb, case ODP_ACTION_ATTR_POP_PRIORITY: skb->priority = priority; break; - - case ODP_ACTION_ATTR_DROP_SPOOFED_ARP: - if (unlikely(is_spoofed_arp(skb))) - goto exit; - break; } if (!skb) return -ENOMEM; } -exit: + if (prev_port != -1) do_output(dp, skb, prev_port); else diff --git a/datapath/datapath.c b/datapath/datapath.c index d607315e..e2846f20 100644 --- a/datapath/datapath.c +++ b/datapath/datapath.c @@ -565,7 +565,6 @@ static int validate_actions(const struct nlattr *attr) [ODP_ACTION_ATTR_SET_TUNNEL] = 8, [ODP_ACTION_ATTR_SET_PRIORITY] = 4, [ODP_ACTION_ATTR_POP_PRIORITY] = 0, - [ODP_ACTION_ATTR_DROP_SPOOFED_ARP] = 0, }; int type = nla_type(a); @@ -587,7 +586,6 @@ static int validate_actions(const struct nlattr *attr) case ODP_ACTION_ATTR_SET_TUNNEL: case ODP_ACTION_ATTR_SET_PRIORITY: case ODP_ACTION_ATTR_POP_PRIORITY: - case ODP_ACTION_ATTR_DROP_SPOOFED_ARP: /* No validation needed. */ break; diff --git a/include/openflow/nicira-ext.h b/include/openflow/nicira-ext.h index 738fd90d..4270c165 100644 --- a/include/openflow/nicira-ext.h +++ b/include/openflow/nicira-ext.h @@ -257,7 +257,7 @@ enum nx_action_subtype { NXAST_SNAT__OBSOLETE, /* No longer used. */ NXAST_RESUBMIT, /* struct nx_action_resubmit */ NXAST_SET_TUNNEL, /* struct nx_action_set_tunnel */ - NXAST_DROP_SPOOFED_ARP, /* struct nx_action_drop_spoofed_arp */ + NXAST_DROP_SPOOFED_ARP__OBSOLETE, NXAST_SET_QUEUE, /* struct nx_action_set_queue */ NXAST_POP_QUEUE, /* struct nx_action_pop_queue */ NXAST_REG_MOVE, /* struct nx_action_reg_move */ @@ -341,24 +341,6 @@ struct nx_action_set_tunnel64 { }; OFP_ASSERT(sizeof(struct nx_action_set_tunnel64) == 24); -/* Action structure for NXAST_DROP_SPOOFED_ARP. - * - * Stops processing further actions, if the packet being processed is an - * Ethernet+IPv4 ARP packet for which the source Ethernet address inside the - * ARP packet differs from the source Ethernet address in the Ethernet header. - * - * (This action is deprecated in favor of defining flows using the - * NXM_NX_ARP_SHA flow match and will likely be removed in a future version - * of Open vSwitch.) */ -struct nx_action_drop_spoofed_arp { - ovs_be16 type; /* OFPAT_VENDOR. */ - ovs_be16 len; /* Length is 16. */ - ovs_be32 vendor; /* NX_VENDOR_ID. */ - ovs_be16 subtype; /* NXAST_DROP_SPOOFED_ARP. */ - uint8_t pad[6]; -}; -OFP_ASSERT(sizeof(struct nx_action_drop_spoofed_arp) == 16); - /* Action structure for NXAST_SET_QUEUE. * * Set the queue that should be used when packets are output. This is similar diff --git a/include/openvswitch/datapath-protocol.h b/include/openvswitch/datapath-protocol.h index e5bbc6ac..e7708ef1 100644 --- a/include/openvswitch/datapath-protocol.h +++ b/include/openvswitch/datapath-protocol.h @@ -422,7 +422,6 @@ enum odp_action_type { ODP_ACTION_ATTR_SET_TUNNEL, /* Set the encapsulating tunnel ID. */ ODP_ACTION_ATTR_SET_PRIORITY, /* Set skb->priority. */ ODP_ACTION_ATTR_POP_PRIORITY, /* Restore original skb->priority. */ - ODP_ACTION_ATTR_DROP_SPOOFED_ARP, /* Drop ARPs with spoofed source MAC. */ __ODP_ACTION_ATTR_MAX }; diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index 5f37197e..3b93a4cf 100644 --- a/lib/dpif-netdev.c +++ b/lib/dpif-netdev.c @@ -708,7 +708,6 @@ dpif_netdev_validate_actions(const struct nlattr *actions, break; case ODP_ACTION_ATTR_CONTROLLER: - case ODP_ACTION_ATTR_DROP_SPOOFED_ARP: break; case ODP_ACTION_ATTR_SET_DL_TCI: @@ -1285,34 +1284,6 @@ dp_netdev_output_control(struct dp_netdev *dp, const struct ofpbuf *packet, return 0; } -/* Returns true if 'packet' is an invalid Ethernet+IPv4 ARP packet: one with - * screwy or truncated header fields or one whose inner and outer Ethernet - * address differ. */ -static bool -dp_netdev_is_spoofed_arp(struct ofpbuf *packet, const struct flow *key) -{ - struct arp_eth_header *arp; - struct eth_header *eth; - ptrdiff_t l3_size; - - if (key->dl_type != htons(ETH_TYPE_ARP)) { - return false; - } - - l3_size = (char *) ofpbuf_end(packet) - (char *) packet->l3; - if (l3_size < sizeof(struct arp_eth_header)) { - return true; - } - - eth = packet->l2; - arp = packet->l3; - return (arp->ar_hrd != htons(ARP_HRD_ETHERNET) - || arp->ar_pro != htons(ARP_PRO_IP) - || arp->ar_hln != ETH_HEADER_LEN - || arp->ar_pln != 4 - || !eth_addr_equals(arp->ar_sha, eth->eth_src)); -} - static int dp_netdev_execute_actions(struct dp_netdev *dp, struct ofpbuf *packet, struct flow *key, @@ -1362,11 +1333,6 @@ dp_netdev_execute_actions(struct dp_netdev *dp, case ODP_ACTION_ATTR_SET_TP_DST: dp_netdev_set_tp_port(packet, key, a); break; - - case ODP_ACTION_ATTR_DROP_SPOOFED_ARP: - if (dp_netdev_is_spoofed_arp(packet, key)) { - return 0; - } } } return 0; diff --git a/lib/odp-util.c b/lib/odp-util.c index 79f4bfc7..d7a3118b 100644 --- a/lib/odp-util.c +++ b/lib/odp-util.c @@ -54,7 +54,6 @@ odp_action_len(uint16_t type) case ODP_ACTION_ATTR_SET_TUNNEL: return 8; case ODP_ACTION_ATTR_SET_PRIORITY: return 4; case ODP_ACTION_ATTR_POP_PRIORITY: return 0; - case ODP_ACTION_ATTR_DROP_SPOOFED_ARP: return 0; case ODP_ACTION_ATTR_UNSPEC: case __ODP_ACTION_ATTR_MAX: @@ -146,9 +145,6 @@ format_odp_action(struct ds *ds, const struct nlattr *a) case ODP_ACTION_ATTR_POP_PRIORITY: ds_put_cstr(ds, "pop_priority"); break; - case ODP_ACTION_ATTR_DROP_SPOOFED_ARP: - ds_put_cstr(ds, "drop_spoofed_arp"); - break; default: format_generic_odp_action(ds, a); break; diff --git a/lib/ofp-parse.c b/lib/ofp-parse.c index 3f3ce62e..afcaf877 100644 --- a/lib/ofp-parse.c +++ b/lib/ofp-parse.c @@ -410,11 +410,6 @@ str_to_action(char *str, struct ofpbuf *b) nast->subtype = htons(NXAST_SET_TUNNEL); nast->tun_id = htonl(tun_id); } - } else if (!strcasecmp(act, "drop_spoofed_arp")) { - struct nx_action_header *nah; - nah = put_action(b, sizeof *nah, OFPAT_VENDOR); - nah->vendor = htonl(NX_VENDOR_ID); - nah->subtype = htons(NXAST_DROP_SPOOFED_ARP); } else if (!strcasecmp(act, "set_queue")) { struct nx_action_set_queue *nasq; nasq = put_action(b, sizeof *nasq, OFPAT_VENDOR); diff --git a/lib/ofp-print.c b/lib/ofp-print.c index 89656cc0..0c3bea1d 100644 --- a/lib/ofp-print.c +++ b/lib/ofp-print.c @@ -210,8 +210,7 @@ nx_action_len(enum nx_action_subtype subtype) case NXAST_SNAT__OBSOLETE: return -1; case NXAST_RESUBMIT: return sizeof(struct nx_action_resubmit); case NXAST_SET_TUNNEL: return sizeof(struct nx_action_set_tunnel); - case NXAST_DROP_SPOOFED_ARP: - return sizeof(struct nx_action_drop_spoofed_arp); + case NXAST_DROP_SPOOFED_ARP__OBSOLETE: return -1; case NXAST_SET_QUEUE: return sizeof(struct nx_action_set_queue); case NXAST_POP_QUEUE: return sizeof(struct nx_action_pop_queue); case NXAST_REG_MOVE: return sizeof(struct nx_action_reg_move); @@ -259,10 +258,6 @@ ofp_print_nx_action(struct ds *string, const struct nx_action_header *nah) ds_put_format(string, "set_tunnel:%#"PRIx32, ntohl(nast->tun_id)); return; - case NXAST_DROP_SPOOFED_ARP: - ds_put_cstr(string, "drop_spoofed_arp"); - return; - case NXAST_SET_QUEUE: nasq = (struct nx_action_set_queue *)nah; ds_put_format(string, "set_queue:%u", ntohl(nasq->queue_id)); @@ -307,6 +302,7 @@ ofp_print_nx_action(struct ds *string, const struct nx_action_header *nah) return; case NXAST_SNAT__OBSOLETE: + case NXAST_DROP_SPOOFED_ARP__OBSOLETE: default: break; } diff --git a/lib/ofp-util.c b/lib/ofp-util.c index e21831fd..750918d8 100644 --- a/lib/ofp-util.c +++ b/lib/ofp-util.c @@ -1864,7 +1864,6 @@ check_nicira_action(const union ofp_action *a, unsigned int len, switch ((enum nx_action_subtype) subtype) { case NXAST_RESUBMIT: case NXAST_SET_TUNNEL: - case NXAST_DROP_SPOOFED_ARP: case NXAST_SET_QUEUE: case NXAST_POP_QUEUE: return check_nx_action_exact_len(nah, len, 16); @@ -1909,6 +1908,7 @@ check_nicira_action(const union ofp_action *a, unsigned int len, return autopath_check((const struct nx_action_autopath *) a); case NXAST_SNAT__OBSOLETE: + case NXAST_DROP_SPOOFED_ARP__OBSOLETE: default: VLOG_WARN_RL(&bad_ofmsg_rl, "unknown Nicira vendor action subtype %d", subtype); diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c index 55d109f3..8088c68d 100644 --- a/ofproto/ofproto-dpif.c +++ b/ofproto/ofproto-dpif.c @@ -3007,18 +3007,6 @@ xlate_nicira_action(struct action_xlate_ctx *ctx, ctx->flow.tun_id = tun_id; break; - case NXAST_DROP_SPOOFED_ARP: - if (ctx->flow.dl_type == htons(ETH_TYPE_ARP)) { - /* XXX: It's not entirely clear whether or not we need to commit - * here. The safer thing to do is commit of course. Hopefully in - * the near future we can rip out NXAST_DROP_SPOOFED_ARP altogether - * and the point will be moot. */ - commit_odp_actions(ctx); - nl_msg_put_flag(ctx->odp_actions, - ODP_ACTION_ATTR_DROP_SPOOFED_ARP); - } - break; - case NXAST_SET_QUEUE: nasq = (const struct nx_action_set_queue *) nah; xlate_set_queue_action(ctx, nasq); @@ -3061,6 +3049,7 @@ xlate_nicira_action(struct action_xlate_ctx *ctx, * update the flow key in ctx->flow at the same time. */ case NXAST_SNAT__OBSOLETE: + case NXAST_DROP_SPOOFED_ARP__OBSOLETE: default: VLOG_DBG_RL(&rl, "unknown Nicira action type %d", (int) subtype); break; diff --git a/tests/ovs-ofctl.at b/tests/ovs-ofctl.at index 8b15f9c0..d9b71aa0 100644 --- a/tests/ovs-ofctl.at +++ b/tests/ovs-ofctl.at @@ -5,7 +5,6 @@ AT_DATA([flows.txt], [[ # comment tcp,tp_src=123,actions=flood in_port=LOCAL dl_vlan=9 dl_src=00:0A:E4:25:6B:B0 actions=drop -arp,nw_src=192.168.0.1 actions=drop_spoofed_arp,NORMAL udp dl_vlan_pcp=7 idle_timeout=5 actions=strip_vlan output:0 tcp,nw_src=192.168.0.3,tp_dst=80 actions=set_queue:37,output:1 udp,nw_src=192.168.0.3,tp_dst=53 actions=pop_queue,output:1 @@ -22,7 +21,6 @@ AT_CHECK([ovs-ofctl parse-flows flows.txt AT_CHECK([[sed 's/ (xid=0x[0-9a-fA-F]*)//' stdout]], [0], [[OFPT_FLOW_MOD: ADD tcp,tp_src=123 actions=FLOOD OFPT_FLOW_MOD: ADD in_port=65534,dl_vlan=9,dl_src=00:0a:e4:25:6b:b0 actions=drop -OFPT_FLOW_MOD: ADD arp,nw_src=192.168.0.1 actions=drop_spoofed_arp,NORMAL OFPT_FLOW_MOD: ADD udp,dl_vlan_pcp=7 idle:5 actions=strip_vlan,output:0 OFPT_FLOW_MOD: ADD tcp,nw_src=192.168.0.3,tp_dst=80 actions=set_queue:37,output:1 OFPT_FLOW_MOD: ADD udp,nw_src=192.168.0.3,tp_dst=53 actions=pop_queue,output:1 @@ -43,7 +41,6 @@ AT_DATA([flows.txt], [ # comment tcp,tp_src=123,actions=flood in_port=LOCAL dl_vlan=9 dl_src=00:0A:E4:25:6B:B0 actions=drop -arp,nw_src=192.168.0.1 actions=drop_spoofed_arp,NORMAL arp,dl_src=00:0A:E4:25:6B:B0,arp_sha=00:0A:E4:25:6B:B0 actions=drop ipv6,ipv6_src=2001:db8:3c4d:1:2:3:4:5 actions=3 ipv6,ipv6_src=2001:db8:3c4d:1:2:3:4:5/64 actions=4 @@ -71,7 +68,6 @@ AT_CHECK([ovs-ofctl -F nxm parse-flows flows.txt], [0], [stdout]) AT_CHECK([[sed 's/ (xid=0x[0-9a-fA-F]*)//' stdout]], [0], [dnl NXT_FLOW_MOD: ADD tcp,tp_src=123 actions=FLOOD NXT_FLOW_MOD: ADD in_port=65534,dl_vlan=9,dl_src=00:0a:e4:25:6b:b0 actions=drop -NXT_FLOW_MOD: ADD arp,nw_src=192.168.0.1 actions=drop_spoofed_arp,NORMAL NXT_FLOW_MOD: ADD arp,dl_src=00:0a:e4:25:6b:b0,arp_sha=00:0a:e4:25:6b:b0 actions=drop NXT_FLOW_MOD: ADD ipv6,ipv6_src=2001:db8:3c4d:1:2:3:4:5 actions=output:3 NXT_FLOW_MOD: ADD ipv6,ipv6_src=2001:db8:3c4d:1::/64 actions=output:4 @@ -102,7 +98,6 @@ AT_DATA([flows.txt], [[ # comment tcp,tp_src=123,actions=flood in_port=LOCAL dl_vlan=9 dl_src=00:0A:E4:25:6B:B0 actions=drop -arp,nw_src=192.168.0.1 actions=drop_spoofed_arp,NORMAL arp,dl_src=00:0A:E4:25:6B:B0,arp_sha=00:0A:E4:25:6B:B0 actions=drop ipv6,ipv6_src=2001:db8:3c4d:1:2:3:4:5 actions=3 ipv6,ipv6_src=2001:db8:3c4d:1:2:3:4:5/64 actions=4 @@ -127,7 +122,6 @@ AT_CHECK([ovs-ofctl -F nxm -mmm parse-flows flows.txt], [0], [stdout]) AT_CHECK([[sed 's/ (xid=0x[0-9a-fA-F]*)//' stdout]], [0], [[NXT_FLOW_MOD: ADD NXM_OF_ETH_TYPE(0800), NXM_OF_IP_PROTO(06), NXM_OF_TCP_SRC(007b) actions=FLOOD NXT_FLOW_MOD: ADD NXM_OF_IN_PORT(fffe), NXM_OF_ETH_SRC(000ae4256bb0), NXM_OF_VLAN_TCI_W(1009/1fff) actions=drop -NXT_FLOW_MOD: ADD NXM_OF_ETH_TYPE(0806), NXM_OF_ARP_SPA(c0a80001) actions=drop_spoofed_arp,NORMAL NXT_FLOW_MOD: ADD NXM_OF_ETH_SRC(000ae4256bb0), NXM_OF_ETH_TYPE(0806), NXM_NX_ARP_SHA(000ae4256bb0) actions=drop NXT_FLOW_MOD: ADD NXM_OF_ETH_TYPE(86dd), NXM_NX_IPV6_SRC(20010db83c4d00010002000300040005) actions=output:3 NXT_FLOW_MOD: ADD NXM_OF_ETH_TYPE(86dd), NXM_NX_IPV6_SRC_W(20010db83c4d00010000000000000000/ffffffffffffffff0000000000000000) actions=output:4 diff --git a/utilities/ovs-ofctl.8.in b/utilities/ovs-ofctl.8.in index 2e866dc5..eb0cebed 100644 --- a/utilities/ovs-ofctl.8.in +++ b/utilities/ovs-ofctl.8.in @@ -618,16 +618,6 @@ then this uses an action extension that is supported by Open vSwitch 1.0 and later. Otherwise, if \fIid\fR is a 64-bit value, it requires Open vSwitch 1.1 or later. . -.IP \fBdrop_spoofed_arp\fR -Stops processing further actions, if the packet being processed is an -Ethernet+IPv4 ARP packet for which the source Ethernet address inside -the ARP packet differs from the source Ethernet address in the -Ethernet header. -.IP -This action is deprecated in favor of defining flows using the -\fBarp_sha\fR match field described earlier and will likely be removed -in a future version of Open vSwitch. -. .IP \fBset_queue\fB:\fIqueue\fR Sets the queue that should be used to \fIqueue\fR when packets are output. The number of supported queues depends on the switch; some -- 2.30.2