Previously, whenever a set_field() action was executed, the entire field
would become masked and the entire field replaced, regardless of the
mask specified in the set_field() action.
In most cases this is fine, although it may lead to more specific
wildcards than strictly necessary. However, in a particular case with
connection tracking actions it could lead to the wrong behaviour.
Unlike most OpenFlow fields, the ct_{mark,labels} fields are typically
unknown until the ct(...,recirc_table=N,...) action is executed however
the packet may actually belong to a connection which has a nonzero value
for one of these fields. This can lead to the wrong behaviour with flows
such as the following:
in_port=1,ip,actions=ct(commit,exec(set_field(0x1/0x1->ct_mark))),2
in_port=2,ip,actions=ct(commit,exec(set_field(0x2/0x2->ct_mark))),1
Connections flowing through these actions will always update the ct_mark
field stored within the conntrack table. However, rather than modifying
only the specified bits (0x1 in one direction, 0x2 in the other), the
entire ct_mark field will be replaced. Such connections will constantly
toggle the value of ct_mark between 0x1 and 0x2, rather than becoming
0x3 and keeping that value.
This commit fixes the issue by ensuring that set_field actions only
modify the modified bits in the wildcards, rather than masking the
entire field.
Fixes: 8e53fe8cf7a1 ("Add connection tracking mark support.")
Fixes: 9daf23484fb1 ("Add connection tracking label support.")
Signed-off-by: Joe Stringer <j...@ovn.org>
---
lib/meta-flow.c | 10 ++++++-
lib/meta-flow.h | 3 ++
ofproto/ofproto-dpif-xlate.c | 2 +-
tests/ofproto-dpif.at | 15 ++++++----
tests/system-traffic.at | 70 ++++++++++++++++++++++++++++++++++++++++++++
5 files changed, 92 insertions(+), 8 deletions(-)
diff --git a/lib/meta-flow.c b/lib/meta-flow.c
index 721152c406d7..32115be4101c 100644
--- a/lib/meta-flow.c
+++ b/lib/meta-flow.c
@@ -421,7 +421,15 @@ mf_are_prereqs_ok(const struct mf_field *mf, const struct
flow *flow)
void
mf_mask_field_and_prereqs(const struct mf_field *mf, struct flow_wildcards *wc)
{
- mf_set_flow_value(mf, &exact_match_mask, &wc->masks);
+ mf_mask_field_and_prereqs__(mf, &exact_match_mask, wc);
+}
+
+void
+mf_mask_field_and_prereqs__(const struct mf_field *mf,
+ const union mf_value *mask,
+ struct flow_wildcards *wc)
+{
+ mf_set_flow_value_masked(mf, &exact_match_mask, mask, &wc->masks);
switch (mf->prereqs) {
case MFP_ND:
diff --git a/lib/meta-flow.h b/lib/meta-flow.h
index c73a1afc6668..fb94e1734c4c 100644
--- a/lib/meta-flow.h
+++ b/lib/meta-flow.h
@@ -1985,6 +1985,9 @@ void mf_get_mask(const struct mf_field *, const struct
flow_wildcards *,
bool mf_are_prereqs_ok(const struct mf_field *, const struct flow *);
void mf_mask_field_and_prereqs(const struct mf_field *,
struct flow_wildcards *);
+void mf_mask_field_and_prereqs__(const struct mf_field *,
+ const union mf_value *,
+ struct flow_wildcards *);
void mf_bitmap_set_field_and_prereqs(const struct mf_field *mf, struct
mf_bitmap *bm);
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index a02dc24f6acd..9b5f174c65b5 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -4647,7 +4647,7 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t
ofpacts_len,
}
/* A flow may wildcard nw_frag. Do nothing if setting a transport
* header field on a packet that does not have them. */
- mf_mask_field_and_prereqs(mf, wc);
+ mf_mask_field_and_prereqs__(mf, &set_field->mask, wc);
if (mf_are_prereqs_ok(mf, flow)) {
mf_set_flow_value_masked(mf, &set_field->value,
&set_field->mask, flow);
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index da29ac2f2b1f..9ac2e2affaeb 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -3876,10 +3876,13 @@ for frag in 4000 6000 6008 4010; do
AT_CHECK([ovs-appctl netdev-dummy/receive p90 "0021853763af 0026b98cb0f9
0800 4500 003c 2e24 $frag 40 06 465d ac11370d ac11370b 828b 0016 751e267b
00000000 a002 16d0 1736 0000 02 04 05 b4 04 02 08 0a 2d 25 08 5f 00 00 00 00 01
03 03 07"])
done
+dnl The set_field action only modifies 8 bits of the tcp_src, so both the flow
+dnl wildcard and the set_field action have a mask of 0xFF. Up to (including)
+dnl OVS-2.5, the wildcards and set_field mask are shared internally.
AT_CHECK([ovs-appctl dpctl/dump-flows | sed
's/used:[[0-9]].[[0-9]]*s/used:0.001s/'], [0], [dnl
flow-dump from non-dpdk interfaces:
-recirc_id(0),in_port(90),eth_type(0x0800),ipv4(proto=6,frag=no),tcp(src=33419),
packets:0, bytes:0, used:never, actions:set(tcp(src=33322)),1
-recirc_id(0),in_port(90),eth_type(0x0800),ipv4(proto=6,frag=first),tcp(src=33419),
packets:0, bytes:0, used:never, actions:set(tcp(src=33322)),1
+recirc_id(0),in_port(90),eth_type(0x0800),ipv4(proto=6,frag=no),tcp(src=33419/0xff),
packets:0, bytes:0, used:never, actions:set(tcp(src=42/0xff)),1
+recirc_id(0),in_port(90),eth_type(0x0800),ipv4(proto=6,frag=first),tcp(src=33419/0xff),
packets:0, bytes:0, used:never, actions:set(tcp(src=42/0xff)),1
recirc_id(0),in_port(90),eth_type(0x0800),ipv4(proto=6,frag=later), packets:1,
bytes:74, used:0.001s, actions:1
])
@@ -3893,8 +3896,8 @@ done
AT_CHECK([ovs-appctl dpctl/dump-flows | sed
's/used:[[0-9]].[[0-9]]*s/used:0.001s/'], [0], [dnl
flow-dump from non-dpdk interfaces:
-recirc_id(0),in_port(90),eth_type(0x0800),ipv4(proto=6,frag=no),tcp(src=0),
packets:0, bytes:0, used:never, actions:set(tcp(src=42)),1
-recirc_id(0),in_port(90),eth_type(0x0800),ipv4(proto=6,frag=first),tcp(src=0),
packets:0, bytes:0, used:never, actions:set(tcp(src=42)),1
+recirc_id(0),in_port(90),eth_type(0x0800),ipv4(proto=6,frag=no),tcp(src=0/0xff),
packets:0, bytes:0, used:never, actions:set(tcp(src=42/0xff)),1
+recirc_id(0),in_port(90),eth_type(0x0800),ipv4(proto=6,frag=first),tcp(src=0/0xff),
packets:0, bytes:0, used:never, actions:set(tcp(src=42/0xff)),1
recirc_id(0),in_port(90),eth_type(0x0800),ipv4(proto=6,frag=later), packets:1,
bytes:60, used:0.001s, actions:1
])
@@ -3908,8 +3911,8 @@ done
AT_CHECK([ovs-appctl dpctl/dump-flows | sed
's/used:[[0-9]].[[0-9]]*s/used:0.001s/'], [0], [dnl
flow-dump from non-dpdk interfaces:
-recirc_id(0),in_port(90),eth_type(0x0800),ipv4(proto=6,frag=no),tcp(src=0),
packets:0, bytes:0, used:never, actions:set(tcp(src=42)),1
-recirc_id(0),in_port(90),eth_type(0x0800),ipv4(proto=6,frag=first),tcp(src=0),
packets:0, bytes:0, used:never, actions:set(tcp(src=42)),1
+recirc_id(0),in_port(90),eth_type(0x0800),ipv4(proto=6,frag=no),tcp(src=0/0xff),
packets:0, bytes:0, used:never, actions:set(tcp(src=42/0xff)),1
+recirc_id(0),in_port(90),eth_type(0x0800),ipv4(proto=6,frag=first),tcp(src=0/0xff),
packets:0, bytes:0, used:never, actions:set(tcp(src=42/0xff)),1
recirc_id(0),in_port(90),eth_type(0x0800),ipv4(proto=6,frag=later), packets:1,
bytes:60, used:0.001s, actions:1
])
diff --git a/tests/system-traffic.at b/tests/system-traffic.at
index 28adbdcb9ee6..fcffc462d4d1 100644
--- a/tests/system-traffic.at
+++ b/tests/system-traffic.at
@@ -770,6 +770,41 @@
tcp,orig=(src=10.1.1.3,dst=10.1.1.4,sport=<cleared>,dport=<cleared>),reply=(src=
OVS_TRAFFIC_VSWITCHD_STOP
AT_CLEANUP
+AT_SETUP([conntrack - ct_mark bit-fiddling])
+CHECK_CONNTRACK()
+OVS_TRAFFIC_VSWITCHD_START()
+
+ADD_NAMESPACES(at_ns0, at_ns1, at_ns2, at_ns3)
+
+ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24")
+ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24")
+
+dnl Allow traffic between ns0<->ns1 using the ct_mark. Return traffic should
+dnl cause an additional bit to be set in the connection (and be allowed).
+AT_DATA([flows.txt], [dnl
+table=0,priority=1,action=drop
+table=0,priority=10,arp,action=normal
+table=0,priority=10,icmp,action=normal
+table=0,priority=100,in_port=1,tcp,action=ct(table=1)
+table=0,priority=100,in_port=2,ct_state=-trk,tcp,action=ct(table=1,commit,exec(set_field:0x2/0x6->ct_mark))
+table=1,priority=100,in_port=1,ct_state=+new,tcp,action=ct(commit,exec(set_field:0x5/0x5->ct_mark)),2
+table=1,priority=100,in_port=1,ct_state=-new,tcp,action=2
+table=1,priority=100,in_port=2,ct_state=+trk,ct_mark=3,tcp,action=1
+])
+
+AT_CHECK([ovs-ofctl --bundle add-flows br0 flows.txt])
+
+dnl HTTP requests from p0->p1 should work fine.
+NETNS_DAEMONIZE([at_ns1], [[$PYTHON $srcdir/test-l7.py]], [http0.pid])
+NS_CHECK_EXEC([at_ns0], [wget 10.1.1.2 -t 3 -T 1 --retry-connrefused -v -o
wget0.log])
+
+AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.1.1.2) | grep TIME],
[0], [dnl
+tcp,orig=(src=10.1.1.1,dst=10.1.1.2,sport=<cleared>,dport=<cleared>),reply=(src=10.1.1.2,dst=10.1.1.1,sport=<cleared>,dport=<cleared>),mark=3,protoinfo=(state=TIME_WAIT)
+])
+
+OVS_TRAFFIC_VSWITCHD_STOP
+AT_CLEANUP
+
AT_SETUP([conntrack - ct_mark from register])
CHECK_CONNTRACK()
OVS_TRAFFIC_VSWITCHD_START()
@@ -855,6 +890,41 @@ NS_CHECK_EXEC([at_ns2], [wget 10.1.1.4 -t 3 -T 1 -v -o
wget1.log], [4])
OVS_TRAFFIC_VSWITCHD_STOP
AT_CLEANUP
+AT_SETUP([conntrack - ct_label bit-fiddling])
+CHECK_CONNTRACK()
+OVS_TRAFFIC_VSWITCHD_START()
+
+ADD_NAMESPACES(at_ns0, at_ns1, at_ns2, at_ns3)
+
+ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24")
+ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24")
+
+dnl Allow traffic between ns0<->ns1 using the ct_labels. Return traffic should
+dnl cause an additional bit to be set in the connection labels (and be allowed)
+AT_DATA([flows.txt], [dnl
+table=0,priority=1,action=drop
+table=0,priority=10,arp,action=normal
+table=0,priority=10,icmp,action=normal
+table=0,priority=100,in_port=1,tcp,action=ct(table=1)
+table=0,priority=100,in_port=2,ct_state=-trk,tcp,action=ct(table=1,commit,exec(set_field:0x200000000/0x200000004->ct_label))
+table=1,priority=100,in_port=1,tcp,ct_state=+new,action=ct(commit,exec(set_field:0x5/0x5->ct_label)),2
+table=1,priority=100,in_port=1,tcp,ct_state=-new,action=2
+table=1,priority=100,in_port=2,ct_state=+trk,ct_label=0x200000001,tcp,action=1
+])
+
+AT_CHECK([ovs-ofctl --bundle add-flows br0 flows.txt])
+
+dnl HTTP requests from p0->p1 should work fine.
+NETNS_DAEMONIZE([at_ns1], [[$PYTHON $srcdir/test-l7.py]], [http0.pid])
+NS_CHECK_EXEC([at_ns0], [wget 10.1.1.2 -t 3 -T 1 --retry-connrefused -v -o
wget0.log])
+
+AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.1.1.2) | grep TIME],
[0], [dnl
+tcp,orig=(src=10.1.1.1,dst=10.1.1.2,sport=<cleared>,dport=<cleared>),reply=(src=10.1.1.2,dst=10.1.1.1,sport=<cleared>,dport=<cleared>),labels=0x200000001,protoinfo=(state=TIME_WAIT)
+])
+
+OVS_TRAFFIC_VSWITCHD_STOP
+AT_CLEANUP
+
AT_SETUP([conntrack - ICMP related])
CHECK_CONNTRACK()
OVS_TRAFFIC_VSWITCHD_START()
--
2.1.4
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
