[ovs-dev] [PATCH 1/2] ofproto-dpif-xlate: xlate ct_{mark, label} correctly.
Joe Stringer
joe at ovn.org
Fri Apr 15 18:36:04 UTC 2016
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 at ovn.org>
---
lib/util.h | 11 +++++++++++
ofproto/ofproto-dpif-xlate.c | 27 ++++++++++++++++-----------
tests/system-traffic.at | 38 ++++++++++++++++++++++++++++++++++++++
3 files changed, 65 insertions(+), 11 deletions(-)
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()
--
2.1.4
More information about the dev
mailing list