ofproto-dpif: Always un-wildcard fields that are being set.

The ODP library has an optimization to not set a header if the field was
not changed, regardless of whether an action to set the field was
present.  That library is also responsible for un-wildcarding fields
that are bieng modified.  This leads to a problem where a packet matches
a flow that updates a field, but that particular packet's field already
has that value.  As such, an overly loose megaflow will be generated
that doesn't match on that field and the actions won't update it.  A
second packet that should have the field set will match that flow and
will not be modified.

This commit changes the behavior to always un-wildcard fields that are
being modified.  Since the ODP library updates the entire header if a
field in it is modified, and all those fields will be un-wildcarded, the
generated flows may be different.  However, they should be correct.

Bug #18946.

Reported-by: Jesse Gross <jesse@nicira.com>
Signed-off-by: Justin Pettit <jpettit@nicira.com>
Acked-by: Ben Pfaff <blp@nicira.com>

diff --git a/lib/multipath.c b/lib/multipath.c
index 1be6964..72cdcdf 100644 (file)
--- a/lib/multipath.c
+++ b/lib/multipath.c
@@ -114,7 +114,7 @@ multipath_execute(const struct ofpact_multipath *mp, struct flow *flow,
                                         mp->max_link + 1, mp->arg);
 
     flow_mask_hash_fields(flow, wc, mp->fields);
-    nxm_reg_load(&mp->dst, link, flow);
+    nxm_reg_load(&mp->dst, link, flow, wc);
 }
 
 static uint16_t

