From patchwork Mon Apr 4 21:56:03 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Joe Stringer X-Patchwork-Id: 605997 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from archives.nicira.com (archives.nicira.com [96.126.127.54]) by ozlabs.org (Postfix) with ESMTP id 3qf5Rv0x0Sz9sD5 for ; Tue, 5 Apr 2016 07:56:59 +1000 (AEST) Received: from archives.nicira.com (localhost [127.0.0.1]) by archives.nicira.com (Postfix) with ESMTP id 59B76102DF; Mon, 4 Apr 2016 14:56:55 -0700 (PDT) X-Original-To: dev@openvswitch.org Delivered-To: dev@openvswitch.org Received: from mx3v3.cudamail.com (mx3.cudamail.com [64.34.241.5]) by archives.nicira.com (Postfix) with ESMTPS id 2D1ED102C5 for ; Mon, 4 Apr 2016 14:56:54 -0700 (PDT) Received: from bar6.cudamail.com (localhost [127.0.0.1]) by mx3v3.cudamail.com (Postfix) with ESMTPS id B7356161A6E for ; Mon, 4 Apr 2016 15:56:53 -0600 (MDT) X-ASG-Debug-ID: 1459807011-0b3237353354e860001-byXFYA Received: from mx1-pf2.cudamail.com ([192.168.24.2]) by bar6.cudamail.com with ESMTP id r34R0ggc3CmqYGng (version=TLSv1 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO) for ; Mon, 04 Apr 2016 15:56:51 -0600 (MDT) X-Barracuda-Envelope-From: joe@ovn.org X-Barracuda-RBL-Trusted-Forwarder: 192.168.24.2 Received: from unknown (HELO relay6-d.mail.gandi.net) (217.70.183.198) by mx1-pf2.cudamail.com with ESMTPS (DHE-RSA-AES256-SHA encrypted); 4 Apr 2016 21:56:51 -0000 Received-SPF: pass (mx1-pf2.cudamail.com: SPF record at ovn.org designates 217.70.183.198 as permitted sender) X-Barracuda-Apparent-Source-IP: 217.70.183.198 X-Barracuda-RBL-IP: 217.70.183.198 Received: from mfilter10-d.gandi.net (mfilter10-d.gandi.net [217.70.178.139]) by relay6-d.mail.gandi.net (Postfix) with ESMTP id 9941DFB8A4; Mon, 4 Apr 2016 23:56:48 +0200 (CEST) X-Virus-Scanned: Debian amavisd-new at mfilter10-d.gandi.net Received: from relay6-d.mail.gandi.net ([IPv6:::ffff:217.70.183.198]) by mfilter10-d.gandi.net (mfilter10-d.gandi.net [::ffff:10.0.15.180]) (amavisd-new, port 10024) with ESMTP id wIUHuhAGhZ9v; Mon, 4 Apr 2016 23:56:46 +0200 (CEST) X-Originating-IP: 208.91.1.34 Received: from localhost.localdomain (unknown [208.91.1.34]) (Authenticated sender: joe@ovn.org) by relay6-d.mail.gandi.net (Postfix) with ESMTPSA id 9171FFB881; Mon, 4 Apr 2016 23:56:44 +0200 (CEST) X-CudaMail-Envelope-Sender: joe@ovn.org From: Joe Stringer To: dev@openvswitch.org X-CudaMail-Whitelist-To: dev@openvswitch.org X-CudaMail-MID: CM-E2-403080080 X-CudaMail-DTE: 040416 X-CudaMail-Originating-IP: 217.70.183.198 Date: Mon, 4 Apr 2016 14:56:03 -0700 X-ASG-Orig-Subj: [##CM-E2-403080080##][PATCHv3 1/2] ofproto-dpif-xlate: Generate bitmasks in set_field. Message-Id: <1459806964-28523-1-git-send-email-joe@ovn.org> X-Mailer: git-send-email 2.1.4 X-Barracuda-Connect: UNKNOWN[192.168.24.2] X-Barracuda-Start-Time: 1459807011 X-Barracuda-Encrypted: DHE-RSA-AES256-SHA X-Barracuda-URL: https://web.cudamail.com:443/cgi-mod/mark.cgi X-ASG-Whitelist: Header =?UTF-8?B?eFwtY3VkYW1haWxcLXdoaXRlbGlzdFwtdG8=?= X-Virus-Scanned: by bsmtpd at cudamail.com X-Barracuda-BRTS-Status: 1 Subject: [ovs-dev] [PATCHv3 1/2] ofproto-dpif-xlate: Generate bitmasks in set_field. X-BeenThere: dev@openvswitch.org X-Mailman-Version: 2.1.16 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Errors-To: dev-bounces@openvswitch.org Sender: "dev" 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 Acked-by: Ben Pfaff --- 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=,dport=),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=,dport=),reply=(src=10.1.1.2,dst=10.1.1.1,sport=,dport=),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=,dport=),reply=(src=10.1.1.2,dst=10.1.1.1,sport=,dport=),labels=0x200000001,protoinfo=(state=TIME_WAIT) +]) + +OVS_TRAFFIC_VSWITCHD_STOP +AT_CLEANUP + AT_SETUP([conntrack - ICMP related]) CHECK_CONNTRACK() OVS_TRAFFIC_VSWITCHD_START()