[PATCH 0/3] netfilter: nf_tables: cleanup and a bugfix

[PATCH 0/3] netfilter: nf_tables: cleanup and a bugfix

[Patrick McHardy]

These patches contain some cleanup and preparation for variable sized data.
To support variable sized data, we need the length of each key for validation
of the register load. Move the key validation back into the init functions
of meta/ct to do this. This also qualifies as a cleanup in my opinion, the
split is quite unnatural and the code is easier to read by handling get and
set operations seperately.

Additionally a patch to add a missing ifdef for NFT_CT_MARK is included.

Please apply.

[PATCH 1/3] netfilter: nft_ct: add missing ifdef for NFT_MARK setting

[Patrick McHardy]

The set operation for ct mark is only valid if CONFIG_NF_CONNTRACK_MARK is
enabled.

Signed-off-by: Patrick McHardy <kaber@trash.net>
---
 net/netfilter/nft_ct.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/netfilter/nft_ct.c b/net/netfilter/nft_ct.c
index bd0d41e..a2c45bd 100644
--- a/net/netfilter/nft_ct.c
+++ b/net/netfilter/nft_ct.c
@@ -268,8 +268,10 @@ static int nft_ct_init_validate_get(const struct nft_expr *expr,
 static int nft_ct_init_validate_set(uint32_t key)
 {
 	switch (key) {
+#ifdef CONFIG_NF_CONNTRACK_MARK
 	case NFT_CT_MARK:
 		break;
+#endif
 	default:
 		return -EOPNOTSUPP;
 	}
-- 
1.9.0

[PATCH 2/3] netfilter: nft_meta: split nft_meta_init() into two functions for get/set

[Patrick McHardy]

For value spanning multiple registers, we need to validate the length
of data loads. In order to add this to nft_meta, we need the length from
key validation. Split the nft_meta_init() function into two functions
for the get and set operations as preparation for that.

Signed-off-by: Patrick McHardy <kaber@trash.net>
---
 net/netfilter/nft_meta.c | 65 ++++++++++++++++++++++--------------------------
 1 file changed, 30 insertions(+), 35 deletions(-)

diff --git a/net/netfilter/nft_meta.c b/net/netfilter/nft_meta.c
index 425cf39..6d0b8cc2 100644
--- a/net/netfilter/nft_meta.c
+++ b/net/netfilter/nft_meta.c
@@ -170,21 +170,15 @@ static const struct nla_policy nft_meta_policy[NFTA_META_MAX + 1] = {
 	[NFTA_META_SREG]	= { .type = NLA_U32 },
 };
 
-static int nft_meta_init_validate_set(uint32_t key)
+static int nft_meta_get_init(const struct nft_ctx *ctx,
+			     const struct nft_expr *expr,
+			     const struct nlattr * const tb[])
 {
-	switch (key) {
-	case NFT_META_MARK:
-	case NFT_META_PRIORITY:
-	case NFT_META_NFTRACE:
-		return 0;
-	default:
-		return -EOPNOTSUPP;
-	}
-}
+	struct nft_meta *priv = nft_expr_priv(expr);
+	int err;
 
-static int nft_meta_init_validate_get(uint32_t key)
-{
-	switch (key) {
+	priv->key = ntohl(nla_get_be32(tb[NFTA_META_KEY]));
+	switch (priv->key) {
 	case NFT_META_LEN:
 	case NFT_META_PROTOCOL:
 	case NFT_META_NFPROTO:
@@ -205,39 +199,40 @@ static int nft_meta_init_validate_get(uint32_t key)
 #ifdef CONFIG_NETWORK_SECMARK
 	case NFT_META_SECMARK:
 #endif
-		return 0;
+		break;
 	default:
 		return -EOPNOTSUPP;
 	}
 
+	priv->dreg = ntohl(nla_get_be32(tb[NFTA_META_DREG]));
+	err = nft_validate_output_register(priv->dreg);
+	if (err < 0)
+		return err;
+
+	err = nft_validate_data_load(ctx, priv->dreg, NULL, NFT_DATA_VALUE);
+	if (err < 0)
+		return err;
+
+	return 0;
 }
 
-static int nft_meta_init(const struct nft_ctx *ctx, const struct nft_expr *expr,
-			 const struct nlattr * const tb[])
+static int nft_meta_set_init(const struct nft_ctx *ctx,
+			     const struct nft_expr *expr,
+			     const struct nlattr * const tb[])
 {
 	struct nft_meta *priv = nft_expr_priv(expr);
 	int err;
 
 	priv->key = ntohl(nla_get_be32(tb[NFTA_META_KEY]));
-
-	if (tb[NFTA_META_DREG]) {
-		err = nft_meta_init_validate_get(priv->key);
-		if (err < 0)
-			return err;
-
-		priv->dreg = ntohl(nla_get_be32(tb[NFTA_META_DREG]));
-		err = nft_validate_output_register(priv->dreg);
-		if (err < 0)
-			return err;
-
-		return nft_validate_data_load(ctx, priv->dreg, NULL,
-					      NFT_DATA_VALUE);
+	switch (priv->key) {
+	case NFT_META_MARK:
+	case NFT_META_PRIORITY:
+	case NFT_META_NFTRACE:
+		break;
+	default:
+		return -EOPNOTSUPP;
 	}
 
-	err = nft_meta_init_validate_set(priv->key);
-	if (err < 0)
-		return err;
-
 	priv->sreg = ntohl(nla_get_be32(tb[NFTA_META_SREG]));
 	err = nft_validate_input_register(priv->sreg);
 	if (err < 0)
@@ -282,7 +277,7 @@ static const struct nft_expr_ops nft_meta_get_ops = {
 	.type		= &nft_meta_type,
 	.size		= NFT_EXPR_SIZE(sizeof(struct nft_meta)),
 	.eval		= nft_meta_get_eval,
-	.init		= nft_meta_init,
+	.init		= nft_meta_get_init,
 	.dump		= nft_meta_get_dump,
 };
 
@@ -290,7 +285,7 @@ static const struct nft_expr_ops nft_meta_set_ops = {
 	.type		= &nft_meta_type,
 	.size		= NFT_EXPR_SIZE(sizeof(struct nft_meta)),
 	.eval		= nft_meta_set_eval,
-	.init		= nft_meta_init,
+	.init		= nft_meta_set_init,
 	.dump		= nft_meta_set_dump,
 };
 
-- 
1.9.0

[PATCH 3/3] netfilter: nft_ct: split nft_ct_init() into two functions for get/set

[Patrick McHardy]

For value spanning multiple registers, we need to validate the length
of data loads. In order to add this to nft_ct, we need the length from
key validation. Split the nft_ct_init() function into two functions
for the get and set operations as preparation for that.

Signed-off-by: Patrick McHardy <kaber@trash.net>
---
 net/netfilter/nft_ct.c | 96 ++++++++++++++++++++++----------------------------
 1 file changed, 43 insertions(+), 53 deletions(-)

diff --git a/net/netfilter/nft_ct.c b/net/netfilter/nft_ct.c
index a2c45bd..cc56030 100644
--- a/net/netfilter/nft_ct.c
+++ b/net/netfilter/nft_ct.c
@@ -215,22 +215,14 @@ static void nft_ct_l3proto_module_put(uint8_t family)
 		nf_ct_l3proto_module_put(family);
 }
 
-static int nft_ct_init_validate_get(const struct nft_expr *expr,
-				    const struct nlattr * const tb[])
+static int nft_ct_get_init(const struct nft_ctx *ctx,
+			   const struct nft_expr *expr,
+			   const struct nlattr * const tb[])
 {
 	struct nft_ct *priv = nft_expr_priv(expr);
+	int err;
 
-	if (tb[NFTA_CT_DIRECTION] != NULL) {
-		priv->dir = nla_get_u8(tb[NFTA_CT_DIRECTION]);
-		switch (priv->dir) {
-		case IP_CT_DIR_ORIGINAL:
-		case IP_CT_DIR_REPLY:
-			break;
-		default:
-			return -EINVAL;
-		}
-	}
-
+	priv->key = ntohl(nla_get_be32(tb[NFTA_CT_KEY]));
 	switch (priv->key) {
 	case NFT_CT_STATE:
 	case NFT_CT_DIRECTION:
@@ -262,12 +254,42 @@ static int nft_ct_init_validate_get(const struct nft_expr *expr,
 		return -EOPNOTSUPP;
 	}
 
+	if (tb[NFTA_CT_DIRECTION] != NULL) {
+		priv->dir = nla_get_u8(tb[NFTA_CT_DIRECTION]);
+		switch (priv->dir) {
+		case IP_CT_DIR_ORIGINAL:
+		case IP_CT_DIR_REPLY:
+			break;
+		default:
+			return -EINVAL;
+		}
+	}
+
+	priv->dreg = ntohl(nla_get_be32(tb[NFTA_CT_DREG]));
+	err = nft_validate_output_register(priv->dreg);
+	if (err < 0)
+		return err;
+
+	err = nft_validate_data_load(ctx, priv->dreg, NULL, NFT_DATA_VALUE);
+	if (err < 0)
+		return err;
+
+	err = nft_ct_l3proto_try_module_get(ctx->afi->family);
+	if (err < 0)
+		return err;
+
 	return 0;
 }
 
-static int nft_ct_init_validate_set(uint32_t key)
+static int nft_ct_set_init(const struct nft_ctx *ctx,
+			   const struct nft_expr *expr,
+			   const struct nlattr * const tb[])
 {
-	switch (key) {
+	struct nft_ct *priv = nft_expr_priv(expr);
+	int err;
+
+	priv->key = ntohl(nla_get_be32(tb[NFTA_CT_KEY]));
+	switch (priv->key) {
 #ifdef CONFIG_NF_CONNTRACK_MARK
 	case NFT_CT_MARK:
 		break;
@@ -276,42 +298,10 @@ static int nft_ct_init_validate_set(uint32_t key)
 		return -EOPNOTSUPP;
 	}
 
-	return 0;
-}
-
-static int nft_ct_init(const struct nft_ctx *ctx,
-		       const struct nft_expr *expr,
-		       const struct nlattr * const tb[])
-{
-	struct nft_ct *priv = nft_expr_priv(expr);
-	int err;
-
-	priv->key = ntohl(nla_get_be32(tb[NFTA_CT_KEY]));
-
-	if (tb[NFTA_CT_DREG]) {
-		err = nft_ct_init_validate_get(expr, tb);
-		if (err < 0)
-			return err;
-
-		priv->dreg = ntohl(nla_get_be32(tb[NFTA_CT_DREG]));
-		err = nft_validate_output_register(priv->dreg);
-		if (err < 0)
-			return err;
-
-		err = nft_validate_data_load(ctx, priv->dreg, NULL,
-					     NFT_DATA_VALUE);
-		if (err < 0)
-			return err;
-	} else {
-		err = nft_ct_init_validate_set(priv->key);
-		if (err < 0)
-			return err;
-
-		priv->sreg = ntohl(nla_get_be32(tb[NFTA_CT_SREG]));
-		err = nft_validate_input_register(priv->sreg);
-		if (err < 0)
-			return err;
-	}
+	priv->sreg = ntohl(nla_get_be32(tb[NFTA_CT_SREG]));
+	err = nft_validate_input_register(priv->sreg);
+	if (err < 0)
+		return err;
 
 	err = nft_ct_l3proto_try_module_get(ctx->afi->family);
 	if (err < 0)
@@ -372,7 +362,7 @@ static const struct nft_expr_ops nft_ct_get_ops = {
 	.type		= &nft_ct_type,
 	.size		= NFT_EXPR_SIZE(sizeof(struct nft_ct)),
 	.eval		= nft_ct_get_eval,
-	.init		= nft_ct_init,
+	.init		= nft_ct_get_init,
 	.destroy	= nft_ct_destroy,
 	.dump		= nft_ct_get_dump,
 };
@@ -381,7 +371,7 @@ static const struct nft_expr_ops nft_ct_set_ops = {
 	.type		= &nft_ct_type,
 	.size		= NFT_EXPR_SIZE(sizeof(struct nft_ct)),
 	.eval		= nft_ct_set_eval,
-	.init		= nft_ct_init,
+	.init		= nft_ct_set_init,
 	.destroy	= nft_ct_destroy,
 	.dump		= nft_ct_set_dump,
 };
-- 
1.9.0

Re: [PATCH 0/3] netfilter: nf_tables: cleanup and a bugfix

[Pablo Neira Ayuso]

On Sat, Mar 29, 2014 at 10:43:00AM +0000, Patrick McHardy wrote:
> These patches contain some cleanup and preparation for variable sized data.
> To support variable sized data, we need the length of each key for validation
> of the register load. Move the key validation back into the init functions
> of meta/ct to do this. This also qualifies as a cleanup in my opinion, the
> split is quite unnatural and the code is easier to read by handling get and
> set operations seperately.
> 
> Additionally a patch to add a missing ifdef for NFT_CT_MARK is included.

Applied, thanks Patrick.

Re: [PATCH 2/3] netfilter: nft_meta: split nft_meta_init() into two functions for get/set

[Tomasz Bursztyka]

Hi Patrick,

> diff --git a/net/netfilter/nft_meta.c b/net/netfilter/nft_meta.c
> index 425cf39..6d0b8cc2 100644
> --- a/net/netfilter/nft_meta.c
> +++ b/net/netfilter/nft_meta.c
> @@ -170,21 +170,15 @@ static const struct nla_policy nft_meta_policy[NFTA_META_MAX + 1] = {
>   	[NFTA_META_SREG]	= { .type = NLA_U32 },
>   };
>   
> -static int nft_meta_init_validate_set(uint32_t key)
> +static int nft_meta_get_init(const struct nft_ctx *ctx,
> +			     const struct nft_expr *expr,
> +			     const struct nlattr * const tb[])
>   {
> -	switch (key) {
> -	case NFT_META_MARK:
> -	case NFT_META_PRIORITY:
> -	case NFT_META_NFTRACE:
> -		return 0;
> -	default:
> -		return -EOPNOTSUPP;
> -	}
> -}
> +	struct nft_meta *priv = nft_expr_priv(expr);
> +	int err;
>   
> -static int nft_meta_init_validate_get(uint32_t key)
> -{
> -	switch (key) {

With the disappearance of nft_meta_init_validate_get(), I will need to add
an #ifdef/#endif clause for bridge key support. Or I would copy and adapt
this new nft_meta_get_init() in nft_meta_bridge.c.

Don't you think nft_meta_validate_get() could be kept?

> +	priv->key = ntohl(nla_get_be32(tb[NFTA_META_KEY]));
> +	switch (priv->key) {
>   	case NFT_META_LEN:
>   	case NFT_META_PROTOCOL:
>   	case NFT_META_NFPROTO:
> @@ -205,39 +199,40 @@ static int nft_meta_init_validate_get(uint32_t key)
>   #ifdef CONFIG_NETWORK_SECMARK
>   	case NFT_META_SECMARK:
>   #endif
> -		return 0;
> +		break;
>   	default:
>   		return -EOPNOTSUPP;
>   	}
>   
> +	priv->dreg = ntohl(nla_get_be32(tb[NFTA_META_DREG]));
> +	err = nft_validate_output_register(priv->dreg);
> +	if (err < 0)
> +		return err;
> +
> +	err = nft_validate_data_load(ctx, priv->dreg, NULL, NFT_DATA_VALUE);
> +	if (err < 0)
> +		return err;
> +
> +	return 0;
>   }
>   
> -static int nft_meta_init(const struct nft_ctx *ctx, const struct nft_expr *expr,
> -			 const struct nlattr * const tb[])
> +static int nft_meta_set_init(const struct nft_ctx *ctx,
> +			     const struct nft_expr *expr,
> +			     const struct nlattr * const tb[])
>   {
>   	struct nft_meta *priv = nft_expr_priv(expr);
>   	int err;
>   
>   	priv->key = ntohl(nla_get_be32(tb[NFTA_META_KEY]));
> -
> -	if (tb[NFTA_META_DREG]) {
> -		err = nft_meta_init_validate_get(priv->key);
> -		if (err < 0)
> -			return err;
> -
> -		priv->dreg = ntohl(nla_get_be32(tb[NFTA_META_DREG]));
> -		err = nft_validate_output_register(priv->dreg);
> -		if (err < 0)
> -			return err;
> -
> -		return nft_validate_data_load(ctx, priv->dreg, NULL,
> -					      NFT_DATA_VALUE);
> +	switch (priv->key) {
> +	case NFT_META_MARK:
> +	case NFT_META_PRIORITY:
> +	case NFT_META_NFTRACE:
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
>   	}
>   
> -	err = nft_meta_init_validate_set(priv->key);
> -	if (err < 0)
> -		return err;
> -
>   	priv->sreg = ntohl(nla_get_be32(tb[NFTA_META_SREG]));
>   	err = nft_validate_input_register(priv->sreg);
>   	if (err < 0)
> @@ -282,7 +277,7 @@ static const struct nft_expr_ops nft_meta_get_ops = {
>   	.type		= &nft_meta_type,
>   	.size		= NFT_EXPR_SIZE(sizeof(struct nft_meta)),
>   	.eval		= nft_meta_get_eval,
> -	.init		= nft_meta_init,
> +	.init		= nft_meta_get_init,
>   	.dump		= nft_meta_get_dump,
>   };
>   
> @@ -290,7 +285,7 @@ static const struct nft_expr_ops nft_meta_set_ops = {
>   	.type		= &nft_meta_type,
>   	.size		= NFT_EXPR_SIZE(sizeof(struct nft_meta)),
>   	.eval		= nft_meta_set_eval,
> -	.init		= nft_meta_init,
> +	.init		= nft_meta_set_init,
>   	.dump		= nft_meta_set_dump,
>   };
>   

Rest is fine and indeed it simplifies things on my side (no need to c/p 
the old nft_meta_init,
here I can reuse existing nft_meta_set_init)

Tomasz

Re: [PATCH 2/3] netfilter: nft_meta: split nft_meta_init() into two functions for get/set

[Patrick McHardy]

On Mon, Mar 31, 2014 at 03:37:43PM +0300, Tomasz Bursztyka wrote:
> Hi Patrick,
> 
> >diff --git a/net/netfilter/nft_meta.c b/net/netfilter/nft_meta.c
> >index 425cf39..6d0b8cc2 100644
> >--- a/net/netfilter/nft_meta.c
> >+++ b/net/netfilter/nft_meta.c
> >@@ -170,21 +170,15 @@ static const struct nla_policy nft_meta_policy[NFTA_META_MAX + 1] = {
> >  	[NFTA_META_SREG]	= { .type = NLA_U32 },
> >  };
> >-static int nft_meta_init_validate_set(uint32_t key)
> >+static int nft_meta_get_init(const struct nft_ctx *ctx,
> >+			     const struct nft_expr *expr,
> >+			     const struct nlattr * const tb[])
> >  {
> >-	switch (key) {
> >-	case NFT_META_MARK:
> >-	case NFT_META_PRIORITY:
> >-	case NFT_META_NFTRACE:
> >-		return 0;
> >-	default:
> >-		return -EOPNOTSUPP;
> >-	}
> >-}
> >+	struct nft_meta *priv = nft_expr_priv(expr);
> >+	int err;
> >-static int nft_meta_init_validate_get(uint32_t key)
> >-{
> >-	switch (key) {
> 
> With the disappearance of nft_meta_init_validate_get(), I will need to add
> an #ifdef/#endif clause for bridge key support. Or I would copy and adapt
> this new nft_meta_get_init() in nft_meta_bridge.c.
> 
> Don't you think nft_meta_validate_get() could be kept?

Why don't you add a bridge specific init function and invoke
nft_meta_get_init() for unknown keys?

It could be kept by passing a unsigned int *size for returning the size
of the key, but I would prefer to solve this differently.