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 <jpettit@nicira.com>
Acked-by: Jesse Gross <jesse@nicira.com>

diff --git a/datapath/actions.c b/datapath/actions.c
index de98d99..6c1ca49 100644 (file)
--- 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 d607315..e2846f2 100644 (file)
--- 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 738fd90..4270c16 100644 (file)
--- 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 e5bbc6a..e7708ef 100644 (file)
--- 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 5f37197..3b93a4c 100644 (file)
--- 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 79f4bfc..d7a3118 100644 (file)
--- 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 3f3ce62..afcaf87 100644 (file)
--- 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 89656cc..0c3bea1 100644 (file)
--- 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 e21831f..750918d 100644 (file)
--- 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 55d109f..8088c68 100644 (file)
--- 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 8b15f9c..d9b71aa 100644 (file)
--- 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 2e866dc..eb0cebe 100644 (file)
--- 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