[PATCH -next v6] nftables: connlabel set support

[PATCH -next v6] nftables: connlabel set support

[Florian Westphal]

Take six.

First (new) patch renames existing nft_ct struct to nft_ct_reg.
Second patch is the real (kernel) patch, adds new nft_ct_imm
struct and uses select_ops to pick it if IMM attr is present.

Last two patches are the userspace changes to libnftnl and nft itself.

[PATCH -next 1/4] netfilter: nft_ct: rename struct nft_ct to nft_ct_reg

[Florian Westphal]

Pablo suggested to have upcoming set support that takes value from an
immediate attribute use distinct struct:

Quoting Pablo:
  'I'd suggest "struct nft_ct_reg" and "struct nft_ct_imm"'.

Make it so.

Suggested-by: Pablo Neira Ayuso <pablo@netfilter.org>
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 net/netfilter/nft_ct.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/net/netfilter/nft_ct.c b/net/netfilter/nft_ct.c
index 25998fa..4f66cb9 100644
--- a/net/netfilter/nft_ct.c
+++ b/net/netfilter/nft_ct.c
@@ -22,7 +22,7 @@
 #include <net/netfilter/nf_conntrack_ecache.h>
 #include <net/netfilter/nf_conntrack_labels.h>
 
-struct nft_ct {
+struct nft_ct_reg {
 	enum nft_ct_keys	key:8;
 	enum ip_conntrack_dir	dir:8;
 	union {
@@ -47,7 +47,7 @@ static void nft_ct_get_eval(const struct nft_expr *expr,
 			    struct nft_regs *regs,
 			    const struct nft_pktinfo *pkt)
 {
-	const struct nft_ct *priv = nft_expr_priv(expr);
+	const struct nft_ct_reg *priv = nft_expr_priv(expr);
 	u32 *dest = &regs->data[priv->dreg];
 	enum ip_conntrack_info ctinfo;
 	const struct nf_conn *ct;
@@ -177,7 +177,7 @@ static void nft_ct_set_eval(const struct nft_expr *expr,
 			    struct nft_regs *regs,
 			    const struct nft_pktinfo *pkt)
 {
-	const struct nft_ct *priv = nft_expr_priv(expr);
+	const struct nft_ct_reg *priv = nft_expr_priv(expr);
 	struct sk_buff *skb = pkt->skb;
 #ifdef CONFIG_NF_CONNTRACK_MARK
 	u32 value = regs->data[priv->sreg];
@@ -247,7 +247,7 @@ 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);
+	struct nft_ct_reg *priv = nft_expr_priv(expr);
 	unsigned int len;
 	int err;
 
@@ -354,7 +354,7 @@ static int nft_ct_set_init(const struct nft_ctx *ctx,
 			   const struct nft_expr *expr,
 			   const struct nlattr * const tb[])
 {
-	struct nft_ct *priv = nft_expr_priv(expr);
+	struct nft_ct_reg *priv = nft_expr_priv(expr);
 	unsigned int len;
 	int err;
 
@@ -381,15 +381,15 @@ static int nft_ct_set_init(const struct nft_ctx *ctx,
 	return 0;
 }
 
-static void nft_ct_destroy(const struct nft_ctx *ctx,
-			   const struct nft_expr *expr)
+static void nft_ct_reg_destroy(const struct nft_ctx *ctx,
+			       const struct nft_expr *expr)
 {
 	nft_ct_l3proto_module_put(ctx->afi->family);
 }
 
 static int nft_ct_get_dump(struct sk_buff *skb, const struct nft_expr *expr)
 {
-	const struct nft_ct *priv = nft_expr_priv(expr);
+	const struct nft_ct_reg *priv = nft_expr_priv(expr);
 
 	if (nft_dump_register(skb, NFTA_CT_DREG, priv->dreg))
 		goto nla_put_failure;
@@ -424,7 +424,7 @@ nla_put_failure:
 
 static int nft_ct_set_dump(struct sk_buff *skb, const struct nft_expr *expr)
 {
-	const struct nft_ct *priv = nft_expr_priv(expr);
+	const struct nft_ct_reg *priv = nft_expr_priv(expr);
 
 	if (nft_dump_register(skb, NFTA_CT_SREG, priv->sreg))
 		goto nla_put_failure;
@@ -439,19 +439,19 @@ nla_put_failure:
 static struct nft_expr_type nft_ct_type;
 static const struct nft_expr_ops nft_ct_get_ops = {
 	.type		= &nft_ct_type,
-	.size		= NFT_EXPR_SIZE(sizeof(struct nft_ct)),
+	.size		= NFT_EXPR_SIZE(sizeof(struct nft_ct_reg)),
 	.eval		= nft_ct_get_eval,
 	.init		= nft_ct_get_init,
-	.destroy	= nft_ct_destroy,
+	.destroy	= nft_ct_reg_destroy,
 	.dump		= nft_ct_get_dump,
 };
 
 static const struct nft_expr_ops nft_ct_set_ops = {
 	.type		= &nft_ct_type,
-	.size		= NFT_EXPR_SIZE(sizeof(struct nft_ct)),
+	.size		= NFT_EXPR_SIZE(sizeof(struct nft_ct_reg)),
 	.eval		= nft_ct_set_eval,
 	.init		= nft_ct_set_init,
-	.destroy	= nft_ct_destroy,
+	.destroy	= nft_ct_reg_destroy,
 	.dump		= nft_ct_set_dump,
 };
 
-- 
2.7.3

[PATCH v6 -next 2/4] netfilter: nftables: add connlabel set support

[Florian Westphal]

Instead of taking the value to set from a source register, userspace
passes the bit that we should set as an immediate netlink value.

This follows a similar approach that xtables 'connlabel'
match uses, so when user inputs

    ct label set bar

then we will set the bit used by the 'bar' label and leave the rest alone.
Pablo suggested to re-use the immediate attributes already used by
nft_immediate, nft_bitwise and nft_cmp to re-use as much code as
possible.

Just add new NFTA_CT_IMM that contains nested data attributes.
We can then use nft_data_init and nft_data_dump for this as well.

In case we can't set a label because the conntrack extension is missing
we abort rule evaluation.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 Changes since v5:
  - use a different struct (nft_ct_imm) and use select_ops
  to pick it if an immediate attribute was provided.

 include/uapi/linux/netfilter/nf_tables.h |   2 +
 net/netfilter/nft_ct.c                   | 131 +++++++++++++++++++++++++++++++
 2 files changed, 133 insertions(+)

diff --git a/include/uapi/linux/netfilter/nf_tables.h b/include/uapi/linux/netfilter/nf_tables.h
index eeffde1..608b647 100644
--- a/include/uapi/linux/netfilter/nf_tables.h
+++ b/include/uapi/linux/netfilter/nf_tables.h
@@ -770,6 +770,7 @@ enum nft_ct_keys {
  * @NFTA_CT_KEY: conntrack data item to load (NLA_U32: nft_ct_keys)
  * @NFTA_CT_DIRECTION: direction in case of directional keys (NLA_U8)
  * @NFTA_CT_SREG: source register (NLA_U32)
+ * @NFTA_CT_IMM: immediate value (NLA_NESTED)
  */
 enum nft_ct_attributes {
 	NFTA_CT_UNSPEC,
@@ -777,6 +778,7 @@ enum nft_ct_attributes {
 	NFTA_CT_KEY,
 	NFTA_CT_DIRECTION,
 	NFTA_CT_SREG,
+	NFTA_CT_IMM,
 	__NFTA_CT_MAX
 };
 #define NFTA_CT_MAX		(__NFTA_CT_MAX - 1)
diff --git a/net/netfilter/nft_ct.c b/net/netfilter/nft_ct.c
index 4f66cb9..a461c3e 100644
--- a/net/netfilter/nft_ct.c
+++ b/net/netfilter/nft_ct.c
@@ -31,6 +31,15 @@ struct nft_ct_reg {
 	};
 };
 
+struct nft_ct_imm {
+	enum nft_ct_keys	key:8;
+	union {
+		u8		set_bit;
+	} imm;
+	unsigned int		imm_len:8;
+	struct nft_data		immediate;
+};
+
 static u64 nft_ct_get_eval_counter(const struct nf_conn_counter *c,
 				   enum nft_ct_keys k,
 				   enum ip_conntrack_dir d)
@@ -203,11 +212,40 @@ static void nft_ct_set_eval(const struct nft_expr *expr,
 	}
 }
 
+static void nft_ct_imm_eval(const struct nft_expr *expr,
+			    struct nft_regs *regs,
+			    const struct nft_pktinfo *pkt)
+{
+	const struct nft_ct_imm *priv = nft_expr_priv(expr);
+	struct sk_buff *skb = pkt->skb;
+	enum ip_conntrack_info ctinfo;
+	struct nf_conn *ct;
+
+	ct = nf_ct_get(skb, &ctinfo);
+	if (!ct)
+		return;
+
+	switch (priv->key) {
+#ifdef CONFIG_NF_CONNTRACK_LABELS
+	case NFT_CT_LABELS:
+		if (nf_connlabel_set(ct, priv->imm.set_bit))
+			goto err;
+		break;
+#endif
+	default:
+		break;
+	}
+	return;
+err:
+	regs->verdict.code = NFT_BREAK;
+}
+
 static const struct nla_policy nft_ct_policy[NFTA_CT_MAX + 1] = {
 	[NFTA_CT_DREG]		= { .type = NLA_U32 },
 	[NFTA_CT_KEY]		= { .type = NLA_U32 },
 	[NFTA_CT_DIRECTION]	= { .type = NLA_U8 },
 	[NFTA_CT_SREG]		= { .type = NLA_U32 },
+	[NFTA_CT_IMM]		= { .type = NLA_NESTED },
 };
 
 static int nft_ct_l3proto_try_module_get(uint8_t family)
@@ -381,12 +419,78 @@ static int nft_ct_set_init(const struct nft_ctx *ctx,
 	return 0;
 }
 
+static int nft_ct_imm_init(const struct nft_ctx *ctx,
+			   const struct nft_expr *expr,
+			   const struct nlattr * const tb[])
+{
+	struct nft_ct_imm *priv = nft_expr_priv(expr);
+	struct nft_data_desc imm_desc = {};
+	int err;
+
+	err = nft_data_init(NULL, &priv->immediate, sizeof(priv->immediate),
+			    &imm_desc, tb[NFTA_CT_IMM]);
+	if (err < 0)
+		return err;
+
+	if (imm_desc.type != NFT_DATA_VALUE)
+		return -EINVAL;
+
+	err = nft_ct_l3proto_try_module_get(ctx->afi->family);
+	if (err < 0)
+		return err;
+
+	priv->imm_len = imm_desc.len;
+	priv->key = ntohl(nla_get_be32(tb[NFTA_CT_KEY]));
+	switch (priv->key) {
+#ifdef CONFIG_NF_CONNTRACK_LABELS
+	case NFT_CT_LABELS:
+		err = -EINVAL;
+		if (tb[NFTA_CT_DIRECTION] || priv->imm_len != sizeof(u32))
+			goto err;
+
+		err = nf_connlabels_get(ctx->net,
+					htonl(priv->immediate.data[0]));
+		if (err < 0)
+			goto err;
+
+		priv->imm.set_bit = htonl(priv->immediate.data[0]);
+		break;
+#endif
+	default:
+		err = -EOPNOTSUPP;
+		goto err;
+	}
+
+	return 0;
+ err:
+	nft_ct_l3proto_module_put(ctx->afi->family);
+	return err;
+}
+
 static void nft_ct_reg_destroy(const struct nft_ctx *ctx,
 			       const struct nft_expr *expr)
 {
 	nft_ct_l3proto_module_put(ctx->afi->family);
 }
 
+static void nft_ct_imm_destroy(const struct nft_ctx *ctx,
+			       const struct nft_expr *expr)
+{
+	struct nft_ct_imm *priv = nft_expr_priv(expr);
+
+	switch (priv->key) {
+#ifdef CONFIG_NF_CONNTRACK_LABELS
+	case NFT_CT_LABELS:
+		nf_connlabels_put(ctx->net);
+		break;
+#endif
+	default:
+		break;
+	}
+
+	nft_ct_l3proto_module_put(ctx->afi->family);
+}
+
 static int nft_ct_get_dump(struct sk_buff *skb, const struct nft_expr *expr)
 {
 	const struct nft_ct_reg *priv = nft_expr_priv(expr);
@@ -436,6 +540,18 @@ nla_put_failure:
 	return -1;
 }
 
+static int nft_ct_imm_dump(struct sk_buff *skb, const struct nft_expr *expr)
+{
+	const struct nft_ct_imm *priv = nft_expr_priv(expr);
+
+	if (nla_put_be32(skb, NFTA_CT_KEY, htonl(priv->key)))
+		return -1;
+	if (nft_data_dump(skb, NFTA_CT_IMM, &priv->immediate,
+			  NFT_DATA_VALUE, priv->imm_len) < 0)
+		return -1;
+	return 0;
+}
+
 static struct nft_expr_type nft_ct_type;
 static const struct nft_expr_ops nft_ct_get_ops = {
 	.type		= &nft_ct_type,
@@ -455,6 +571,15 @@ static const struct nft_expr_ops nft_ct_set_ops = {
 	.dump		= nft_ct_set_dump,
 };
 
+static const struct nft_expr_ops nft_ct_imm_ops = {
+	.type		= &nft_ct_type,
+	.size		= NFT_EXPR_SIZE(sizeof(struct nft_ct_imm)),
+	.eval		= nft_ct_imm_eval,
+	.init		= nft_ct_imm_init,
+	.destroy	= nft_ct_imm_destroy,
+	.dump		= nft_ct_imm_dump,
+};
+
 static const struct nft_expr_ops *
 nft_ct_select_ops(const struct nft_ctx *ctx,
 		    const struct nlattr * const tb[])
@@ -465,12 +590,18 @@ nft_ct_select_ops(const struct nft_ctx *ctx,
 	if (tb[NFTA_CT_DREG] && tb[NFTA_CT_SREG])
 		return ERR_PTR(-EINVAL);
 
+	if (tb[NFTA_CT_SREG] && tb[NFTA_CT_IMM])
+		return ERR_PTR(-EINVAL);
+
 	if (tb[NFTA_CT_DREG])
 		return &nft_ct_get_ops;
 
 	if (tb[NFTA_CT_SREG])
 		return &nft_ct_set_ops;
 
+	if (tb[NFTA_CT_IMM])
+		return &nft_ct_imm_ops;
+
 	return ERR_PTR(-EINVAL);
 }
 
-- 
2.7.3

Re: [PATCH v6 -next 2/4] netfilter: nftables: add connlabel set support

[Patrick McHardy]

On 21.04, Florian Westphal wrote:
> Instead of taking the value to set from a source register, userspace
> passes the bit that we should set as an immediate netlink value.
> 
> This follows a similar approach that xtables 'connlabel'
> match uses, so when user inputs
> 
>     ct label set bar
> 
> then we will set the bit used by the 'bar' label and leave the rest alone.
> Pablo suggested to re-use the immediate attributes already used by
> nft_immediate, nft_bitwise and nft_cmp to re-use as much code as
> possible.
> 
> Just add new NFTA_CT_IMM that contains nested data attributes.
> We can then use nft_data_init and nft_data_dump for this as well.

What's the argument against using immediate and a register?

> diff --git a/net/netfilter/nft_ct.c b/net/netfilter/nft_ct.c
> index 4f66cb9..a461c3e 100644
> --- a/net/netfilter/nft_ct.c
> +++ b/net/netfilter/nft_ct.c
> @@ -31,6 +31,15 @@ struct nft_ct_reg {
>  	};
>  };
>  
> +struct nft_ct_imm {
> +	enum nft_ct_keys	key:8;
> +	union {
> +		u8		set_bit;
> +	} imm;
> +	unsigned int		imm_len:8;
> +	struct nft_data		immediate;

Is this really needed? It should be possible to reconstruct from the bit, no? 

> +static void nft_ct_imm_eval(const struct nft_expr *expr,
> +			    struct nft_regs *regs,
> +			    const struct nft_pktinfo *pkt)
> +{
> +	const struct nft_ct_imm *priv = nft_expr_priv(expr);
> +	struct sk_buff *skb = pkt->skb;
> +	enum ip_conntrack_info ctinfo;
> +	struct nf_conn *ct;
> +
> +	ct = nf_ct_get(skb, &ctinfo);
> +	if (!ct)
> +		return;
> +
> +	switch (priv->key) {
> +#ifdef CONFIG_NF_CONNTRACK_LABELS
> +	case NFT_CT_LABELS:
> +		if (nf_connlabel_set(ct, priv->imm.set_bit))
> +			goto err;
> +		break;
> +#endif
> +	default:
> +		break;
> +	}
> +	return;
> +err:
> +	regs->verdict.code = NFT_BREAK;
> +}
> +
>  static const struct nla_policy nft_ct_policy[NFTA_CT_MAX + 1] = {
>  	[NFTA_CT_DREG]		= { .type = NLA_U32 },
>  	[NFTA_CT_KEY]		= { .type = NLA_U32 },
>  	[NFTA_CT_DIRECTION]	= { .type = NLA_U8 },
>  	[NFTA_CT_SREG]		= { .type = NLA_U32 },
> +	[NFTA_CT_IMM]		= { .type = NLA_NESTED },
>  };
>  
>  static int nft_ct_l3proto_try_module_get(uint8_t family)
> @@ -381,12 +419,78 @@ static int nft_ct_set_init(const struct nft_ctx *ctx,
>  	return 0;
>  }
>  
> +static int nft_ct_imm_init(const struct nft_ctx *ctx,
> +			   const struct nft_expr *expr,
> +			   const struct nlattr * const tb[])
> +{
> +	struct nft_ct_imm *priv = nft_expr_priv(expr);
> +	struct nft_data_desc imm_desc = {};
> +	int err;
> +
> +	err = nft_data_init(NULL, &priv->immediate, sizeof(priv->immediate),
> +			    &imm_desc, tb[NFTA_CT_IMM]);
> +	if (err < 0)
> +		return err;
> +
> +	if (imm_desc.type != NFT_DATA_VALUE)
> +		return -EINVAL;
> +
> +	err = nft_ct_l3proto_try_module_get(ctx->afi->family);
> +	if (err < 0)
> +		return err;

What about the inet table?

> +
> +	priv->imm_len = imm_desc.len;
> +	priv->key = ntohl(nla_get_be32(tb[NFTA_CT_KEY]));
> +	switch (priv->key) {
> +#ifdef CONFIG_NF_CONNTRACK_LABELS
> +	case NFT_CT_LABELS:
> +		err = -EINVAL;
> +		if (tb[NFTA_CT_DIRECTION] || priv->imm_len != sizeof(u32))

Why do we need to store imm_len if the size is fixed?

> +			goto err;
> +
> +		err = nf_connlabels_get(ctx->net,
> +					htonl(priv->immediate.data[0]));
> +		if (err < 0)
> +			goto err;
> +
> +		priv->imm.set_bit = htonl(priv->immediate.data[0]);
> +		break;
> +#endif
> +	default:
> +		err = -EOPNOTSUPP;
> +		goto err;
> +	}
> +
> +	return 0;
> + err:
> +	nft_ct_l3proto_module_put(ctx->afi->family);
> +	return err;
> +}
> +
>  static void nft_ct_reg_destroy(const struct nft_ctx *ctx,
>  			       const struct nft_expr *expr)
>  {
>  	nft_ct_l3proto_module_put(ctx->afi->family);
>  }
>  
> +static void nft_ct_imm_destroy(const struct nft_ctx *ctx,
> +			       const struct nft_expr *expr)
> +{
> +	struct nft_ct_imm *priv = nft_expr_priv(expr);
> +
> +	switch (priv->key) {
> +#ifdef CONFIG_NF_CONNTRACK_LABELS
> +	case NFT_CT_LABELS:
> +		nf_connlabels_put(ctx->net);
> +		break;
> +#endif
> +	default:
> +		break;
> +	}
> +
> +	nft_ct_l3proto_module_put(ctx->afi->family);
> +}
> +
>  static int nft_ct_get_dump(struct sk_buff *skb, const struct nft_expr *expr)
>  {
>  	const struct nft_ct_reg *priv = nft_expr_priv(expr);
> @@ -436,6 +540,18 @@ nla_put_failure:
>  	return -1;
>  }
>  
> +static int nft_ct_imm_dump(struct sk_buff *skb, const struct nft_expr *expr)
> +{
> +	const struct nft_ct_imm *priv = nft_expr_priv(expr);
> +
> +	if (nla_put_be32(skb, NFTA_CT_KEY, htonl(priv->key)))
> +		return -1;
> +	if (nft_data_dump(skb, NFTA_CT_IMM, &priv->immediate,
> +			  NFT_DATA_VALUE, priv->imm_len) < 0)
> +		return -1;
> +	return 0;
> +}
> +
>  static struct nft_expr_type nft_ct_type;
>  static const struct nft_expr_ops nft_ct_get_ops = {
>  	.type		= &nft_ct_type,
> @@ -455,6 +571,15 @@ static const struct nft_expr_ops nft_ct_set_ops = {
>  	.dump		= nft_ct_set_dump,
>  };
>  
> +static const struct nft_expr_ops nft_ct_imm_ops = {
> +	.type		= &nft_ct_type,
> +	.size		= NFT_EXPR_SIZE(sizeof(struct nft_ct_imm)),
> +	.eval		= nft_ct_imm_eval,
> +	.init		= nft_ct_imm_init,
> +	.destroy	= nft_ct_imm_destroy,
> +	.dump		= nft_ct_imm_dump,
> +};
> +
>  static const struct nft_expr_ops *
>  nft_ct_select_ops(const struct nft_ctx *ctx,
>  		    const struct nlattr * const tb[])
> @@ -465,12 +590,18 @@ nft_ct_select_ops(const struct nft_ctx *ctx,
>  	if (tb[NFTA_CT_DREG] && tb[NFTA_CT_SREG])
>  		return ERR_PTR(-EINVAL);
>  
> +	if (tb[NFTA_CT_SREG] && tb[NFTA_CT_IMM])
> +		return ERR_PTR(-EINVAL);
> +
>  	if (tb[NFTA_CT_DREG])
>  		return &nft_ct_get_ops;
>  
>  	if (tb[NFTA_CT_SREG])
>  		return &nft_ct_set_ops;
>  
> +	if (tb[NFTA_CT_IMM])
> +		return &nft_ct_imm_ops;
> +
>  	return ERR_PTR(-EINVAL);
>  }
>  
> -- 
> 2.7.3
> 
> --
> 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
> 

Re: [PATCH v6 -next 2/4] netfilter: nftables: add connlabel set support

[Florian Westphal]

Patrick McHardy <kaber@trash.net> wrote:
> On 21.04, Florian Westphal wrote:
> > Instead of taking the value to set from a source register, userspace
> > passes the bit that we should set as an immediate netlink value.
> > 
> > This follows a similar approach that xtables 'connlabel'
> > match uses, so when user inputs
> > 
> >     ct label set bar
> > 
> > then we will set the bit used by the 'bar' label and leave the rest alone.
> > Pablo suggested to re-use the immediate attributes already used by
> > nft_immediate, nft_bitwise and nft_cmp to re-use as much code as
> > possible.
> > 
> > Just add new NFTA_CT_IMM that contains nested data attributes.
> > We can then use nft_data_init and nft_data_dump for this as well.
> 
> What's the argument against using immediate and a register?

http://marc.info/?l=netfilter-devel&m=145800804914781&w=2

<quote>
> However, with nft, the input is just a register with arbitrary runtime
> content.
> 
> We therefore ask for the upper ceiling we currently have, which is
> enough room to store 128 bits.

We can probably allow passing the label value as attribute to the
nft_ct expression so you don't have to use the upper ceiling. Patrick
suggested something similar for nft_ct set helper support.
</unqote>

> > diff --git a/net/netfilter/nft_ct.c b/net/netfilter/nft_ct.c
> > index 4f66cb9..a461c3e 100644
> > --- a/net/netfilter/nft_ct.c
> > +++ b/net/netfilter/nft_ct.c
> > @@ -31,6 +31,15 @@ struct nft_ct_reg {
> >  	};
> >  };
> >  
> > +struct nft_ct_imm {
> > +	enum nft_ct_keys	key:8;
> > +	union {
> > +		u8		set_bit;
> > +	} imm;
> > +	unsigned int		imm_len:8;
> > +	struct nft_data		immediate;
> 
> Is this really needed? It should be possible to reconstruct from the bit, no? 

Yes, I can change that but it seemed much more simple to just stash the
original value/length and dump that via

	if (nft_data_dump(skb, NFTA_CT_IMM, &priv->immediate,
		  NFT_DATA_VALUE, priv->imm_len) < 0)

Rather than rebuilding nft_data for all supported keys
(right now its only one, but there was interest to add helper assignment
 support too).

> > +	err = nft_ct_l3proto_try_module_get(ctx->afi->family);
> > +	if (err < 0)
> > +		return err;
> 
> What about the inet table?

nft_ct_l3proto_try_module_get() dispatches to IPV4 and IPV6 if
afi->family is INET.

> > +#ifdef CONFIG_NF_CONNTRACK_LABELS
> > +	case NFT_CT_LABELS:
> > +		err = -EINVAL;
> > +		if (tb[NFTA_CT_DIRECTION] || priv->imm_len != sizeof(u32))
> 
> Why do we need to store imm_len if the size is fixed?

We don't, but then dumping would be something like

struct nft_data         immediate = {};

switch (key) {
case NFT_CT_LABELS:
	size = sizeof(u32);
	immedate.data[0] = htonl(priv->immediate.data[0]);
	break;
case ...
}

if (nft_data_dump(skb, NFTA_CT_IMM, &immediate,
                  NFT_DATA_VALUE, size)) ...

If that is preferred I can change it accordingly.

Re: [PATCH v6 -next 2/4] netfilter: nftables: add connlabel set support

[Patrick McHardy]

On 25.04, Florian Westphal wrote:
> Patrick McHardy <kaber@trash.net> wrote:
> > On 21.04, Florian Westphal wrote:
> > > Pablo suggested to re-use the immediate attributes already used by
> > > nft_immediate, nft_bitwise and nft_cmp to re-use as much code as
> > > possible.
> > > 
> > > Just add new NFTA_CT_IMM that contains nested data attributes.
> > > We can then use nft_data_init and nft_data_dump for this as well.
> > 
> > What's the argument against using immediate and a register?
> 
> http://marc.info/?l=netfilter-devel&m=145800804914781&w=2
> 
> <quote>
> > However, with nft, the input is just a register with arbitrary runtime
> > content.
> > 
> > We therefore ask for the upper ceiling we currently have, which is
> > enough room to store 128 bits.
> 
> We can probably allow passing the label value as attribute to the
> nft_ct expression so you don't have to use the upper ceiling. Patrick
> suggested something similar for nft_ct set helper support.
> </unqote>

Helpers are somewhat special because we need to load the modules and get
a reference to the helper structure, so we need the context of what the
immediate will be used for. I'd certainly prefer not to use immediates
since that means we'll need a single rule per assignment. Especially with
helpers it seems a lot nicer to simply use a map.

The alternative to internally handling it would be to some propagating
validation to immediates / sets which invoke the actual user of the data.
So in the case of helpers, we could replace the name by references to
the helper structures and reverse this during dumping.

Regarding connlabels this doesn't really apply though. We expect userspace
to create a reasonable ruleset and anything that does not cause critical
errors is validated in userspace.

> > > diff --git a/net/netfilter/nft_ct.c b/net/netfilter/nft_ct.c
> > > index 4f66cb9..a461c3e 100644
> > > --- a/net/netfilter/nft_ct.c
> > > +++ b/net/netfilter/nft_ct.c
> > > @@ -31,6 +31,15 @@ struct nft_ct_reg {
> > >  	};
> > >  };
> > >  
> > > +struct nft_ct_imm {
> > > +	enum nft_ct_keys	key:8;
> > > +	union {
> > > +		u8		set_bit;
> > > +	} imm;
> > > +	unsigned int		imm_len:8;
> > > +	struct nft_data		immediate;
> > 
> > Is this really needed? It should be possible to reconstruct from the bit, no? 
> 
> Yes, I can change that but it seemed much more simple to just stash the
> original value/length and dump that via
> 
> 	if (nft_data_dump(skb, NFTA_CT_IMM, &priv->immediate,
> 		  NFT_DATA_VALUE, priv->imm_len) < 0)
> 
> Rather than rebuilding nft_data for all supported keys
> (right now its only one, but there was interest to add helper assignment
>  support too).

Well, we try to keep the runtime data as compact as possible, so if it is
not *really* needed, I'd suggest reconstructing it.

> > > +	err = nft_ct_l3proto_try_module_get(ctx->afi->family);
> > > +	if (err < 0)
> > > +		return err;
> > 
> > What about the inet table?
> 
> nft_ct_l3proto_try_module_get() dispatches to IPV4 and IPV6 if
> afi->family is INET.

I wasn't aware of that. That's what I would have suggested to do instead :)

> > > +#ifdef CONFIG_NF_CONNTRACK_LABELS
> > > +	case NFT_CT_LABELS:
> > > +		err = -EINVAL;
> > > +		if (tb[NFTA_CT_DIRECTION] || priv->imm_len != sizeof(u32))
> > 
> > Why do we need to store imm_len if the size is fixed?
> 
> We don't, but then dumping would be something like
> 
> struct nft_data         immediate = {};
> 
> switch (key) {
> case NFT_CT_LABELS:
> 	size = sizeof(u32);
> 	immedate.data[0] = htonl(priv->immediate.data[0]);
> 	break;
> case ...
> }
> 
> if (nft_data_dump(skb, NFTA_CT_IMM, &immediate,
>                   NFT_DATA_VALUE, size)) ...
> 
> If that is preferred I can change it accordingly.

Same argument as for the immediate data, we want to keep the size to the
absolute minimum. Storing a fixed value seems rather wasteful.

Re: [PATCH v6 -next 2/4] netfilter: nftables: add connlabel set support

[Florian Westphal]

Patrick McHardy <kaber@trash.net> wrote:
> On 25.04, Florian Westphal wrote:
> > Patrick McHardy <kaber@trash.net> wrote:
> > > On 21.04, Florian Westphal wrote:
> > > > Pablo suggested to re-use the immediate attributes already used by
> > > > nft_immediate, nft_bitwise and nft_cmp to re-use as much code as
> > > > possible.
> > > > 
> > > > Just add new NFTA_CT_IMM that contains nested data attributes.
> > > > We can then use nft_data_init and nft_data_dump for this as well.
> > > 
> > > What's the argument against using immediate and a register?
> > 
> > http://marc.info/?l=netfilter-devel&m=145800804914781&w=2
> > 
> > <quote>
> > > However, with nft, the input is just a register with arbitrary runtime
> > > content.
> > > 
> > > We therefore ask for the upper ceiling we currently have, which is
> > > enough room to store 128 bits.
> > 
> > We can probably allow passing the label value as attribute to the
> > nft_ct expression so you don't have to use the upper ceiling. Patrick
> > suggested something similar for nft_ct set helper support.
> > </unqote>
> 
> Helpers are somewhat special because we need to load the modules and get
> a reference to the helper structure, so we need the context of what the
> immediate will be used for. I'd certainly prefer not to use immediates
> since that means we'll need a single rule per assignment. Especially with
> helpers it seems a lot nicer to simply use a map.

Okay, fair enough.

> The alternative to internally handling it would be to some propagating
> validation to immediates / sets which invoke the actual user of the data.
> So in the case of helpers, we could replace the name by references to
> the helper structures and reverse this during dumping.
> 
> Regarding connlabels this doesn't really apply though. We expect userspace
> to create a reasonable ruleset and anything that does not cause critical
> errors is validated in userspace.

Yes.  So we have three choices here (pseudo-code)

memcpy(ct->labels, regs->data[priv->sreg], sizeof(reg));
vs.
set_bit(priv->imm, ct->labels);

The latter is what the iptables module does, I do not mind if we
go for #1 (treat the label area just like an 128bit register and
replace it completely with whatever is in the source register).

My only problem is that Pablo suggested #2 whereas you recommend #1.

I don't want to resubmit until there is consensus as to what the
preferred solution is.

We could go for a 3rd alternative, namely:

u16 bit = regs->data[priv->sreg];
set_bit(bit, ct->labels);

i.e. have userspace place the _bit_ that we want to set in the
source register.

If we go for sreg that would be my favored solution.

The only drawback vs #1 is that get and set work differently
(get places all labels into dreg, set expects bit to set).

(We also need to validate at eval time but thats not a problem
 in this case).

Re: [PATCH v6 -next 2/4] netfilter: nftables: add connlabel set support

[Pablo Neira Ayuso]

On Mon, Apr 25, 2016 at 01:56:22PM +0200, Florian Westphal wrote:
> Patrick McHardy <kaber@trash.net> wrote:
> > On 25.04, Florian Westphal wrote:
> > > Patrick McHardy <kaber@trash.net> wrote:
> > > > On 21.04, Florian Westphal wrote:
> > > > > Pablo suggested to re-use the immediate attributes already used by
> > > > > nft_immediate, nft_bitwise and nft_cmp to re-use as much code as
> > > > > possible.
> > > > > 
> > > > > Just add new NFTA_CT_IMM that contains nested data attributes.
> > > > > We can then use nft_data_init and nft_data_dump for this as well.
> > > > 
> > > > What's the argument against using immediate and a register?
> > > 
> > > http://marc.info/?l=netfilter-devel&m=145800804914781&w=2
> > > 
> > > <quote>
> > > > However, with nft, the input is just a register with arbitrary runtime
> > > > content.
> > > > 
> > > > We therefore ask for the upper ceiling we currently have, which is
> > > > enough room to store 128 bits.
> > > 
> > > We can probably allow passing the label value as attribute to the
> > > nft_ct expression so you don't have to use the upper ceiling. Patrick
> > > suggested something similar for nft_ct set helper support.
> > > </unqote>
> > 
> > Helpers are somewhat special because we need to load the modules and get
> > a reference to the helper structure, so we need the context of what the
> > immediate will be used for. I'd certainly prefer not to use immediates
> > since that means we'll need a single rule per assignment. Especially with
> > helpers it seems a lot nicer to simply use a map.
> 
> Okay, fair enough.
> 
> > The alternative to internally handling it would be to some propagating
> > validation to immediates / sets which invoke the actual user of the data.
> > So in the case of helpers, we could replace the name by references to
> > the helper structures and reverse this during dumping.
> > 
> > Regarding connlabels this doesn't really apply though. We expect userspace
> > to create a reasonable ruleset and anything that does not cause critical
> > errors is validated in userspace.
> 
> Yes.  So we have three choices here (pseudo-code)
> 
> memcpy(ct->labels, regs->data[priv->sreg], sizeof(reg));
> vs.
> set_bit(priv->imm, ct->labels);
> 
> The latter is what the iptables module does, I do not mind if we
> go for #1 (treat the label area just like an 128bit register and
> replace it completely with whatever is in the source register).
> 
> My only problem is that Pablo suggested #2 whereas you recommend #1.

I don't think we should go memcpy(). This won't be equivalent to what
iptables does.

Different thing is to indicate the bit number from an immediate, ie.
we use set_bit() based on the register data that we get, so we can use
maps as Patrick suggests.

> I don't want to resubmit until there is consensus as to what the
> preferred solution is.
>
> We could go for a 3rd alternative, namely:
> 
> u16 bit = regs->data[priv->sreg];
> set_bit(bit, ct->labels);
>
> i.e. have userspace place the _bit_ that we want to set in the
> source register.
> 
> If we go for sreg that would be my favored solution.

I'm fine with this.

> The only drawback vs #1 is that get and set work differently
> (get places all labels into dreg, set expects bit to set).
> 
> (We also need to validate at eval time but thats not a problem
>  in this case).

You mean a check to make sure we don't go over the boundary, just to
avoid crashing. That seems OK to me.

We can still reuse part of your ct immediate work for the helper setup
anyway.

Re: [PATCH v6 -next 2/4] netfilter: nftables: add connlabel set support

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> Different thing is to indicate the bit number from an immediate, ie.
> we use set_bit() based on the register data that we get, so we can use
> maps as Patrick suggests.

Right.

> > I don't want to resubmit until there is consensus as to what the
> > preferred solution is.
> >
> > We could go for a 3rd alternative, namely:
> > 
> > u16 bit = regs->data[priv->sreg];
> > set_bit(bit, ct->labels);
> >
> > i.e. have userspace place the _bit_ that we want to set in the
> > source register.
> > 
> > If we go for sreg that would be my favored solution.
> 
> I'm fine with this.

Ok.  Unless Patrick objects this is what I'll work on, i.e.
have nft_ct grab the bit number to toggle from the source register.

> > The only drawback vs #1 is that get and set work differently
> > (get places all labels into dreg, set expects bit to set).
> > 
> > (We also need to validate at eval time but thats not a problem
> >  in this case).
> 
> You mean a check to make sure we don't go over the boundary, just to
> avoid crashing. That seems OK to me.

Yes, thats what I meant.

Re: [PATCH v6 -next 2/4] netfilter: nftables: add connlabel set support

[Patrick McHardy]

On 25.04, Florian Westphal wrote:
> Patrick McHardy <kaber@trash.net> wrote:
> 
> > The alternative to internally handling it would be to some propagating
> > validation to immediates / sets which invoke the actual user of the data.
> > So in the case of helpers, we could replace the name by references to
> > the helper structures and reverse this during dumping.
> > 
> > Regarding connlabels this doesn't really apply though. We expect userspace
> > to create a reasonable ruleset and anything that does not cause critical
> > errors is validated in userspace.
> 
> Yes.  So we have three choices here (pseudo-code)
> 
> memcpy(ct->labels, regs->data[priv->sreg], sizeof(reg));
> vs.
> set_bit(priv->imm, ct->labels);
> 
> The latter is what the iptables module does, I do not mind if we
> go for #1 (treat the label area just like an 128bit register and
> replace it completely with whatever is in the source register).
> 
> My only problem is that Pablo suggested #2 whereas you recommend #1.
> 
> I don't want to resubmit until there is consensus as to what the
> preferred solution is.
> 
> We could go for a 3rd alternative, namely:
> 
> u16 bit = regs->data[priv->sreg];
> set_bit(bit, ct->labels);
> 
> i.e. have userspace place the _bit_ that we want to set in the
> source register.
> 
> If we go for sreg that would be my favored solution.
> 
> The only drawback vs #1 is that get and set work differently
> (get places all labels into dreg, set expects bit to set).

That seems like a problem. I agree that #3 would generally be fine, but
we should also really have "ct labels set ct labels" not change the labels,
that would be highly counterintuitive.

Re: [PATCH v6 -next 2/4] netfilter: nftables: add connlabel set support

[Florian Westphal]

Patrick McHardy <kaber@trash.net> wrote:
> On 25.04, Florian Westphal wrote:
> > We could go for a 3rd alternative, namely:
> > 
> > u16 bit = regs->data[priv->sreg];
> > set_bit(bit, ct->labels);
> > 
> > i.e. have userspace place the _bit_ that we want to set in the
> > source register.
> > 
> > If we go for sreg that would be my favored solution.
> > 
> > The only drawback vs #1 is that get and set work differently
> > (get places all labels into dreg, set expects bit to set).
> 
> That seems like a problem. I agree that #3 would generally be fine, but
> we should also really have "ct labels set ct labels" not change the labels,
> that would be highly counterintuitive.

Right.  What about just re-working this approach:

https://patchwork.ozlabs.org/patch/613136/

(sans the CT_IMM part)?  We'd reject all expressions other than
EXPR_VALUE in the eval phase -- 'ct labels set ct labels' would yield
'label expected' error message.

Does that seem acceptable to you?

If not, I see no choice other than resubmitting the original V1 kernel
patch that simply copied the entire sreg into the label area, this way
no userspace changes are needed.

Re: [PATCH v6 -next 2/4] netfilter: nftables: add connlabel set support

[Patrick McHardy]

On 25.04, Florian Westphal wrote:
> Patrick McHardy <kaber@trash.net> wrote:
> > On 25.04, Florian Westphal wrote:
> > > We could go for a 3rd alternative, namely:
> > > 
> > > u16 bit = regs->data[priv->sreg];
> > > set_bit(bit, ct->labels);
> > > 
> > > i.e. have userspace place the _bit_ that we want to set in the
> > > source register.
> > > 
> > > If we go for sreg that would be my favored solution.
> > > 
> > > The only drawback vs #1 is that get and set work differently
> > > (get places all labels into dreg, set expects bit to set).
> > 
> > That seems like a problem. I agree that #3 would generally be fine, but
> > we should also really have "ct labels set ct labels" not change the labels,
> > that would be highly counterintuitive.
> 
> Right.  What about just re-working this approach:
> 
> https://patchwork.ozlabs.org/patch/613136/
> 
> (sans the CT_IMM part)?  We'd reject all expressions other than
> EXPR_VALUE in the eval phase -- 'ct labels set ct labels' would yield
> 'label expected' error message.
> 
> Does that seem acceptable to you?

Well, that pretty much defeats the use of the sreg since we can't use maps
or any other way to derive the label.

> If not, I see no choice other than resubmitting the original V1 kernel
> patch that simply copied the entire sreg into the label area, this way
> no userspace changes are needed.

I have to follow up on the previous discussion. Just wondering, what's wrong
with simply memcpy'ing and supplying the full set of labels?

Re: [PATCH v6 -next 2/4] netfilter: nftables: add connlabel set support

[Pablo Neira Ayuso]

On Mon, Apr 25, 2016 at 10:35:19PM +0100, Patrick McHardy wrote:
> On 25.04, Florian Westphal wrote:
> > Patrick McHardy <kaber@trash.net> wrote:
> > > On 25.04, Florian Westphal wrote:
> > > > We could go for a 3rd alternative, namely:
> > > > 
> > > > u16 bit = regs->data[priv->sreg];
> > > > set_bit(bit, ct->labels);
> > > > 
> > > > i.e. have userspace place the _bit_ that we want to set in the
> > > > source register.
> > > > 
> > > > If we go for sreg that would be my favored solution.
> > > > 
> > > > The only drawback vs #1 is that get and set work differently
> > > > (get places all labels into dreg, set expects bit to set).
> > > 
> > > That seems like a problem. I agree that #3 would generally be fine, but
> > > we should also really have "ct labels set ct labels" not change the labels,
> > > that would be highly counterintuitive.
> > 
> > Right.  What about just re-working this approach:
> > 
> > https://patchwork.ozlabs.org/patch/613136/
> > 
> > (sans the CT_IMM part)?  We'd reject all expressions other than
> > EXPR_VALUE in the eval phase -- 'ct labels set ct labels' would yield
> > 'label expected' error message.
> > 
> > Does that seem acceptable to you?
> 
> Well, that pretty much defeats the use of the sreg since we can't use maps
> or any other way to derive the label.
> 
> > If not, I see no choice other than resubmitting the original V1 kernel
> > patch that simply copied the entire sreg into the label area, this way
> > no userspace changes are needed.
> 
> I have to follow up on the previous discussion. Just wondering, what's wrong
> with simply memcpy'ing and supplying the full set of labels?

How can you make this atomic with a 128 bit connlabel?

Re: [PATCH v6 -next 2/4] netfilter: nftables: add connlabel set support

[Patrick McHardy]

On 25.04, Pablo Neira Ayuso wrote:
> On Mon, Apr 25, 2016 at 10:35:19PM +0100, Patrick McHardy wrote:
> > On 25.04, Florian Westphal wrote:
> > 
> > > If not, I see no choice other than resubmitting the original V1 kernel
> > > patch that simply copied the entire sreg into the label area, this way
> > > no userspace changes are needed.
> > 
> > I have to follow up on the previous discussion. Just wondering, what's wrong
> > with simply memcpy'ing and supplying the full set of labels?
> 
> How can you make this atomic with a 128 bit connlabel?

Good point. But then we already have a theoretical problem in the existing
NFT_CT_LABELS support, at least if we're looking at multiple bits at the
same time.

I'm fine either way, I just wand to understand the reason for the inconsistency
between get and set. If we need atomicity, we probably need to make sure
that get is either a bitop or we only allow to use a single bit of the result.

Re: [PATCH v6 -next 2/4] netfilter: nftables: add connlabel set support

[Florian Westphal]

Patrick McHardy <kaber@trash.net> wrote:
> > If not, I see no choice other than resubmitting the original V1 kernel
> > patch that simply copied the entire sreg into the label area, this way
> > no userspace changes are needed.
> 
> I have to follow up on the previous discussion. Just wondering, what's wrong
> with simply memcpy'ing and supplying the full set of labels?

Ok, here is a summary.

iptables: userspace gives u16 indicating bit, kernel test_bit()s or
set_bits depending if set or get is wanted.

The checkentry hook tells the kernel that label extension for X bits should be
allocated for new conntracks (where X is what userspace gave in the
match struct).


nftables:
get operation simply copies the label area to the sreg.
Since we only support up to 129 labels at the moment this fits into
an nft register.

nft ct label bla

simply expands to a bit test on the sreg filled from nft_ct expression.

There is currently no 'please enable labels for new conntracks'.

First proposal for set operation was to simply expect userspace to
populate the label space for us and memcpy the sreg to the label area.

So

nft label set bla

will *replace* the entire area with the sreg content, i.e. we delete
any other label(s) present.

To only set a label, one has to use

ct labels set ct labels | bla

I have no preference; this is the most simple patch/approach since
it requires no userspace changes (except documenting it).

Second drawback is that we don't know which label(s) will be set or
requested so we either have to unconditionally allocate 16 byte
in the conntrack extension, or, alternatively, raise the 'use this many
bits when allocating a new conntrack' variable in the packet path (should
not be a huge deal since after some time all conntracks would have
sufficient space).

3rd drawback is that its not atomic anymore since we replace
the entire thing (but I don't think this is a problem in practice).

Re: [PATCH v6 -next 2/4] netfilter: nftables: add connlabel set support

[Florian Westphal]

Florian Westphal <fw@strlen.de> wrote:
> Patrick McHardy <kaber@trash.net> wrote:
> > > If not, I see no choice other than resubmitting the original V1 kernel
> > > patch that simply copied the entire sreg into the label area, this way
> > > no userspace changes are needed.
> > 
> > I have to follow up on the previous discussion. Just wondering, what's wrong
> > with simply memcpy'ing and supplying the full set of labels?
> 
> Ok, here is a summary.
> 
> iptables: userspace gives u16 indicating bit, kernel test_bit()s or
> set_bits depending if set or get is wanted.
> 
> The checkentry hook tells the kernel that label extension for X bits should be
> allocated for new conntracks (where X is what userspace gave in the
> match struct).
> 
> 
> nftables:
> get operation simply copies the label area to the sreg.
> Since we only support up to 129 labels at the moment this fits into
> an nft register.
> 
> nft ct label bla
> 
> simply expands to a bit test on the sreg filled from nft_ct expression.
> 
> There is currently no 'please enable labels for new conntracks'.
> 
> First proposal for set operation was to simply expect userspace to
> populate the label space for us and memcpy the sreg to the label area.
> 
> So
> 
> nft label set bla
> 
> will *replace* the entire area with the sreg content, i.e. we delete
> any other label(s) present.
> 
> To only set a label, one has to use
> 
> ct labels set ct labels | bla
> 
> I have no preference; this is the most simple patch/approach since
> it requires no userspace changes (except documenting it).
> 
> Second drawback is that we don't know which label(s) will be set or
> requested so we either have to unconditionally allocate 16 byte
> in the conntrack extension, or, alternatively, raise the 'use this many
> bits when allocating a new conntrack' variable in the packet path (should
> not be a huge deal since after some time all conntracks would have
> sufficient space).
> 
> 3rd drawback is that its not atomic anymore since we replace
> the entire thing (but I don't think this is a problem in practice).

Disregard this.  I think i have a very simple solution for this, by
reusing V1 and passing the sreg also as the mask argument in
nf_connlabels_replace() [ which is the helper to alter labels via
ctnetlink ], i.e.

nf_connlabels_replace(ct, &regs->data[priv->sreg],
			&regs->data[priv->sreg],
			NF_CT_LABELS_MAX_SIZE / sizeof(u32)))

It gives xtables-connlabel like semantics, i.e.

ct label set bla

will only set bla, leaving all other labels alone.
This even matches the 'get' part since 'ct label bla' already uses a '&' op,
not eq.

Only drawback is that nft will ask to allocate enough room for 128
labels (maximum) in the ct extension area as we don't have the individual
highest desired label bit that xtables can use to ask for a lower
number.

I think its not a big deal.  Will test and submit patch in the next few
hours.

Sorry for not thinking of this sooner, I forgot that this mask argument
even existed 8-/

Re: [PATCH v6 -next 2/4] netfilter: nftables: add connlabel set support

[Pablo Neira Ayuso]

On Mon, Apr 25, 2016 at 06:05:41PM +0100, Patrick McHardy wrote:
> On 25.04, Florian Westphal wrote:
> > Patrick McHardy <kaber@trash.net> wrote:
> > 
> > > The alternative to internally handling it would be to some propagating
> > > validation to immediates / sets which invoke the actual user of the data.
> > > So in the case of helpers, we could replace the name by references to
> > > the helper structures and reverse this during dumping.
> > > 
> > > Regarding connlabels this doesn't really apply though. We expect userspace
> > > to create a reasonable ruleset and anything that does not cause critical
> > > errors is validated in userspace.
> > 
> > Yes.  So we have three choices here (pseudo-code)
> > 
> > memcpy(ct->labels, regs->data[priv->sreg], sizeof(reg));
> > vs.
> > set_bit(priv->imm, ct->labels);
> > 
> > The latter is what the iptables module does, I do not mind if we
> > go for #1 (treat the label area just like an 128bit register and
> > replace it completely with whatever is in the source register).
> > 
> > My only problem is that Pablo suggested #2 whereas you recommend #1.
> > 
> > I don't want to resubmit until there is consensus as to what the
> > preferred solution is.
> > 
> > We could go for a 3rd alternative, namely:
> > 
> > u16 bit = regs->data[priv->sreg];
> > set_bit(bit, ct->labels);
> > 
> > i.e. have userspace place the _bit_ that we want to set in the
> > source register.
> > 
> > If we go for sreg that would be my favored solution.
> > 
> > The only drawback vs #1 is that get and set work differently
> > (get places all labels into dreg, set expects bit to set).
> 
> That seems like a problem. I agree that #3 would generally be fine, but
> we should also really have "ct labels set ct labels" not change the labels,
> that would be highly counterintuitive.

'ct labels set' is actually operating a bitwise level.

I suggested to introduce something like:

'ct labels bitset' so the user becomes aware of this operation.

And we can just disallow "ct labels set ct labels" since it doesn't
make sense for what Florian needs AFAIK.

[PATCH libnftnl 3/4] ct: add connlabel set support

[Florian Westphal]

label set support is implemented by passing the bit value in a
nftnl_data_reg rather than using an sreg.

The advantage is that the kernel can use set_bit() api to toggle a
connlabel bit rather than having to set the entire label area in the
conntrack based on register contents.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 include/libnftnl/expr.h             |  1 +
 include/linux/netfilter/nf_tables.h |  2 ++
 src/expr/ct.c                       | 48 ++++++++++++++++++++++++++++++++++++-
 3 files changed, 50 insertions(+), 1 deletion(-)

diff --git a/include/libnftnl/expr.h b/include/libnftnl/expr.h
index da6a251..d4dccb1 100644
--- a/include/libnftnl/expr.h
+++ b/include/libnftnl/expr.h
@@ -140,6 +140,7 @@ enum {
 	NFTNL_EXPR_CT_KEY,
 	NFTNL_EXPR_CT_DIR,
 	NFTNL_EXPR_CT_SREG,
+	NFTNL_EXPR_CT_IMM,
 };
 
 enum {
diff --git a/include/linux/netfilter/nf_tables.h b/include/linux/netfilter/nf_tables.h
index b5fa7cb..e9cf806 100644
--- a/include/linux/netfilter/nf_tables.h
+++ b/include/linux/netfilter/nf_tables.h
@@ -768,6 +768,7 @@ enum nft_ct_keys {
  * @NFTA_CT_KEY: conntrack data item to load (NLA_U32: nft_ct_keys)
  * @NFTA_CT_DIRECTION: direction in case of directional keys (NLA_U8)
  * @NFTA_CT_SREG: source register (NLA_U32)
+ * @NFTA_CT_IMM: immediate value (NLA_NESTED)
  */
 enum nft_ct_attributes {
 	NFTA_CT_UNSPEC,
@@ -775,6 +776,7 @@ enum nft_ct_attributes {
 	NFTA_CT_KEY,
 	NFTA_CT_DIRECTION,
 	NFTA_CT_SREG,
+	NFTA_CT_IMM,
 	__NFTA_CT_MAX
 };
 #define NFTA_CT_MAX		(__NFTA_CT_MAX - 1)
diff --git a/src/expr/ct.c b/src/expr/ct.c
index a38f40c..8fb13b9 100644
--- a/src/expr/ct.c
+++ b/src/expr/ct.c
@@ -26,6 +26,7 @@ struct nftnl_expr_ct {
 	enum nft_registers	dreg;
 	enum nft_registers	sreg;
 	uint8_t			dir;
+	union nftnl_data_reg	imm;
 };
 
 #define IP_CT_DIR_ORIGINAL	0
@@ -54,6 +55,10 @@ nftnl_expr_ct_set(struct nftnl_expr *e, uint16_t type,
 	case NFTNL_EXPR_CT_SREG:
 		ct->sreg = *((uint32_t *)data);
 		break;
+	case NFTNL_EXPR_CT_IMM:
+		memcpy(&ct->imm.val, data, data_len);
+		ct->imm.len = data_len;
+		break;
 	default:
 		return -1;
 	}
@@ -79,6 +84,9 @@ nftnl_expr_ct_get(const struct nftnl_expr *e, uint16_t type,
 	case NFTNL_EXPR_CT_SREG:
 		*data_len = sizeof(ct->sreg);
 		return &ct->sreg;
+	case NFTNL_EXPR_CT_IMM:
+		*data_len = ct->imm.len;
+		return &ct->imm.val;
 	}
 	return NULL;
 }
@@ -102,6 +110,10 @@ static int nftnl_expr_ct_cb(const struct nlattr *attr, void *data)
 		if (mnl_attr_validate(attr, MNL_TYPE_U8) < 0)
 			abi_breakage();
 		break;
+	case NFTA_CT_IMM:
+		if (mnl_attr_validate(attr, MNL_TYPE_NESTED) < 0)
+			abi_breakage();
+		break;
 	}
 
 	tb[type] = attr;
@@ -121,6 +133,13 @@ nftnl_expr_ct_build(struct nlmsghdr *nlh, struct nftnl_expr *e)
 		mnl_attr_put_u8(nlh, NFTA_CT_DIRECTION, ct->dir);
 	if (e->flags & (1 << NFTNL_EXPR_CT_SREG))
 		mnl_attr_put_u32(nlh, NFTA_CT_SREG, htonl(ct->sreg));
+	if (e->flags & (1 << NFTNL_EXPR_CT_IMM)) {
+		struct nlattr *nest;
+
+		nest = mnl_attr_nest_start(nlh, NFTA_CT_IMM);
+		mnl_attr_put(nlh, NFTA_DATA_VALUE, ct->imm.len, ct->imm.val);
+		mnl_attr_nest_end(nlh, nest);
+	}
 }
 
 static int
@@ -128,6 +147,7 @@ nftnl_expr_ct_parse(struct nftnl_expr *e, struct nlattr *attr)
 {
 	struct nftnl_expr_ct *ct = nftnl_expr_data(e);
 	struct nlattr *tb[NFTA_CT_MAX+1] = {};
+	int ret = 0;
 
 	if (mnl_attr_parse_nested(attr, nftnl_expr_ct_cb, tb) < 0)
 		return -1;
@@ -148,8 +168,12 @@ nftnl_expr_ct_parse(struct nftnl_expr *e, struct nlattr *attr)
 		ct->dir = mnl_attr_get_u8(tb[NFTA_CT_DIRECTION]);
 		e->flags |= (1 << NFTNL_EXPR_CT_DIR);
 	}
+	if (tb[NFTA_CT_IMM]) {
+		ret = nftnl_parse_data(&ct->imm, tb[NFTA_CT_IMM], NULL);
+		e->flags |= (1 << NFTNL_EXPR_CT_IMM);
+	}
 
-	return 0;
+	return ret;
 }
 
 const char *ctkey2str_array[NFT_CT_MAX] = {
@@ -224,6 +248,7 @@ static int nftnl_expr_ct_json_parse(struct nftnl_expr *e, json_t *root,
 #ifdef JSON_PARSING
 	const char *key_str, *dir_str;
 	uint32_t reg;
+	uint16_t bit;
 	uint8_t dir;
 	int key;
 
@@ -252,6 +277,10 @@ static int nftnl_expr_ct_json_parse(struct nftnl_expr *e, json_t *root,
 		nftnl_expr_set_u8(e, NFTNL_EXPR_CT_DIR, dir);
 	}
 
+	if (nftnl_jansson_data_reg_parse(root, "imm", &ct->imm,
+				       err) == DATA_VALUE)
+		e->flags |= (1 << NFTNL_EXPR_CT_IMM);
+
 	return 0;
 err:
 	errno = EINVAL;
@@ -270,6 +299,7 @@ static int nftnl_expr_ct_xml_parse(struct nftnl_expr *e, mxml_node_t *tree,
 	const char *key_str, *dir_str;
 	int key;
 	uint8_t dir;
+	uint16_t bit;
 	uint32_t dreg, sreg;
 
 	if (nftnl_mxml_reg_parse(tree, "dreg", &dreg, MXML_DESCEND_FIRST,
@@ -300,6 +330,10 @@ static int nftnl_expr_ct_xml_parse(struct nftnl_expr *e, mxml_node_t *tree,
 		nftnl_expr_set_u8(e, NFTNL_EXPR_CT_DIR, dir);
 	}
 
+	if (nftnl_mxml_data_reg_parse(tree, "imm", &ct->imm, NFTNL_XML_OPT,
+				    err) == DATA_VALUE)
+		e->flags |= (1 << NFTNL_EXPR_CT_IMM);
+
 	return 0;
 err:
 	errno = EINVAL;
@@ -324,6 +358,8 @@ nftnl_expr_ct_export(char *buf, size_t size, struct nftnl_expr *e, int type)
 		nftnl_buf_str(&b, type, ctkey2str(ct->key), KEY);
 	if (e->flags & (1 << NFTNL_EXPR_CT_DIR))
 		nftnl_buf_str(&b, type, ctdir2str(ct->dir), DIR);
+	if (e->flags & (1 << NFTNL_EXPR_CT_IMM))
+		nftnl_buf_reg(&b, type, &ct->imm, DATA_VALUE, DATA);
 
 	return nftnl_buf_done(&b);
 }
@@ -340,6 +376,16 @@ nftnl_expr_ct_snprintf_default(char *buf, size_t size, struct nftnl_expr *e)
 		SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
 	}
 
+	if (nftnl_expr_is_set(e, NFTNL_EXPR_CT_IMM)) {
+		ret = snprintf(buf, size, "set %s with imm ",
+				ctkey2str(ct->key));
+		SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
+
+		ret = nftnl_data_reg_snprintf(buf+offset, len, &ct->imm,
+					      NFTNL_OUTPUT_DEFAULT, 0, DATA_VALUE);
+		SNPRINTF_BUFFER_SIZE(ret, size, len, offset);
+	}
+
 	if (e->flags & (1 << NFTNL_EXPR_CT_DREG)) {
 		ret = snprintf(buf, len, "load %s => reg %u ",
 			       ctkey2str(ct->key), ct->dreg);
-- 
2.7.3

[PATCH nft 4/4] ct: add conntrack label set support

[Florian Westphal]

Pablo suggested to support this by indicating the label bit number that we
want to set via netlink attribute.

IOW, ct label set doesn't use an sreg -- instead the bit that we
should set in the conntrack label area is passed as immediate attribute.

To make this work we have to dissect the argument and derive the
bit from the bitfield (only one bit can be set).

For nft list, we create a new bitfield and set the appropriate label bit.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 src/evaluate.c            | 28 ++++++++++++++++++++++++----
 src/netlink_delinearize.c | 44 +++++++++++++++++++++++++++++++++++++++-----
 src/netlink_linearize.c   | 23 +++++++++++++++++------
 3 files changed, 80 insertions(+), 15 deletions(-)

diff --git a/src/evaluate.c b/src/evaluate.c
index 346e34f..5983159 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -1468,10 +1468,30 @@ static int stmt_evaluate_meta(struct eval_ctx *ctx, struct stmt *stmt)
 
 static int stmt_evaluate_ct(struct eval_ctx *ctx, struct stmt *stmt)
 {
-	return stmt_evaluate_arg(ctx, stmt,
-				 stmt->ct.tmpl->dtype,
-				 stmt->ct.tmpl->len,
-				 &stmt->ct.expr);
+	int ret = stmt_evaluate_arg(ctx, stmt, stmt->ct.tmpl->dtype,
+				    stmt->ct.tmpl->len, &stmt->ct.expr);
+	if (ret < 0)
+		return ret;
+
+	switch (stmt->ct.key) {
+	case NFT_CT_LABELS: {
+		struct expr *e = stmt->ct.expr;
+		int bit;
+
+		if (e->ops->type != EXPR_VALUE ||
+		    mpz_popcount(e->value) != 1)
+			return stmt_error(ctx, stmt, "label expected");
+
+		bit = mpz_scan1(e->value, 0);
+		mpz_set_ui(e->value, htonl(bit));
+		e->len = sizeof(uint32_t) * BITS_PER_BYTE;
+		break;
+	}
+	default:
+		break;
+	}
+
+	return 0;
 }
 
 static int reject_payload_gen_dependency_tcp(struct eval_ctx *ctx,
diff --git a/src/netlink_delinearize.c b/src/netlink_delinearize.c
index ca1c6e6..2327fcf 100644
--- a/src/netlink_delinearize.c
+++ b/src/netlink_delinearize.c
@@ -525,10 +525,20 @@ static void netlink_parse_ct_stmt(struct netlink_parse_ctx *ctx,
 	struct stmt *stmt;
 	struct expr *expr;
 
-	sreg = netlink_parse_register(nle, NFTNL_EXPR_CT_SREG);
-	expr = netlink_get_register(ctx, loc, sreg);
-
 	key  = nftnl_expr_get_u32(nle, NFTNL_EXPR_CT_KEY);
+
+	if (nftnl_expr_is_set(nle, NFTNL_EXPR_CT_SREG)) {
+		sreg = netlink_parse_register(nle, NFTNL_EXPR_CT_SREG);
+		expr = netlink_get_register(ctx, loc, sreg);
+	} else if (nftnl_expr_is_set(nle, NFTNL_EXPR_CT_IMM)) {
+		struct nft_data_delinearize nld;
+
+		nld.value = nftnl_expr_get(nle, NFTNL_EXPR_CT_IMM, &nld.len);
+		expr = netlink_alloc_value(loc, &nld);
+	} else {
+		BUG("neither sreg nor immediate present");
+	}
+
 	stmt = ct_stmt_alloc(loc, key, expr);
 	expr_set_type(expr, stmt->ct.tmpl->dtype, stmt->ct.tmpl->byteorder);
 
@@ -1554,6 +1564,31 @@ static void expr_postprocess(struct rule_pp_ctx *ctx, struct expr **exprp)
 	}
 }
 
+static void stmt_ct_postprocess(struct rule_pp_ctx *rctx)
+{
+	struct stmt *stmt = rctx->stmt;
+
+	switch (stmt->ct.key) {
+	case NFT_CT_LABELS: {
+		struct expr *expr = stmt->ct.expr;
+		unsigned int bit;
+
+		assert(expr && expr->ops->type == EXPR_VALUE);
+
+		bit = mpz_get_uint32(expr->value);
+		mpz_set_ui(expr->value, 0);
+		mpz_setbit(expr->value, bit);
+		expr->byteorder = BYTEORDER_BIG_ENDIAN;
+		break;
+	}
+	default:
+		break;
+	}
+
+	if (stmt->ct.expr != NULL)
+		expr_postprocess(rctx, &stmt->ct.expr);
+}
+
 static void stmt_reject_postprocess(struct rule_pp_ctx *rctx)
 {
 	const struct proto_desc *desc, *base;
@@ -1711,8 +1746,7 @@ static void rule_parse_postprocess(struct netlink_parse_ctx *ctx, struct rule *r
 				expr_postprocess(&rctx, &stmt->meta.expr);
 			break;
 		case STMT_CT:
-			if (stmt->ct.expr != NULL)
-				expr_postprocess(&rctx, &stmt->ct.expr);
+			stmt_ct_postprocess(&rctx);
 			break;
 		case STMT_NAT:
 			if (stmt->nat.addr != NULL)
diff --git a/src/netlink_linearize.c b/src/netlink_linearize.c
index 3263043..417e999 100644
--- a/src/netlink_linearize.c
+++ b/src/netlink_linearize.c
@@ -1083,15 +1083,26 @@ static void netlink_gen_queue_stmt(struct netlink_linearize_ctx *ctx,
 static void netlink_gen_ct_stmt(struct netlink_linearize_ctx *ctx,
 				  const struct stmt *stmt)
 {
-	struct nftnl_expr *nle;
+	struct nftnl_expr *nle = alloc_nft_expr("ct");
 	enum nft_registers sreg;
 
-	sreg = get_register(ctx, stmt->ct.expr);
-	netlink_gen_expr(ctx, stmt->ct.expr, sreg);
-	release_register(ctx, stmt->ct.expr);
+	switch (stmt->ct.key) {
+	case NFT_CT_LABELS: {
+		struct nft_data_linearize nld;
+
+		netlink_gen_data(stmt->ct.expr, &nld);
+		nftnl_expr_set(nle, NFTNL_EXPR_CT_IMM, nld.value, nld.len);
+		break;
+	}
+	default:
+		sreg = get_register(ctx, stmt->ct.expr);
+		netlink_gen_expr(ctx, stmt->ct.expr, sreg);
+		release_register(ctx, stmt->ct.expr);
+
+		netlink_put_register(nle, NFTNL_EXPR_CT_SREG, sreg);
+		break;
+	}
 
-	nle = alloc_nft_expr("ct");
-	netlink_put_register(nle, NFTNL_EXPR_CT_SREG, sreg);
 	nftnl_expr_set_u32(nle, NFTNL_EXPR_CT_KEY, stmt->ct.key);
 	nftnl_rule_add_expr(ctx->nlr, nle);
 }
-- 
2.7.3