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

Message ID 20180611221740.6343-1-harshasharmaiitr@gmail.com
State Under Review
Delegated to: Pablo Neira
Headers show
Series
  • [nf-next,v4] netfilter: nft_ct: add ct timeout support
Related show

Commit Message

Harsha Sharma June 11, 2018, 10:17 p.m.
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 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                       | 199 ++++++++++++++++++++++++++-
 3 files changed, 211 insertions(+), 3 deletions(-)

Comments

Harsha Sharma June 11, 2018, 10:23 p.m. | #1
Hello,

On Tue, Jun 12, 2018 at 12:17 AM, Harsha Sharma
<harshasharmaiitr@gmail.com> wrote:
> 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 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                       | 199 ++++++++++++++++++++++++++-
>  3 files changed, 211 insertions(+), 3 deletions(-)
>
> diff --git a/include/net/netfilter/nf_conntrack_timeout.h b/include/net/netfilter/nf_conntrack_timeout.h
> index 9468ab4ad12d..ae0f59cc1ffa 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>
>
>  #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 ae00a3c49b8a..80d4bb35d30d 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)
> @@ -1370,13 +1372,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..96988c7be037 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,17 @@ 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;
> +};
> +
>  #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 +420,6 @@ static int nft_ct_get_init(const struct nft_ctx *ctx,
>                         return -EINVAL;
>                 len = NF_CT_HELPER_NAME_LEN;
>                 break;
> -
>         case NFT_CT_L3PROTOCOL:
>         case NFT_CT_PROTOCOL:
>                 /* For compatibility, do not report error if NFTA_CT_DIRECTION
> @@ -765,6 +772,165 @@ static struct nft_expr_type nft_notrack_type __read_mostly = {
>         .owner          = THIS_MODULE,
>  };
>
> +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;
> +
> +       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 nf_conn_timeout *timeout_ext;
> +       struct sk_buff *skb = pkt->skb;
> +
> +       if (!priv->tmpl ||
> +           nf_ct_is_confirmed(priv->tmpl))
> +               return;
> +
> +       to_assign = priv->timeout;
> +
> +       nf_ct_set(skb, priv->tmpl, IP_CT_NEW);
> +       timeout_ext = nf_ct_timeout_ext_add(priv->tmpl, priv->timeout, GFP_ATOMIC);
> +
> +       if (timeout_ext) {
> +               rcu_assign_pointer(timeout_ext->timeout, to_assign);
> +               __set_bit(IPS_CONFIRMED_BIT, &priv->tmpl->status);
> +       }
> +}
> +
> +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;
> +
> +       if (!tb[NFTA_CT_TIMEOUT_L4PROTO] ||
> +           !tb[NFTA_CT_TIMEOUT_DATA])
> +               return -EINVAL;
> +
> +       if (tb[NFTA_CT_TIMEOUT_L3PROTO])
> +               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);
> +       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);
> +       priv->timeout = timeout;
> +       tmpl = nf_ct_tmpl_alloc(ctx->net, zone, GFP_ATOMIC);
> +       tmpl->zone.id = 4;

If zone id is 0 as default then events are missed in
nf_ct_deliver_cached_events().
This is definitely not correct, so any suggestions are welcome on this.
Thanks.

> +       priv->tmpl = 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;
> +}
> +
>  static int nft_ct_helper_obj_init(const struct nft_ctx *ctx,
>                                   const struct nlattr * const tb[],
>                                   struct nft_object *obj)
> @@ -932,6 +1098,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,
> +       .policy = nft_ct_timeout_policy,
> +       .owner          = THIS_MODULE,
> +};
> +
>  static int __init nft_ct_module_init(void)
>  {
>         int err;
> @@ -947,6 +1137,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;
>
> @@ -962,6 +1155,7 @@ static int __init nft_ct_module_init(void)
>  static void __exit nft_ct_module_exit(void)
>  {
>         nft_unregister_obj(&nft_ct_helper_obj_type);
> +       nft_unregister_obj(&nft_ct_timeout_obj_type);
>         nft_unregister_expr(&nft_notrack_type);
>         nft_unregister_expr(&nft_ct_type);
>  }
> @@ -974,3 +1168,4 @@ MODULE_AUTHOR("Patrick McHardy <kaber@trash.net>");
>  MODULE_ALIAS_NFT_EXPR("ct");
>  MODULE_ALIAS_NFT_EXPR("notrack");
>  MODULE_ALIAS_NFT_OBJ(NFT_OBJECT_CT_HELPER);
> +MODULE_ALIAS_NFT_OBJ(NFT_OBJECT_CT_TIMEOUT);
> --
> 2.14.1
>
--
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
kbuild test robot June 11, 2018, 11:45 p.m. | #2
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/20180612-061838
base:   https://git.kernel.org/pub/scm/linux/kernel/git/pablo/nf-next.git master
config: m68k-sun3_defconfig (attached as .config)
compiler: m68k-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.2.0 make.cross ARCH=m68k 

All errors (new ones prefixed by >>):

   net/netfilter/nft_ct.c: In function 'ctnl_timeout_parse_policy':
   net/netfilter/nft_ct.c:783:16: error: 'const struct nf_conntrack_l4proto' has no member named 'ctnl_timeout'; did you mean 'get_timeouts'?
     if (!l4proto->ctnl_timeout.nlattr_to_obj)
                   ^~~~~~~~~~~~
                   get_timeouts
   net/netfilter/nft_ct.c:786:24: error: 'const struct nf_conntrack_l4proto' has no member named 'ctnl_timeout'; did you mean 'get_timeouts'?
     tb = kcalloc(l4proto->ctnl_timeout.nlattr_max + 1, sizeof(*tb),
                           ^~~~~~~~~~~~
                           get_timeouts
   net/netfilter/nft_ct.c:792:38: error: 'const struct nf_conntrack_l4proto' has no member named 'ctnl_timeout'; did you mean 'get_timeouts'?
     ret = nla_parse_nested(tb, l4proto->ctnl_timeout.nlattr_max,
                                         ^~~~~~~~~~~~
                                         get_timeouts
   net/netfilter/nft_ct.c:793:26: error: 'const struct nf_conntrack_l4proto' has no member named 'ctnl_timeout'; did you mean 'get_timeouts'?
              attr, l4proto->ctnl_timeout.nla_policy,
                             ^~~~~~~~~~~~
                             get_timeouts
   net/netfilter/nft_ct.c:798:17: error: 'const struct nf_conntrack_l4proto' has no member named 'ctnl_timeout'; did you mean 'get_timeouts'?
     ret = l4proto->ctnl_timeout.nlattr_to_obj(tb, net, timeouts);
                    ^~~~~~~~~~~~
                    get_timeouts
   net/netfilter/nft_ct.c: In function 'nft_ct_timeout_obj_init':
>> net/netfilter/nft_ct.c:850:28: error: 'struct net' has no member named 'nfct_timeout_list'; did you mean 'nfnl_acct_list'?
     INIT_LIST_HEAD(&ctx->net->nfct_timeout_list);
                               ^~~~~~~~~~~~~~~~~
                               nfnl_acct_list
   In file included from net/netfilter/nft_ct.c:12:0:
   net/netfilter/nft_ct.c:851:42: error: 'struct net' has no member named 'nfct_timeout_list'; did you mean 'nfnl_acct_list'?
     list_for_each_entry(timeout, &ctx->net->nfct_timeout_list, head) {
                                             ^
   include/linux/kernel.h:961:26: note: in definition of macro 'container_of'
     void *__mptr = (void *)(ptr);     \
                             ^~~
   include/linux/list.h:377:2: note: in expansion of macro 'list_entry'
     list_entry((ptr)->next, type, member)
     ^~~~~~~~~~
   include/linux/list.h:464:13: note: in expansion of macro 'list_first_entry'
     for (pos = list_first_entry(head, typeof(*pos), member); \
                ^~~~~~~~~~~~~~~~
   net/netfilter/nft_ct.c:851:2: note: in expansion of macro 'list_for_each_entry'
     list_for_each_entry(timeout, &ctx->net->nfct_timeout_list, head) {
     ^~~~~~~~~~~~~~~~~~~
   In file included from include/linux/kernel.h:10:0,
                    from net/netfilter/nft_ct.c:12:
   net/netfilter/nft_ct.c:851:42: error: 'struct net' has no member named 'nfct_timeout_list'; did you mean 'nfnl_acct_list'?
     list_for_each_entry(timeout, &ctx->net->nfct_timeout_list, head) {
                                             ^
   include/linux/compiler.h:316:19: note: in definition of macro '__compiletime_assert'
      bool __cond = !(condition);    \
                      ^~~~~~~~~
   include/linux/compiler.h:339:2: note: in expansion of macro '_compiletime_assert'
     _compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
     ^~~~~~~~~~~~~~~~~~~
   include/linux/build_bug.h:45:37: note: in expansion of macro 'compiletime_assert'
    #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
                                        ^~~~~~~~~~~~~~~~~~
   include/linux/kernel.h:962:2: note: in expansion of macro 'BUILD_BUG_ON_MSG'
     BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) && \
     ^~~~~~~~~~~~~~~~
   include/linux/kernel.h:962:20: note: in expansion of macro '__same_type'
     BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) && \
                       ^~~~~~~~~~~
   include/linux/list.h:366:2: note: in expansion of macro 'container_of'
     container_of(ptr, type, member)
     ^~~~~~~~~~~~
   include/linux/list.h:377:2: note: in expansion of macro 'list_entry'
     list_entry((ptr)->next, type, member)
     ^~~~~~~~~~
   include/linux/list.h:464:13: note: in expansion of macro 'list_first_entry'
     for (pos = list_first_entry(head, typeof(*pos), member); \
                ^~~~~~~~~~~~~~~~
   net/netfilter/nft_ct.c:851:2: note: in expansion of macro 'list_for_each_entry'
     list_for_each_entry(timeout, &ctx->net->nfct_timeout_list, head) {
     ^~~~~~~~~~~~~~~~~~~
   net/netfilter/nft_ct.c:851:42: error: 'struct net' has no member named 'nfct_timeout_list'; did you mean 'nfnl_acct_list'?
     list_for_each_entry(timeout, &ctx->net->nfct_timeout_list, head) {
                                             ^
   include/linux/compiler.h:316:19: note: in definition of macro '__compiletime_assert'
      bool __cond = !(condition);    \
                      ^~~~~~~~~
   include/linux/compiler.h:339:2: note: in expansion of macro '_compiletime_assert'
     _compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
     ^~~~~~~~~~~~~~~~~~~
   include/linux/build_bug.h:45:37: note: in expansion of macro 'compiletime_assert'
    #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
                                        ^~~~~~~~~~~~~~~~~~
   include/linux/kernel.h:962:2: note: in expansion of macro 'BUILD_BUG_ON_MSG'
     BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) && \
     ^~~~~~~~~~~~~~~~
   include/linux/kernel.h:963:6: note: in expansion of macro '__same_type'
        !__same_type(*(ptr), void),   \
         ^~~~~~~~~~~
   include/linux/list.h:366:2: note: in expansion of macro 'container_of'
     container_of(ptr, type, member)
     ^~~~~~~~~~~~
   include/linux/list.h:377:2: note: in expansion of macro 'list_entry'
     list_entry((ptr)->next, type, member)
     ^~~~~~~~~~
   include/linux/list.h:464:13: note: in expansion of macro 'list_first_entry'
     for (pos = list_first_entry(head, typeof(*pos), member); \
                ^~~~~~~~~~~~~~~~
   net/netfilter/nft_ct.c:851:2: note: in expansion of macro 'list_for_each_entry'
     list_for_each_entry(timeout, &ctx->net->nfct_timeout_list, head) {
     ^~~~~~~~~~~~~~~~~~~
   In file included from include/linux/module.h:9:0,
                    from net/netfilter/nft_ct.c:14:
   net/netfilter/nft_ct.c:851:42: error: 'struct net' has no member named 'nfct_timeout_list'; did you mean 'nfnl_acct_list'?
     list_for_each_entry(timeout, &ctx->net->nfct_timeout_list, head) {
                                             ^
   include/linux/list.h:465:24: note: in definition of macro 'list_for_each_entry'
          &pos->member != (head);     \
                           ^~~~
   net/netfilter/nft_ct.c:873:15: error: 'const struct nf_conntrack_l4proto' has no member named 'ctnl_timeout'; did you mean 'get_timeouts'?
         l4proto->ctnl_timeout.obj_size, GFP_KERNEL);
                  ^~~~~~~~~~~~
                  get_timeouts
   net/netfilter/nft_ct.c:885:47: error: 'struct net' has no member named 'nfct_timeout_list'; did you mean 'nfnl_acct_list'?
     list_add_tail_rcu(&timeout->head, &ctx->net->nfct_timeout_list);
                                                  ^~~~~~~~~~~~~~~~~
                                                  nfnl_acct_list
   net/netfilter/nft_ct.c: In function 'nft_ct_timeout_obj_dump':
   net/netfilter/nft_ct.c:927:26: error: 'const struct nf_conntrack_l4proto' has no member named 'ctnl_timeout'; did you mean 'get_timeouts'?
     ret = timeout->l4proto->ctnl_timeout.obj_to_nlattr(skb, &timeout->data);

vim +850 net/netfilter/nft_ct.c

   774	
   775	static int
   776	ctnl_timeout_parse_policy(void *timeouts,
   777				  const struct nf_conntrack_l4proto *l4proto,
   778				  struct net *net, const struct nlattr *attr)
   779	{
   780		int ret = 0;
   781		struct nlattr **tb;
   782	
   783		if (!l4proto->ctnl_timeout.nlattr_to_obj)
   784			return 0;
   785	
   786		tb = kcalloc(l4proto->ctnl_timeout.nlattr_max + 1, sizeof(*tb),
   787			     GFP_KERNEL);
   788	
   789		if (!tb)
   790			return -ENOMEM;
   791	
   792		ret = nla_parse_nested(tb, l4proto->ctnl_timeout.nlattr_max,
 > 793				       attr, l4proto->ctnl_timeout.nla_policy,
   794				       NULL);
   795		if (ret < 0)
   796			goto err;
   797	
   798		ret = l4proto->ctnl_timeout.nlattr_to_obj(tb, net, timeouts);
   799	
   800	err:
   801		kfree(tb);
   802		return ret;
   803	}
   804	
   805	static void nft_ct_timeout_obj_eval(struct nft_object *obj,
   806					    struct nft_regs *regs,
   807					    const struct nft_pktinfo *pkt)
   808	{
   809		const struct nft_ct_timeout_obj *priv = nft_obj_data(obj);
   810		struct ctnl_timeout *to_assign = NULL;
   811		struct nf_conn_timeout *timeout_ext;
   812		struct sk_buff *skb = pkt->skb;
   813	
   814		if (!priv->tmpl ||
   815		    nf_ct_is_confirmed(priv->tmpl))
   816			return;
   817	
   818		to_assign = priv->timeout;
   819	
   820		nf_ct_set(skb, priv->tmpl, IP_CT_NEW);
   821		timeout_ext = nf_ct_timeout_ext_add(priv->tmpl, priv->timeout, GFP_ATOMIC);
   822	
   823		if (timeout_ext) {
   824			rcu_assign_pointer(timeout_ext->timeout, to_assign);
   825			__set_bit(IPS_CONFIRMED_BIT, &priv->tmpl->status);
   826		}
   827	}
   828	
   829	static int nft_ct_timeout_obj_init(const struct nft_ctx *ctx,
   830					   const struct nlattr * const tb[],
   831					   struct nft_object *obj)
   832	{
   833		__u8 l4num;
   834		struct nft_ct_timeout_obj *priv = nft_obj_data(obj);
   835		const struct nf_conntrack_l4proto *l4proto;
   836		struct ctnl_timeout *timeout, *matching = NULL;
   837		struct nf_conn *tmpl;
   838		const struct nf_conntrack_zone *zone = &nf_ct_zone_dflt;
   839		int ret;
   840		int l3num = ctx->family;
   841	
   842		if (!tb[NFTA_CT_TIMEOUT_L4PROTO] ||
   843		    !tb[NFTA_CT_TIMEOUT_DATA])
   844			return -EINVAL;
   845	
   846		if (tb[NFTA_CT_TIMEOUT_L3PROTO])
   847			l3num = ntohs(nla_get_be16(tb[NFTA_CT_TIMEOUT_L3PROTO]));
   848		l4num = nla_get_u8(tb[NFTA_CT_TIMEOUT_L4PROTO]);
   849	
 > 850		INIT_LIST_HEAD(&ctx->net->nfct_timeout_list);
   851		list_for_each_entry(timeout, &ctx->net->nfct_timeout_list, head) {
   852			matching = timeout;
   853			break;
   854		}
   855	
   856		if (matching) {
   857			if (matching->l3num != l3num ||
   858			    matching->l4proto->l4proto != l4num)
   859				return -EINVAL;
   860			return ctnl_timeout_parse_policy(&matching->data,
   861							 matching->l4proto, ctx->net,
   862							 tb[NFTA_CT_TIMEOUT_DATA]);
   863		}
   864	
   865		l4proto = nf_ct_l4proto_find_get(l3num, l4num);
   866	
   867		if (l4proto->l4proto != l4num) {
   868			ret = -EOPNOTSUPP;
   869			goto err_proto_put;
   870		}
   871	
   872		timeout = kzalloc(sizeof(struct ctnl_timeout) +
   873				  l4proto->ctnl_timeout.obj_size, GFP_KERNEL);
   874		if (timeout == NULL) {
   875			ret = -ENOMEM;
   876			goto err_proto_put;
   877		}
   878	
   879		ret = ctnl_timeout_parse_policy(&timeout->data, l4proto, ctx->net,
   880						tb[NFTA_CT_TIMEOUT_DATA]);
   881		if (ret < 0)
   882			goto err;
   883		timeout->l3num = l3num;
   884		timeout->l4proto = l4proto;
   885		list_add_tail_rcu(&timeout->head, &ctx->net->nfct_timeout_list);
   886		priv->timeout = timeout;
   887		tmpl = nf_ct_tmpl_alloc(ctx->net, zone, GFP_ATOMIC);
   888		tmpl->zone.id = 4;
   889		priv->tmpl = tmpl;
   890	
   891		return 0;
   892	err:
   893		kfree(timeout);
   894	err_proto_put:
   895		nf_ct_l4proto_put(l4proto);
   896		return ret;
   897	}
   898	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
kbuild test robot June 12, 2018, 2:07 a.m. | #3
Hi Harsha,

Thank you for the patch! Perhaps something to improve:

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

url:    https://github.com/0day-ci/linux/commits/Harsha-Sharma/netfilter-nft_ct-add-ct-timeout-support/20180612-061838
base:   https://git.kernel.org/pub/scm/linux/kernel/git/pablo/nf-next.git master
config: x86_64-randconfig-s0-06120905 (attached as .config)
compiler: gcc-6 (Debian 6.4.0-9) 6.4.0 20171026
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All warnings (new ones prefixed by >>):

   In file included from include/linux/kernel.h:10:0,
                    from net//netfilter/nft_ct.c:12:
   net//netfilter/nft_ct.c: In function 'ctnl_timeout_parse_policy':
   net//netfilter/nft_ct.c:783:14: error: 'const struct nf_conntrack_l4proto' has no member named 'ctnl_timeout'; did you mean 'get_timeouts'?
     if (!l4proto->ctnl_timeout.nlattr_to_obj)
                 ^
   include/linux/compiler.h:58:30: note: in definition of macro '__trace_if'
     if (__builtin_constant_p(!!(cond)) ? !!(cond) :   \
                                 ^~~~
>> net//netfilter/nft_ct.c:783:2: note: in expansion of macro 'if'
     if (!l4proto->ctnl_timeout.nlattr_to_obj)
     ^~
   net//netfilter/nft_ct.c:783:14: error: 'const struct nf_conntrack_l4proto' has no member named 'ctnl_timeout'; did you mean 'get_timeouts'?
     if (!l4proto->ctnl_timeout.nlattr_to_obj)
                 ^
   include/linux/compiler.h:58:42: note: in definition of macro '__trace_if'
     if (__builtin_constant_p(!!(cond)) ? !!(cond) :   \
                                             ^~~~
>> net//netfilter/nft_ct.c:783:2: note: in expansion of macro 'if'
     if (!l4proto->ctnl_timeout.nlattr_to_obj)
     ^~
   net//netfilter/nft_ct.c:783:14: error: 'const struct nf_conntrack_l4proto' has no member named 'ctnl_timeout'; did you mean 'get_timeouts'?
     if (!l4proto->ctnl_timeout.nlattr_to_obj)
                 ^
   include/linux/compiler.h:69:16: note: in definition of macro '__trace_if'
      ______r = !!(cond);     \
                   ^~~~
>> net//netfilter/nft_ct.c:783:2: note: in expansion of macro 'if'
     if (!l4proto->ctnl_timeout.nlattr_to_obj)
     ^~
   net//netfilter/nft_ct.c:786:22: error: 'const struct nf_conntrack_l4proto' has no member named 'ctnl_timeout'; did you mean 'get_timeouts'?
     tb = kcalloc(l4proto->ctnl_timeout.nlattr_max + 1, sizeof(*tb),
                         ^~
   net//netfilter/nft_ct.c:792:36: error: 'const struct nf_conntrack_l4proto' has no member named 'ctnl_timeout'; did you mean 'get_timeouts'?
     ret = nla_parse_nested(tb, l4proto->ctnl_timeout.nlattr_max,
                                       ^~
   net//netfilter/nft_ct.c:793:24: error: 'const struct nf_conntrack_l4proto' has no member named 'ctnl_timeout'; did you mean 'get_timeouts'?
              attr, l4proto->ctnl_timeout.nla_policy,
                           ^~
   net//netfilter/nft_ct.c:798:15: error: 'const struct nf_conntrack_l4proto' has no member named 'ctnl_timeout'; did you mean 'get_timeouts'?
     ret = l4proto->ctnl_timeout.nlattr_to_obj(tb, net, timeouts);
                  ^~
   net//netfilter/nft_ct.c: In function 'nft_ct_timeout_obj_init':
   net//netfilter/nft_ct.c:850:26: error: 'struct net' has no member named 'nfct_timeout_list'
     INIT_LIST_HEAD(&ctx->net->nfct_timeout_list);
                             ^~
   In file included from net//netfilter/nft_ct.c:12:0:
   net//netfilter/nft_ct.c:851:40: error: 'struct net' has no member named 'nfct_timeout_list'
     list_for_each_entry(timeout, &ctx->net->nfct_timeout_list, head) {
                                           ^
   include/linux/kernel.h:961:26: note: in definition of macro 'container_of'
     void *__mptr = (void *)(ptr);     \
                             ^~~
   include/linux/list.h:377:2: note: in expansion of macro 'list_entry'
     list_entry((ptr)->next, type, member)
     ^~~~~~~~~~
   include/linux/list.h:464:13: note: in expansion of macro 'list_first_entry'
     for (pos = list_first_entry(head, typeof(*pos), member); \
                ^~~~~~~~~~~~~~~~
   net//netfilter/nft_ct.c:851:2: note: in expansion of macro 'list_for_each_entry'
     list_for_each_entry(timeout, &ctx->net->nfct_timeout_list, head) {
     ^~~~~~~~~~~~~~~~~~~
   In file included from include/linux/kernel.h:10:0,
                    from net//netfilter/nft_ct.c:12:
   net//netfilter/nft_ct.c:851:40: error: 'struct net' has no member named 'nfct_timeout_list'
     list_for_each_entry(timeout, &ctx->net->nfct_timeout_list, head) {
                                           ^
   include/linux/compiler.h:316:19: note: in definition of macro '__compiletime_assert'
      bool __cond = !(condition);    \
                      ^~~~~~~~~
   include/linux/compiler.h:339:2: note: in expansion of macro '_compiletime_assert'
     _compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
     ^~~~~~~~~~~~~~~~~~~
   include/linux/build_bug.h:45:37: note: in expansion of macro 'compiletime_assert'
    #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
                                        ^~~~~~~~~~~~~~~~~~
   include/linux/kernel.h:962:2: note: in expansion of macro 'BUILD_BUG_ON_MSG'
     BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) && \
     ^~~~~~~~~~~~~~~~
   include/linux/kernel.h:962:20: note: in expansion of macro '__same_type'
     BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) && \
                       ^~~~~~~~~~~
   include/linux/list.h:366:2: note: in expansion of macro 'container_of'
     container_of(ptr, type, member)
     ^~~~~~~~~~~~
   include/linux/list.h:377:2: note: in expansion of macro 'list_entry'
     list_entry((ptr)->next, type, member)
     ^~~~~~~~~~
   include/linux/list.h:464:13: note: in expansion of macro 'list_first_entry'
     for (pos = list_first_entry(head, typeof(*pos), member); \
                ^~~~~~~~~~~~~~~~
   net//netfilter/nft_ct.c:851:2: note: in expansion of macro 'list_for_each_entry'
     list_for_each_entry(timeout, &ctx->net->nfct_timeout_list, head) {
     ^~~~~~~~~~~~~~~~~~~
   net//netfilter/nft_ct.c:851:40: error: 'struct net' has no member named 'nfct_timeout_list'
     list_for_each_entry(timeout, &ctx->net->nfct_timeout_list, head) {
                                           ^
   include/linux/compiler.h:316:19: note: in definition of macro '__compiletime_assert'
      bool __cond = !(condition);    \
                      ^~~~~~~~~
   include/linux/compiler.h:339:2: note: in expansion of macro '_compiletime_assert'
     _compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
     ^~~~~~~~~~~~~~~~~~~
   include/linux/build_bug.h:45:37: note: in expansion of macro 'compiletime_assert'
    #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
                                        ^~~~~~~~~~~~~~~~~~
   include/linux/kernel.h:962:2: note: in expansion of macro 'BUILD_BUG_ON_MSG'
     BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) && \
     ^~~~~~~~~~~~~~~~
   include/linux/kernel.h:963:6: note: in expansion of macro '__same_type'
        !__same_type(*(ptr), void),   \
         ^~~~~~~~~~~
   include/linux/list.h:366:2: note: in expansion of macro 'container_of'
     container_of(ptr, type, member)
     ^~~~~~~~~~~~
   include/linux/list.h:377:2: note: in expansion of macro 'list_entry'
     list_entry((ptr)->next, type, member)
     ^~~~~~~~~~
   include/linux/list.h:464:13: note: in expansion of macro 'list_first_entry'
     for (pos = list_first_entry(head, typeof(*pos), member); \
                ^~~~~~~~~~~~~~~~
   net//netfilter/nft_ct.c:851:2: note: in expansion of macro 'list_for_each_entry'
     list_for_each_entry(timeout, &ctx->net->nfct_timeout_list, head) {
     ^~~~~~~~~~~~~~~~~~~
   In file included from include/linux/module.h:9:0,
                    from net//netfilter/nft_ct.c:14:
   net//netfilter/nft_ct.c:851:40: error: 'struct net' has no member named 'nfct_timeout_list'
     list_for_each_entry(timeout, &ctx->net->nfct_timeout_list, head) {

vim +/if +783 net//netfilter/nft_ct.c

   774	
   775	static int
   776	ctnl_timeout_parse_policy(void *timeouts,
   777				  const struct nf_conntrack_l4proto *l4proto,
   778				  struct net *net, const struct nlattr *attr)
   779	{
   780		int ret = 0;
   781		struct nlattr **tb;
   782	
 > 783		if (!l4proto->ctnl_timeout.nlattr_to_obj)
   784			return 0;
   785	
   786		tb = kcalloc(l4proto->ctnl_timeout.nlattr_max + 1, sizeof(*tb),
   787			     GFP_KERNEL);
   788	
   789		if (!tb)
   790			return -ENOMEM;
   791	
   792		ret = nla_parse_nested(tb, l4proto->ctnl_timeout.nlattr_max,
   793				       attr, l4proto->ctnl_timeout.nla_policy,
   794				       NULL);
   795		if (ret < 0)
   796			goto err;
   797	
   798		ret = l4proto->ctnl_timeout.nlattr_to_obj(tb, net, timeouts);
   799	
   800	err:
   801		kfree(tb);
   802		return ret;
   803	}
   804	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
Florian Westphal June 12, 2018, 1:21 p.m. | #4
Harsha Sharma <harshasharmaiitr@gmail.com> wrote:
> +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;
> +
> +	if (!l4proto->ctnl_timeout.nlattr_to_obj)
> +		return 0;

You'll need to warp this with
#if IS_ENABLED(CONFIG_NF_CT_NETLINK_TIMEOUT)

else this fails to build with CONFIG_NF_CT_NETLINK_TIMEOUT=n.

> +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 nf_conn_timeout *timeout_ext;
> +	struct sk_buff *skb = pkt->skb;
> +
> +	if (!priv->tmpl ||

Better let nft_ct_timeout_obj_init() fail so priv->tmpl is always set.

> +	    nf_ct_is_confirmed(priv->tmpl))
> +		return;

Uncessery, the template cannot be confirmed (else bug).

> +	to_assign = priv->timeout;
> +
> +	nf_ct_set(skb, priv->tmpl, IP_CT_NEW);

You will need to check that no previous conntrack exists here.

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

This looks strange.  The timeout extension either has to be attached to
the template (but then this has to happen in init, not here).

> +	if (timeout_ext) {
> +		rcu_assign_pointer(timeout_ext->timeout, to_assign);
> +		__set_bit(IPS_CONFIRMED_BIT, &priv->tmpl->status);

Hmm, no, CONFIRMED means conntrack was inserted into the hash table,
this must never happen for templates.

Unrelated to your patch: I think timeout handling is braindead
in current conntrack, we should revisit this.

Right now there is a bizarre mix of getter (l4proto->get_timeout()),
which is then passed to l4proto->new() (but only used by gre), and a
conntrack extension, to pass timeouts from raw table to nf_conntrack_in.

It seems saner to revisit this design, remove l4proto->get_timeout(),
and have the l4 trackers get the timeout from the conntrack extension
(if present) or the pernetns data themselves.

This would also simplify this patch: you'd only have to fetch the
conntrack, check if its unconfirmed, and then call
nf_ct_timeout_ext_add() on it.  No template needed.
--
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
Pablo Neira Ayuso June 12, 2018, 5:23 p.m. | #5
On Tue, Jun 12, 2018 at 03:21:35PM +0200, Florian Westphal wrote:
> Harsha Sharma <harshasharmaiitr@gmail.com> wrote:
> > +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;
> > +
> > +	if (!l4proto->ctnl_timeout.nlattr_to_obj)
> > +		return 0;
> 
> You'll need to warp this with
> #if IS_ENABLED(CONFIG_NF_CT_NETLINK_TIMEOUT)
> 
> else this fails to build with CONFIG_NF_CT_NETLINK_TIMEOUT=n.

That's a short term solution, yes. But we need native interface for
this that integrates with the nft interface, let's talk about this
during the workshop.

> > +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 nf_conn_timeout *timeout_ext;
> > +	struct sk_buff *skb = pkt->skb;
> > +
> > +	if (!priv->tmpl ||
> 
> Better let nft_ct_timeout_obj_init() fail so priv->tmpl is always set.
> 
> > +	    nf_ct_is_confirmed(priv->tmpl))
> > +		return;
> 
> Uncessery, the template cannot be confirmed (else bug).
> 
> > +	to_assign = priv->timeout;
> > +
> > +	nf_ct_set(skb, priv->tmpl, IP_CT_NEW);
> 
> You will need to check that no previous conntrack exists here.
> 
> > +	timeout_ext = nf_ct_timeout_ext_add(priv->tmpl, priv->timeout, GFP_ATOMIC);
> 
> This looks strange.  The timeout extension either has to be attached to
> the template (but then this has to happen in init, not here).
> 
> > +	if (timeout_ext) {
> > +		rcu_assign_pointer(timeout_ext->timeout, to_assign);
> > +		__set_bit(IPS_CONFIRMED_BIT, &priv->tmpl->status);
> 
> Hmm, no, CONFIRMED means conntrack was inserted into the hash table,
> this must never happen for templates.

Harsha, please address comments from Florian.

Regarding this one below...

> Unrelated to your patch: I think timeout handling is braindead
> in current conntrack, we should revisit this.

By now, I think it's fine as is, I mean using the template, so
Harsha/someone else can have a look at this in a second step.

We'll need to update iptables too if we want to get rid of the
template in any case.
--
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
Florian Westphal June 12, 2018, 5:24 p.m. | #6
Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > Unrelated to your patch: I think timeout handling is braindead
> > in current conntrack, we should revisit this.
> 
> By now, I think it's fine as is, I mean using the template, so
> Harsha/someone else can have a look at this in a second step.

Yes, sure.  Its on my nfws agenda list.
--
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
Harsha Sharma June 13, 2018, 11:16 p.m. | #7
Hello,

On Tue, Jun 12, 2018 at 7:23 PM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Tue, Jun 12, 2018 at 03:21:35PM +0200, Florian Westphal wrote:
>> Harsha Sharma <harshasharmaiitr@gmail.com> wrote:
>> > +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;
>> > +
>> > +   if (!l4proto->ctnl_timeout.nlattr_to_obj)
>> > +           return 0;
>>
>> You'll need to warp this with
>> #if IS_ENABLED(CONFIG_NF_CT_NETLINK_TIMEOUT)
>>
>> else this fails to build with CONFIG_NF_CT_NETLINK_TIMEOUT=n.
>
> That's a short term solution, yes. But we need native interface for
> this that integrates with the nft interface, let's talk about this
> during the workshop.
>
>> > +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 nf_conn_timeout *timeout_ext;
>> > +   struct sk_buff *skb = pkt->skb;
>> > +
>> > +   if (!priv->tmpl ||
>>
>> Better let nft_ct_timeout_obj_init() fail so priv->tmpl is always set.
>>
>> > +       nf_ct_is_confirmed(priv->tmpl))
>> > +           return;
>>
>> Uncessery, the template cannot be confirmed (else bug).
>>
>> > +   to_assign = priv->timeout;
>> > +
>> > +   nf_ct_set(skb, priv->tmpl, IP_CT_NEW);
>>
>> You will need to check that no previous conntrack exists here.
>>
>> > +   timeout_ext = nf_ct_timeout_ext_add(priv->tmpl, priv->timeout, GFP_ATOMIC);
>>
>> This looks strange.  The timeout extension either has to be attached to
>> the template (but then this has to happen in init, not here).
>>
>> > +   if (timeout_ext) {
>> > +           rcu_assign_pointer(timeout_ext->timeout, to_assign);
>> > +           __set_bit(IPS_CONFIRMED_BIT, &priv->tmpl->status);
>>
>> Hmm, no, CONFIRMED means conntrack was inserted into the hash table,
>> this must never happen for templates.
>
> Harsha, please address comments from Florian.

Yes, sure. Thanks for the review.

> Regarding this one below...
>
>> Unrelated to your patch: I think timeout handling is braindead
>> in current conntrack, we should revisit this.
>
> By now, I think it's fine as is, I mean using the template, so
> Harsha/someone else can have a look at this in a second step.
>
> We'll need to update iptables too if we want to get rid of the
> template in any case.
--
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

Patch

diff --git a/include/net/netfilter/nf_conntrack_timeout.h b/include/net/netfilter/nf_conntrack_timeout.h
index 9468ab4ad12d..ae0f59cc1ffa 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>
 
 #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 ae00a3c49b8a..80d4bb35d30d 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)
@@ -1370,13 +1372,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..96988c7be037 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,17 @@  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;
+};
+
 #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 +420,6 @@  static int nft_ct_get_init(const struct nft_ctx *ctx,
 			return -EINVAL;
 		len = NF_CT_HELPER_NAME_LEN;
 		break;
-
 	case NFT_CT_L3PROTOCOL:
 	case NFT_CT_PROTOCOL:
 		/* For compatibility, do not report error if NFTA_CT_DIRECTION
@@ -765,6 +772,165 @@  static struct nft_expr_type nft_notrack_type __read_mostly = {
 	.owner		= THIS_MODULE,
 };
 
+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;
+
+	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 nf_conn_timeout *timeout_ext;
+	struct sk_buff *skb = pkt->skb;
+
+	if (!priv->tmpl ||
+	    nf_ct_is_confirmed(priv->tmpl))
+		return;
+
+	to_assign = priv->timeout;
+
+	nf_ct_set(skb, priv->tmpl, IP_CT_NEW);
+	timeout_ext = nf_ct_timeout_ext_add(priv->tmpl, priv->timeout, GFP_ATOMIC);
+
+	if (timeout_ext) {
+		rcu_assign_pointer(timeout_ext->timeout, to_assign);
+		__set_bit(IPS_CONFIRMED_BIT, &priv->tmpl->status);
+	}
+}
+
+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;
+
+	if (!tb[NFTA_CT_TIMEOUT_L4PROTO] ||
+	    !tb[NFTA_CT_TIMEOUT_DATA])
+		return -EINVAL;
+
+	if (tb[NFTA_CT_TIMEOUT_L3PROTO])
+		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);
+	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);
+	priv->timeout = timeout;
+	tmpl = nf_ct_tmpl_alloc(ctx->net, zone, GFP_ATOMIC);
+	tmpl->zone.id = 4;
+	priv->tmpl = 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;
+}
+
 static int nft_ct_helper_obj_init(const struct nft_ctx *ctx,
 				  const struct nlattr * const tb[],
 				  struct nft_object *obj)
@@ -932,6 +1098,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,
+	.policy	= nft_ct_timeout_policy,
+	.owner		= THIS_MODULE,
+};
+
 static int __init nft_ct_module_init(void)
 {
 	int err;
@@ -947,6 +1137,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;
 
@@ -962,6 +1155,7 @@  static int __init nft_ct_module_init(void)
 static void __exit nft_ct_module_exit(void)
 {
 	nft_unregister_obj(&nft_ct_helper_obj_type);
+	nft_unregister_obj(&nft_ct_timeout_obj_type);
 	nft_unregister_expr(&nft_notrack_type);
 	nft_unregister_expr(&nft_ct_type);
 }
@@ -974,3 +1168,4 @@  MODULE_AUTHOR("Patrick McHardy <kaber@trash.net>");
 MODULE_ALIAS_NFT_EXPR("ct");
 MODULE_ALIAS_NFT_EXPR("notrack");
 MODULE_ALIAS_NFT_OBJ(NFT_OBJECT_CT_HELPER);
+MODULE_ALIAS_NFT_OBJ(NFT_OBJECT_CT_TIMEOUT);