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 <je...@nicira.com>
Signed-off-by: Justin Pettit <jpet...@nicira.com>
---
ofproto/ofproto-dpif-xlate.c | 10 ++++++++++
tests/ofproto-dpif.at | 29 +++++++++++++++++++++++++++++
2 files changed, 39 insertions(+), 0 deletions(-)
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index ae677e7..3916b16 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -1947,12 +1947,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)
@@ -1969,26 +1971,31 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t
ofpacts_len,
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 +2005,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 +2013,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);
}
@@ -2065,6 +2074,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
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