diff --git a/lib/nx-match.c b/lib/nx-match.c
index b24f95d..fc5c83c 100644 (file)
--- a/lib/nx-match.c
+++ b/lib/nx-match.c
@@ -1291,6 +1291,7 @@ nxm_execute_reg_move(const struct ofpact_reg_move *move,
     union mf_value dst_value;
 
     memset(&mask_value, 0xff, sizeof mask_value);
+    mf_write_subfield_flow(&move->dst, &mask_value, &wc->masks);
     mf_write_subfield_flow(&move->src, &mask_value, &wc->masks);
 
     mf_get_value(move->dst.field, flow, &dst_value);
@@ -1309,11 +1310,15 @@ nxm_execute_reg_load(const struct ofpact_reg_load *load, struct flow *flow)
 
 void
 nxm_reg_load(const struct mf_subfield *dst, uint64_t src_data,
-             struct flow *flow)
+             struct flow *flow, struct flow_wildcards *wc)
 {
     union mf_subvalue src_subvalue;
+    union mf_subvalue mask_value;
     ovs_be64 src_data_be = htonll(src_data);
 
+    memset(&mask_value, 0xff, sizeof mask_value);
+    mf_write_subfield_flow(dst, &mask_value, &wc->masks);
+
     bitwise_copy(&src_data_be, sizeof src_data_be, 0,
                  &src_subvalue, sizeof src_subvalue, 0,
                  sizeof src_data_be * 8);
@@ -1452,7 +1457,8 @@ nxm_execute_stack_push(const struct ofpact_stack *push,
 
 void
 nxm_execute_stack_pop(const struct ofpact_stack *pop,
-                      struct flow *flow, struct ofpbuf *stack)
+                      struct flow *flow, struct flow_wildcards *wc,
+                      struct ofpbuf *stack)
 {
     union mf_subvalue *src_value;
 
@@ -1460,6 +1466,10 @@ nxm_execute_stack_pop(const struct ofpact_stack *pop,
 
     /* Only pop if stack is not empty. Otherwise, give warning. */
     if (src_value) {
+        union mf_subvalue mask_value;
+
+        memset(&mask_value, 0xff, sizeof mask_value);
+        mf_write_subfield_flow(&pop->subfield, &mask_value, &wc->masks);
         mf_write_subfield_flow(&pop->subfield, src_value, flow);
     } else {
         if (!VLOG_DROP_WARN(&rl)) {

diff --git a/lib/nx-match.h b/lib/nx-match.h
index bb63b90..f40863b 100644 (file)
--- a/lib/nx-match.h
+++ b/lib/nx-match.h
@@ -84,7 +84,7 @@ void nxm_execute_reg_move(const struct ofpact_reg_move *, struct flow *,
                           struct flow_wildcards *);
 void nxm_execute_reg_load(const struct ofpact_reg_load *, struct flow *);
 void nxm_reg_load(const struct mf_subfield *, uint64_t src_data,
-                  struct flow *);
+                  struct flow *, struct flow_wildcards *);
 
 void nxm_parse_stack_action(struct ofpact_stack *, const char *);
 
@@ -109,7 +109,8 @@ void nxm_execute_stack_push(const struct ofpact_stack *,
                             const struct flow *, struct flow_wildcards *,
                             struct ofpbuf *);
 void nxm_execute_stack_pop(const struct ofpact_stack *,
-                            struct flow *, struct ofpbuf *);
+                            struct flow *, struct flow_wildcards *,
+                            struct ofpbuf *);
 
 int nxm_field_bytes(uint32_t header);
 int nxm_field_bits(uint32_t header);

diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 79ad711..9064809 100644 (file)
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -6509,6 +6509,7 @@ execute_set_mpls_ttl_action(struct xlate_ctx *ctx, uint8_t ttl)
         return true;
     }
 
+    ctx->xout->wc.masks.mpls_lse |= htonl(MPLS_TTL_MASK);
     set_mpls_lse_ttl(&ctx->xin->flow.mpls_lse, ttl);
     return false;
 }
@@ -6684,7 +6685,7 @@ xlate_bundle_action(struct xlate_ctx *ctx,
     port = bundle_execute(bundle, &ctx->xin->flow, &ctx->xout->wc,
                           slave_enabled_cb, ctx->ofproto);
     if (bundle->dst.field) {
-        nxm_reg_load(&bundle->dst, port, &ctx->xin->flow);
+        nxm_reg_load(&bundle->dst, port, &ctx->xin->flow, &ctx->xout->wc);
     } else {
         xlate_output_action(ctx, port, 0, false);
     }
@@ -6808,6 +6809,7 @@ static void
 do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
                  struct xlate_ctx *ctx)
 {
+    struct flow_wildcards *wc = &ctx->xout->wc;
     bool was_evictable = true;
     const struct ofpact *a;
 
@@ -6844,6 +6846,7 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
             break;
 
         case OFPACT_SET_VLAN_VID:
+            wc->masks.vlan_tci |= htons(VLAN_VID_MASK | VLAN_CFI);
             ctx->xin->flow.vlan_tci &= ~htons(VLAN_VID_MASK);
             ctx->xin->flow.vlan_tci |=
                 (htons(ofpact_get_SET_VLAN_VID(a)->vlan_vid)
@@ -6851,6 +6854,7 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
             break;
 
         case OFPACT_SET_VLAN_PCP:
+            wc->masks.vlan_tci |= htons(VLAN_PCP_MASK | VLAN_CFI);
             ctx->xin->flow.vlan_tci &= ~htons(VLAN_PCP_MASK);
             ctx->xin->flow.vlan_tci |=
                 htons((ofpact_get_SET_VLAN_PCP(a)->vlan_pcp << VLAN_PCP_SHIFT)
@@ -6858,37 +6862,44 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
             break;
 
         case OFPACT_STRIP_VLAN:
+            memset(&wc->masks.vlan_tci, 0xff, sizeof wc->masks.vlan_tci);
             ctx->xin->flow.vlan_tci = htons(0);
             break;
 
         case OFPACT_PUSH_VLAN:
             /* XXX 802.1AD(QinQ) */
+            memset(&wc->masks.vlan_tci, 0xff, sizeof wc->masks.vlan_tci);
             ctx->xin->flow.vlan_tci = htons(VLAN_CFI);
             break;
 
         case OFPACT_SET_ETH_SRC:
+            memset(&wc->masks.dl_src, 0xff, sizeof wc->masks.dl_src);
             memcpy(ctx->xin->flow.dl_src, ofpact_get_SET_ETH_SRC(a)->mac,
                    ETH_ADDR_LEN);
             break;
 
         case OFPACT_SET_ETH_DST:
+            memset(&wc->masks.dl_dst, 0xff, sizeof wc->masks.dl_dst);
             memcpy(ctx->xin->flow.dl_dst, ofpact_get_SET_ETH_DST(a)->mac,
                    ETH_ADDR_LEN);
             break;
 
         case OFPACT_SET_IPV4_SRC:
+            memset(&wc->masks.nw_src, 0xff, sizeof wc->masks.nw_src);
             if (ctx->xin->flow.dl_type == htons(ETH_TYPE_IP)) {
                 ctx->xin->flow.nw_src = ofpact_get_SET_IPV4_SRC(a)->ipv4;
             }
             break;
 
         case OFPACT_SET_IPV4_DST:
+            memset(&wc->masks.nw_dst, 0xff, sizeof wc->masks.nw_dst);
             if (ctx->xin->flow.dl_type == htons(ETH_TYPE_IP)) {
                 ctx->xin->flow.nw_dst = ofpact_get_SET_IPV4_DST(a)->ipv4;
             }
             break;
 
         case OFPACT_SET_IPV4_DSCP:
+            wc->masks.nw_tos |= IP_DSCP_MASK;
             /* OpenFlow 1.0 only supports IPv4. */
             if (ctx->xin->flow.dl_type == htons(ETH_TYPE_IP)) {
                 ctx->xin->flow.nw_tos &= ~IP_DSCP_MASK;
@@ -6897,8 +6908,8 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
             break;
 
         case OFPACT_SET_L4_SRC_PORT:
-            memset(&ctx->xout->wc.masks.nw_proto, 0xff,
-                    sizeof ctx->xout->wc.masks.nw_proto);
+            memset(&wc->masks.nw_proto, 0xff, sizeof wc->masks.nw_proto);
+            memset(&wc->masks.tp_src, 0xff, sizeof wc->masks.tp_src);
             if (is_ip_any(&ctx->xin->flow)) {
                 ctx->xin->flow.tp_src =
                     htons(ofpact_get_SET_L4_SRC_PORT(a)->port);
@@ -6906,8 +6917,8 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
             break;
 
         case OFPACT_SET_L4_DST_PORT:
-            memset(&ctx->xout->wc.masks.nw_proto, 0xff,
-                    sizeof ctx->xout->wc.masks.nw_proto);
+            memset(&wc->masks.nw_proto, 0xff, sizeof wc->masks.nw_proto);
+            memset(&wc->masks.tp_dst, 0xff, sizeof wc->masks.tp_dst);
             if (is_ip_any(&ctx->xin->flow)) {
                 ctx->xin->flow.tp_dst =
                     htons(ofpact_get_SET_L4_DST_PORT(a)->port);
@@ -6932,8 +6943,7 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
             break;
 
         case OFPACT_REG_MOVE:
-            nxm_execute_reg_move(ofpact_get_REG_MOVE(a), &ctx->xin->flow,
-                                 &ctx->xout->wc);
+            nxm_execute_reg_move(ofpact_get_REG_MOVE(a), &ctx->xin->flow, wc);
             break;
 
         case OFPACT_REG_LOAD:
@@ -6942,12 +6952,12 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
 
         case OFPACT_STACK_PUSH:
             nxm_execute_stack_push(ofpact_get_STACK_PUSH(a), &ctx->xin->flow,
-                                   &ctx->xout->wc, &ctx->stack);
+                                   wc, &ctx->stack);
             break;
 
         case OFPACT_STACK_POP:
             nxm_execute_stack_pop(ofpact_get_STACK_POP(a), &ctx->xin->flow,
-                                  &ctx->stack);
+                                  wc, &ctx->stack);
             break;
 
         case OFPACT_PUSH_MPLS:
@@ -6972,6 +6982,7 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
             break;
 
         case OFPACT_DEC_TTL:
+            wc->masks.nw_ttl = 0xff;
             if (compose_dec_ttl(ctx, ofpact_get_DEC_TTL(a))) {
                 goto out;
             }
@@ -6982,8 +6993,7 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
             break;
 
         case OFPACT_MULTIPATH:
-            multipath_execute(ofpact_get_MULTIPATH(a), &ctx->xin->flow,
-                              &ctx->xout->wc);
+            multipath_execute(ofpact_get_MULTIPATH(a), &ctx->xin->flow, wc);
             break;
 
         case OFPACT_BUNDLE:
@@ -7004,8 +7014,7 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
             break;
 
         case OFPACT_FIN_TIMEOUT:
-            memset(&ctx->xout->wc.masks.nw_proto, 0xff,
-                   sizeof ctx->xout->wc.masks.nw_proto);
+            memset(&wc->masks.nw_proto, 0xff, sizeof wc->masks.nw_proto);
             ctx->xout->has_fin_timeout = true;
             xlate_fin_timeout(ctx, ofpact_get_FIN_TIMEOUT(a));
             break;

diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index 1ebecf8..9c51f42 100644 (file)
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -1957,6 +1957,9 @@ AT_BANNER([ofproto-dpif -- megaflows])
 
 # Strips out uninteresting parts of megaflow output, as well as parts
 # that vary from one run to another (e.g., timing and bond actions).
+m4_define([STRIP_USED], [[sed '
+    s/used:[0-9]*\.[0-9]*/used:0.0/
+' | sort]])
 m4_define([STRIP_XOUT], [[sed '
     s/used:[0-9]*\.[0-9]*/used:0.0/
     s/Datapath actions:.*/Datapath actions: <del>/
@@ -2381,3 +2384,31 @@ skb_priority=0,icmp,in_port=1,nw_src=10.0.0.4,nw_dst=10.0.0.3,nw_tos=0,nw_ecn=0,
 ])
 OVS_VSWITCHD_STOP
 AT_CLEANUP
+
+AT_SETUP([ofproto-dpif megaflow - set dl_dst])
+OVS_VSWITCHD_START
+ADD_OF_PORTS([br0], [1], [2])
+AT_DATA([flows.txt], [dnl
+table=0 in_port=1 actions=mod_dl_dst(50:54:00:00:00:0a),output(2)
+])
+AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
+AT_CHECK([ovs-appctl netdev-dummy/receive p1 'in_port(1),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800),ipv4(src=10.0.0.2,dst=10.0.0.1,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0)'], [0], [success
+])
+AT_CHECK([ovs-appctl netdev-dummy/receive p1 'in_port(1),eth(src=50:54:00:00:00:0b,dst=50:54:00:00:00:0c),eth_type(0x0800),ipv4(src=10.0.0.4,dst=10.0.0.3,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0)'], [0], [success
+])
+dnl The megaflows do not match the same fields, since the first packet
+dnl is essentially a no-op.  (The new destination MAC is the same as the
+dnl original.) The ofproto-dpif library un-wildcards the destination MAC
+dnl so that a packet that doesn't need its MAC address changed doesn't
+dnl hide one that does.  Since the first entry doesn't need to change,
+dnl only the destination MAC address is matched (as decided by
+dnl ofproto-dpif).  The second entry actually updates the destination
+dnl MAC, so both the source and destination MAC addresses are
+dnl un-wildcarded, since the ODP commit functions update both the source
+dnl and destination MAC addresses.
+AT_CHECK([ovs-appctl dpif/dump-megaflows br0 | STRIP_USED], [0], [dnl
+skb_priority=0,ip,in_port=1,dl_dst=50:54:00:00:00:0a,nw_frag=no, n_subfacets:1, used:0.0s, Datapath actions: 2
+skb_priority=0,ip,in_port=1,dl_src=50:54:00:00:00:0b,dl_dst=50:54:00:00:00:0c,nw_frag=no, n_subfacets:1, used:0.0s, Datapath actions: set(eth(src=50:54:00:00:00:0b,dst=50:54:00:00:00:0a)),2
+])
+OVS_VSWITCHD_STOP
+AT_CLEANUP