Re: [ovs-dev] [PATCH v2 04/15] conntrack: New userspace connection tracker.
On Wed, Apr 27, 2016 at 06:36:20AM +, Daniele Di Proietto wrote:
> Thank you for your comments, Flavio!
Thank you for the patchset :-)
>
> On 26/04/2016 16:35, "Flavio Leitner" wrote:
>
> >On Fri, Apr 15, 2016 at 05:02:36PM -0700, Daniele Di Proietto wrote:
[...]
> >> diff --git a/lib/conntrack-private.h b/lib/conntrack-private.h
> >> new file mode 100644
> >> index 000..e668c44
> >> --- /dev/null
> >> +++ b/lib/conntrack-private.h
> >> @@ -0,0 +1,77 @@
> >> +/*
> >> + * Copyright (c) 2015, 2016 Nicira, Inc.
> >> + *
> >> + * Licensed under the Apache License, Version 2.0 (the "License");
> >> + * you may not use this file except in compliance with the License.
> >> + * You may obtain a copy of the License at:
> >> + *
> >> + * http://www.apache.org/licenses/LICENSE-2.0
> >> + *
> >> + * Unless required by applicable law or agreed to in writing, software
> >> + * distributed under the License is distributed on an "AS IS" BASIS,
> >> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
> >> implied.
> >> + * See the License for the specific language governing permissions and
> >> + * limitations under the License.
> >> + */
> >> +
> >> +#ifndef CONNTRACK_PRIVATE_H
> >> +#define CONNTRACK_PRIVATE_H 1
> >> +
> >> +#include
> >> +#include
> >> +#include
> >> +
> >> +#include "hmap.h"
> >> +#include "openvswitch/types.h"
> >> +#include "packets.h"
> >> +#include "unaligned.h"
> >> +
> >> +struct ct_addr {
> >> +union {
> >> +ovs_16aligned_be32 ipv4;
> >> +union ovs_16aligned_in6_addr ipv6;
> >> +ovs_be32 ipv4_aligned;
> >> +struct in6_addr ipv6_aligned;
> >> +};
> >> +};
> >> +
> >> +struct ct_endpoint {
> >> +struct ct_addr addr;
> >> +ovs_be16 port;
> >> +};
> >> +
> >> +struct conn_key {
> >> +struct ct_endpoint src;
> >> +struct ct_endpoint dst;
> >> +
> >> +ovs_be16 dl_type;
> >> +uint8_t nw_proto;
> >> +uint16_t zone;
> >> +};
> >
> >Based on the above I presume we consider all IPs in the same space
> >regardless of the vlan, correct?
>
> Yes, I think this is what the kernel connection tracker does.
That's right.
> >> +
> >> +struct conn {
> >> +struct conn_key key;
> >> +struct conn_key rev_key;
> >> +long long expiration;
> >> +struct hmap_node node;
> >> +uint32_t mark;
> >> +ovs_u128 label;
> >> +};
> >> +
> >> +enum ct_update_res {
> >> +CT_UPDATE_INVALID,
> >> +CT_UPDATE_VALID,
> >> +CT_UPDATE_NEW,
> >> +};
> >> +
> >> +struct ct_l4_proto {
> >> +struct conn *(*new_conn)(struct dp_packet *pkt, long long now);
> >> +bool (*valid_new)(struct dp_packet *pkt);
> >> +enum ct_update_res (*conn_update)(struct conn *conn, struct dp_packet
> >> *pkt,
> >> + bool reply, long long now);
> >> +};
> >> +
> >> +extern struct ct_l4_proto ct_proto_tcp;
> >> +extern struct ct_l4_proto ct_proto_other;
> >> +
> >> +#endif /* conntrack-private.h */
> >> diff --git a/lib/conntrack-tcp.c b/lib/conntrack-tcp.c
> >> new file mode 100644
> >> index 000..4d80038
> >> --- /dev/null
> >> +++ b/lib/conntrack-tcp.c
> >> @@ -0,0 +1,476 @@
> >> +/*-
> >> + * Copyright (c) 2001 Daniel Hartmeier
> >> + * Copyright (c) 2002 - 2008 Henning Brauer
> >> + * Copyright (c) 2012 Gleb Smirnoff
> >> + * Copyright (c) 2015, 2016 Nicira, Inc.
> >> + * All rights reserved.
> >> + *
> >> + * Redistribution and use in source and binary forms, with or without
> >> + * modification, are permitted provided that the following conditions
> >> + * are met:
> >> + *
> >> + *- Redistributions of source code must retain the above copyright
> >> + * notice, this list of conditions and the following disclaimer.
> >> + *- Redistributions in binary form must reproduce the above
> >> + * copyright notice, this list of conditions and the following
> >> + * disclaimer in the documentation and/or other materials provided
> >> + * with the distribution.
> >> + *
> >> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> >> + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> >> + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS
> >> + * FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE
> >> + * COPYRIGHT HOLDERS OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT,
> >> + * INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING,
> >> + * BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
> >> + * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER
> >> + * CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
> >> + * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN
> >> + * ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
> >> + * POSSIBILITY OF SUCH DAMAGE.
> >> + *
> >> + * Effort sponsored in part by the Defense Advanced Research Projects
> >> + * Agency (DARPA) and Air Force Research
Re: [ovs-dev] [PATCH v2 04/15] conntrack: New userspace connection tracker.
Hi Daniele, few comments inline. > -Original Message- > From: Daniele Di Proietto [mailto:diproiet...@vmware.com] > Sent: Wednesday, April 27, 2016 7:36 AM > To: Fischetti, Antonio > Cc: dev@openvswitch.org > Subject: Re: [ovs-dev] [PATCH v2 04/15] conntrack: New userspace > connection tracker. > > Hi Antonio, > > Thank you for your comments, replies inline > > > > > On 25/04/2016 09:14, "Fischetti, Antonio" > wrote: > > >Hi Daniele, > >some comments inline prefixed with [Antonio F]. > > > >Regards, > >Antonio > > > >> -Original Message- > >> From: dev [mailto:dev-boun...@openvswitch.org] On Behalf Of > Daniele Di > >> Proietto > >> Sent: Saturday, April 16, 2016 1:03 AM > >> To: dev@openvswitch.org > >> Subject: [ovs-dev] [PATCH v2 04/15] conntrack: New userspace > connection > >> tracker. > >> > >> This commit adds the conntrack module. > >> > >> It is a connection tracker that resides entirely in userspace. > Its > >> primary user will be the dpif-netdev datapath. > >> > >> The module main goal is to provide conntrack_execute(), which > offers a > >> convenient interface to implement the datapath ct() action. > >> > >> The conntrack module uses two submodules to deal with the l4 > protocol > >> details (conntrack-other for UDP and ICMP, conntrack-tcp for TCP). > >> > >> The conntrack-tcp submodule implementation is adapted from > FreeBSD's pf > >> subsystem, therefore it's BSD licensed. It has been slightly > altered to > >> match the OVS coding style and to allow the pickup of already > >> established connections. > >> > >> Signed-off-by: Daniele Di Proietto > >> --- > >> COPYING | 1 + > >> debian/copyright.in | 4 + > >> lib/automake.mk | 5 + > >> lib/conntrack-other.c | 91 ++ > >> lib/conntrack-private.h | 77 + > >> lib/conntrack-tcp.c | 476 +++ > >> lib/conntrack.c | 851 > >> > >> lib/conntrack.h | 144 > >> 8 files changed, 1649 insertions(+) > >> create mode 100644 lib/conntrack-other.c > >> create mode 100644 lib/conntrack-private.h > >> create mode 100644 lib/conntrack-tcp.c > >> create mode 100644 lib/conntrack.c > >> create mode 100644 lib/conntrack.h > >> > >> diff --git a/COPYING b/COPYING > >> index 308e3ea..afb98b9 100644 > >> --- a/COPYING > >> +++ b/COPYING > >> @@ -25,6 +25,7 @@ License, version 2. > >> The following files are licensed under the 2-clause BSD license. > >> include/windows/getopt.h > >> lib/getopt_long.c > >> +lib/conntrack-tcp.c > >> > >> The following files are licensed under the 3-clause BSD-license > >> include/windows/netinet/icmp6.h > >> diff --git a/debian/copyright.in b/debian/copyright.in > >> index 57d007a..a15f4dd 100644 > >> --- a/debian/copyright.in > >> +++ b/debian/copyright.in > >> @@ -21,6 +21,9 @@ Upstream Copyright Holders: > >>Copyright (c) 2014 Michael Chapman > >>Copyright (c) 2014 WindRiver, Inc. > >>Copyright (c) 2014 Avaya, Inc. > >> + Copyright (c) 2001 Daniel Hartmeier > >> + Copyright (c) 2002 - 2008 Henning Brauer > >> + Copyright (c) 2012 Gleb Smirnoff > >> > >> License: > >> > >> @@ -90,6 +93,7 @@ License: > >>lib/getopt_long.c > >>include/windows/getopt.h > >>datapath-windows/ovsext/Conntrack-tcp.c > >> + lib/conntrack-tcp.c > >> > >> * The following files are licensed under the 3-clause BSD-license > >> > >> diff --git a/lib/automake.mk b/lib/automake.mk > >> index 1ec2115..ba30442 100644 > >> --- a/lib/automake.mk > >> +++ b/lib/automake.mk > >> @@ -47,6 +47,11 @@ lib_libopenvswitch_la_SOURCES = \ > >>lib/compiler.h \ > >>lib/connectivity.c \ > >>lib/connectivity.h \ > >> + lib/conntrack-private.h \ > >> + lib/conntrack-tcp.c \ > >> + lib/conntrack-other.c \ > >> + lib/conntrack.c \ > >> + lib/conntrack.h \ > >>lib/coverage.c \ > >>lib/coverage.h \ > >>lib/crc32c.c \ > >> diff --git a/lib/conntrack-other.c b/lib/connt
Re: [ovs-dev] [PATCH v2 04/15] conntrack: New userspace connection tracker.
Thank you for your comments, Flavio!
On 26/04/2016 16:35, "Flavio Leitner" wrote:
>On Fri, Apr 15, 2016 at 05:02:36PM -0700, Daniele Di Proietto wrote:
>> This commit adds the conntrack module.
>>
>> It is a connection tracker that resides entirely in userspace. Its
>> primary user will be the dpif-netdev datapath.
>>
>> The module main goal is to provide conntrack_execute(), which offers a
>> convenient interface to implement the datapath ct() action.
>>
>> The conntrack module uses two submodules to deal with the l4 protocol
>> details (conntrack-other for UDP and ICMP, conntrack-tcp for TCP).
>>
>> The conntrack-tcp submodule implementation is adapted from FreeBSD's pf
>> subsystem, therefore it's BSD licensed. It has been slightly altered to
>> match the OVS coding style and to allow the pickup of already
>> established connections.
>>
>> Signed-off-by: Daniele Di Proietto
>> ---
>> COPYING | 1 +
>> debian/copyright.in | 4 +
>> lib/automake.mk | 5 +
>> lib/conntrack-other.c | 91 ++
>> lib/conntrack-private.h | 77 +
>> lib/conntrack-tcp.c | 476 +++
>> lib/conntrack.c | 851
>>
>> lib/conntrack.h | 144
>> 8 files changed, 1649 insertions(+)
>> create mode 100644 lib/conntrack-other.c
>> create mode 100644 lib/conntrack-private.h
>> create mode 100644 lib/conntrack-tcp.c
>> create mode 100644 lib/conntrack.c
>> create mode 100644 lib/conntrack.h
>>
>> diff --git a/COPYING b/COPYING
>> index 308e3ea..afb98b9 100644
>> --- a/COPYING
>> +++ b/COPYING
>> @@ -25,6 +25,7 @@ License, version 2.
>> The following files are licensed under the 2-clause BSD license.
>> include/windows/getopt.h
>> lib/getopt_long.c
>> +lib/conntrack-tcp.c
>>
>> The following files are licensed under the 3-clause BSD-license
>> include/windows/netinet/icmp6.h
>> diff --git a/debian/copyright.in b/debian/copyright.in
>> index 57d007a..a15f4dd 100644
>> --- a/debian/copyright.in
>> +++ b/debian/copyright.in
>> @@ -21,6 +21,9 @@ Upstream Copyright Holders:
>> Copyright (c) 2014 Michael Chapman
>> Copyright (c) 2014 WindRiver, Inc.
>> Copyright (c) 2014 Avaya, Inc.
>> +Copyright (c) 2001 Daniel Hartmeier
>> +Copyright (c) 2002 - 2008 Henning Brauer
>> +Copyright (c) 2012 Gleb Smirnoff
>>
>> License:
>>
>> @@ -90,6 +93,7 @@ License:
>> lib/getopt_long.c
>> include/windows/getopt.h
>> datapath-windows/ovsext/Conntrack-tcp.c
>> +lib/conntrack-tcp.c
>>
>> * The following files are licensed under the 3-clause BSD-license
>>
>> diff --git a/lib/automake.mk b/lib/automake.mk
>> index 1ec2115..ba30442 100644
>> --- a/lib/automake.mk
>> +++ b/lib/automake.mk
>> @@ -47,6 +47,11 @@ lib_libopenvswitch_la_SOURCES = \
>> lib/compiler.h \
>> lib/connectivity.c \
>> lib/connectivity.h \
>> +lib/conntrack-private.h \
>> +lib/conntrack-tcp.c \
>> +lib/conntrack-other.c \
>> +lib/conntrack.c \
>> +lib/conntrack.h \
>> lib/coverage.c \
>> lib/coverage.h \
>> lib/crc32c.c \
>> diff --git a/lib/conntrack-other.c b/lib/conntrack-other.c
>> new file mode 100644
>> index 000..65d02a9
>> --- /dev/null
>> +++ b/lib/conntrack-other.c
>> @@ -0,0 +1,91 @@
>> +/*
>> + * Copyright (c) 2015, 2016 Nicira, Inc.
>> + *
>> + * Licensed under the Apache License, Version 2.0 (the "License");
>> + * you may not use this file except in compliance with the License.
>> + * You may obtain a copy of the License at:
>> + *
>> + * http://www.apache.org/licenses/LICENSE-2.0
>> + *
>> + * Unless required by applicable law or agreed to in writing, software
>> + * distributed under the License is distributed on an "AS IS" BASIS,
>> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
>> + * See the License for the specific language governing permissions and
>> + * limitations under the License.
>> + */
>> +
>> +#include
>> +
>> +#include "conntrack-private.h"
>> +#include "dp-packet.h"
>> +
>> +enum other_state {
>> +OTHERS_FIRST,
>> +OTHERS_MULTIPLE,
>> +OTHERS_BIDIR,
>> +};
>> +
>> +struct conn_other {
>> +struct conn up;
>> +enum other_state state;
>> +};
>> +
>> +static const long long other_timeouts[] = {
>> +[OTHERS_FIRST] = 60 * 1000,
>> +[OTHERS_MULTIPLE] = 60 * 1000,
>> +[OTHERS_BIDIR] = 30 * 1000,
>> +};
>
>I missed a description here, like the unit.
Right, I added a comment.
>
>> +
>> +static struct conn_other *
>> +conn_other_cast(const struct conn *conn)
>> +{
>> +return CONTAINER_OF(conn, struct conn_other, up);
>> +}
>> +
>> +static void
>> +update_expiration(struct conn_other *conn, long long now)
>> +{
>> +conn->up.expiration = now + other_timeouts[conn->state];
>> +}
>> +
>> +static enum ct_update_res
>> +other_conn_update(struct conn *conn_, struct dp_packet *pkt OVS_UNUSED,
>> +bo
Re: [ovs-dev] [PATCH v2 04/15] conntrack: New userspace connection tracker.
Hi Antonio, Thank you for your comments, replies inline On 25/04/2016 09:14, "Fischetti, Antonio" wrote: >Hi Daniele, >some comments inline prefixed with [Antonio F]. > >Regards, >Antonio > >> -Original Message- >> From: dev [mailto:dev-boun...@openvswitch.org] On Behalf Of Daniele Di >> Proietto >> Sent: Saturday, April 16, 2016 1:03 AM >> To: dev@openvswitch.org >> Subject: [ovs-dev] [PATCH v2 04/15] conntrack: New userspace connection >> tracker. >> >> This commit adds the conntrack module. >> >> It is a connection tracker that resides entirely in userspace. Its >> primary user will be the dpif-netdev datapath. >> >> The module main goal is to provide conntrack_execute(), which offers a >> convenient interface to implement the datapath ct() action. >> >> The conntrack module uses two submodules to deal with the l4 protocol >> details (conntrack-other for UDP and ICMP, conntrack-tcp for TCP). >> >> The conntrack-tcp submodule implementation is adapted from FreeBSD's pf >> subsystem, therefore it's BSD licensed. It has been slightly altered to >> match the OVS coding style and to allow the pickup of already >> established connections. >> >> Signed-off-by: Daniele Di Proietto >> --- >> COPYING | 1 + >> debian/copyright.in | 4 + >> lib/automake.mk | 5 + >> lib/conntrack-other.c | 91 ++ >> lib/conntrack-private.h | 77 + >> lib/conntrack-tcp.c | 476 +++ >> lib/conntrack.c | 851 >> >> lib/conntrack.h | 144 >> 8 files changed, 1649 insertions(+) >> create mode 100644 lib/conntrack-other.c >> create mode 100644 lib/conntrack-private.h >> create mode 100644 lib/conntrack-tcp.c >> create mode 100644 lib/conntrack.c >> create mode 100644 lib/conntrack.h >> >> diff --git a/COPYING b/COPYING >> index 308e3ea..afb98b9 100644 >> --- a/COPYING >> +++ b/COPYING >> @@ -25,6 +25,7 @@ License, version 2. >> The following files are licensed under the 2-clause BSD license. >> include/windows/getopt.h >> lib/getopt_long.c >> +lib/conntrack-tcp.c >> >> The following files are licensed under the 3-clause BSD-license >> include/windows/netinet/icmp6.h >> diff --git a/debian/copyright.in b/debian/copyright.in >> index 57d007a..a15f4dd 100644 >> --- a/debian/copyright.in >> +++ b/debian/copyright.in >> @@ -21,6 +21,9 @@ Upstream Copyright Holders: >> Copyright (c) 2014 Michael Chapman >> Copyright (c) 2014 WindRiver, Inc. >> Copyright (c) 2014 Avaya, Inc. >> +Copyright (c) 2001 Daniel Hartmeier >> +Copyright (c) 2002 - 2008 Henning Brauer >> +Copyright (c) 2012 Gleb Smirnoff >> >> License: >> >> @@ -90,6 +93,7 @@ License: >> lib/getopt_long.c >> include/windows/getopt.h >> datapath-windows/ovsext/Conntrack-tcp.c >> +lib/conntrack-tcp.c >> >> * The following files are licensed under the 3-clause BSD-license >> >> diff --git a/lib/automake.mk b/lib/automake.mk >> index 1ec2115..ba30442 100644 >> --- a/lib/automake.mk >> +++ b/lib/automake.mk >> @@ -47,6 +47,11 @@ lib_libopenvswitch_la_SOURCES = \ >> lib/compiler.h \ >> lib/connectivity.c \ >> lib/connectivity.h \ >> +lib/conntrack-private.h \ >> +lib/conntrack-tcp.c \ >> +lib/conntrack-other.c \ >> +lib/conntrack.c \ >> +lib/conntrack.h \ >> lib/coverage.c \ >> lib/coverage.h \ >> lib/crc32c.c \ >> diff --git a/lib/conntrack-other.c b/lib/conntrack-other.c >> new file mode 100644 >> index 000..65d02a9 >> --- /dev/null >> +++ b/lib/conntrack-other.c >> @@ -0,0 +1,91 @@ >> +/* >> + * Copyright (c) 2015, 2016 Nicira, Inc. >> + * >> + * Licensed under the Apache License, Version 2.0 (the "License"); >> + * you may not use this file except in compliance with the License. >> + * You may obtain a copy of the License at: >> + * >> + * http://www.apache.org/licenses/LICENSE-2.0 >> + * >> + * Unless required by applicable law or agreed to in writing, software >> + * distributed under the License is distributed on an "AS IS" BASIS, >> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or >> implied. >> + * See the License for the specific lang
Re: [ovs-dev] [PATCH v2 04/15] conntrack: New userspace connection tracker.
Thanks for the detailed review Joe, replies inline
On 19/04/2016 14:36, "Joe Stringer" wrote:
>On 15 April 2016 at 17:02, Daniele Di Proietto wrote:
>> This commit adds the conntrack module.
>>
>> It is a connection tracker that resides entirely in userspace. Its
>> primary user will be the dpif-netdev datapath.
>>
>> The module main goal is to provide conntrack_execute(), which offers a
>> convenient interface to implement the datapath ct() action.
>>
>> The conntrack module uses two submodules to deal with the l4 protocol
>> details (conntrack-other for UDP and ICMP, conntrack-tcp for TCP).
>>
>> The conntrack-tcp submodule implementation is adapted from FreeBSD's pf
>> subsystem, therefore it's BSD licensed. It has been slightly altered to
>> match the OVS coding style and to allow the pickup of already
>> established connections.
>>
>> Signed-off-by: Daniele Di Proietto
>
>Thanks for submitting this, I know there's a few people excited about
>this feature.
>
>I notice that there is no NEWS item about this, were you intending to
>hold off on announcing it until there is feature parity with the
>kernel, eg support for IPv[46] fragments; ALGs; NAT?
Actually I didn't think about this :-)
I'll add a line in NEWS
>
>
>> diff --git a/lib/conntrack-other.c b/lib/conntrack-other.c
>> new file mode 100644
>> index 000..65d02a9
>> --- /dev/null
>> +++ b/lib/conntrack-other.c
>> @@ -0,0 +1,91 @@
>> +/*
>> + * Copyright (c) 2015, 2016 Nicira, Inc.
>> + *
>> + * Licensed under the Apache License, Version 2.0 (the "License");
>> + * you may not use this file except in compliance with the License.
>> + * You may obtain a copy of the License at:
>> + *
>> + * http://www.apache.org/licenses/LICENSE-2.0
>> + *
>> + * Unless required by applicable law or agreed to in writing, software
>> + * distributed under the License is distributed on an "AS IS" BASIS,
>> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
>> + * See the License for the specific language governing permissions and
>> + * limitations under the License.
>> + */
>> +
>> +#include
>> +
>> +#include "conntrack-private.h"
>> +#include "dp-packet.h"
>> +
>> +enum other_state {
>> +OTHERS_FIRST,
>> +OTHERS_MULTIPLE,
>> +OTHERS_BIDIR,
>> +};
>> +
>> +struct conn_other {
>> +struct conn up;
>> +enum other_state state;
>> +};
>> +
>> +static const long long other_timeouts[] = {
>> +[OTHERS_FIRST] = 60 * 1000,
>> +[OTHERS_MULTIPLE] = 60 * 1000,
>> +[OTHERS_BIDIR] = 30 * 1000,
>> +};
>
>Are these timeouts in milliseconds? (a comment or #define might help with that)
Sure, I'll add a comment.
>
>
>
>> diff --git a/lib/conntrack.c b/lib/conntrack.c
>> new file mode 100644
>> index 000..840335b
>> --- /dev/null
>> +++ b/lib/conntrack.c
>...
>> +static struct ct_l4_proto *l4_protos[] = {
>> +[IPPROTO_TCP] = &ct_proto_tcp,
>> +[IPPROTO_UDP] = &ct_proto_other,
>> +[IPPROTO_ICMP] = &ct_proto_other,
>> +};
>
>I see that conntrack-other is shared by UDP and ICMP. In the Linux
>datapath, the UDP handler also checks UDP length and UDP checksum - I
>wonder if we can share most of the code here for these protocols but
>add these additional checks for the UDP case?
Thanks for pointing this out!
I think it's a good idea to check the UDP length, I've added a check later.
This didn't handle at all checksum verification. I thought it was out of
scope for the connection tracker, but since the kernel connection tracker
does this, I've added the validation.
None of this need (yet) a separate conntrack-icmp module, I'm implementing
it when the key is being extracted (as you point out later).
We would need a separate conntrack-icmp module if we wanted to track icmp
type and code.
>
>> +static struct conn *
>> +conn_not_found(struct conntrack *ct, struct dp_packet *pkt,
>> + struct conn_lookup_ctx *ctx, uint8_t *state, bool commit,
>> + long long now)
>> +{
>> +unsigned bucket = hash_to_bucket(ctx->hash);
>> +struct conn *nc = NULL;
>> +
>> +if (!valid_new(pkt, &ctx->key)) {
>> +*state |= CS_INVALID;
>> +return nc;
>> +}
>> +
>> +*state |= CS_NEW;
>> +
>> +if (commit) {
>> +nc = new_conn(pkt, &ctx->key, now);
>> +
>> +memcpy(&nc->rev_key, &ctx->key, sizeof nc->rev_key);
>> +
>> +conn_key_reverse(&nc->rev_key);
>> +hmap_insert(&ct->connections[bucket], &nc->node, ctx->hash);
>> +}
>> +
>> +return nc;
>> +}
>
>This function can return NULL
>
>> +static struct conn *
>> +process_one(struct conntrack *ct, struct dp_packet *pkt,
>> +struct conn_lookup_ctx *ctx, uint16_t zone,
>> +bool commit, long long now)
>> +{
>> +unsigned bucket = hash_to_bucket(ctx->hash);
>> +struct conn *conn = ctx->conn;
>> +uint8_t state = 0;
>> +
>> +if (conn) {
>> +if (ctx->related) {
>> +state |= CS_RELATED;
>> +if (ctx->reply) {
>> +
Re: [ovs-dev] [PATCH v2 04/15] conntrack: New userspace connection tracker.
On Fri, Apr 15, 2016 at 05:02:36PM -0700, Daniele Di Proietto wrote:
> This commit adds the conntrack module.
>
> It is a connection tracker that resides entirely in userspace. Its
> primary user will be the dpif-netdev datapath.
>
> The module main goal is to provide conntrack_execute(), which offers a
> convenient interface to implement the datapath ct() action.
>
> The conntrack module uses two submodules to deal with the l4 protocol
> details (conntrack-other for UDP and ICMP, conntrack-tcp for TCP).
>
> The conntrack-tcp submodule implementation is adapted from FreeBSD's pf
> subsystem, therefore it's BSD licensed. It has been slightly altered to
> match the OVS coding style and to allow the pickup of already
> established connections.
>
> Signed-off-by: Daniele Di Proietto
> ---
> COPYING | 1 +
> debian/copyright.in | 4 +
> lib/automake.mk | 5 +
> lib/conntrack-other.c | 91 ++
> lib/conntrack-private.h | 77 +
> lib/conntrack-tcp.c | 476 +++
> lib/conntrack.c | 851
>
> lib/conntrack.h | 144
> 8 files changed, 1649 insertions(+)
> create mode 100644 lib/conntrack-other.c
> create mode 100644 lib/conntrack-private.h
> create mode 100644 lib/conntrack-tcp.c
> create mode 100644 lib/conntrack.c
> create mode 100644 lib/conntrack.h
>
> diff --git a/COPYING b/COPYING
> index 308e3ea..afb98b9 100644
> --- a/COPYING
> +++ b/COPYING
> @@ -25,6 +25,7 @@ License, version 2.
> The following files are licensed under the 2-clause BSD license.
> include/windows/getopt.h
> lib/getopt_long.c
> +lib/conntrack-tcp.c
>
> The following files are licensed under the 3-clause BSD-license
> include/windows/netinet/icmp6.h
> diff --git a/debian/copyright.in b/debian/copyright.in
> index 57d007a..a15f4dd 100644
> --- a/debian/copyright.in
> +++ b/debian/copyright.in
> @@ -21,6 +21,9 @@ Upstream Copyright Holders:
> Copyright (c) 2014 Michael Chapman
> Copyright (c) 2014 WindRiver, Inc.
> Copyright (c) 2014 Avaya, Inc.
> + Copyright (c) 2001 Daniel Hartmeier
> + Copyright (c) 2002 - 2008 Henning Brauer
> + Copyright (c) 2012 Gleb Smirnoff
>
> License:
>
> @@ -90,6 +93,7 @@ License:
> lib/getopt_long.c
> include/windows/getopt.h
> datapath-windows/ovsext/Conntrack-tcp.c
> + lib/conntrack-tcp.c
>
> * The following files are licensed under the 3-clause BSD-license
>
> diff --git a/lib/automake.mk b/lib/automake.mk
> index 1ec2115..ba30442 100644
> --- a/lib/automake.mk
> +++ b/lib/automake.mk
> @@ -47,6 +47,11 @@ lib_libopenvswitch_la_SOURCES = \
> lib/compiler.h \
> lib/connectivity.c \
> lib/connectivity.h \
> + lib/conntrack-private.h \
> + lib/conntrack-tcp.c \
> + lib/conntrack-other.c \
> + lib/conntrack.c \
> + lib/conntrack.h \
> lib/coverage.c \
> lib/coverage.h \
> lib/crc32c.c \
> diff --git a/lib/conntrack-other.c b/lib/conntrack-other.c
> new file mode 100644
> index 000..65d02a9
> --- /dev/null
> +++ b/lib/conntrack-other.c
> @@ -0,0 +1,91 @@
> +/*
> + * Copyright (c) 2015, 2016 Nicira, Inc.
> + *
> + * Licensed under the Apache License, Version 2.0 (the "License");
> + * you may not use this file except in compliance with the License.
> + * You may obtain a copy of the License at:
> + *
> + * http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +
> +#include
> +
> +#include "conntrack-private.h"
> +#include "dp-packet.h"
> +
> +enum other_state {
> +OTHERS_FIRST,
> +OTHERS_MULTIPLE,
> +OTHERS_BIDIR,
> +};
> +
> +struct conn_other {
> +struct conn up;
> +enum other_state state;
> +};
> +
> +static const long long other_timeouts[] = {
> +[OTHERS_FIRST] = 60 * 1000,
> +[OTHERS_MULTIPLE] = 60 * 1000,
> +[OTHERS_BIDIR] = 30 * 1000,
> +};
I missed a description here, like the unit.
> +
> +static struct conn_other *
> +conn_other_cast(const struct conn *conn)
> +{
> +return CONTAINER_OF(conn, struct conn_other, up);
> +}
> +
> +static void
> +update_expiration(struct conn_other *conn, long long now)
> +{
> +conn->up.expiration = now + other_timeouts[conn->state];
> +}
> +
> +static enum ct_update_res
> +other_conn_update(struct conn *conn_, struct dp_packet *pkt OVS_UNUSED,
> +bool reply, long long now)
> +{
> +struct conn_other *conn = conn_other_cast(conn_);
> +
> +if (reply && conn->state != OTHERS_BIDIR) {
> +conn->state = OTHERS_BIDIR;
> +} else if (conn->state == OTHERS_FIRST) {
> +
Re: [ovs-dev] [PATCH v2 04/15] conntrack: New userspace connection tracker.
Hi Daniele,
some comments inline prefixed with [Antonio F].
Regards,
Antonio
> -Original Message-
> From: dev [mailto:dev-boun...@openvswitch.org] On Behalf Of Daniele Di
> Proietto
> Sent: Saturday, April 16, 2016 1:03 AM
> To: dev@openvswitch.org
> Subject: [ovs-dev] [PATCH v2 04/15] conntrack: New userspace connection
> tracker.
>
> This commit adds the conntrack module.
>
> It is a connection tracker that resides entirely in userspace. Its
> primary user will be the dpif-netdev datapath.
>
> The module main goal is to provide conntrack_execute(), which offers a
> convenient interface to implement the datapath ct() action.
>
> The conntrack module uses two submodules to deal with the l4 protocol
> details (conntrack-other for UDP and ICMP, conntrack-tcp for TCP).
>
> The conntrack-tcp submodule implementation is adapted from FreeBSD's pf
> subsystem, therefore it's BSD licensed. It has been slightly altered to
> match the OVS coding style and to allow the pickup of already
> established connections.
>
> Signed-off-by: Daniele Di Proietto
> ---
> COPYING | 1 +
> debian/copyright.in | 4 +
> lib/automake.mk | 5 +
> lib/conntrack-other.c | 91 ++
> lib/conntrack-private.h | 77 +
> lib/conntrack-tcp.c | 476 +++
> lib/conntrack.c | 851
>
> lib/conntrack.h | 144
> 8 files changed, 1649 insertions(+)
> create mode 100644 lib/conntrack-other.c
> create mode 100644 lib/conntrack-private.h
> create mode 100644 lib/conntrack-tcp.c
> create mode 100644 lib/conntrack.c
> create mode 100644 lib/conntrack.h
>
> diff --git a/COPYING b/COPYING
> index 308e3ea..afb98b9 100644
> --- a/COPYING
> +++ b/COPYING
> @@ -25,6 +25,7 @@ License, version 2.
> The following files are licensed under the 2-clause BSD license.
> include/windows/getopt.h
> lib/getopt_long.c
> +lib/conntrack-tcp.c
>
> The following files are licensed under the 3-clause BSD-license
> include/windows/netinet/icmp6.h
> diff --git a/debian/copyright.in b/debian/copyright.in
> index 57d007a..a15f4dd 100644
> --- a/debian/copyright.in
> +++ b/debian/copyright.in
> @@ -21,6 +21,9 @@ Upstream Copyright Holders:
> Copyright (c) 2014 Michael Chapman
> Copyright (c) 2014 WindRiver, Inc.
> Copyright (c) 2014 Avaya, Inc.
> + Copyright (c) 2001 Daniel Hartmeier
> + Copyright (c) 2002 - 2008 Henning Brauer
> + Copyright (c) 2012 Gleb Smirnoff
>
> License:
>
> @@ -90,6 +93,7 @@ License:
> lib/getopt_long.c
> include/windows/getopt.h
> datapath-windows/ovsext/Conntrack-tcp.c
> + lib/conntrack-tcp.c
>
> * The following files are licensed under the 3-clause BSD-license
>
> diff --git a/lib/automake.mk b/lib/automake.mk
> index 1ec2115..ba30442 100644
> --- a/lib/automake.mk
> +++ b/lib/automake.mk
> @@ -47,6 +47,11 @@ lib_libopenvswitch_la_SOURCES = \
> lib/compiler.h \
> lib/connectivity.c \
> lib/connectivity.h \
> + lib/conntrack-private.h \
> + lib/conntrack-tcp.c \
> + lib/conntrack-other.c \
> + lib/conntrack.c \
> + lib/conntrack.h \
> lib/coverage.c \
> lib/coverage.h \
> lib/crc32c.c \
> diff --git a/lib/conntrack-other.c b/lib/conntrack-other.c
> new file mode 100644
> index 000..65d02a9
> --- /dev/null
> +++ b/lib/conntrack-other.c
> @@ -0,0 +1,91 @@
> +/*
> + * Copyright (c) 2015, 2016 Nicira, Inc.
> + *
> + * Licensed under the Apache License, Version 2.0 (the "License");
> + * you may not use this file except in compliance with the License.
> + * You may obtain a copy of the License at:
> + *
> + * http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
> implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +
> +#include
> +
> +#include "conntrack-private.h"
> +#include "dp-packet.h"
> +
> +enum other_state {
> +OTHERS_FIRST,
> +OTHERS_MULTIPLE,
> +OTHERS_BIDIR,
> +};
> +
> +struct conn_other {
> +struct conn up;
> +enum other_state state;
> +};
> +
> +static const long long other_timeouts[] = {
> +[OTHERS_FIRST] = 60 * 1000,
> +[OTHERS_MULTIPLE] = 60 * 1000,
> +[OTHERS_BIDIR] =
Re: [ovs-dev] [PATCH v2 04/15] conntrack: New userspace connection tracker.
On 15 April 2016 at 17:02, Daniele Di Proietto wrote:
> This commit adds the conntrack module.
>
> It is a connection tracker that resides entirely in userspace. Its
> primary user will be the dpif-netdev datapath.
>
> The module main goal is to provide conntrack_execute(), which offers a
> convenient interface to implement the datapath ct() action.
>
> The conntrack module uses two submodules to deal with the l4 protocol
> details (conntrack-other for UDP and ICMP, conntrack-tcp for TCP).
>
> The conntrack-tcp submodule implementation is adapted from FreeBSD's pf
> subsystem, therefore it's BSD licensed. It has been slightly altered to
> match the OVS coding style and to allow the pickup of already
> established connections.
>
> Signed-off-by: Daniele Di Proietto
Thanks for submitting this, I know there's a few people excited about
this feature.
I notice that there is no NEWS item about this, were you intending to
hold off on announcing it until there is feature parity with the
kernel, eg support for IPv[46] fragments; ALGs; NAT?
> diff --git a/lib/conntrack-other.c b/lib/conntrack-other.c
> new file mode 100644
> index 000..65d02a9
> --- /dev/null
> +++ b/lib/conntrack-other.c
> @@ -0,0 +1,91 @@
> +/*
> + * Copyright (c) 2015, 2016 Nicira, Inc.
> + *
> + * Licensed under the Apache License, Version 2.0 (the "License");
> + * you may not use this file except in compliance with the License.
> + * You may obtain a copy of the License at:
> + *
> + * http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +
> +#include
> +
> +#include "conntrack-private.h"
> +#include "dp-packet.h"
> +
> +enum other_state {
> +OTHERS_FIRST,
> +OTHERS_MULTIPLE,
> +OTHERS_BIDIR,
> +};
> +
> +struct conn_other {
> +struct conn up;
> +enum other_state state;
> +};
> +
> +static const long long other_timeouts[] = {
> +[OTHERS_FIRST] = 60 * 1000,
> +[OTHERS_MULTIPLE] = 60 * 1000,
> +[OTHERS_BIDIR] = 30 * 1000,
> +};
Are these timeouts in milliseconds? (a comment or #define might help with that)
> diff --git a/lib/conntrack.c b/lib/conntrack.c
> new file mode 100644
> index 000..840335b
> --- /dev/null
> +++ b/lib/conntrack.c
...
> +static struct ct_l4_proto *l4_protos[] = {
> +[IPPROTO_TCP] = &ct_proto_tcp,
> +[IPPROTO_UDP] = &ct_proto_other,
> +[IPPROTO_ICMP] = &ct_proto_other,
> +};
I see that conntrack-other is shared by UDP and ICMP. In the Linux
datapath, the UDP handler also checks UDP length and UDP checksum - I
wonder if we can share most of the code here for these protocols but
add these additional checks for the UDP case?
> +static struct conn *
> +conn_not_found(struct conntrack *ct, struct dp_packet *pkt,
> + struct conn_lookup_ctx *ctx, uint8_t *state, bool commit,
> + long long now)
> +{
> +unsigned bucket = hash_to_bucket(ctx->hash);
> +struct conn *nc = NULL;
> +
> +if (!valid_new(pkt, &ctx->key)) {
> +*state |= CS_INVALID;
> +return nc;
> +}
> +
> +*state |= CS_NEW;
> +
> +if (commit) {
> +nc = new_conn(pkt, &ctx->key, now);
> +
> +memcpy(&nc->rev_key, &ctx->key, sizeof nc->rev_key);
> +
> +conn_key_reverse(&nc->rev_key);
> +hmap_insert(&ct->connections[bucket], &nc->node, ctx->hash);
> +}
> +
> +return nc;
> +}
This function can return NULL
> +static struct conn *
> +process_one(struct conntrack *ct, struct dp_packet *pkt,
> +struct conn_lookup_ctx *ctx, uint16_t zone,
> +bool commit, long long now)
> +{
> +unsigned bucket = hash_to_bucket(ctx->hash);
> +struct conn *conn = ctx->conn;
> +uint8_t state = 0;
> +
> +if (conn) {
> +if (ctx->related) {
> +state |= CS_RELATED;
> +if (ctx->reply) {
> +state |= CS_REPLY_DIR;
> +}
> +} else {
> +enum ct_update_res res;
> +
> +res = conn_update(conn, pkt, ctx->reply, now);
> +
> +switch (res) {
> +case CT_UPDATE_VALID:
> +state |= CS_ESTABLISHED;
> +if (ctx->reply) {
> +state |= CS_REPLY_DIR;
> +}
> +break;
> +case CT_UPDATE_INVALID:
> +state |= CS_INVALID;
> +break;
> +case CT_UPDATE_NEW:
> +hmap_remove(&ct->connections[bucket], &conn->node);
> +delete_conn(conn);
> +conn = conn_not_found(ct, pkt, ctx, &state, commit, now);
...which is then stored here...
> +break;
> +}
> +}
> +
> +
[ovs-dev] [PATCH v2 04/15] conntrack: New userspace connection tracker.
This commit adds the conntrack module.
It is a connection tracker that resides entirely in userspace. Its
primary user will be the dpif-netdev datapath.
The module main goal is to provide conntrack_execute(), which offers a
convenient interface to implement the datapath ct() action.
The conntrack module uses two submodules to deal with the l4 protocol
details (conntrack-other for UDP and ICMP, conntrack-tcp for TCP).
The conntrack-tcp submodule implementation is adapted from FreeBSD's pf
subsystem, therefore it's BSD licensed. It has been slightly altered to
match the OVS coding style and to allow the pickup of already
established connections.
Signed-off-by: Daniele Di Proietto
---
COPYING | 1 +
debian/copyright.in | 4 +
lib/automake.mk | 5 +
lib/conntrack-other.c | 91 ++
lib/conntrack-private.h | 77 +
lib/conntrack-tcp.c | 476 +++
lib/conntrack.c | 851
lib/conntrack.h | 144
8 files changed, 1649 insertions(+)
create mode 100644 lib/conntrack-other.c
create mode 100644 lib/conntrack-private.h
create mode 100644 lib/conntrack-tcp.c
create mode 100644 lib/conntrack.c
create mode 100644 lib/conntrack.h
diff --git a/COPYING b/COPYING
index 308e3ea..afb98b9 100644
--- a/COPYING
+++ b/COPYING
@@ -25,6 +25,7 @@ License, version 2.
The following files are licensed under the 2-clause BSD license.
include/windows/getopt.h
lib/getopt_long.c
+lib/conntrack-tcp.c
The following files are licensed under the 3-clause BSD-license
include/windows/netinet/icmp6.h
diff --git a/debian/copyright.in b/debian/copyright.in
index 57d007a..a15f4dd 100644
--- a/debian/copyright.in
+++ b/debian/copyright.in
@@ -21,6 +21,9 @@ Upstream Copyright Holders:
Copyright (c) 2014 Michael Chapman
Copyright (c) 2014 WindRiver, Inc.
Copyright (c) 2014 Avaya, Inc.
+ Copyright (c) 2001 Daniel Hartmeier
+ Copyright (c) 2002 - 2008 Henning Brauer
+ Copyright (c) 2012 Gleb Smirnoff
License:
@@ -90,6 +93,7 @@ License:
lib/getopt_long.c
include/windows/getopt.h
datapath-windows/ovsext/Conntrack-tcp.c
+ lib/conntrack-tcp.c
* The following files are licensed under the 3-clause BSD-license
diff --git a/lib/automake.mk b/lib/automake.mk
index 1ec2115..ba30442 100644
--- a/lib/automake.mk
+++ b/lib/automake.mk
@@ -47,6 +47,11 @@ lib_libopenvswitch_la_SOURCES = \
lib/compiler.h \
lib/connectivity.c \
lib/connectivity.h \
+ lib/conntrack-private.h \
+ lib/conntrack-tcp.c \
+ lib/conntrack-other.c \
+ lib/conntrack.c \
+ lib/conntrack.h \
lib/coverage.c \
lib/coverage.h \
lib/crc32c.c \
diff --git a/lib/conntrack-other.c b/lib/conntrack-other.c
new file mode 100644
index 000..65d02a9
--- /dev/null
+++ b/lib/conntrack-other.c
@@ -0,0 +1,91 @@
+/*
+ * Copyright (c) 2015, 2016 Nicira, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at:
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include
+
+#include "conntrack-private.h"
+#include "dp-packet.h"
+
+enum other_state {
+OTHERS_FIRST,
+OTHERS_MULTIPLE,
+OTHERS_BIDIR,
+};
+
+struct conn_other {
+struct conn up;
+enum other_state state;
+};
+
+static const long long other_timeouts[] = {
+[OTHERS_FIRST] = 60 * 1000,
+[OTHERS_MULTIPLE] = 60 * 1000,
+[OTHERS_BIDIR] = 30 * 1000,
+};
+
+static struct conn_other *
+conn_other_cast(const struct conn *conn)
+{
+return CONTAINER_OF(conn, struct conn_other, up);
+}
+
+static void
+update_expiration(struct conn_other *conn, long long now)
+{
+conn->up.expiration = now + other_timeouts[conn->state];
+}
+
+static enum ct_update_res
+other_conn_update(struct conn *conn_, struct dp_packet *pkt OVS_UNUSED,
+bool reply, long long now)
+{
+struct conn_other *conn = conn_other_cast(conn_);
+
+if (reply && conn->state != OTHERS_BIDIR) {
+conn->state = OTHERS_BIDIR;
+} else if (conn->state == OTHERS_FIRST) {
+conn->state = OTHERS_MULTIPLE;
+}
+
+update_expiration(conn, now);
+
+return CT_UPDATE_VALID;
+}
+
+static bool
+other_valid_new(struct dp_packet *pkt OVS_UNUSED)
+{
+return true;
+}
+
+static struct conn *
+other_new_conn(struct dp_packet *pkt OVS_UNUSED, long long now)
+{
+struct conn_other *conn;
+
+conn = xzalloc(sizeof(struct conn_other));
+
