On Wed, Mar 20, 2019 at 2:19 PM Yi-Hung Wei <yihung....@gmail.com> wrote:
>
> Add support for fine-grain timeout support to conntrack action.
> The new OVS_CT_ATTR_TIMEOUT attribute of the conntrack action
> specifies a timeout to be associated with this connection.
> If no timeout is specified, it acts as is, that is the default
> timeout for the connection will be automatically applied.
>
> Example usage:
> $ nfct timeout add timeout_1 inet tcp syn_sent 100 established 200
> $ ovs-ofctl add-flow br0 in_port=1,ip,tcp,action=ct(commit,timeout=timeout_1)
>
> CC: Pravin Shelar <pshe...@ovn.org>
> Signed-off-by: Yi-Hung Wei <yihung....@gmail.com>
> ---
> include/uapi/linux/openvswitch.h | 3 ++
> net/openvswitch/conntrack.c | 81
> +++++++++++++++++++++++++++++++++++++++-
> 2 files changed, 83 insertions(+), 1 deletion(-)
>
> diff --git a/include/uapi/linux/openvswitch.h
> b/include/uapi/linux/openvswitch.h
> index dbe0cbe4f1b7..9bccc6b9ed3d 100644
> --- a/include/uapi/linux/openvswitch.h
> +++ b/include/uapi/linux/openvswitch.h
> @@ -734,6 +734,7 @@ struct ovs_action_hash {
> * be received on NFNLGRP_CONNTRACK_NEW and NFNLGRP_CONNTRACK_DESTROY groups,
> * respectively. Remaining bits control the changes for which an event is
> * delivered on the NFNLGRP_CONNTRACK_UPDATE group.
> + * @OVS_CT_ATTR_TIMEOUT: Variable length string defining conntrack timeout.
> */
> enum ovs_ct_attr {
> OVS_CT_ATTR_UNSPEC,
> @@ -746,6 +747,8 @@ enum ovs_ct_attr {
> OVS_CT_ATTR_NAT, /* Nested OVS_NAT_ATTR_* */
> OVS_CT_ATTR_FORCE_COMMIT, /* No argument */
> OVS_CT_ATTR_EVENTMASK, /* u32 mask of IPCT_* events. */
> + OVS_CT_ATTR_TIMEOUT, /* Associate timeout with this connection for
> + fine-grain timeout tuning. */
> __OVS_CT_ATTR_MAX
> };
>
> diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
> index 1b6896896fff..10a2c73f22f2 100644
> --- a/net/openvswitch/conntrack.c
> +++ b/net/openvswitch/conntrack.c
> @@ -24,6 +24,7 @@
> #include <net/netfilter/nf_conntrack_helper.h>
> #include <net/netfilter/nf_conntrack_labels.h>
> #include <net/netfilter/nf_conntrack_seqadj.h>
> +#include <net/netfilter/nf_conntrack_timeout.h>
> #include <net/netfilter/nf_conntrack_zones.h>
> #include <net/netfilter/ipv6/nf_defrag_ipv6.h>
> #include <net/ipv6_frag.h>
> @@ -73,6 +74,7 @@ struct ovs_conntrack_info {
> u32 eventmask; /* Mask of 1 << IPCT_*. */
> struct md_mark mark;
> struct md_labels labels;
> + char timeout[CTNL_TIMEOUT_NAME_MAX];
> #ifdef CONFIG_NF_NAT_NEEDED
> struct nf_nat_range2 range; /* Only present for SRC NAT and DST NAT.
> */
> #endif
> @@ -1139,6 +1141,59 @@ static int ovs_ct_check_limit(struct net *net,
> }
> #endif
>
> +static void ovs_ct_add_timeout(struct net *net, struct nf_conn *ct,
> + const char *timeout_name, u16 l3num, u8 l4num)
> +{
> +#ifdef CONFIG_NF_CONNTRACK_TIMEOUT
> + typeof(nf_ct_timeout_find_get_hook) timeout_find_get;
> + typeof(nf_ct_timeout_put_hook) timeout_put;
> + struct nf_ct_timeout *timeout;
> + struct nf_conn_timeout *timeout_ext;
> +
> + rcu_read_lock();
> + timeout_find_get = rcu_dereference(nf_ct_timeout_find_get_hook);
> + if (!timeout_find_get) {
> + net_info_ratelimited("Timeout policy base is empty");
> + goto out;
> + }
> +
> + timeout = timeout_find_get(net, timeout_name);
> + if (!timeout) {
> + net_info_ratelimited("No such timeout policy \"%s\"\n",
> + timeout_name);
> + goto out;
> + }
> +
> + if (timeout->l3num != l3num) {
> + net_info_ratelimited("Timeout policy `%s' can only be used by
> "
> + "L3 protocol number %d\n", timeout_name,
> + timeout->l3num);
> + goto err_put_timeout;
> + }
> +
> + if (timeout->l4proto->l4proto != l4num) {
> + net_info_ratelimited("Timeout policy `%s' can only be used by
> "
> + "L4 protocol number %d\n", timeout_name,
> + timeout->l4proto->l4proto);
> + goto err_put_timeout;
> + }
> +
> + timeout_ext = nf_ct_timeout_ext_add(ct, timeout, GFP_ATOMIC);
> + if (!timeout_ext)
> + goto err_put_timeout;
> +
> + goto out;
> +
> +err_put_timeout:
> + timeout_put = rcu_dereference(nf_ct_timeout_put_hook);
> + if (timeout_put)
> + timeout_put(timeout);
> +out:
> + rcu_read_unlock();
> + return;
This code looks very similar to xt_ct_set_timeout(), can you refactor
it to avoid code duplication?
> +#endif
> +}
> +
> /* Lookup connection and confirm if unconfirmed. */
> static int ovs_ct_commit(struct net *net, struct sw_flow_key *key,
> const struct ovs_conntrack_info *info,
> @@ -1465,6 +1520,8 @@ static const struct ovs_ct_len_tbl
> ovs_ct_attr_lens[OVS_CT_ATTR_MAX + 1] = {
> #endif
> [OVS_CT_ATTR_EVENTMASK] = { .minlen = sizeof(u32),
> .maxlen = sizeof(u32) },
> + [OVS_CT_ATTR_TIMEOUT] = { .minlen = 1,
> + .maxlen = CTNL_TIMEOUT_NAME_MAX },
> };
>
> static int parse_ct(const struct nlattr *attr, struct ovs_conntrack_info
> *info,
> @@ -1550,6 +1607,15 @@ static int parse_ct(const struct nlattr *attr, struct
> ovs_conntrack_info *info,
> info->have_eventmask = true;
> info->eventmask = nla_get_u32(a);
> break;
> +#ifdef CONFIG_NF_CONNTRACK_TIMEOUT
> + case OVS_CT_ATTR_TIMEOUT:
> + memcpy(info->timeout, nla_data(a), nla_len(a));
Before copying timeout, we need to check sizeof source string.
'nla_len(a)' needs to be less than CTNL_TIMEOUT_NAME_MAX.
otherwise looks good.
Thanks.
