[PATCH v5 nf-next 0/4] netfilter: nftables: add connlabel set support

[PATCH v5 nf-next 0/4] netfilter: nftables: add connlabel set support

[Florian Westphal]

Hi.

This is round 5 of the connlabel set support set.
I'm only sending the kernel patches for now.

First 4 patches are preparation changes, patch #4 adds set support.
I added a more generic CT_IMM nested attr that expects a nft_data struct.

Its up to the kernel to (using the key) to figure out how to interpret it.
This approach is hopefully generic enough so it can be re-used for other
set options that want to use an immediate value.

Florian Westphal (4):
      netfilter: connlabels: move helpers to xt_connlabel
      netfilter: labels: don't emit ct event if labels were not changed
      netfilter: connlabels: change nf_connlabels_get bit arg to 'highest used'
      netfilter: nftables: add connlabel set support

 include/net/netfilter/nf_conntrack_labels.h |    5 -
 include/uapi/linux/netfilter/nf_tables.h    |    2 
 net/netfilter/nf_conntrack_labels.c         |   44 +++++----------
 net/netfilter/nft_ct.c                      |   78 ++++++++++++++++++++++++++--
 net/netfilter/xt_connlabel.c                |   14 ++++-
 net/openvswitch/conntrack.c                 |    2 
 6 files changed, 108 insertions(+), 37 deletions(-)

[PATCH v5 nf-next 1/4] netfilter: connlabels: move helpers to xt_connlabel

[Florian Westphal]

Currently labels can only be set either by iptables connlabel
match or via ctnetlink.

Before adding nftables set support, clean up the clabel core and move
helpers that nft will not need after all to the xtables module.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 No changes since last iteration.

 include/net/netfilter/nf_conntrack_labels.h |  1 -
 net/netfilter/nf_conntrack_labels.c         | 19 +------------------
 net/netfilter/xt_connlabel.c                | 12 +++++++++++-
 3 files changed, 12 insertions(+), 20 deletions(-)

diff --git a/include/net/netfilter/nf_conntrack_labels.h b/include/net/netfilter/nf_conntrack_labels.h
index 7e2b1d0..5167818 100644
--- a/include/net/netfilter/nf_conntrack_labels.h
+++ b/include/net/netfilter/nf_conntrack_labels.h
@@ -45,7 +45,6 @@ static inline struct nf_conn_labels *nf_ct_labels_ext_add(struct nf_conn *ct)
 #endif
 }
 
