Re: [ovs-dev] [PATCH v2 04/15] conntrack: New userspace connection tracker.

2016-05-13 Thread Flavio Leitner 
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
> >> +

Re: [ovs-dev] [PATCH v2 04/15] conntrack: New userspace connection tracker.

2016-04-27 Thread Fischetti, Antonio 
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 <antonio.fische...@intel.com>
> 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"
> <antonio.fische...@intel.com> 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 <diproiet...@vmware.com>
> >> ---
> >>  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 <gleb...@freebsd.org>
> >>
> >>  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/

Re: [ovs-dev] [PATCH v2 04/15] conntrack: New userspace connection tracker.

2016-04-27 Thread Daniele Di Proietto 
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

Re: [ovs-dev] [PATCH v2 04/15] conntrack: New userspace connection tracker.

2016-04-27 Thread Daniele Di Proietto 
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 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

Re: [ovs-dev] [PATCH v2 04/15] conntrack: New userspace connection tracker.

2016-04-27 Thread Daniele Di Proietto 
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] = _proto_tcp,
>> +[IPPROTO_UDP] = _proto_other,
>> +[IPPROTO_ICMP] = _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, >key)) {
>> +*state |= CS_INVALID;
>> +return nc;
>> +}
>> +
>> +*state |= CS_NEW;
>> +
>> +if (commit) {
>> +nc = new_conn(pkt, >key, now);
>> +
>> +memcpy(>rev_key, >key, sizeof nc->rev_key);
>> +
>> +conn_key_reverse(>rev_key);
>> +hmap_insert(>connections[bucket], >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;
>> +

Re: [ovs-dev] [PATCH v2 04/15] conntrack: New userspace connection tracker.

2016-04-26 Thread Flavio Leitner 
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

Re: [ovs-dev] [PATCH v2 04/15] conntrack: New userspace connection tracker.

2016-04-25 Thread Fischetti, Antonio 
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] = 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

Re: [ovs-dev] [PATCH v2 04/15] conntrack: New userspace connection tracker.

2016-04-19 Thread Joe Stringer 
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] = _proto_tcp,
> +[IPPROTO_UDP] = _proto_other,
> +[IPPROTO_ICMP] = _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, >key)) {
> +*state |= CS_INVALID;
> +return nc;
> +}
> +
> +*state |= CS_NEW;
> +
> +if (commit) {
> +nc = new_conn(pkt, >key, now);
> +
> +memcpy(>rev_key, >key, sizeof nc->rev_key);
> +
> +conn_key_reverse(>rev_key);
> +hmap_insert(>connections[bucket], >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(>connections[bucket], >node);
> +delete_conn(conn);
> +conn = conn_not_found(ct, pkt, ctx, , commit, now);

...which is then stored here...

> +break;
> +}
> +}
> +
> +