[nf-next,v5] netfilter: nft_ct: add ct timeout support

This patch allows to add, list and delete connection tracking timeout
policies via nft objref infrastructure and assigning these timeout
via nft rule.

Ruleset:

table ip raw {
   ct timeout cttime {
       protocol tcp
       established 111 close 13
       l3proto ip
   }

   chain output {
       type filter hook output priority -300; policy accept;
       ct timeout set "cttime"
   }
}

%./libnftnl/examples/nft-ct-timeout-add ip raw cttime tcp
%./libnftnl/examples/nft-rule-ct-timeout-add ip raw output cttime

%conntrack -E
[NEW] tcp      6 111 ESTABLISHED src=172.16.19.128 dst=172.16.19.1
sport=22 dport=41360 [UNREPLIED] src=172.16.19.1 dst=172.16.19.128
sport=41360 dport=22 zone=4

Signed-off-by: Harsha Sharma <harshasharmaiitr@gmail.com>
---
Changes in v5:
 - wrap with NF_CT_NETLINK_TIMEOUT option
 - attach timeout template in init
 - other minor changes
Changes in v4:
 - Remove unused attributes
 - allocate template from init() path
 - minor changes
 - updated log message
 - pull to latest tree
Changes in v3:
 - Use nf_ct_tmpl_alloc to attach timeout via template conntrack.
Changes in v2:
 - Add code for nft_ct_timeout_obj_eval
 - remove likely() from code
 - remove vla in ctnl_timeout_parse_policy

 include/net/netfilter/nf_conntrack_timeout.h |   1 +
 include/uapi/linux/netfilter/nf_tables.h     |  14 +-
 net/netfilter/nft_ct.c                       | 203 ++++++++++++++++++++++++++-
 3 files changed, 215 insertions(+), 3 deletions(-)

On Thu, Jul 05, 2018 at 06:06:15PM +0200, Harsha Sharma wrote:
> diff --git a/include/net/netfilter/nf_conntrack_timeout.h b/include/net/netfilter/nf_conntrack_timeout.h
> index 80ceb3d0291d..c3b81a82b3c9 100644
> --- a/include/net/netfilter/nf_conntrack_timeout.h
> +++ b/include/net/netfilter/nf_conntrack_timeout.h
> @@ -8,6 +8,7 @@
>  #include <linux/refcount.h>
>  #include <net/netfilter/nf_conntrack.h>
>  #include <net/netfilter/nf_conntrack_extend.h>
> +#include <net/netfilter/nf_conntrack_l4proto.h>

Why this is needed? Probably doesn't belong here?

