diff mbox

[ovs-dev,1/2] ofproto-dpif-xlate: xlate ct_{mark, label} correctly.

Message ID 1460745365-29259-1-git-send-email-joe@ovn.org
State Accepted
Headers show

Commit Message

Joe Stringer April 15, 2016, 6:36 p.m. UTC 
When translating multiple ct actions in a row which include modification
of ct_mark or ct_labels, these fields could be incorrectly translated
into datapath actions, resulting in modification of these fields for
entries when the OpenFlow rules didn't actually specify the change.

For instance, the following OpenFlow actions:
ct(zone=1,commit,exec(set_field(1->ct_mark))),ct(zone=2,table=1),...

Would translate into the datapath actions:
ct(zone=1,commit,mark=1),ct(zone=2,mark=1),recirc(...),...

This commit fixes the issue by zeroing the wildcards for these fields
prior to performing nested actions translation (and restoring
afterwards). As such, these fields do not hold both the match and the
field modification values at the same time. As a result, the ct_mark and
ct_labels don't leak from one ct action to the next.

Fixes: 8e53fe8cf7a1 ("Add connection tracking mark support.")
Fixes: 9daf23484fb1 ("Add connection tracking label support.")
Signed-off-by: Joe Stringer <joe@ovn.org>
---
 lib/util.h                   | 11 +++++++++++
 ofproto/ofproto-dpif-xlate.c | 27 ++++++++++++++++-----------
 tests/system-traffic.at      | 38 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 65 insertions(+), 11 deletions(-)

Comments

Ben Pfaff April 22, 2016, 3:37 p.m. UTC | #1 
On Fri, Apr 15, 2016 at 11:36:04AM -0700, Joe Stringer wrote:
> When translating multiple ct actions in a row which include modification
> of ct_mark or ct_labels, these fields could be incorrectly translated
> into datapath actions, resulting in modification of these fields for
> entries when the OpenFlow rules didn't actually specify the change.
> 
> For instance, the following OpenFlow actions:
> ct(zone=1,commit,exec(set_field(1->ct_mark))),ct(zone=2,table=1),...
> 
> Would translate into the datapath actions:
> ct(zone=1,commit,mark=1),ct(zone=2,mark=1),recirc(...),...
> 
> This commit fixes the issue by zeroing the wildcards for these fields
> prior to performing nested actions translation (and restoring
> afterwards). As such, these fields do not hold both the match and the
> field modification values at the same time. As a result, the ct_mark and
> ct_labels don't leak from one ct action to the next.
> 
> Fixes: 8e53fe8cf7a1 ("Add connection tracking mark support.")
> Fixes: 9daf23484fb1 ("Add connection tracking label support.")
> Signed-off-by: Joe Stringer <joe@ovn.org>

I looked this over carefully and did not spot any problems.  Thank you!

Acked-by: Ben Pfaff <blp@ovn.org>
Joe Stringer April 22, 2016, 10:48 p.m. UTC | #2 
On 22 April 2016 at 08:37, Ben Pfaff <blp@ovn.org> wrote:
> On Fri, Apr 15, 2016 at 11:36:04AM -0700, Joe Stringer wrote:
>> When translating multiple ct actions in a row which include modification
>> of ct_mark or ct_labels, these fields could be incorrectly translated
>> into datapath actions, resulting in modification of these fields for
>> entries when the OpenFlow rules didn't actually specify the change.
>>
>> For instance, the following OpenFlow actions:
>> ct(zone=1,commit,exec(set_field(1->ct_mark))),ct(zone=2,table=1),...
>>
>> Would translate into the datapath actions:
>> ct(zone=1,commit,mark=1),ct(zone=2,mark=1),recirc(...),...
>>
>> This commit fixes the issue by zeroing the wildcards for these fields
>> prior to performing nested actions translation (and restoring
>> afterwards). As such, these fields do not hold both the match and the
>> field modification values at the same time. As a result, the ct_mark and
>> ct_labels don't leak from one ct action to the next.
>>
>> Fixes: 8e53fe8cf7a1 ("Add connection tracking mark support.")
>> Fixes: 9daf23484fb1 ("Add connection tracking label support.")
>> Signed-off-by: Joe Stringer <joe@ovn.org>
>
> I looked this over carefully and did not spot any problems.  Thank you!
>
> Acked-by: Ben Pfaff <blp@ovn.org>

Thanks, I applied this patch to master and branch-2.5.
diff mbox

Patch

diff --git a/lib/util.h b/lib/util.h
index 55e2c6722623..a9082678c635 100644
--- a/lib/util.h
+++ b/lib/util.h
@@ -424,6 +424,17 @@  ovs_be128_is_zero(const ovs_be128 *val)
     return !(val->be64.hi || val->be64.lo);
 }
 
+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 5937913e3457..2d0d76912cc3 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -4282,29 +4282,28 @@  freeze_unroll_actions(const struct ofpact *a, const struct ofpact *end,
 }
 
 static void
