| Message ID | 1459336874-2956-1-git-send-email-joe@ovn.org |
|---|---|
| State | Superseded |
| Headers | show |
On Thu, Mar 31, 2016 at 12:21:13AM +1300, Joe Stringer wrote: > Using the action ct(commit,set_field:0x1/0x1->ct_mark), ie, specifying a > mask, would previously overwrite the entire ct_mark field rather than > modifying only the specified bits. Fix the issue. > > Fixes: 8e53fe8cf7a1 ("Add connection tracking mark support.") > Signed-off-by: Joe Stringer <joe@ovn.org> > --- > v2: Remove wildcards from put_ct_mark(). I didn't test this but it looks good and compiles cleanly. A possible (though unnecessary) enhancement to the test would be to verify that setting a bit to 0 also works. Acked-by: Ben Pfaff <blp@ovn.org>
On 30 March 2016 at 19:55, Ben Pfaff <blp@ovn.org> wrote: > On Thu, Mar 31, 2016 at 12:21:13AM +1300, Joe Stringer wrote: >> Using the action ct(commit,set_field:0x1/0x1->ct_mark), ie, specifying a >> mask, would previously overwrite the entire ct_mark field rather than >> modifying only the specified bits. Fix the issue. >> >> Fixes: 8e53fe8cf7a1 ("Add connection tracking mark support.") >> Signed-off-by: Joe Stringer <joe@ovn.org> >> --- >> v2: Remove wildcards from put_ct_mark(). > > I didn't test this but it looks good and compiles cleanly. > > A possible (though unnecessary) enhancement to the test would be to > verify that setting a bit to 0 also works. > > Acked-by: Ben Pfaff <blp@ovn.org> Thanks for the review. Actually, the "set-0" test is necessary and reveals that this series isn't really targeting the core issue. I was a bit confused about how 'wc' were being used and what's the 'correct' way to handle these. After discussion with Jarno I understand that the 'wc' represents both the incoming match on a given field, but also the bits that have been modified in a 'set_field' action. Furthermore, as soon as a 'set_field' action is translated, the field is fully masked. This is actually what leads to the incorrect behaviour for setting the ct_mark/ct_labels. Even when modifying just a single bit of a field, the entire field is marked to change in the 'wc'. I'm looking at preparing a patch which will only mask the bits which are modified by the set_field action.
On 4 April 2016 at 11:20, Joe Stringer <joe@ovn.org> wrote: > On 30 March 2016 at 19:55, Ben Pfaff <blp@ovn.org> wrote: >> On Thu, Mar 31, 2016 at 12:21:13AM +1300, Joe Stringer wrote: >>> Using the action ct(commit,set_field:0x1/0x1->ct_mark), ie, specifying a >>> mask, would previously overwrite the entire ct_mark field rather than >>> modifying only the specified bits. Fix the issue. >>> >>> Fixes: 8e53fe8cf7a1 ("Add connection tracking mark support.") >>> Signed-off-by: Joe Stringer <joe@ovn.org> >>> --- >>> v2: Remove wildcards from put_ct_mark(). >> >> I didn't test this but it looks good and compiles cleanly. >> >> A possible (though unnecessary) enhancement to the test would be to >> verify that setting a bit to 0 also works. >> >> Acked-by: Ben Pfaff <blp@ovn.org> > > Thanks for the review. > > Actually, the "set-0" test is necessary and reveals that this series > isn't really targeting the core issue. > > I was a bit confused about how 'wc' were being used and what's the > 'correct' way to handle these. After discussion with Jarno I > understand that the 'wc' represents both the incoming match on a given > field, but also the bits that have been modified in a 'set_field' > action. Furthermore, as soon as a 'set_field' action is translated, > the field is fully masked. This is actually what leads to the > incorrect behaviour for setting the ct_mark/ct_labels. Even when > modifying just a single bit of a field, the entire field is marked to > change in the 'wc'. > > I'm looking at preparing a patch which will only mask the bits which > are modified by the set_field action. I should add that this bug only affects bitmask generation when performing ct(...,exec(set_field...)) on packets that haven't yet been through the connection tracker, eg: in_port=1,actions=ct(commit,exec(set_field(0x1/0x1->ct_mark))),2 in_port=2,actions=ct(commit,exec(set_field(0x2/0x2->ct_mark))),1 Connections flowing through this pipeline will constantly flip between having ct_mark=0x1 and ct_mark=0x2, rather than having ct_mark=0x3. I intend to drop this series in favour of a different fix.
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index 19e690ec1ecb..0c226201a580 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -4283,15 +4283,15 @@ 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) + struct ofpbuf *odp_actions) { struct { uint32_t key; uint32_t mask; } odp_attr; - odp_attr.key = flow->ct_mark; - odp_attr.mask = wc->masks.ct_mark; + odp_attr.mask = base_flow->ct_mark ^ flow->ct_mark; + odp_attr.key = flow->ct_mark & odp_attr.mask; if (odp_attr.mask && odp_attr.key != base_flow->ct_mark) { nl_msg_put_unspec(odp_actions, OVS_CT_ATTR_MARK, &odp_attr, @@ -4413,7 +4413,7 @@ 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_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_helper(ctx->odp_actions, ofc); put_ct_nat(ctx); diff --git a/tests/system-traffic.at b/tests/system-traffic.at index 28adbdcb9ee6..9d2c57faa6d7 100644 --- a/tests/system-traffic.at +++ b/tests/system-traffic.at @@ -770,6 +770,40 @@ 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 +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_mark)),2 +priority=100,in_port=2,ct_state=-trk,tcp,action=ct(table=0,commit,exec(set_field:0x2/0x2->ct_mark)) +priority=100,in_port=2,ct_state=+trk,ct_mark=1,tcp,action=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() diff --git a/utilities/ovs-ofctl.8.in b/utilities/ovs-ofctl.8.in index 6e2613207979..51a57f777b37 100644 --- a/utilities/ovs-ofctl.8.in +++ b/utilities/ovs-ofctl.8.in @@ -1725,7 +1725,7 @@ fields are accepted within the \fBexec\fR action, and these fields may only be modified with this option. For example: . .RS -.IP \fBset_field:\fIvalue\fR->ct_mark\fR +.IP \fBset_field:\fIvalue\fR[\fB/\fImask\fR]->ct_mark\fR 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
Using the action ct(commit,set_field:0x1/0x1->ct_mark), ie, specifying a mask, would previously overwrite the entire ct_mark field rather than modifying only the specified bits. Fix the issue. Fixes: 8e53fe8cf7a1 ("Add connection tracking mark support.") Signed-off-by: Joe Stringer <joe@ovn.org> --- v2: Remove wildcards from put_ct_mark(). --- ofproto/ofproto-dpif-xlate.c | 8 ++++---- tests/system-traffic.at | 34 ++++++++++++++++++++++++++++++++++ utilities/ovs-ofctl.8.in | 2 +- 3 files changed, 39 insertions(+), 5 deletions(-)