[ovs-dev] [PATCHv2 2/2] ofproto-dpif-xlate: Fix bitwise ops on ct_labels.

Joe Stringer joe at ovn.org
Wed Mar 30 11:21:14 UTC 2016


Using the action ct(commit,set_field:0x1/0x1->ct_label), ie, specifying
a mask, would previously overwrite the entire ct_labels field rather than
modifying only the specified bits. Fix the issue.

Fixes: 9daf23484fb1 ("Add connection tracking label support.")
Signed-off-by: Joe Stringer <joe at ovn.org>
---
v2: Remove wildcards from put_ct_label().
---
 lib/util.h                   | 22 ++++++++++++++++++++++
 ofproto/ofproto-dpif-xlate.c | 11 +++++------
 tests/system-traffic.at      | 34 ++++++++++++++++++++++++++++++++++
 utilities/ovs-ofctl.8.in     |  2 +-
 4 files changed, 62 insertions(+), 7 deletions(-)

diff --git a/lib/util.h b/lib/util.h
index 389c093a2fd7..ece0b552860f 100644
--- a/lib/util.h
+++ b/lib/util.h
@@ -611,6 +611,28 @@ ovs_be128_is_zero(const ovs_be128 *val)
     return !(val->be64.hi || val->be64.lo);
 }
 
+static inline ovs_u128
+ovs_u128_xor(const ovs_u128 a, const ovs_u128 b)
+{
+    ovs_u128 dst;
+
+    dst.u64.hi = a.u64.hi ^ b.u64.hi;
+    dst.u64.lo = a.u64.lo ^ b.u64.lo;
+
+    return dst;
+}
+
+static inline ovs_u128
+ovs_u128_and(const ovs_u128 a, const ovs_u128 b)
+{
+    ovs_u128 dst;
+
+    dst.u64.hi = a.u64.hi & b.u64.hi;
+    dst.u64.lo = a.u64.lo & b.u64.lo;
+
+    return dst;
+}
+
 void xsleep(unsigned int seconds);
 
 bool is_stdout_a_tty(void);
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 0c226201a580..f26e02940dee 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -4301,10 +4301,9 @@ put_ct_mark(const struct flow *flow, struct flow *base_flow,
 
 static void
 put_ct_label(const struct flow *flow, struct flow *base_flow,
-             struct ofpbuf *odp_actions, struct flow_wildcards *wc)
+             struct ofpbuf *odp_actions)
 {
-    if (!ovs_u128_is_zero(&wc->masks.ct_label)
-        && !ovs_u128_equals(&flow->ct_label, &base_flow->ct_label)) {
+    if (!ovs_u128_equals(&flow->ct_label, &base_flow->ct_label)) {
         struct {
             ovs_u128 key;
             ovs_u128 mask;
@@ -4313,8 +4312,8 @@ put_ct_label(const struct flow *flow, struct flow *base_flow,
         odp_ct_label = nl_msg_put_unspec_uninit(odp_actions,
                                                 OVS_CT_ATTR_LABELS,
                                                 sizeof(*odp_ct_label));
-        odp_ct_label->key = flow->ct_label;
-        odp_ct_label->mask = wc->masks.ct_label;
+        odp_ct_label->mask = ovs_u128_xor(base_flow->ct_label, flow->ct_label);
+        odp_ct_label->key = ovs_u128_and(flow->ct_label, odp_ct_label->mask);
     }
 }
 
@@ -4414,7 +4413,7 @@ compose_conntrack_action(struct xlate_ctx *ctx, struct ofpact_conntrack *ofc)
     }
     nl_msg_put_u16(ctx->odp_actions, OVS_CT_ATTR_ZONE, zone);
     put_ct_mark(&ctx->xin->flow, &ctx->base_flow, ctx->odp_actions);
-    put_ct_label(&ctx->xin->flow, &ctx->base_flow, ctx->odp_actions, ctx->wc);
+    put_ct_label(&ctx->xin->flow, &ctx->base_flow, ctx->odp_actions);
     put_ct_helper(ctx->odp_actions, ofc);
     put_ct_nat(ctx);
     ctx->ct_nat_action = NULL;
diff --git a/tests/system-traffic.at b/tests/system-traffic.at
index 9d2c57faa6d7..9c0068410d85 100644
--- a/tests/system-traffic.at
+++ b/tests/system-traffic.at
@@ -889,6 +889,40 @@ 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
+priority=1,action=drop
+priority=10,arp,action=normal
+priority=10,icmp,action=normal
+priority=100,in_port=1,tcp,action=ct(commit,exec(set_field:0x1/0x1->ct_label)),2
+priority=100,in_port=2,ct_state=-trk,tcp,action=ct(table=0,commit,exec(set_field:0x200000000/0x200000000->ct_label))
+priority=100,in_port=2,ct_state=+trk,ct_label=0x1,tcp,action=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()
diff --git a/utilities/ovs-ofctl.8.in b/utilities/ovs-ofctl.8.in
index 51a57f777b37..8188b5352349 100644
--- a/utilities/ovs-ofctl.8.in
+++ b/utilities/ovs-ofctl.8.in
@@ -1730,7 +1730,7 @@ Store a 32-bit metadata value with the connection. If the connection is
 committed, then subsequent lookups for packets in this connection will
 populate the \fBct_mark\fR flow field when the packet is sent to the
 connection tracker with the \fBtable\fR specified.
-.IP \fBset_field:\fIvalue\fR->ct_label\fR
+.IP \fBset_field:\fIvalue\fR[\fB/\fImask\fR]->ct_label\fR
 Store a 128-bit metadata value with the connection.  If the connection is
 committed, then subsequent lookups for packets in this connection will
 populate the \fBct_label\fR flow field when the packet is sent to the
-- 
2.1.4




More information about the dev mailing list