>  #define CTNL_TIMEOUT_NAME_MAX	32
>  
> diff --git a/include/uapi/linux/netfilter/nf_tables.h b/include/uapi/linux/netfilter/nf_tables.h
> index 89438e68dc03..552fa5a6b7c3 100644
> --- a/include/uapi/linux/netfilter/nf_tables.h
> +++ b/include/uapi/linux/netfilter/nf_tables.h
> @@ -955,6 +955,7 @@ enum nft_socket_keys {
>   * @NFT_CT_DST_IP: conntrack layer 3 protocol destination (IPv4 address)
>   * @NFT_CT_SRC_IP6: conntrack layer 3 protocol source (IPv6 address)
>   * @NFT_CT_DST_IP6: conntrack layer 3 protocol destination (IPv6 address)
> + * @NFT_CT_TIMEOUT: connection tracking timeout policy assigned to conntrack
>   */
>  enum nft_ct_keys {
>  	NFT_CT_STATE,
> @@ -980,6 +981,7 @@ enum nft_ct_keys {
>  	NFT_CT_DST_IP,
>  	NFT_CT_SRC_IP6,
>  	NFT_CT_DST_IP6,
> +	NFT_CT_TIMEOUT,
>  	__NFT_CT_MAX
>  };
>  #define NFT_CT_MAX		(__NFT_CT_MAX - 1)
> @@ -1392,13 +1394,23 @@ enum nft_ct_helper_attributes {
>  };
>  #define NFTA_CT_HELPER_MAX	(__NFTA_CT_HELPER_MAX - 1)
>  
> +enum nft_ct_timeout_timeout_attributes {
> +	NFTA_CT_TIMEOUT_UNSPEC,
> +	NFTA_CT_TIMEOUT_L3PROTO,
> +	NFTA_CT_TIMEOUT_L4PROTO,
> +	NFTA_CT_TIMEOUT_DATA,
> +	__NFTA_CT_TIMEOUT_MAX,
> +};
> +#define NFTA_CT_TIMEOUT_MAX	(__NFTA_CT_TIMEOUT_MAX - 1)
> +
>  #define NFT_OBJECT_UNSPEC	0
>  #define NFT_OBJECT_COUNTER	1
>  #define NFT_OBJECT_QUOTA	2
>  #define NFT_OBJECT_CT_HELPER	3
>  #define NFT_OBJECT_LIMIT	4
>  #define NFT_OBJECT_CONNLIMIT	5
> -#define __NFT_OBJECT_MAX	6
> +#define NFT_OBJECT_CT_TIMEOUT	6
> +#define __NFT_OBJECT_MAX	7
>  #define NFT_OBJECT_MAX		(__NFT_OBJECT_MAX - 1)
>  
>  /**
> diff --git a/net/netfilter/nft_ct.c b/net/netfilter/nft_ct.c
> index 1435ffc5f57e..40678a2e65a4 100644
> --- a/net/netfilter/nft_ct.c
> +++ b/net/netfilter/nft_ct.c
> @@ -22,6 +22,9 @@
>  #include <net/netfilter/nf_conntrack_helper.h>
>  #include <net/netfilter/nf_conntrack_ecache.h>
>  #include <net/netfilter/nf_conntrack_labels.h>
> +#include <linux/netfilter/nfnetlink_cttimeout.h>
> +#include <net/netfilter/nf_conntrack_timeout.h>
> +#include <net/netfilter/nf_conntrack_l4proto.h>
>  
>  struct nft_ct {
>  	enum nft_ct_keys	key:8;
> @@ -32,12 +35,18 @@ struct nft_ct {
>  	};
>  };
>  
> -struct nft_ct_helper_obj  {
> +struct nft_ct_helper_obj {
>  	struct nf_conntrack_helper *helper4;
>  	struct nf_conntrack_helper *helper6;
>  	u8 l4proto;
>  };
>  
> +struct nft_ct_timeout_obj {
> +	struct ctnl_timeout *timeout;
> +	struct nf_conn *tmpl;
> +	struct nf_conn_timeout *ext;

Why not use struct nf_conn_timeout from struct nf_conn? You can use
helpers in net/netfilter/nf_conntrack_timeout.h that allows you to
reach the timeout extension for tmpl.

> +};
> +
>  #ifdef CONFIG_NF_CONNTRACK_ZONES
>  static DEFINE_PER_CPU(struct nf_conn *, nft_ct_pcpu_template);
>  static unsigned int nft_ct_pcpu_template_refcnt __read_mostly;
> @@ -412,7 +421,6 @@ static int nft_ct_get_init(const struct nft_ctx *ctx,
>  			return -EINVAL;
>  		len = NF_CT_HELPER_NAME_LEN;
>  		break;
> -

Please, remove this chunk. Well this empty line can be removed, but
better not do it in this patch.

>  	case NFT_CT_L3PROTOCOL:
>  	case NFT_CT_PROTOCOL:
>  		/* For compatibility, do not report error if NFTA_CT_DIRECTION
> @@ -765,6 +773,168 @@ static struct nft_expr_type nft_notrack_type __read_mostly = {
>  	.owner		= THIS_MODULE,
>  };
>  
> +#ifdef CONFIG_NF_CT_NETLINK_TIMEOUT
> +static int
> +ctnl_timeout_parse_policy(void *timeouts,
> +			  const struct nf_conntrack_l4proto *l4proto,
> +			  struct net *net, const struct nlattr *attr)
> +{
> +	int ret = 0;
> +	struct nlattr **tb;

Reverse order for variable definition is preferred, ie.

	struct nlattr **tb;
	int ret = 0;

I mean, largest line with variable definition first.

> +
> +	if (!l4proto->ctnl_timeout.nlattr_to_obj)
> +		return 0;
> +
> +	tb = kcalloc(l4proto->ctnl_timeout.nlattr_max + 1, sizeof(*tb),
> +		     GFP_KERNEL);
> +
> +	if (!tb)
> +		return -ENOMEM;
> +
> +	ret = nla_parse_nested(tb, l4proto->ctnl_timeout.nlattr_max,
> +			       attr, l4proto->ctnl_timeout.nla_policy,
> +			       NULL);
> +	if (ret < 0)
> +		goto err;
> +
> +	ret = l4proto->ctnl_timeout.nlattr_to_obj(tb, net, timeouts);
> +
> +err:
> +	kfree(tb);
> +	return ret;
> +}
> +
> +static void nft_ct_timeout_obj_eval(struct nft_object *obj,
> +				    struct nft_regs *regs,
> +				    const struct nft_pktinfo *pkt)
> +{
> +	const struct nft_ct_timeout_obj *priv = nft_obj_data(obj);
> +	struct ctnl_timeout *to_assign = NULL;
> +	struct sk_buff *skb = pkt->skb;
> +	enum ip_conntrack_info ctinfo;
> +
> +	if (!priv->tmpl)

This cannot ever happen, right? If priv->tmpl is NULL, then it means
we have had an initialization problem.

> +		return;
> +
> +	to_assign = priv->timeout;
> +
> +	if (!nf_ct_get(skb, &ctinfo))

Better make it:

	if (nf_ct_get(skb, &ctinfo))
                return;

> +		nf_ct_set(skb, priv->tmpl, IP_CT_NEW);
        ^^^^^^^^
So you save an indentation level here.

> +
> +	if (priv->ext)
> +		rcu_assign_pointer(priv->ext->timeout, to_assign);
> +}
> +
> +static int nft_ct_timeout_obj_init(const struct nft_ctx *ctx,
> +				   const struct nlattr * const tb[],
> +				   struct nft_object *obj)
> +{
> +	__u8 l4num;
> +	struct nft_ct_timeout_obj *priv = nft_obj_data(obj);
> +	const struct nf_conntrack_l4proto *l4proto;
> +	struct ctnl_timeout *timeout, *matching = NULL;
> +	struct nf_conn *tmpl;
> +	const struct nf_conntrack_zone *zone = &nf_ct_zone_dflt;
> +	int ret;
> +	int l3num = ctx->family;
> +	struct nf_conn_timeout *timeout_ext;

Please, reorder variable definitions.

> +
> +	if (!tb[NFTA_CT_TIMEOUT_L4PROTO] ||
> +	    !tb[NFTA_CT_TIMEOUT_DATA])
> +		return -EINVAL;
> +
> +	if (tb[NFTA_CT_TIMEOUT_L3PROTO])

This attribute above is mandatory too I think, right? Better place it
in the EINVAL check above.

> +		l3num = ntohs(nla_get_be16(tb[NFTA_CT_TIMEOUT_L3PROTO]));
> +	l4num = nla_get_u8(tb[NFTA_CT_TIMEOUT_L4PROTO]);
> +
> +	INIT_LIST_HEAD(&ctx->net->nfct_timeout_list);

This initialization is not OK. It will clear the global timeout list,
you have to do this from net/netfilter/nf_tables_api.c, from the
_init() module path, not from here.

> +	list_for_each_entry(timeout, &ctx->net->nfct_timeout_list, head) {
> +		matching = timeout;
> +		break;
> +	}
> +
> +	if (matching) {
> +		if (matching->l3num != l3num ||
> +		    matching->l4proto->l4proto != l4num)
> +			return -EINVAL;
> +		return ctnl_timeout_parse_policy(&matching->data,
> +						 matching->l4proto, ctx->net,
> +						 tb[NFTA_CT_TIMEOUT_DATA]);
> +	}
> +
> +	l4proto = nf_ct_l4proto_find_get(l3num, l4num);
> +
> +	if (l4proto->l4proto != l4num) {
> +		ret = -EOPNOTSUPP;
> +		goto err_proto_put;
> +	}
> +
> +	timeout = kzalloc(sizeof(struct ctnl_timeout) +
> +			  l4proto->ctnl_timeout.obj_size, GFP_KERNEL);
> +	if (timeout == NULL) {
> +		ret = -ENOMEM;
> +		goto err_proto_put;
> +	}
> +
> +	ret = ctnl_timeout_parse_policy(&timeout->data, l4proto, ctx->net,
> +					tb[NFTA_CT_TIMEOUT_DATA]);
> +	if (ret < 0)
> +		goto err;
> +	timeout->l3num = l3num;
> +	timeout->l4proto = l4proto;
> +	list_add_tail_rcu(&timeout->head, &ctx->net->nfct_timeout_list);

You better do the list addition as the last thing, after timeout
object is fully initialized.

> +	priv->timeout = timeout;
> +	tmpl = nf_ct_tmpl_alloc(ctx->net, zone, GFP_ATOMIC);

You have to check if nf_ct_tmpl_alloc() if nf_ct_tmpl_alloc() fails,
ie. NULL

> +	tmpl->zone.id = 4;

Remove this line above, just set zone to zero.

> +	priv->tmpl = tmpl;
> +	timeout_ext = nf_ct_timeout_ext_add(priv->tmpl, priv->timeout, GFP_ATOMIC);
> +	priv->ext = timeout_ext;

Ah I see, no need for priv->ext. You can just get it using the helpers
I mentioned above to fetch the extension from priv->tmpl.

> +	return 0;
> +
> +err:
> +	kfree(timeout);
> +err_proto_put:
> +	nf_ct_l4proto_put(l4proto);
> +	return ret;
> +}
> +
> +static void nft_ct_timeout_obj_destroy(const struct nft_ctx *ctx,
> +				       struct nft_object *obj)
> +{
> +	struct nft_ct_timeout_obj *priv = nft_obj_data(obj);
> +
> +	if (refcount_dec_if_one(&priv->timeout->refcnt)) {
> +		nf_ct_l4proto_put(priv->timeout->l4proto);
> +		list_del_rcu(&priv->timeout->head);
> +		nf_ct_tmpl_free(priv->tmpl);
> +	}
> +}
> +
> +static int nft_ct_timeout_obj_dump(struct sk_buff *skb,
> +				   struct nft_object *obj, bool reset)
> +{
> +	const struct nft_ct_timeout_obj *priv = nft_obj_data(obj);
> +	const struct ctnl_timeout *timeout = priv->timeout;
> +	struct nlattr *nest_params;
> +	int ret;
> +
> +	if (nla_put_u8(skb, NFTA_CT_TIMEOUT_L4PROTO, timeout->l4proto->l4proto) ||
> +	    nla_put_be16(skb, NFTA_CT_TIMEOUT_L3PROTO, htons(priv->timeout->l3num)))
> +		return -1;
> +
> +	nest_params = nla_nest_start(skb, NFTA_CT_TIMEOUT_DATA | NLA_F_NESTED);
> +	if (!nest_params)
> +		return -1;
> +
> +	ret = timeout->l4proto->ctnl_timeout.obj_to_nlattr(skb, &timeout->data);
> +	if (ret < 0)
> +		return -1;
> +	nla_nest_end(skb, nest_params);
> +	return 0;
> +}
> +#endif
> +
>  static int nft_ct_helper_obj_init(const struct nft_ctx *ctx,
>  				  const struct nlattr * const tb[],
>  				  struct nft_object *obj)
> @@ -932,6 +1102,30 @@ static struct nft_object_type nft_ct_helper_obj_type __read_mostly = {
>  	.owner		= THIS_MODULE,
>  };
>  
> +static const struct nla_policy nft_ct_timeout_policy[NFTA_CT_TIMEOUT_MAX + 1] = {
> +	[NFTA_CT_TIMEOUT_L3PROTO] = {.type = NLA_U16 },
> +	[NFTA_CT_TIMEOUT_L4PROTO] = {.type = NLA_U8 },
> +	[NFTA_CT_TIMEOUT_DATA]	  = {.type = NLA_NESTED },
> +};
> +
> +static struct nft_object_type nft_ct_timeout_obj_type;
> +static const struct nft_object_ops nft_ct_timeout_obj_ops = {
> +	.type		= &nft_ct_timeout_obj_type,
> +	.size		= sizeof(struct nft_ct_timeout_obj),
> +	.eval		= nft_ct_timeout_obj_eval,
> +	.init		= nft_ct_timeout_obj_init,
> +	.destroy	= nft_ct_timeout_obj_destroy,
> +	.dump		= nft_ct_timeout_obj_dump,
> +};
> +
> +static struct nft_object_type nft_ct_timeout_obj_type __read_mostly = {
> +	.type		= NFT_OBJECT_CT_TIMEOUT,
> +	.ops		= &nft_ct_timeout_obj_ops,
> +	.maxattr = NFTA_CT_TIMEOUT_MAX,

Missing indentation.

> +	.policy	= nft_ct_timeout_policy,

Missing indentation.

> +	.owner		= THIS_MODULE,
> +};
> +
>  static int __init nft_ct_module_init(void)
>  {
>  	int err;
> @@ -947,6 +1141,9 @@ static int __init nft_ct_module_init(void)
>  		goto err1;
>  
>  	err = nft_register_obj(&nft_ct_helper_obj_type);
> +	if (err < 0)
> +		goto err2;
> +	err = nft_register_obj(&nft_ct_timeout_obj_type);
>  	if (err < 0)
>  		goto err2;

You have to add "goto err3" here and unwind nft_ct_helper_obj_type
registration.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hi Harsha,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on nf-next/master]

url:    https://github.com/0day-ci/linux/commits/Harsha-Sharma/netfilter-nft_ct-add-ct-timeout-support/20180706-001529
base:   https://git.kernel.org/pub/scm/linux/kernel/git/pablo/nf-next.git master
config: i386-randconfig-x013-201826 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

>> net/netfilter/nft_ct.c:1115:11: error: 'nft_ct_timeout_obj_eval' undeclared here (not in a function); did you mean 'nft_ct_timeout_obj_ops'?
     .eval  = nft_ct_timeout_obj_eval,
              ^~~~~~~~~~~~~~~~~~~~~~~
              nft_ct_timeout_obj_ops
>> net/netfilter/nft_ct.c:1116:11: error: 'nft_ct_timeout_obj_init' undeclared here (not in a function); did you mean 'nft_ct_timeout_obj_eval'?
     .init  = nft_ct_timeout_obj_init,
              ^~~~~~~~~~~~~~~~~~~~~~~
              nft_ct_timeout_obj_eval
>> net/netfilter/nft_ct.c:1117:13: error: 'nft_ct_timeout_obj_destroy' undeclared here (not in a function); did you mean 'nft_ct_timeout_obj_init'?
     .destroy = nft_ct_timeout_obj_destroy,
                ^~~~~~~~~~~~~~~~~~~~~~~~~~
                nft_ct_timeout_obj_init
>> net/netfilter/nft_ct.c:1118:11: error: 'nft_ct_timeout_obj_dump' undeclared here (not in a function); did you mean 'nft_ct_timeout_obj_init'?
     .dump  = nft_ct_timeout_obj_dump,
              ^~~~~~~~~~~~~~~~~~~~~~~
              nft_ct_timeout_obj_init

vim +1115 net/netfilter/nft_ct.c

  1110	
  1111	static struct nft_object_type nft_ct_timeout_obj_type;
  1112	static const struct nft_object_ops nft_ct_timeout_obj_ops = {
  1113		.type		= &nft_ct_timeout_obj_type,
  1114		.size		= sizeof(struct nft_ct_timeout_obj),
> 1115		.eval		= nft_ct_timeout_obj_eval,
> 1116		.init		= nft_ct_timeout_obj_init,
> 1117		.destroy	= nft_ct_timeout_obj_destroy,
> 1118		.dump		= nft_ct_timeout_obj_dump,
  1119	};
  1120	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation