From patchwork Fri Mar 3 02:18:05 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Joe Stringer X-Patchwork-Id: 734904 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from mail.linuxfoundation.org (mail.linuxfoundation.org [140.211.169.12]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 3vZCXY1HX2z9s7L for ; Fri, 3 Mar 2017 13:18:37 +1100 (AEDT) Received: from mail.linux-foundation.org (localhost [127.0.0.1]) by mail.linuxfoundation.org (Postfix) with ESMTP id CA466BE6; Fri, 3 Mar 2017 02:18:33 +0000 (UTC) X-Original-To: dev@openvswitch.org Delivered-To: ovs-dev@mail.linuxfoundation.org Received: from smtp1.linuxfoundation.org (smtp1.linux-foundation.org [172.17.192.35]) by mail.linuxfoundation.org (Postfix) with ESMTPS id EA7AEB8A for ; Fri, 3 Mar 2017 02:18:31 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.7.6 Received: from relay6-d.mail.gandi.net (relay6-d.mail.gandi.net [217.70.183.198]) by smtp1.linuxfoundation.org (Postfix) with ESMTPS id 80A2B131 for ; Fri, 3 Mar 2017 02:18:30 +0000 (UTC) Received: from mfilter20-d.gandi.net (mfilter20-d.gandi.net [217.70.178.148]) by relay6-d.mail.gandi.net (Postfix) with ESMTP id 1EB0DFB881 for ; Fri, 3 Mar 2017 03:18:29 +0100 (CET) X-Virus-Scanned: Debian amavisd-new at mfilter20-d.gandi.net Received: from relay6-d.mail.gandi.net ([IPv6:::ffff:217.70.183.198]) by mfilter20-d.gandi.net (mfilter20-d.gandi.net [::ffff:10.0.15.180]) (amavisd-new, port 10024) with ESMTP id Nqa26hpBbeiD for ; Fri, 3 Mar 2017 03:18:27 +0100 (CET) X-Originating-IP: 209.85.220.170 Received: from mail-qk0-f170.google.com (mail-qk0-f170.google.com [209.85.220.170]) (Authenticated sender: joe@ovn.org) by relay6-d.mail.gandi.net (Postfix) with ESMTPSA id 372ECFB882 for ; Fri, 3 Mar 2017 03:18:27 +0100 (CET) Received: by mail-qk0-f170.google.com with SMTP id n127so155576802qkf.0 for ; Thu, 02 Mar 2017 18:18:27 -0800 (PST) X-Gm-Message-State: AMke39m/zhZ7wk0J4o8tED+Aoi3Q47vOdMRqHn+7Cgqj9/W3LhytHEUVzGXHtBifA1Ah44wL//0yllpRgbHgWg== X-Received: by 10.237.60.41 with SMTP id t38mr362847qte.148.1488507506015; Thu, 02 Mar 2017 18:18:26 -0800 (PST) MIME-Version: 1.0 Received: by 10.12.163.229 with HTTP; Thu, 2 Mar 2017 18:18:05 -0800 (PST) In-Reply-To: <1488331058-40038-19-git-send-email-jarno@ovn.org> References: <1488331058-40038-1-git-send-email-jarno@ovn.org> <1488331058-40038-19-git-send-email-jarno@ovn.org> From: Joe Stringer Date: Thu, 2 Mar 2017 18:18:05 -0800 X-Gmail-Original-Message-ID: Message-ID: To: Jarno Rajahalme X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_LOW, RCVD_IN_SORBS_SPAM autolearn=ham version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on smtp1.linux-foundation.org Cc: ovs dev Subject: Re: [ovs-dev] [PATCH v2 18/22] datapath: Add force commit. X-BeenThere: ovs-dev@openvswitch.org X-Mailman-Version: 2.1.12 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: ovs-dev-bounces@openvswitch.org Errors-To: ovs-dev-bounces@openvswitch.org On 28 February 2017 at 17:17, Jarno Rajahalme wrote: > Upstream patch: > > commit dd41d33f0b033885211a5d6f3ee19e73238aa9ee > Author: Jarno Rajahalme > Date: Thu Feb 9 11:22:00 2017 -0800 > > openvswitch: Add force commit. > > Stateful network admission policy may allow connections to one > direction and reject connections initiated in the other direction. > After policy change it is possible that for a new connection an > overlapping conntrack entry already exists, where the original > direction of the existing connection is opposed to the new > connection's initial packet. > > Most importantly, conntrack state relating to the current packet gets > the "reply" designation based on whether the original direction tuple > or the reply direction tuple matched. If this "directionality" is > wrong w.r.t. to the stateful network admission policy it may happen > that packets in neither direction are correctly admitted. > > This patch adds a new "force commit" option to the OVS conntrack > action that checks the original direction of an existing conntrack > entry. If that direction is opposed to the current packet, the > existing conntrack entry is deleted and a new one is subsequently > created in the correct direction. > > Signed-off-by: Jarno Rajahalme > Acked-by: Pravin B Shelar > Acked-by: Joe Stringer > Signed-off-by: David S. Miller > > Signed-off-by: Jarno Rajahalme > --- With the following minor incremental I was able to compile this commit, otherwise looks good. Note that the below doesn't implement the force logic yet, which comes in the next patch: > datapath/conntrack.c | 26 +++++++++++++++++++++-- > datapath/linux/compat/include/linux/openvswitch.h | 5 +++++ > 2 files changed, 29 insertions(+), 2 deletions(-) > > diff --git a/datapath/conntrack.c b/datapath/conntrack.c > index d8309c9..041a557 100644 > --- a/datapath/conntrack.c > +++ b/datapath/conntrack.c > @@ -72,6 +72,7 @@ struct ovs_conntrack_info { > u8 commit : 1; > u8 nat : 3; /* enum ovs_ct_nat */ > u8 random_fully_compat : 1; /* bool */ > + u8 force : 1; > u16 family; > struct md_mark mark; > struct md_labels labels; > @@ -658,10 +659,13 @@ static bool skb_nfct_cached(struct net *net, > */ > if (!ct && key->ct.state & OVS_CS_F_TRACKED && > !(key->ct.state & OVS_CS_F_INVALID) && > - key->ct.zone == info->zone.id) > + key->ct.zone == info->zone.id) { > ct = ovs_ct_find_existing(net, &info->zone, info->family, skb, > !!(key->ct.state > & OVS_CS_F_NAT_MASK)); > + if (ct) > + nf_ct_get(skb, &ctinfo); > + } > if (!ct) > return false; > if (!net_eq(net, read_pnet(&ct->ct_net))) > @@ -675,6 +679,18 @@ static bool skb_nfct_cached(struct net *net, > if (help && rcu_access_pointer(help->helper) != info->helper) > return false; > } > + /* Force conntrack entry direction to the current packet? */ > + if (info->force && CTINFO2DIR(ctinfo) != IP_CT_DIR_ORIGINAL) { > + /* Delete the conntrack entry if confirmed, else just release > + * the reference. > + */ > + if (nf_ct_is_confirmed(ct)) > + nf_ct_delete(ct, 0, 0); > + else > + nf_conntrack_put(&ct->ct_general); > + nf_ct_set(skb, NULL, 0); > + return false; > + } > > return true; > } > @@ -1259,6 +1275,7 @@ static int parse_nat(const struct nlattr *attr, > > static const struct ovs_ct_len_tbl ovs_ct_attr_lens[OVS_CT_ATTR_MAX + 1] = { > [OVS_CT_ATTR_COMMIT] = { .minlen = 0, .maxlen = 0 }, > + [OVS_CT_ATTR_FORCE_COMMIT] = { .minlen = 0, .maxlen = 0 }, > [OVS_CT_ATTR_ZONE] = { .minlen = sizeof(u16), > .maxlen = sizeof(u16) }, > [OVS_CT_ATTR_MARK] = { .minlen = sizeof(struct md_mark), > @@ -1298,6 +1315,9 @@ static int parse_ct(const struct nlattr *attr, struct ovs_conntrack_info *info, > } > > switch (type) { > + case OVS_CT_ATTR_FORCE_COMMIT: > + info->force = true; > + /* fall through. */ > case OVS_CT_ATTR_COMMIT: > info->commit = true; > break; > @@ -1528,7 +1548,9 @@ int ovs_ct_action_to_attr(const struct ovs_conntrack_info *ct_info, > if (!start) > return -EMSGSIZE; > > - if (ct_info->commit && nla_put_flag(skb, OVS_CT_ATTR_COMMIT)) > + if (ct_info->commit && nla_put_flag(skb, ct_info->force > + ? OVS_CT_ATTR_FORCE_COMMIT > + : OVS_CT_ATTR_COMMIT)) > return -EMSGSIZE; > if (IS_ENABLED(CONFIG_NF_CONNTRACK_ZONES) && > nla_put_u16(skb, OVS_CT_ATTR_ZONE, ct_info->zone.id)) > diff --git a/datapath/linux/compat/include/linux/openvswitch.h b/datapath/linux/compat/include/linux/openvswitch.h > index 23f8845..2fd0963 100644 > --- a/datapath/linux/compat/include/linux/openvswitch.h > +++ b/datapath/linux/compat/include/linux/openvswitch.h > @@ -718,6 +718,10 @@ struct ovs_action_push_tnl { > * mask. For each bit set in the mask, the corresponding bit in the value is > * copied to the connection tracking label field in the connection. > * @OVS_CT_ATTR_HELPER: variable length string defining conntrack ALG. > + * @OVS_CT_ATTR_FORCE_COMMIT: Like %OVS_CT_ATTR_COMMIT, but instead of doing > + * nothing if the connection is already committed will check that the current > + * packet is in conntrack entry's original direction. If directionality does > + * not match, will delete the existing conntrack entry and create a new one. > */ > enum ovs_ct_attr { > OVS_CT_ATTR_UNSPEC, > @@ -728,6 +732,7 @@ enum ovs_ct_attr { > OVS_CT_ATTR_HELPER, /* netlink helper to assist detection of > related connections. */ > OVS_CT_ATTR_NAT, /* Nested OVS_NAT_ATTR_* */ > + OVS_CT_ATTR_FORCE_COMMIT, /* No argument */ > __OVS_CT_ATTR_MAX > }; > > -- > 2.1.4 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index 30907b7f27cf..d082cccc06b4 100644 --- a/lib/dpif-netdev.c +++ b/lib/dpif-netdev.c @@ -4746,6 +4746,8 @@ dp_execute_cb(void *aux_, struct dp_packet_batch *packets_, enum ovs_ct_attr sub_type = nl_attr_type(b); switch(sub_type) { + case OVS_CT_ATTR_FORCE_COMMIT: case OVS_CT_ATTR_COMMIT: commit = true; break;