-bool nf_connlabel_match(const struct nf_conn *ct, u16 bit);
 int nf_connlabel_set(struct nf_conn *ct, u16 bit);
 
 int nf_connlabels_replace(struct nf_conn *ct,
diff --git a/net/netfilter/nf_conntrack_labels.c b/net/netfilter/nf_conntrack_labels.c
index 3ce5c31..3a30900 100644
--- a/net/netfilter/nf_conntrack_labels.c
+++ b/net/netfilter/nf_conntrack_labels.c
@@ -16,28 +16,11 @@
 
 static spinlock_t nf_connlabels_lock;
 
-static unsigned int label_bits(const struct nf_conn_labels *l)
-{
-	unsigned int longs = l->words;
-	return longs * BITS_PER_LONG;
-}
-
-bool nf_connlabel_match(const struct nf_conn *ct, u16 bit)
-{
-	struct nf_conn_labels *labels = nf_ct_labels_find(ct);
-
-	if (!labels)
-		return false;
-
-	return bit < label_bits(labels) && test_bit(bit, labels->bits);
-}
-EXPORT_SYMBOL_GPL(nf_connlabel_match);
-
 int nf_connlabel_set(struct nf_conn *ct, u16 bit)
 {
 	struct nf_conn_labels *labels = nf_ct_labels_find(ct);
 
-	if (!labels || bit >= label_bits(labels))
+	if (!labels || BIT_WORD(bit) >= labels->words)
 		return -ENOSPC;
 
 	if (test_bit(bit, labels->bits))
diff --git a/net/netfilter/xt_connlabel.c b/net/netfilter/xt_connlabel.c
index bb9cbeb..d9b3e53 100644
--- a/net/netfilter/xt_connlabel.c
+++ b/net/netfilter/xt_connlabel.c
@@ -18,6 +18,16 @@ MODULE_DESCRIPTION("Xtables: add/match connection trackling labels");
 MODULE_ALIAS("ipt_connlabel");
 MODULE_ALIAS("ip6t_connlabel");
 
+static bool connlabel_match(const struct nf_conn *ct, u16 bit)
+{
+	struct nf_conn_labels *labels = nf_ct_labels_find(ct);
+
+	if (!labels)
+		return false;
+
+	return BIT_WORD(bit) < labels->words && test_bit(bit, labels->bits);
+}
+
 static bool
 connlabel_mt(const struct sk_buff *skb, struct xt_action_param *par)
 {
@@ -33,7 +43,7 @@ connlabel_mt(const struct sk_buff *skb, struct xt_action_param *par)
 	if (info->options & XT_CONNLABEL_OP_SET)
 		return (nf_connlabel_set(ct, info->bit) == 0) ^ invert;
 
-	return nf_connlabel_match(ct, info->bit) ^ invert;
+	return connlabel_match(ct, info->bit) ^ invert;
 }
 
 static int connlabel_mt_check(const struct xt_mtchk_param *par)
-- 
2.7.3

[PATCH v5 nf-next 2/4] netfilter: labels: don't emit ct event if labels were not changed

[Florian Westphal]

make the replace function only send a ctnetlink event if the contents
of the new set is different.

Otherwise 'ct label set ct label | bar'

will cause netlink event storm since we "replace" labels for each packet.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 No changes since last version.

 net/netfilter/nf_conntrack_labels.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/net/netfilter/nf_conntrack_labels.c b/net/netfilter/nf_conntrack_labels.c
index 3a30900..bd7f26b 100644
--- a/net/netfilter/nf_conntrack_labels.c
+++ b/net/netfilter/nf_conntrack_labels.c
@@ -33,14 +33,18 @@ int nf_connlabel_set(struct nf_conn *ct, u16 bit)
 }
 EXPORT_SYMBOL_GPL(nf_connlabel_set);
 
-static void replace_u32(u32 *address, u32 mask, u32 new)
+static int replace_u32(u32 *address, u32 mask, u32 new)
 {
 	u32 old, tmp;
 
 	do {
 		old = *address;
 		tmp = (old & mask) ^ new;
+		if (old == tmp)
+			return 0;
 	} while (cmpxchg(address, old, tmp) != old);
+
+	return 1;
 }
 
 int nf_connlabels_replace(struct nf_conn *ct,
@@ -49,6 +53,7 @@ int nf_connlabels_replace(struct nf_conn *ct,
 {
 	struct nf_conn_labels *labels;
 	unsigned int size, i;
+	int changed = 0;
 	u32 *dst;
 
 	labels = nf_ct_labels_find(ct);
@@ -60,16 +65,15 @@ int nf_connlabels_replace(struct nf_conn *ct,
 		words32 = size / sizeof(u32);
 
 	dst = (u32 *) labels->bits;
-	if (words32) {
-		for (i = 0; i < words32; i++)
-			replace_u32(&dst[i], mask ? ~mask[i] : 0, data[i]);
-	}
+	for (i = 0; i < words32; i++)
+		changed |= replace_u32(&dst[i], mask ? ~mask[i] : 0, data[i]);
 
 	size /= sizeof(u32);
 	for (i = words32; i < size; i++) /* pad */
 		replace_u32(&dst[i], 0, 0);
 
-	nf_conntrack_event_cache(IPCT_LABEL, ct);
+	if (changed)
+		nf_conntrack_event_cache(IPCT_LABEL, ct);
 	return 0;
 }
 EXPORT_SYMBOL_GPL(nf_connlabels_replace);
-- 
2.7.3

[PATCH v5 nf-next 3/4] netfilter: connlabels: change nf_connlabels_get bit arg to 'highest used'

[Florian Westphal]

nf_connlabel_set() takes the bit number that we would like to set.
nf_connlabels_get() however took the number of bits that we want to
support.

So e.g. nf_connlabels_get(32) support bits 0 to 31, but not 32.
This changes nf_connlabels_get() to take the highest bit that we want
to set.

Callers then don't have to cope with a potential integer wrap
when using nf_connlabels_get(bit + 1) anymore.

Current callers are fine, this change is only to make folloup
nft ct label set support simpler.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 No changes since last version.

 include/net/netfilter/nf_conntrack_labels.h | 4 ++--
 net/netfilter/nf_conntrack_labels.c         | 9 +++++----
 net/netfilter/nft_ct.c                      | 2 ++
 net/netfilter/xt_connlabel.c                | 2 +-
 net/openvswitch/conntrack.c                 | 2 +-
 5 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/include/net/netfilter/nf_conntrack_labels.h b/include/net/netfilter/nf_conntrack_labels.h
index 5167818..c5f8fc73 100644
--- a/include/net/netfilter/nf_conntrack_labels.h
+++ b/include/net/netfilter/nf_conntrack_labels.h
@@ -53,11 +53,11 @@ int nf_connlabels_replace(struct nf_conn *ct,
 #ifdef CONFIG_NF_CONNTRACK_LABELS
 int nf_conntrack_labels_init(void);
 void nf_conntrack_labels_fini(void);
-int nf_connlabels_get(struct net *net, unsigned int n_bits);
+int nf_connlabels_get(struct net *net, unsigned int bit);
 void nf_connlabels_put(struct net *net);
 #else
 static inline int nf_conntrack_labels_init(void) { return 0; }
 static inline void nf_conntrack_labels_fini(void) {}
-static inline int nf_connlabels_get(struct net *net, unsigned int n_bits) { return 0; }
+static inline int nf_connlabels_get(struct net *net, unsigned int bit) { return 0; }
 static inline void nf_connlabels_put(struct net *net) {}
 #endif
diff --git a/net/netfilter/nf_conntrack_labels.c b/net/netfilter/nf_conntrack_labels.c
index bd7f26b..252e6a7 100644
--- a/net/netfilter/nf_conntrack_labels.c
+++ b/net/netfilter/nf_conntrack_labels.c
@@ -78,15 +78,14 @@ int nf_connlabels_replace(struct nf_conn *ct,
 }
 EXPORT_SYMBOL_GPL(nf_connlabels_replace);
 
-int nf_connlabels_get(struct net *net, unsigned int n_bits)
+int nf_connlabels_get(struct net *net, unsigned int bits)
 {
 	size_t words;
 
-	if (n_bits > (NF_CT_LABELS_MAX_SIZE * BITS_PER_BYTE))
+	words = BIT_WORD(bits) + 1;
+	if (words > NF_CT_LABELS_MAX_SIZE / sizeof(long))
 		return -ERANGE;
 
-	words = BITS_TO_LONGS(n_bits);
-
 	spin_lock(&nf_connlabels_lock);
 	net->ct.labels_used++;
 	if (words > net->ct.label_words)
@@ -115,6 +114,8 @@ static struct nf_ct_ext_type labels_extend __read_mostly = {
 
 int nf_conntrack_labels_init(void)
 {
+	BUILD_BUG_ON(NF_CT_LABELS_MAX_SIZE / sizeof(long) >= U8_MAX);
+
 	spin_lock_init(&nf_connlabels_lock);
 	return nf_ct_extend_register(&labels_extend);
 }
diff --git a/net/netfilter/nft_ct.c b/net/netfilter/nft_ct.c
index d4a4619..25998fa 100644
--- a/net/netfilter/nft_ct.c
+++ b/net/netfilter/nft_ct.c
@@ -484,6 +484,8 @@ static struct nft_expr_type nft_ct_type __read_mostly = {
 
 static int __init nft_ct_module_init(void)
 {
+	BUILD_BUG_ON(NF_CT_LABELS_MAX_SIZE > NFT_REG_SIZE);
+
 	return nft_register_expr(&nft_ct_type);
 }
 
diff --git a/net/netfilter/xt_connlabel.c b/net/netfilter/xt_connlabel.c
index d9b3e53..a79af25 100644
--- a/net/netfilter/xt_connlabel.c
+++ b/net/netfilter/xt_connlabel.c
@@ -65,7 +65,7 @@ static int connlabel_mt_check(const struct xt_mtchk_param *par)
 		return ret;
 	}
 
-	ret = nf_connlabels_get(par->net, info->bit + 1);
+	ret = nf_connlabels_get(par->net, info->bit);
 	if (ret < 0)
 		nf_ct_l3proto_module_put(par->family);
 	return ret;
diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
index dc5eb29..3da6107 100644
--- a/net/openvswitch/conntrack.c
+++ b/net/openvswitch/conntrack.c
@@ -1342,7 +1342,7 @@ void ovs_ct_init(struct net *net)
 	unsigned int n_bits = sizeof(struct ovs_key_ct_labels) * BITS_PER_BYTE;
 	struct ovs_net *ovs_net = net_generic(net, ovs_net_id);
 
-	if (nf_connlabels_get(net, n_bits)) {
+	if (nf_connlabels_get(net, n_bits - 1)) {
 		ovs_net->xt_label = false;
 		OVS_NLERR(true, "Failed to set connlabel length");
 	} else {
-- 
2.7.3

[PATCH v5 nf-next 4/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.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 Changes since last version:
 - add generic NFTA_CT_IMM and pass it a struct nft_data.

 include/uapi/linux/netfilter/nf_tables.h |  2 +
 net/netfilter/nft_ct.c                   | 76 ++++++++++++++++++++++++++++++--
 2 files changed, 75 insertions(+), 3 deletions(-)

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 25998fa..4ec1cea 100644
--- a/net/netfilter/nft_ct.c
+++ b/net/netfilter/nft_ct.c
@@ -29,6 +29,11 @@ struct nft_ct {
 		enum nft_registers	dreg:8;
 		enum nft_registers	sreg: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,
@@ -198,6 +203,11 @@ static void nft_ct_set_eval(const struct nft_expr *expr,
 		}
 		break;
 #endif
+#ifdef CONFIG_NF_CONNTRACK_LABELS
+	case NFT_CT_LABELS:
+		nf_connlabel_set(ct, priv->imm.set_bit);
+		break;
+#endif
 	default:
 		break;
 	}
@@ -208,6 +218,7 @@ static const struct nla_policy nft_ct_policy[NFTA_CT_MAX + 1] = {
 	[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)
@@ -276,6 +287,9 @@ static int nft_ct_get_init(const struct nft_ctx *ctx,
 		if (tb[NFTA_CT_DIRECTION] != NULL)
 			return -EINVAL;
 		len = NF_CT_LABELS_MAX_SIZE;
+		err = nf_connlabels_get(ctx->net, (len * BITS_PER_BYTE) - 1);
+		if (err)
+			return err;
 		break;
 #endif
 	case NFT_CT_HELPER:
@@ -355,16 +369,50 @@ static int nft_ct_set_init(const struct nft_ctx *ctx,
 			   const struct nlattr * const tb[])
 {
 	struct nft_ct *priv = nft_expr_priv(expr);
+	struct nft_data_desc imm_desc = {};
 	unsigned int len;
 	int err;
 
 	priv->key = ntohl(nla_get_be32(tb[NFTA_CT_KEY]));
+
+	if (tb[NFTA_CT_IMM]) {
+		/* We currently do not support both sreg and imm */
+		if (tb[NFTA_CT_SREG])
+			return -EINVAL;
+
+		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;
+	}
 	switch (priv->key) {
 #ifdef CONFIG_NF_CONNTRACK_MARK
 	case NFT_CT_MARK:
+		if (tb[NFTA_CT_DIRECTION])
+			return -EINVAL;
 		len = FIELD_SIZEOF(struct nf_conn, mark);
 		break;
 #endif
+#ifdef CONFIG_NF_CONNTRACK_LABELS
+	case NFT_CT_LABELS:
+		if (tb[NFTA_CT_DIRECTION] || imm_desc.len != sizeof(u32))
+			return -EINVAL;
+
+		err = nf_connlabels_get(ctx->net, htonl(priv->immediate.data[0]));
+		if (err < 0)
+			return err;
+
+		priv->imm_len = sizeof(u32);
+		priv->imm.set_bit = htonl(priv->immediate.data[0]);
+
+		err = nft_ct_l3proto_try_module_get(ctx->afi->family);
+		if (err < 0)
+			nf_connlabels_put(ctx->net);
+		return err;
+#endif
 	default:
 		return -EOPNOTSUPP;
 	}
@@ -384,6 +432,18 @@ static int nft_ct_set_init(const struct nft_ctx *ctx,
 static void nft_ct_destroy(const struct nft_ctx *ctx,
 			   const struct nft_expr *expr)
 {
+	struct nft_ct *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);
 }
 
@@ -426,10 +486,20 @@ static int nft_ct_set_dump(struct sk_buff *skb, const struct nft_expr *expr)
 {
 	const struct nft_ct *priv = nft_expr_priv(expr);
 
-	if (nft_dump_register(skb, NFTA_CT_SREG, priv->sreg))
-		goto nla_put_failure;
 	if (nla_put_be32(skb, NFTA_CT_KEY, htonl(priv->key)))
 		goto nla_put_failure;
+
+	if (priv->imm_len) {
+		if (nft_data_dump(skb, NFTA_CT_IMM, &priv->immediate,
+				  NFT_DATA_VALUE, priv->imm_len) < 0)
+			goto nla_put_failure;
+
+		return 0;
+	}
+
+	if (nft_dump_register(skb, NFTA_CT_SREG, priv->sreg))
+		goto nla_put_failure;
+
 	return 0;
 
 nla_put_failure:
@@ -468,7 +538,7 @@ nft_ct_select_ops(const struct nft_ctx *ctx,
 	if (tb[NFTA_CT_DREG])
 		return &nft_ct_get_ops;
 
-	if (tb[NFTA_CT_SREG])
+	if (tb[NFTA_CT_SREG] || tb[NFTA_CT_IMM])
 		return &nft_ct_set_ops;
 
 	return ERR_PTR(-EINVAL);
-- 
2.7.3

Re: [PATCH v5 nf-next 4/4] netfilter: nftables: add connlabel set support

[Pablo Neira Ayuso]

On Tue, Apr 12, 2016 at 06:14:26PM +0200, Florian Westphal wrote:
> diff --git a/net/netfilter/nft_ct.c b/net/netfilter/nft_ct.c
> index 25998fa..4ec1cea 100644
> --- a/net/netfilter/nft_ct.c
> +++ b/net/netfilter/nft_ct.c
> @@ -29,6 +29,11 @@ struct nft_ct {
>  		enum nft_registers	dreg:8;
>  		enum nft_registers	sreg:8;
>  	};
> +	union {
> +		u8		set_bit;
> +	} imm;
> +	unsigned int		imm_len:8;
> +	struct nft_data		immediate;

Could you use select_ops() so we don't increase the size of nft_ct for
other users?

Re: [PATCH v5 nf-next 4/4] netfilter: nftables: add connlabel set support

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Tue, Apr 12, 2016 at 06:14:26PM +0200, Florian Westphal wrote:
> > diff --git a/net/netfilter/nft_ct.c b/net/netfilter/nft_ct.c
> > index 25998fa..4ec1cea 100644
> > --- a/net/netfilter/nft_ct.c
> > +++ b/net/netfilter/nft_ct.c
> > @@ -29,6 +29,11 @@ struct nft_ct {
> >  		enum nft_registers	dreg:8;
> >  		enum nft_registers	sreg:8;
> >  	};
> > +	union {
> > +		u8		set_bit;
> > +	} imm;
> > +	unsigned int		imm_len:8;
> > +	struct nft_data		immediate;
> 
> Could you use select_ops() so we don't increase the size of nft_ct for
> other users?

Sure.

I'd split this into nft_ct (sreg/dreg)
and nft_ct_set_imm (set from immediate).

Does that sound okay?

Re: [PATCH v5 nf-next 4/4] netfilter: nftables: add connlabel set support

[Pablo Neira Ayuso]

On Thu, Apr 14, 2016 at 12:05:27PM +0200, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > On Tue, Apr 12, 2016 at 06:14:26PM +0200, Florian Westphal wrote:
> > > diff --git a/net/netfilter/nft_ct.c b/net/netfilter/nft_ct.c
> > > index 25998fa..4ec1cea 100644
> > > --- a/net/netfilter/nft_ct.c
> > > +++ b/net/netfilter/nft_ct.c
> > > @@ -29,6 +29,11 @@ struct nft_ct {
> > >  		enum nft_registers	dreg:8;
> > >  		enum nft_registers	sreg:8;
> > >  	};
> > > +	union {
> > > +		u8		set_bit;
> > > +	} imm;

BTW, do you really need this set_bit? I think we can just take the
data from the nft_data structure.

> > > +	unsigned int		imm_len:8;

This length, you will not need anymore with select_ops(), right=

> > > +	struct nft_data		immediate;
> > 
> > Could you use select_ops() so we don't increase the size of nft_ct for
> > other users?
> 
> Sure.
> 
> I'd split this into nft_ct (sreg/dreg)
> and nft_ct_set_imm (set from immediate).

I'd suggest "struct nft_ct_reg" and "struct nft_ct_imm", so we can
reuse the immediate from the get part if we can get rid of the imm_len
and set_bit fields.

Thanks.

Re: [PATCH v5 nf-next 4/4] netfilter: nftables: add connlabel set support

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Thu, Apr 14, 2016 at 12:05:27PM +0200, Florian Westphal wrote:
> > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > On Tue, Apr 12, 2016 at 06:14:26PM +0200, Florian Westphal wrote:
> > > > diff --git a/net/netfilter/nft_ct.c b/net/netfilter/nft_ct.c
> > > > index 25998fa..4ec1cea 100644
> > > > --- a/net/netfilter/nft_ct.c
> > > > +++ b/net/netfilter/nft_ct.c
> > > > @@ -29,6 +29,11 @@ struct nft_ct {
> > > >  		enum nft_registers	dreg:8;
> > > >  		enum nft_registers	sreg:8;
> > > >  	};
> > > > +	union {
> > > > +		u8		set_bit;
> > > > +	} imm;
> 
> BTW, do you really need this set_bit? I think we can just take the
> data from the nft_data structure.

An earlier patch did that (did not submit it); I found it ugly
(set_bit("ntohl(priv->data.data[0])").

 I can cahnge it back if you want.

> > > > +	unsigned int		imm_len:8;
> 
> This length, you will not need anymore with select_ops(), right=

Hmm, I followed nft_cmp_fast implementation.
We expect a 32bit data type for the "label" key.

The alternative to storing length is to have a hard-coded size dispatch
based on the key (e.g. use sizeof(u32) for labels).

But this might not work for all future cases.

I found saving the length from the desc struct to be much simpler
(because dump function is shorter and doesn't have to guess the right
 size when dumping).

> I'd suggest "struct nft_ct_reg" and "struct nft_ct_imm", so we can
> reuse the immediate from the get part if we can get rid of the imm_len
> and set_bit fields.

Oh.  I did not expect use of IMM for get operations.

Do we need a distinct attribute (IMM_GET vs IMM_SET)?

At the moment the assumption is that presence of IMM requests
a set operation (presence of both IMM and sreg is an error).

Re: [PATCH v5 nf-next 4/4] netfilter: nftables: add connlabel set support

[Pablo Neira Ayuso]

On Thu, Apr 14, 2016 at 01:26:52PM +0200, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > On Thu, Apr 14, 2016 at 12:05:27PM +0200, Florian Westphal wrote:
> > > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > > On Tue, Apr 12, 2016 at 06:14:26PM +0200, Florian Westphal wrote:
> > > > > diff --git a/net/netfilter/nft_ct.c b/net/netfilter/nft_ct.c
> > > > > index 25998fa..4ec1cea 100644
> > > > > --- a/net/netfilter/nft_ct.c
> > > > > +++ b/net/netfilter/nft_ct.c
> > > > > @@ -29,6 +29,11 @@ struct nft_ct {
> > > > >  		enum nft_registers	dreg:8;
> > > > >  		enum nft_registers	sreg:8;
> > > > >  	};
> > > > > +	union {
> > > > > +		u8		set_bit;
> > > > > +	} imm;
> > 
> > BTW, do you really need this set_bit? I think we can just take the
> > data from the nft_data structure.
> 
> An earlier patch did that (did not submit it); I found it ugly
> (set_bit("ntohl(priv->data.data[0])").
> 
>  I can cahnge it back if you want.
> 
> > > > > +	unsigned int		imm_len:8;
> > 
> > This length, you will not need anymore with select_ops(), right=
> 
> Hmm, I followed nft_cmp_fast implementation.
> We expect a 32bit data type for the "label" key.
> 
> The alternative to storing length is to have a hard-coded size dispatch
> based on the key (e.g. use sizeof(u32) for labels).
>
> But this might not work for all future cases.

No problem, we can revisit this. If you believe this is the right
structure layout I'm ok with it, so just focus on the reworking this
to use select_ops().

> I found saving the length from the desc struct to be much simpler
> (because dump function is shorter and doesn't have to guess the right
>  size when dumping).

Dumping is not performance critical path, so I wouldn't bother on
this.

> > I'd suggest "struct nft_ct_reg" and "struct nft_ct_imm", so we can
> > reuse the immediate from the get part if we can get rid of the imm_len
> > and set_bit fields.
> 
> Oh.  I did not expect use of IMM for get operations.

Right.

> Do we need a distinct attribute (IMM_GET vs IMM_SET)?
>
> At the moment the assumption is that presence of IMM requests
> a set operation (presence of both IMM and sreg is an error).

If you use select_ops(), we can just reject whenever the register
attribute is set via -EINVAL.

Re: [PATCH v5 nf-next 0/4] netfilter: nftables: add connlabel set support

[Pablo Neira Ayuso]

On Tue, Apr 12, 2016 at 06:14:22PM +0200, Florian Westphal wrote:
> Hi.
> 
> This is round 5 of the connlabel set support set.
> I'm only sending the kernel patches for now.
> 
> First 4 patches are preparation changes, patch #4 adds set support.

Florian, I can push into the tree the 3 preparation patches, I don't
see many changes on them. You can follow up with the nft_ct changes
later on.

Re: [PATCH v5 nf-next 0/4] netfilter: nftables: add connlabel set support

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > First 4 patches are preparation changes, patch #4 adds set support.
> 
> Florian, I can push into the tree the 3 preparation patches, I don't
> see many changes on them. You can follow up with the nft_ct changes
> later on.

That would be great, thanks!

Re: [PATCH v5 nf-next 0/4] netfilter: nftables: add connlabel set support

[Pablo Neira Ayuso]

On Fri, Apr 15, 2016 at 03:14:14PM +0200, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > First 4 patches are preparation changes, patch #4 adds set support.
> > 
> > Florian, I can push into the tree the 3 preparation patches, I don't
> > see many changes on them. You can follow up with the nft_ct changes
> > later on.
> 
> That would be great, thanks!

Done, thanks!