| Message ID | 1459806964-28523-1-git-send-email-joe@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
On 4 April 2016 at 14:56, Joe Stringer <joe@ovn.org> wrote: > 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 <joe@ovn.org> A couple of questions came up around this offline, which I looked into: * How does this work for OpenFlow versions that don't support masked set_field? OpenFlow versions which can't express masks for field modification will have an all-1s mask generated during OpenFlow deserialization. So, internally here we will copy from an all-1s field and the effective behaviour is the same. * What happens when you use this with an older datapath that doesn't support masked set action? During translation, if the datapath doesn't support masked set actions then full field modification actions will be written. This is effectively the same as today's behaviour for all non-ct fields (but occurring at the next layer down rather than during action translation). For ct_labels/ct_mark, as these are not encoded in the set(...) action in the datapath, so it could not introduce a regression for those fields; masked set action is not a requirement to provide the correct behaviour in ct_{mark,label} modification.
On Tue, Apr 12, 2016 at 02:23:47PM -0700, Joe Stringer wrote: > On 4 April 2016 at 14:56, Joe Stringer <joe@ovn.org> wrote: > > 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 <joe@ovn.org> > > A couple of questions came up around this offline, which I looked into: > > * How does this work for OpenFlow versions that don't support masked set_field? Is this a real issue? Open vSwitch effectively supports masked set_field on older versions of OpenFlow by encoding them as an equivalent series of one or more NXAST_REG_LOAD operations. > OpenFlow versions which can't express masks for field modification > will have an all-1s mask generated during OpenFlow deserialization. > So, internally here we will copy from an all-1s field and the > effective behaviour is the same.
On 12 April 2016 at 21:19, Ben Pfaff <blp@ovn.org> wrote: > On Tue, Apr 12, 2016 at 02:23:47PM -0700, Joe Stringer wrote: >> On 4 April 2016 at 14:56, Joe Stringer <joe@ovn.org> wrote: >> > 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 <joe@ovn.org> >> >> A couple of questions came up around this offline, which I looked into: >> >> * How does this work for OpenFlow versions that don't support masked set_field? > > Is this a real issue? Open vSwitch effectively supports masked > set_field on older versions of OpenFlow by encoding them as an > equivalent series of one or more NXAST_REG_LOAD operations. Not really; I think the concern was more: if a set_field mask is not expressible in the OF protocol version, then is the mask populated correctly with this change - ie full mask? (Yes, it is).
On Mon, Apr 04, 2016 at 02:56:03PM -0700, Joe Stringer wrote: > 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 <joe@ovn.org> Acked-by: Ben Pfaff <blp@ovn.org>
On 14 April 2016 at 10:08, Ben Pfaff <blp@ovn.org> wrote: > On Mon, Apr 04, 2016 at 02:56:03PM -0700, Joe Stringer wrote: >> 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 <joe@ovn.org> > > Acked-by: Ben Pfaff <blp@ovn.org> Thanks, I applied this to master and branch-2.5.
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=<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 +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=<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() @@ -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=<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()
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 <joe@ovn.org> --- 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(-)