[ovs-dev] [PATCH v2 04/15] conntrack: New userspace connection tracker.
Flavio Leitner
fbl at sysclose.org
Sat May 14 04:20:22 UTC 2016
On Wed, Apr 27, 2016 at 06:36:20AM +0000, Daniele Di Proietto wrote:
> Thank you for your comments, Flavio!
Thank you for the patchset :-)
>
> On 26/04/2016 16:35, "Flavio Leitner" <fbl at sysclose.org> 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 0000000..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 <sys/types.h>
> >> +#include <netinet/in.h>
> >> +#include <netinet/ip6.h>
> >> +
> >> +#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 0000000..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 <glebius at FreeBSD.org>
> >> + * 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 Laboratory, Air Force
> >> + * Materiel Command, USAF, under agreement number F30602-01-2-0537.
> >> + *
> >> + * $OpenBSD: pf.c,v 1.634 2009/02/27 12:37:45 henning Exp $
> >> + */
> >> +
> >> +#include <config.h>
> >> +
> >> +#include "conntrack-private.h"
> >> +#include "ct-dpif.h"
> >> +#include "dp-packet.h"
> >> +#include "util.h"
> >> +
> >> +struct tcp_peer {
> >> + enum ct_dpif_tcp_state state;
> >> + uint32_t seqlo; /* Max sequence number sent */
> >> + uint32_t seqhi; /* Max the other end ACKd + win */
> >> + uint16_t max_win; /* largest window (pre scaling) */
> >> + uint8_t wscale; /* window scaling factor */
> >> +};
> >> +
> >> +struct conn_tcp {
> >> + struct conn up;
> >> + struct tcp_peer peer[2];
> >> +};
> >> +
> >> +enum {
> >> + TCPOPT_EOL,
> >> + TCPOPT_NOP,
> >> + TCPOPT_WINDOW = 3,
> >> +};
> >> +
> >> +/* TCP sequence numbers are 32 bit integers operated
> >> + * on with modular arithmetic. These macros can be
> >> + * used to compare such integers. */
> >> +#define SEQ_LT(a,b) ((int)((a)-(b)) < 0)
> >> +#define SEQ_LEQ(a,b) ((int)((a)-(b)) <= 0)
> >> +#define SEQ_GT(a,b) ((int)((a)-(b)) > 0)
> >> +#define SEQ_GEQ(a,b) ((int)((a)-(b)) >= 0)
> >> +
> >> +#define SEQ_MIN(a, b) ((SEQ_LT(a, b)) ? (a) : (b))
> >> +#define SEQ_MAX(a, b) ((SEQ_GT(a, b)) ? (a) : (b))
> >
> >I am okay to leave those here, but since they are generic
> >operations, maybe we could define somewhere else and just
> >redefine here as SEQ_ operations.
>
> I am fine both ways. Do you have a suggestion for the name
> of the generic macro?
s/SEQ/INT/ ? Then put somewhere common and just redefine
here SEQ_LT() to be INT_LT()? I am not good with names :-)
Just that it seems to be generic enough to be useful in
other places.
[...]
> >> +static uint8_t
> >> +tcp_get_wscale(const struct tcp_header *tcp)
> >> +{
> >> + int len = TCP_OFFSET(tcp->tcp_ctl) * 4 - sizeof *tcp;
> >> + const uint8_t *opt = (const uint8_t *)(tcp + 1);
> >> + uint8_t wscale = 0;
> >> + uint8_t optlen;
> >> +
> >> + while (len >= 3) {
> >> + if (*opt == TCPOPT_EOL) {
> >> + break;
> >> + }
> >> + switch (*opt) {
> >
> >Maybe we could do:
> >
> > case TCPOPT_EOL:
> > break;
>
> I think
>
> The goal was to exit the while loop. I guess we could do
>
> case TCPOPT_EOL:
> return wscale;
That looks better to me.
Thanks!
fbl
More information about the dev
mailing list