-put_ct_mark(const struct flow *flow, struct flow *base_flow,
-            struct ofpbuf *odp_actions, struct flow_wildcards *wc)
+put_ct_mark(const struct flow *flow, struct ofpbuf *odp_actions,
+            struct flow_wildcards *wc)
 {
     struct {
         uint32_t key;
         uint32_t mask;
     } odp_attr;
 
-    odp_attr.key = flow->ct_mark;
+    odp_attr.key = flow->ct_mark & wc->masks.ct_mark;
     odp_attr.mask = wc->masks.ct_mark;
 
-    if (odp_attr.mask && odp_attr.key != base_flow->ct_mark) {
+    if (odp_attr.mask) {
         nl_msg_put_unspec(odp_actions, OVS_CT_ATTR_MARK, &odp_attr,
                           sizeof(odp_attr));
     }
 }
 
 static void
-put_ct_label(const struct flow *flow, struct flow *base_flow,
-             struct ofpbuf *odp_actions, struct flow_wildcards *wc)
+put_ct_label(const struct flow *flow, struct ofpbuf *odp_actions,
+             struct flow_wildcards *wc)
 {
-    if (!ovs_u128_is_zero(&wc->masks.ct_label)
-        && !ovs_u128_equals(&flow->ct_label, &base_flow->ct_label)) {
+    if (!ovs_u128_is_zero(&wc->masks.ct_label)) {
         struct {
             ovs_u128 key;
             ovs_u128 mask;
@@ -4313,7 +4312,7 @@  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->key = ovs_u128_and(flow->ct_label, wc->masks.ct_label);
         odp_ct_label->mask = wc->masks.ct_label;
     }
 }
@@ -4390,7 +4389,9 @@  static void
 compose_conntrack_action(struct xlate_ctx *ctx, struct ofpact_conntrack *ofc)
 {
     ovs_u128 old_ct_label = ctx->base_flow.ct_label;
+    ovs_u128 old_ct_label_mask = ctx->wc->masks.ct_label;
     uint32_t old_ct_mark = ctx->base_flow.ct_mark;
+    uint32_t old_ct_mark_mask = ctx->wc->masks.ct_mark;
     size_t ct_offset;
     uint16_t zone;
 
@@ -4400,6 +4401,8 @@  compose_conntrack_action(struct xlate_ctx *ctx, struct ofpact_conntrack *ofc)
 
     /* Process nested actions first, to populate the key. */
     ctx->ct_nat_action = NULL;
+    ctx->wc->masks.ct_mark = 0;
+    ctx->wc->masks.ct_label.u64.hi = ctx->wc->masks.ct_label.u64.lo = 0;
     do_xlate_actions(ofc->actions, ofpact_ct_get_action_len(ofc), ctx);
 
     if (ofc->zone_src.field) {
@@ -4413,8 +4416,8 @@  compose_conntrack_action(struct xlate_ctx *ctx, struct ofpact_conntrack *ofc)
         nl_msg_put_flag(ctx->odp_actions, OVS_CT_ATTR_COMMIT);
     }
     nl_msg_put_u16(ctx->odp_actions, OVS_CT_ATTR_ZONE, zone);
-    put_ct_mark(&ctx->xin->flow, &ctx->base_flow, ctx->odp_actions, ctx->wc);
-    put_ct_label(&ctx->xin->flow, &ctx->base_flow, ctx->odp_actions, ctx->wc);
+    put_ct_mark(&ctx->xin->flow, ctx->odp_actions, ctx->wc);
+    put_ct_label(&ctx->xin->flow, ctx->odp_actions, ctx->wc);
     put_ct_helper(ctx->odp_actions, ofc);
     put_ct_nat(ctx);
     ctx->ct_nat_action = NULL;
@@ -4423,7 +4426,9 @@  compose_conntrack_action(struct xlate_ctx *ctx, struct ofpact_conntrack *ofc)
     /* Restore the original ct fields in the key. These should only be exposed
      * after recirculation to another table. */
     ctx->base_flow.ct_mark = old_ct_mark;
+    ctx->wc->masks.ct_mark = old_ct_mark_mask;
     ctx->base_flow.ct_label = old_ct_label;
+    ctx->wc->masks.ct_label = old_ct_label_mask;
 
     if (ofc->recirc_table == NX_CT_RECIRC_NONE) {
         /* If we do not recirculate as part of this action, hide the results of
diff --git a/tests/system-traffic.at b/tests/system-traffic.at
index dceae150d148..5eb6bcff66df 100644
--- a/tests/system-traffic.at
+++ b/tests/system-traffic.at
@@ -925,6 +925,44 @@  tcp,orig=(src=10.1.1.1,dst=10.1.1.2,sport=<cleared>,dport=<cleared>),reply=(src=
 OVS_TRAFFIC_VSWITCHD_STOP
 AT_CLEANUP
 
+AT_SETUP([conntrack - ct metadata, multiple zones])
+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 and ct_labels in zone=1,
+dnl but do *not* set any of these for the ct() in zone=2. Traffic should pass,
+dnl and we should see that the conntrack entries only apply the ct_mark and
+dnl ct_labels to the connection in zone=1.
+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(zone=1,table=1)
+table=0,priority=100,in_port=2,ct_state=-trk,tcp,action=ct(zone=1,table=1,commit,exec(set_field:0x200000000/0x200000004->ct_label,set_field:0x2/0x6->ct_mark))
+table=1,priority=100,in_port=1,tcp,ct_state=+new,action=ct(zone=1,commit,exec(set_field:0x5/0x5->ct_label,set_field:0x5/0x5->ct_mark)),ct(commit,zone=2),2
+table=1,priority=100,in_port=1,tcp,ct_state=-new,action=ct(zone=2),2
+table=1,priority=100,in_port=2,tcp,action=ct(zone=2),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>),zone=1,mark=3,labels=0x200000001,protoinfo=(state=TIME_WAIT)
+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>),zone=2,protoinfo=(state=TIME_WAIT)
+])
+
+OVS_TRAFFIC_VSWITCHD_STOP
+AT_CLEANUP
+
 AT_SETUP([conntrack - ICMP related])
 CHECK_CONNTRACK()
 OVS_TRAFFIC_VSWITCHD_START()