On Mon, Jul 23, 2018 at 11:13:39PM +0200, Harsha Sharma wrote:
> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> index c0fb2bcd30fe..3b98ceba002f 100644
> --- a/net/netfilter/nf_tables_api.c
> +++ b/net/netfilter/nf_tables_api.c
> @@ -7233,6 +7233,10 @@ static int __net_init nf_tables_init_net(struct net *net)
> INIT_LIST_HEAD(&net->nft.tables);
> INIT_LIST_HEAD(&net->nft.commit_list);
> mutex_init(&net->nft.commit_mutex);
> +
> +#if IS_ENABLED(CONFIG_NF_CT_NETLINK_TIMEOUT)
> + INIT_LIST_HEAD(&net->nft.cttimeout_list);
> +#endif
> net->nft.base_seq = 1;
> net->nft.validate_state = NFT_VALIDATE_SKIP;
>
> diff --git a/net/netfilter/nft_ct.c b/net/netfilter/nft_ct.c
> index 3bc82ee5464d..8ed65e7a28de 100644
> --- a/net/netfilter/nft_ct.c
> +++ b/net/netfilter/nft_ct.c
> @@ -22,6 +22,8 @@
> #include <net/netfilter/nf_conntrack_helper.h>
> #include <net/netfilter/nf_conntrack_ecache.h>
> #include <net/netfilter/nf_conntrack_labels.h>
> +#include <net/netfilter/nf_conntrack_timeout.h>
> +#include <net/netfilter/nf_conntrack_l4proto.h>
>
> struct nft_ct {
> enum nft_ct_keys key:8;
> @@ -38,6 +40,11 @@ struct nft_ct_helper_obj {
> 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;
> @@ -765,6 +772,156 @@ static struct nft_expr_type nft_notrack_type __read_mostly = {
> .owner = THIS_MODULE,
> };
>
> +#if IS_ENABLED(CONFIG_NF_CT_NETLINK_TIMEOUT)
You can remove the ifdef, see below.
> +static int
> +nft_ct_timeout_parse_policy(void *timeouts,
> + const struct nf_conntrack_l4proto *l4proto,
> + struct net *net, const struct nlattr *attr)
> +{
> + struct nlattr **tb;
> + int ret = 0;
> +
> + 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 sk_buff *skb = pkt->skb;
> + enum ip_conntrack_info ctinfo;
> +
> + if (nf_ct_get(skb, &ctinfo))
> + return;
> +
> + nf_ct_set(skb, priv->tmpl, IP_CT_NEW);
> +}
> +
> +static int nft_ct_timeout_obj_init(const struct nft_ctx *ctx,
> + const struct nlattr * const tb[],
> + struct nft_object *obj)
> +{
> + const struct nf_conntrack_zone *zone = &nf_ct_zone_dflt;
> + struct nft_ct_timeout_obj *priv = nft_obj_data(obj);
> + struct ctnl_timeout *timeout, *to_assign = NULL;
> + const struct nf_conntrack_l4proto *l4proto;
> + struct nf_conn_timeout *timeout_ext;
> + int l3num = ctx->family;
> + struct nf_conn *tmpl;
> + __u8 l4num;
> + int ret;
> +
> + if (!tb[NFTA_CT_TIMEOUT_L3PROTO] ||
> + !tb[NFTA_CT_TIMEOUT_L4PROTO] ||
> + !tb[NFTA_CT_TIMEOUT_DATA])
> + return -EINVAL;
> +
> + l3num = ntohs(nla_get_be16(tb[NFTA_CT_TIMEOUT_L3PROTO]));
> + l4num = nla_get_u8(tb[NFTA_CT_TIMEOUT_L4PROTO]);
> + 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 = nft_ct_timeout_parse_policy(&timeout->data, l4proto, ctx->net,
> + tb[NFTA_CT_TIMEOUT_DATA]);
> + if (ret < 0)
> + goto err;
> + timeout->l3num = l3num;
> + timeout->l4proto = l4proto;
> + priv->timeout = timeout;
> + tmpl = nf_ct_tmpl_alloc(ctx->net, zone, GFP_ATOMIC);
> + if (!tmpl)
> + return -ENOMEM;
timeout is not released, there's a memleak.
> + timeout_ext = nf_ct_timeout_ext_add(tmpl, priv->timeout,
> + GFP_ATOMIC);
> +
> + if (!timeout_ext) {
> + nf_ct_tmpl_free(tmpl);
same thing here. I guess you have to use goto here to unwind errors.
> + return -ENOMEM;
> + }
> +
> + priv->tmpl = tmpl;
> + refcount_set(&priv->timeout->refcnt, 1);
> + to_assign = priv->timeout;
Why not just:
rcu_assign_pointer(timeout_ext->timeout, priv->timeout);
so you can remove 'to_assign'.
> + rcu_assign_pointer(timeout_ext->timeout, to_assign);
> + list_add_tail_rcu(&timeout->head, &ctx->net->nft.cttimeout_list);
> + 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);
> +
> + nf_ct_tmpl_free(priv->tmpl);
> +
> + if (refcount_dec_if_one(&priv->timeout->refcnt)) {
> + nf_ct_l4proto_put(priv->timeout->l4proto);
> + list_del_rcu(&priv->timeout->head);
> + nf_ct_untimeout(ctx->net, priv->timeout);
> + }
> +}
> +
> +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(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 +1089,33 @@ 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),
> +#if IS_ENABLED(CONFIG_NF_CT_NETLINK_TIMEOUT)
> + .eval = nft_ct_timeout_obj_eval,
> + .init = nft_ct_timeout_obj_init,
> + .destroy = nft_ct_timeout_obj_destroy,
> + .dump = nft_ct_timeout_obj_dump,
> +#endif
Better do not register nft_ct_timeout_obj_ops if
CONFIG_NF_CT_NETLINK_TIMEOUT is not set.
OR if you remove ifdef CONFIG_NF_CT_NETLINK_TIMEOUT in struct
nf_conntrack_l4proto definition and you include:
net/netfilter/nf_conntrack_l4proto.h
from net/netfilter/nft_ct.c, then we can avoid all ifdefs in this
patch.
Thanks Harsha!
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
