diff mbox

[ovs-dev,v2,18/22] datapath: Add force commit.

Message ID CAPWQB7GpfNifz2H+ACHsURaKwR98AK6kNkyBaFbuf5Z84B+8Vw@mail.gmail.com
State Not Applicable
Headers show

Commit Message

Joe Stringer March 3, 2017, 2:18 a.m. UTC 
On 28 February 2017 at 17:17, Jarno Rajahalme <jarno@ovn.org> wrote:
> Upstream patch:
>
>     commit dd41d33f0b033885211a5d6f3ee19e73238aa9ee
>     Author: Jarno Rajahalme <jarno@ovn.org>
>     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 <jarno@ovn.org>
>     Acked-by: Pravin B Shelar <pshelar@ovn.org>
>     Acked-by: Joe Stringer <joe@ovn.org>
>     Signed-off-by: David S. Miller <davem@davemloft.net>
>
> Signed-off-by: Jarno Rajahalme <jarno@ovn.org>
> ---

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 mbox

Patch

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;