[PATCH nf-next resend 0/3] netfilter: connlabel set support

[PATCH nf-next resend 0/3] netfilter: connlabel set support

[Florian Westphal]

Patch #1 is a cleanup, #2 avoids ctnetlink even storm when label replacement
doesn't change anything, #3 is the actual change.

With patch #3 'ct label set ct label | foo' works, there are no changes
needed on nft/userspace side.

[PATCH nf-next v3 1/3] 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>
---
 Changes since v2:
  keep nf_connlabel_set where it is
  kill label_bits helper, use BIT_WORD() instead

 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.4.10

[PATCH nf-next v3 2/3] netfilter: labels: don't emit ct event if labels are unchanged

[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.4.10

[PATCH nf-next v3 3/3] netfilter: nftables: add connlabel set support

[Florian Westphal]

Conntrack labels are currently sized depending on the iptables
ruleset, i.e. if we're asked to test or set bits 1, 2, and 65 then we
would allocate enough room to store at least bit 65.

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.

Alternatively, we could alter nf_connlabel_replace to increase
net->ct.label_words at run time, but since 128 bits is not that
big we'd only save sizeof(long) so it doesn't seem worth it for now.

Caveats: to set a label one has to use:

ct label set ct label | bar

as 'ct label set bar' will clear all other labels.

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

 net/netfilter/nft_ct.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/net/netfilter/nft_ct.c b/net/netfilter/nft_ct.c
index d4a4619..f926861 100644
--- a/net/netfilter/nft_ct.c
+++ b/net/netfilter/nft_ct.c
@@ -198,6 +198,12 @@ static void nft_ct_set_eval(const struct nft_expr *expr,
 		}
 		break;
 #endif
+#ifdef CONFIG_NF_CONNTRACK_LABELS
+	case NFT_CT_LABELS:
+		nf_connlabels_replace(ct, &regs->data[priv->sreg], NULL,
+				      NF_CT_LABELS_MAX_SIZE / sizeof(u32));
+		break;
+#endif
 	default:
 		break;
 	}
@@ -276,6 +282,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:
@@ -362,9 +371,21 @@ static int nft_ct_set_init(const struct nft_ctx *ctx,
 	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])
+			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
 	default:
 		return -EOPNOTSUPP;
 	}
@@ -384,6 +405,16 @@ 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
+	}
+
 	nft_ct_l3proto_module_put(ctx->afi->family);
 }
 
-- 
2.4.10

Re: [PATCH nf-next v3 3/3] netfilter: nftables: add connlabel set support

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 3047 bytes --]

Hi Florian,

[auto build test WARNING on nf-next/master]

url:    https://github.com/0day-ci/linux/commits/Florian-Westphal/netfilter-connlabel-set-support/20160314-214814
base:   https://git.kernel.org/pub/scm/linux/kernel/git/pablo/nf-next master
config: i386-randconfig-x009-201611 (attached as .config)
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All warnings (new ones prefixed by >>):

   net/netfilter/nft_ct.c: In function 'nft_ct_destroy':
   net/netfilter/nft_ct.c:410:2: warning: enumeration value 'NFT_CT_STATE' not handled in switch [-Wswitch]
     switch (priv->key) {
     ^
   net/netfilter/nft_ct.c:410:2: warning: enumeration value 'NFT_CT_DIRECTION' not handled in switch [-Wswitch]
   net/netfilter/nft_ct.c:410:2: warning: enumeration value 'NFT_CT_STATUS' not handled in switch [-Wswitch]
   net/netfilter/nft_ct.c:410:2: warning: enumeration value 'NFT_CT_MARK' not handled in switch [-Wswitch]
   net/netfilter/nft_ct.c:410:2: warning: enumeration value 'NFT_CT_SECMARK' not handled in switch [-Wswitch]
   net/netfilter/nft_ct.c:410:2: warning: enumeration value 'NFT_CT_EXPIRATION' not handled in switch [-Wswitch]
   net/netfilter/nft_ct.c:410:2: warning: enumeration value 'NFT_CT_HELPER' not handled in switch [-Wswitch]
   net/netfilter/nft_ct.c:410:2: warning: enumeration value 'NFT_CT_L3PROTOCOL' not handled in switch [-Wswitch]
   net/netfilter/nft_ct.c:410:2: warning: enumeration value 'NFT_CT_SRC' not handled in switch [-Wswitch]
   net/netfilter/nft_ct.c:410:2: warning: enumeration value 'NFT_CT_DST' not handled in switch [-Wswitch]
   net/netfilter/nft_ct.c:410:2: warning: enumeration value 'NFT_CT_PROTOCOL' not handled in switch [-Wswitch]
   net/netfilter/nft_ct.c:410:2: warning: enumeration value 'NFT_CT_PROTO_SRC' not handled in switch [-Wswitch]
   net/netfilter/nft_ct.c:410:2: warning: enumeration value 'NFT_CT_PROTO_DST' not handled in switch [-Wswitch]
>> net/netfilter/nft_ct.c:410:2: warning: enumeration value 'NFT_CT_PKTS' not handled in switch [-Wswitch]
>> net/netfilter/nft_ct.c:410:2: warning: enumeration value 'NFT_CT_BYTES' not handled in switch [-Wswitch]

vim +/NFT_CT_PKTS +410 net/netfilter/nft_ct.c

   394		err = nft_validate_register_load(priv->sreg, len);
   395		if (err < 0)
   396			return err;
   397	
   398		err = nft_ct_l3proto_try_module_get(ctx->afi->family);
   399		if (err < 0)
   400			return err;
   401	
   402		return 0;
   403	}
   404	
   405	static void nft_ct_destroy(const struct nft_ctx *ctx,
   406				   const struct nft_expr *expr)
   407	{
   408		struct nft_ct *priv = nft_expr_priv(expr);
   409	
 > 410		switch (priv->key) {
   411	#ifdef CONFIG_NF_CONNTRACK_LABELS
   412		case NFT_CT_LABELS:
   413			nf_connlabels_put(ctx->net);
   414			break;
   415	#endif
   416		}
   417	
   418		nft_ct_l3proto_module_put(ctx->afi->family);

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 26373 bytes --]

