Re: [PATCH nf-next,v3] netfilter: nft_ct: add ct expectations support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, May 17, 2019 at 06:40:29PM +0200, Stéphane Veyret wrote:
> This patch allows to add, list and delete expectations via nft objref
> infrastructure and assigning these expectations via nft rule.
> 
> This allows manual port triggering when no helper is defined to manage a
> specific protocol. For example, if I have an online game which protocol
> is based on initial connection to TCP port 9753 of the server, and where
> the server opens a connection to port 9876, I can set rules as follow:
> 
> table ip filter {
>     ct expectation mygame {
>         protocol udp;
>         dport 9876;

I think we should set a maximum number of expectations to be created,
as a mandatory field, eg.

          size 10;

>     }
> 
>     chain input {
>         type filter hook input priority 0; policy drop;
>         tcp dport 9753 ct expectation set "mygame";
>     }
> 
>     chain output {
>         type filter hook output priority 0; policy drop;
>         udp dport 9876 ct status expected accept;
>     }
> }
[...]
> diff --git a/net/netfilter/nft_ct.c b/net/netfilter/nft_ct.c
> index f043936763f3..d01cb175ab30 100644
> --- a/net/netfilter/nft_ct.c
> +++ b/net/netfilter/nft_ct.c
> @@ -24,6 +24,7 @@
>  #include <net/netfilter/nf_conntrack_labels.h>
>  #include <net/netfilter/nf_conntrack_timeout.h>
>  #include <net/netfilter/nf_conntrack_l4proto.h>
> +#include <net/netfilter/nf_conntrack_expect.h>
>  
>  struct nft_ct {
>  	enum nft_ct_keys	key:8;
> @@ -790,6 +791,114 @@ static struct nft_expr_type nft_notrack_type __read_mostly = {
>  	.owner		= THIS_MODULE,
>  };
>  
> +struct nft_ct_expect_obj {
> +	int			l3num;
> +	u8			l4proto;
> +	__be16		dport;
> +	u32			timeout;
> +};
> +
> +static int nft_ct_expect_obj_init(const struct nft_ctx *ctx,
> +				   const struct nlattr * const tb[],
> +				   struct nft_object *obj)
> +{
> +	struct nft_ct_expect_obj *priv = nft_obj_data(obj);
> +	int ret;
> +
> +	if (!tb[NFTA_CT_EXPECT_L4PROTO] ||
> +		!tb[NFTA_CT_EXPECT_DPORT] ||
> +		!tb[NFTA_CT_EXPECT_TIMEOUT])

Coding style: Align parameter to parens:

	if (!tb[NFTA_CT_EXPECT_L4PROTO] ||
            !tb[NFTA_CT_EXPECT_DPORT] ||
            !tb[NFTA_CT_EXPECT_TIMEOUT])
		return -EINVAL;

> +	priv->l3num = ctx->family;

priv->l3num is only set and never used, remove it. You'll also have to
remove NFTA_CT_EXPECT_L3PROTO.

> +	if (tb[NFTA_CT_EXPECT_L3PROTO])
> +		priv->l3num = ntohs(nla_get_be16(tb[NFTA_CT_EXPECT_L3PROTO]));
> +
> +	priv->l4proto = nla_get_u8(tb[NFTA_CT_EXPECT_L4PROTO]);
> +	priv->dport = nla_get_be16(tb[NFTA_CT_EXPECT_DPORT]);
> +	priv->timeout = nla_get_u32(tb[NFTA_CT_EXPECT_TIMEOUT]);
> +
> +	ret = nf_ct_netns_get(ctx->net, ctx->family);

Just:

        return nf_ct_netns_get(ctx->net, ctx->family);

should be fine.

> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static void nft_ct_expect_obj_destroy(const struct nft_ctx *ctx,
> +				       struct nft_object *obj)
> +{
> +	nf_ct_netns_put(ctx->net, ctx->family);
> +}
> +
> +static int nft_ct_expect_obj_dump(struct sk_buff *skb,
> +				   struct nft_object *obj, bool reset)
> +{
> +	const struct nft_ct_expect_obj *priv = nft_obj_data(obj);
> +
> +	if (nla_put_be16(skb, NFTA_CT_EXPECT_L3PROTO, htons(priv->l3num)) ||
> +		nla_put_u8(skb, NFTA_CT_EXPECT_L4PROTO, priv->l4proto) ||
> +		nla_put_be16(skb, NFTA_CT_EXPECT_DPORT, priv->dport) ||
> +		nla_put_u32(skb, NFTA_CT_EXPECT_TIMEOUT, priv->timeout))
> +	return -1;

Coding style: Align parameter to parens:

	if (nla_put_be16(skb, NFTA_CT_EXPECT_L3PROTO, htons(priv->l3num)) ||
            nla_put_u8(skb, NFTA_CT_EXPECT_L4PROTO, priv->l4proto) ||
            nla_put_be16(skb, NFTA_CT_EXPECT_DPORT, priv->dport) ||
            nla_put_u32(skb, NFTA_CT_EXPECT_TIMEOUT, priv->timeout))
                return -1;

> +	return 0;
> +}
> +
> +static void nft_ct_expect_obj_eval(struct nft_object *obj,
> +				    struct nft_regs *regs,
> +				    const struct nft_pktinfo *pkt)
> +{
> +	const struct nft_ct_expect_obj *priv = nft_obj_data(obj);
> +	enum ip_conntrack_info ctinfo;
> +	struct nf_conn *ct = nf_ct_get(pkt->skb, &ctinfo);
> +	int dir = CTINFO2DIR(ctinfo);
> +	struct nf_conntrack_expect *exp;

Please, revert xmas tree for variable definitions, ie.

	const struct nft_ct_expect_obj *priv = nft_obj_data(obj);
	struct nf_conntrack_expect *exp;
	enum ip_conntrack_info ctinfo;
	int dir = CTINFO2DIR(ctinfo);
	struct nf_conn *ct;

Then, you have to check if ct is unset or is untrackedie.

        ct = nf_ct_get(pkt->skb, &ctinfo);
        if (!ct || ctinfo == IP_CT_UNTRACKED)
                goto err;

        ...
err:
        regs->verdict.code = NFT_BREAK;

> +	nf_ct_helper_ext_add(ct, GFP_ATOMIC);

I think you don't need nf_ct_helper_ext_add(...);

> +	exp = nf_ct_expect_alloc(ct);
> +	if (exp == NULL) {
> +		regs->verdict.code = NF_DROP;
> +		return;
> +	}
> +
> +	nf_ct_expect_init(exp, NF_CT_EXPECT_CLASS_DEFAULT, priv->l3num,
> +		&ct->tuplehash[!dir].tuple.src.u3,
> +		&ct->tuplehash[!dir].tuple.dst.u3,
> +		priv->l4proto, NULL, &priv->dport);

Coding style: Align parameter to parens:

	nf_ct_expect_init(exp, NF_CT_EXPECT_CLASS_DEFAULT, priv->l3num,
                          &ct->tuplehash[!dir].tuple.src.u3,
                          &ct->tuplehash[!dir].tuple.dst.u3,
                          priv->l4proto, NULL, &priv->dport);

> +	exp->timeout.expires = jiffies + priv->timeout * HZ;
> +
> +	if (nf_ct_expect_related(exp) != 0) {
> +		regs->verdict.code = NF_DROP;
> +	}

No need for curly braces here, with single statement, the following is
fine:

	if (nf_ct_expect_related(exp) != 0)
                regs->verdict.code = NF_DROP;

Thanks for submitting your patch.



[Index of Archives]     [Netfitler Users]     [Berkeley Packet Filter]     [LARTC]     [Bugtraq]     [Yosemite Forum]

  Powered by Linux