[ovs-dev] [PATCHv2] ofproto-dpif: Always un-wildcard fields that are being set.

Justin Pettit jpettit at nicira.com
Sat Aug 3 05:57:53 UTC 2013


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 at nicira.com>
Signed-off-by: Justin Pettit <jpettit at nicira.com>
---
 lib/multipath.c              |    2 +-
 lib/nx-match.c               |   14 ++++++++++++--
 lib/nx-match.h               |    5 +++--
 ofproto/ofproto-dpif-xlate.c |   19 +++++++++++++++++--
 tests/ofproto-dpif.at        |   29 +++++++++++++++++++++++++++++
 5 files changed, 62 insertions(+), 7 deletions(-)

diff --git a/lib/multipath.c b/lib/multipath.c
index 6c0560d..4b9e4af 100644
--- 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 bdb3a2b..940dd9a 100644
--- a/lib/nx-match.c
+++ b/lib/nx-match.c
@@ -1304,6 +1304,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);
@@ -1322,11 +1323,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);
@@ -1479,7 +1484,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;
 
@@ -1487,6 +1493,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 a6b7c52..9dcc19a 100644
--- a/lib/nx-match.h
+++ b/lib/nx-match.h
@@ -87,7 +87,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 *);
 
 char *nxm_parse_stack_action(struct ofpact_stack *, const char *)
     WARN_UNUSED_RESULT;
@@ -113,7 +113,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-xlate.c b/ofproto/ofproto-dpif-xlate.c
index ae677e7..46982a4 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -1608,6 +1608,7 @@ compose_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;
 }
@@ -1787,7 +1788,8 @@ xlate_bundle_action(struct xlate_ctx *ctx,
                           slave_enabled_cb,
                           CONST_CAST(struct xbridge *, ctx->xbridge));
     if (bundle->dst.field) {
-        nxm_reg_load(&bundle->dst, ofp_to_u16(port), &ctx->xin->flow);
+        nxm_reg_load(&bundle->dst, ofp_to_u16(port), &ctx->xin->flow,
+                     &ctx->xout->wc);
     } else {
         xlate_output_action(ctx, port, 0, false);
     }
@@ -1947,12 +1949,14 @@ 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);
             flow->vlan_tci &= ~htons(VLAN_VID_MASK);
             flow->vlan_tci |= (htons(ofpact_get_SET_VLAN_VID(a)->vlan_vid)
                                | htons(VLAN_CFI));
             break;
 
         case OFPACT_SET_VLAN_PCP:
+            wc->masks.vlan_tci |= htons(VLAN_PCP_MASK | VLAN_CFI);
             flow->vlan_tci &= ~htons(VLAN_PCP_MASK);
             flow->vlan_tci |=
                 htons((ofpact_get_SET_VLAN_PCP(a)->vlan_pcp << VLAN_PCP_SHIFT)
@@ -1960,35 +1964,42 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
             break;
 
         case OFPACT_STRIP_VLAN:
+            wc->masks.vlan_tci |= htons(VLAN_CFI);
             flow->vlan_tci = htons(0);
             break;
 
         case OFPACT_PUSH_VLAN:
             /* XXX 802.1AD(QinQ) */
+            wc->masks.vlan_tci |= htons(VLAN_CFI);
             flow->vlan_tci = htons(VLAN_CFI);
             break;
 
         case OFPACT_SET_ETH_SRC:
+            memset(&wc->masks.dl_src, 0xff, sizeof wc->masks.dl_src);
             memcpy(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(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 (flow->dl_type == htons(ETH_TYPE_IP)) {
                 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 (flow->dl_type == htons(ETH_TYPE_IP)) {
                 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 (flow->dl_type == htons(ETH_TYPE_IP)) {
                 flow->nw_tos &= ~IP_DSCP_MASK;
@@ -1998,6 +2009,7 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
 
         case OFPACT_SET_L4_SRC_PORT:
             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(flow)) {
                 flow->tp_src = htons(ofpact_get_SET_L4_SRC_PORT(a)->port);
             }
@@ -2005,6 +2017,7 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
 
         case OFPACT_SET_L4_DST_PORT:
             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(flow)) {
                 flow->tp_dst = htons(ofpact_get_SET_L4_DST_PORT(a)->port);
             }
@@ -2040,7 +2053,8 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
             break;
 
         case OFPACT_STACK_POP:
-            nxm_execute_stack_pop(ofpact_get_STACK_POP(a), flow, &ctx->stack);
+            nxm_execute_stack_pop(ofpact_get_STACK_POP(a), flow, wc,
+                                  &ctx->stack);
             break;
 
         case OFPACT_PUSH_MPLS:
@@ -2065,6 +2079,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;
             }
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index d5e5e3c..00affe8 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -2249,6 +2249,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>/
@@ -2633,6 +2636,32 @@ 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)'])
+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)'])
+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
+
 AT_SETUP([ofproto-dpif - datapath port number change])
 OVS_VSWITCHD_START([set Bridge br0 fail-mode=standalone])
 ADD_OF_PORTS([br0], 1)
-- 
1.7.5.4




More information about the dev mailing list