[PATCH nf-next v4 3/3] netfilter: nftables: add connlabel set support

[Florian Westphal]

Conntrack labels are currently sized depending on the iptables
ruleset, i.e. if we're asked to test or set bits 1, 2, and 65 then we
would allocate enough room to store at least bit 65.

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.

Alternatively, we could alter nf_connlabel_replace to increase
net->ct.label_words at run time, but since 128 bits is not that
big we'd only save sizeof(long) so it doesn't seem worth it for now.

Caveats: to set a label one has to use:

ct label set ct label | bar

as 'ct label set bar' will clear all other labels.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 Supersede of v3, kbuild robot reported error wrt. unhandled
 case in nft_ct_destroy, add default: label.

 net/netfilter/nft_ct.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/net/netfilter/nft_ct.c b/net/netfilter/nft_ct.c
index d4a4619..b0e8102 100644
--- a/net/netfilter/nft_ct.c
+++ b/net/netfilter/nft_ct.c
@@ -198,6 +198,12 @@ static void nft_ct_set_eval(const struct nft_expr *expr,
 		}
 		break;
 #endif
+#ifdef CONFIG_NF_CONNTRACK_LABELS
+	case NFT_CT_LABELS:
+		nf_connlabels_replace(ct, &regs->data[priv->sreg], NULL,
+				      NF_CT_LABELS_MAX_SIZE / sizeof(u32));
+		break;
+#endif
 	default:
 		break;
 	}
@@ -276,6 +282,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:
@@ -362,9 +371,21 @@ static int nft_ct_set_init(const struct nft_ctx *ctx,
 	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])
+			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
 	default:
 		return -EOPNOTSUPP;
 	}
@@ -384,6 +405,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);
 }
 
-- 
2.4.10

Re: [PATCH nf-next v4 3/3] netfilter: nftables: add connlabel set support

[Pablo Neira Ayuso]

Hi Florian,

On Mon, Mar 14, 2016 at 03:09:14PM +0100, Florian Westphal wrote:
> Conntrack labels are currently sized depending on the iptables
> ruleset, i.e. if we're asked to test or set bits 1, 2, and 65 then we
> would allocate enough room to store at least bit 65.
> 
> 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.

BTW, during NetDev 0.1 he also suggested an alternative to maps that
would suit well for this case, the idea is to use the NFT_SET_EXT_EXPR
to store the connlabel set expression and build a dictionary as
alternative, it would be something like:

        ip saddr map {
                1.2.3.4 : connlabel bitset foo,
        }

So based on the source IP address, for the matching key we run the
expression on the data part of the element.

> Alternatively, we could alter nf_connlabel_replace to increase
> net->ct.label_words at run time, but since 128 bits is not that
> big we'd only save sizeof(long) so it doesn't seem worth it for now.

That's right, we save little memory.

Sorry for this late spinning on this, giving another think after
looking at nf_connlabels_replace() that we don't have it xt_connlabel.