[PATCH 0/3] netfilter: add connlabel conntrack extension

[PATCH 0/3] netfilter: add connlabel conntrack extension

[Florian Westphal]

The following three patches add the connlabel extension, plus
ctnetlink support.

Connlabels are similar to connmarks, except labels are bit-based; i.e.
all labels may be attached to a flow at the same time.

Up to 128 labels are supported at this time.

Mapping of bit-identifier to label name is done in userspace.

The extension is enabled at run-time once "-m connlabel" netfilter rules
are added.

The '128-labels' limit could be increased, but we need to check that
extension array won't wrap, when all conntrack extensions are being
used (offsets are stored in 'u8' array).

Changes since RFCv2:
- make it a variable-size extension and remove dynamic
  reallocation of the label array
- add ctnetlink support for receiving/setting labels
- limit to 128 instead of 1k labels due to limited extension
  space (128 is more than enough for now, so this is no problem).

summary of changes:

 include/net/netfilter/nf_conntrack_extend.h        |    4 
 include/net/netfilter/nf_conntrack_labels.h        |   57 +++++++++++
 include/net/netns/conntrack.h                      |    4 
 include/uapi/linux/netfilter/nf_conntrack_common.h |    1 
 include/uapi/linux/netfilter/nfnetlink_conntrack.h |    1 
 include/uapi/linux/netfilter/xt_connlabel.h        |   13 ++
 net/netfilter/Kconfig                              |   18 +++
 net/netfilter/Makefile                             |    2 
 net/netfilter/nf_conntrack_core.c                  |   12 ++
 net/netfilter/nf_conntrack_labels.c                |  107 +++++++++++++++++++++
 net/netfilter/nf_conntrack_netlink.c               |   67 +++++++++++++
 net/netfilter/xt_connlabel.c                       |   96 ++++++++++++++++++
12 files changed, 382 insertions(+)

Best regards,
Florian

[PATCH 1/3] netfilter: add connlabel conntrack extension

[Florian Westphal]

similar to connmarks, except labels are bit-based; i.e.
all labels may be attached to a flow at the same time.

Up to 128 labels are supported.  Supporting more labels
is possible, but requires increasing the ct offset delta
from u8 to u16 type due to increased extension sizes.

Mapping of bit-identifier to label name is done in userspace.

The extension is enabled at run-time once "-m connlabel" netfilter
rules are added.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 include/net/netfilter/nf_conntrack_extend.h |    4 +
 include/net/netfilter/nf_conntrack_labels.h |   54 +++++++++++++++
 include/net/netns/conntrack.h               |    4 +
 include/uapi/linux/netfilter/xt_connlabel.h |   13 ++++
 net/netfilter/Kconfig                       |   18 +++++
 net/netfilter/Makefile                      |    2 +
 net/netfilter/nf_conntrack_core.c           |   12 ++++
 net/netfilter/nf_conntrack_labels.c         |   73 ++++++++++++++++++++
 net/netfilter/nf_conntrack_netlink.c        |    3 +
 net/netfilter/xt_connlabel.c                |   96 +++++++++++++++++++++++++++
 10 files changed, 279 insertions(+), 0 deletions(-)
 create mode 100644 include/net/netfilter/nf_conntrack_labels.h
 create mode 100644 include/uapi/linux/netfilter/xt_connlabel.h
 create mode 100644 net/netfilter/nf_conntrack_labels.c
 create mode 100644 net/netfilter/xt_connlabel.c

diff --git a/include/net/netfilter/nf_conntrack_extend.h b/include/net/netfilter/nf_conntrack_extend.h
index 8b4d1fc2..977bc8a 100644
--- a/include/net/netfilter/nf_conntrack_extend.h
+++ b/include/net/netfilter/nf_conntrack_extend.h
@@ -23,6 +23,9 @@ enum nf_ct_ext_id {
 #ifdef CONFIG_NF_CONNTRACK_TIMEOUT
 	NF_CT_EXT_TIMEOUT,
 #endif
+#ifdef CONFIG_NF_CONNTRACK_LABELS
+	NF_CT_EXT_LABELS,
+#endif
 	NF_CT_EXT_NUM,
 };
 
@@ -33,6 +36,7 @@ enum nf_ct_ext_id {
 #define NF_CT_EXT_ZONE_TYPE struct nf_conntrack_zone
 #define NF_CT_EXT_TSTAMP_TYPE struct nf_conn_tstamp
 #define NF_CT_EXT_TIMEOUT_TYPE struct nf_conn_timeout
+#define NF_CT_EXT_LABELS_TYPE struct nf_conn_labels
 
 /* Extensions: optional stuff which isn't permanently in struct. */
 struct nf_ct_ext {
diff --git a/include/net/netfilter/nf_conntrack_labels.h b/include/net/netfilter/nf_conntrack_labels.h
new file mode 100644
index 0000000..fdd88fb
--- /dev/null
+++ b/include/net/netfilter/nf_conntrack_labels.h
@@ -0,0 +1,54 @@
+#include <linux/types.h>
+#include <net/net_namespace.h>
+#include <linux/netfilter/nf_conntrack_common.h>
+#include <linux/netfilter/nf_conntrack_tuple_common.h>
+#include <net/netfilter/nf_conntrack.h>
+#include <net/netfilter/nf_conntrack_extend.h>
+
+#include <uapi/linux/netfilter/xt_connlabel.h>
+
+struct nf_conn_labels {
+	u8 words;
+	unsigned long bits[];
+};
+
+static inline struct nf_conn_labels *nf_ct_labels_find(const struct nf_conn *ct)
+{
+#ifdef CONFIG_NF_CONNTRACK_LABELS
+	return nf_ct_ext_find(ct, NF_CT_EXT_LABELS);
+#else
+	return NULL;
+#endif
+}
+
+static inline struct nf_conn_labels *nf_ct_labels_ext_add(struct nf_conn *ct)
+{
+#ifdef CONFIG_NF_CONNTRACK_LABELS
+	struct nf_conn_labels *cl_ext;
+	struct net *net = nf_ct_net(ct);
+	u8 words;
+
+	words = ACCESS_ONCE(net->ct.label_words);
+	if (words == 0 || WARN_ON_ONCE(words > 8))
+		return NULL;
+
+	cl_ext = nf_ct_ext_add_length(ct, NF_CT_EXT_LABELS, words * sizeof(long), GFP_ATOMIC);
+	if (cl_ext != NULL)
+		cl_ext->words = words;
+
+	return cl_ext;
+#else
+	return NULL;
+#endif
+}
+
+#ifdef CONFIG_NF_CONNTRACK_LABELS
+int nf_conntrack_labels_init(struct net *net);
+void nf_conntrack_labels_fini(struct net *net);
+#else
+static inline int nf_conntrack_labels_init(struct net *n) { return 0; }
+static inline void nf_conntrack_labels_fini(struct net *net) {}
+#endif
+
+bool nf_connlabel_match(const struct nf_conn *ct, u16 bit);
+int nf_connlabel_set(struct nf_conn *ct, u16 bit);
diff --git a/include/net/netns/conntrack.h b/include/net/netns/conntrack.h
index a1d83cc..8cc41f4 100644
--- a/include/net/netns/conntrack.h
+++ b/include/net/netns/conntrack.h
@@ -83,6 +83,10 @@ struct netns_ct {
 	int			sysctl_auto_assign_helper;
 	bool			auto_assign_helper_warned;
 	struct nf_ip_net	nf_ct_proto;
+#if defined(CONFIG_NF_CONNTRACK_LABELS)
+	unsigned int labels_used;
+	u8 label_words;
+#endif
 #ifdef CONFIG_NF_NAT_NEEDED
 	struct hlist_head	*nat_bysource;
 	unsigned int		nat_htable_size;
diff --git a/include/uapi/linux/netfilter/xt_connlabel.h b/include/uapi/linux/netfilter/xt_connlabel.h
new file mode 100644
index 0000000..a6ffd33
--- /dev/null
+++ b/include/uapi/linux/netfilter/xt_connlabel.h
@@ -0,0 +1,13 @@
+#include <linux/types.h>
+
+#define XT_CONNLABEL_MAXBIT 127
+enum xt_connlabel_mtopts {
+	XT_CONNLABEL_OP_INVERT = 1 << 0,
+	XT_CONNLABEL_OP_SET    = 1 << 1,
+};
+
+struct xt_connlabel_mtinfo {
+	__u16 bit;
+	__u16 options;
+};
+
diff --git a/net/netfilter/Kconfig b/net/netfilter/Kconfig
index fefa514..acfed41 100644
--- a/net/netfilter/Kconfig
+++ b/net/netfilter/Kconfig
@@ -124,6 +124,12 @@ config NF_CONNTRACK_TIMESTAMP
 
 	  If unsure, say `N'.
 
+config NF_CONNTRACK_LABELS
+	bool
+	help
+	  This option enables support for assigning user-defined flag bits
+	  to connection tracking entries.  It selected by the connlabel match.
+
 config NF_CT_PROTO_DCCP
 	tristate 'DCCP protocol connection tracking support (EXPERIMENTAL)'
 	depends on EXPERIMENTAL
@@ -835,6 +841,18 @@ config NETFILTER_XT_MATCH_CONNBYTES
 	  If you want to compile it as a module, say M here and read
 	  <file:Documentation/kbuild/modules.txt>.  If unsure, say `N'.
 
+config NETFILTER_XT_MATCH_CONNLABEL
+	tristate '"connlabel" match support'
+	select NF_CONNTRACK_LABELS
+	depends on NETFILTER_ADVANCED
+	---help---
+	  This match allows you to test and assign userspace-defined labels names
+	  to a connection.  The kernel only stores bit values - mapping
+	  names to bits is done by userspace.
+
+	  Unlike connmark, more than 32 flag bits may be assigned to a
+	  connection simultaneously.
+
 config NETFILTER_XT_MATCH_CONNLIMIT
 	tristate '"connlimit" match support"'
 	depends on NF_CONNTRACK
diff --git a/net/netfilter/Makefile b/net/netfilter/Makefile
index 3259697..b3bbda6 100644
--- a/net/netfilter/Makefile
+++ b/net/netfilter/Makefile
@@ -4,6 +4,7 @@ nf_conntrack-y	:= nf_conntrack_core.o nf_conntrack_standalone.o nf_conntrack_exp
 nf_conntrack-$(CONFIG_NF_CONNTRACK_TIMEOUT) += nf_conntrack_timeout.o
 nf_conntrack-$(CONFIG_NF_CONNTRACK_TIMESTAMP) += nf_conntrack_timestamp.o
 nf_conntrack-$(CONFIG_NF_CONNTRACK_EVENTS) += nf_conntrack_ecache.o
+nf_conntrack-$(CONFIG_NF_CONNTRACK_LABELS) += nf_conntrack_labels.o
 
 obj-$(CONFIG_NETFILTER) = netfilter.o
 
@@ -101,6 +102,7 @@ obj-$(CONFIG_NETFILTER_XT_MATCH_ADDRTYPE) += xt_addrtype.o
 obj-$(CONFIG_NETFILTER_XT_MATCH_CLUSTER) += xt_cluster.o
 obj-$(CONFIG_NETFILTER_XT_MATCH_COMMENT) += xt_comment.o
 obj-$(CONFIG_NETFILTER_XT_MATCH_CONNBYTES) += xt_connbytes.o
+obj-$(CONFIG_NETFILTER_XT_MATCH_CONNLABEL) += xt_connlabel.o
 obj-$(CONFIG_NETFILTER_XT_MATCH_CONNLIMIT) += xt_connlimit.o
 obj-$(CONFIG_NETFILTER_XT_MATCH_CONNTRACK) += xt_conntrack.o
 obj-$(CONFIG_NETFILTER_XT_MATCH_CPU) += xt_cpu.o
diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
index 0f241be..35a2d47 100644
--- a/net/netfilter/nf_conntrack_core.c
+++ b/net/netfilter/nf_conntrack_core.c
@@ -45,6 +45,7 @@
 #include <net/netfilter/nf_conntrack_zones.h>
 #include <net/netfilter/nf_conntrack_timestamp.h>
 #include <net/netfilter/nf_conntrack_timeout.h>
+#include <net/netfilter/nf_conntrack_labels.h>
 #include <net/netfilter/nf_nat.h>
 #include <net/netfilter/nf_nat_core.h>
 
@@ -770,6 +771,7 @@ void nf_conntrack_free(struct nf_conn *ct)
 }
 EXPORT_SYMBOL_GPL(nf_conntrack_free);
 
+
 /* Allocate a new conntrack: we return -ENOMEM if classification
    failed due to stress.  Otherwise it really is unclassifiable. */
 static struct nf_conntrack_tuple_hash *
@@ -816,6 +818,7 @@ init_conntrack(struct net *net, struct nf_conn *tmpl,
 
 	nf_ct_acct_ext_add(ct, GFP_ATOMIC);
 	nf_ct_tstamp_ext_add(ct, GFP_ATOMIC);
+	nf_ct_labels_ext_add(ct);
 
 	ecache = tmpl ? nf_ct_ecache_find(tmpl) : NULL;
 	nf_ct_ecache_ext_add(ct, ecache ? ecache->ctmask : 0,
@@ -1359,6 +1362,7 @@ static void nf_conntrack_cleanup_net(struct net *net)
 	}
 
 	nf_ct_free_hashtable(net->ct.hash, net->ct.htable_size);
+	nf_conntrack_labels_fini(net);
 	nf_conntrack_helper_fini(net);
 	nf_conntrack_timeout_fini(net);
 	nf_conntrack_ecache_fini(net);
@@ -1585,7 +1589,15 @@ static int nf_conntrack_init_net(struct net *net)
 	ret = nf_conntrack_helper_init(net);
 	if (ret < 0)
 		goto err_helper;
+
+	ret = nf_conntrack_labels_init(net);
+	if (ret < 0)
+		goto err_labels;
+
 	return 0;
+
+err_labels:
+	nf_conntrack_helper_fini(net);
 err_helper:
 	nf_conntrack_timeout_fini(net);
 err_timeout:
diff --git a/net/netfilter/nf_conntrack_labels.c b/net/netfilter/nf_conntrack_labels.c
new file mode 100644
index 0000000..22f4823
--- /dev/null
+++ b/net/netfilter/nf_conntrack_labels.c
@@ -0,0 +1,73 @@
+/*
+ * test/set flag bits stored in conntrack extension area.
+ *
+ * (C) 2012 Astaro GmbH & Co KG
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/ctype.h>
+#include <linux/export.h>
+#include <linux/jhash.h>
+#include <linux/spinlock.h>
+#include <linux/types.h>
+#include <linux/slab.h>
+
+#include <net/netfilter/nf_conntrack_ecache.h>
+#include <net/netfilter/nf_conntrack_labels.h>
+
+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))
+		return -ENOSPC;
+
+	if (test_bit(bit, labels->bits))
+		return 0;
+
+	if (test_and_set_bit(bit, labels->bits))
+		return 0;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(nf_connlabel_set);
+
+static struct nf_ct_ext_type labels_extend __read_mostly = {
+	.len    = sizeof(struct nf_conn_labels),
+	.align  = __alignof__(struct nf_conn_labels),
+	.id     = NF_CT_EXT_LABELS,
+};
+
+int nf_conntrack_labels_init(struct net *net)
+{
+	if (net_eq(net, &init_net))
+		return nf_ct_extend_register(&labels_extend);
+	return 0;
+}
+
+void nf_conntrack_labels_fini(struct net *net)
+{
+	if (net_eq(net, &init_net))
+		nf_ct_extend_unregister(&labels_extend);
+}
+
diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
index 7bbfb3d..cf22362 100644
--- a/net/netfilter/nf_conntrack_netlink.c
+++ b/net/netfilter/nf_conntrack_netlink.c
@@ -43,6 +43,7 @@
 #include <net/netfilter/nf_conntrack_acct.h>
 #include <net/netfilter/nf_conntrack_zones.h>
 #include <net/netfilter/nf_conntrack_timestamp.h>
+#include <net/netfilter/nf_conntrack_labels.h>
 #ifdef CONFIG_NF_NAT_NEEDED
 #include <net/netfilter/nf_nat_core.h>
 #include <net/netfilter/nf_nat_l4proto.h>
@@ -1489,6 +1490,8 @@ ctnetlink_create_conntrack(struct net *net, u16 zone,
 	nf_ct_acct_ext_add(ct, GFP_ATOMIC);
 	nf_ct_tstamp_ext_add(ct, GFP_ATOMIC);
 	nf_ct_ecache_ext_add(ct, 0, 0, GFP_ATOMIC);
+	nf_ct_labels_ext_add(ct);
+
 	/* we must add conntrack extensions before confirmation. */
 	ct->status |= IPS_CONFIRMED;
 
diff --git a/net/netfilter/xt_connlabel.c b/net/netfilter/xt_connlabel.c
new file mode 100644
index 0000000..6256dc3
--- /dev/null
+++ b/net/netfilter/xt_connlabel.c
@@ -0,0 +1,96 @@
+/*
+ * (C) 2012 Astaro GmbH & Co KG
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/module.h>
+#include <linux/skbuff.h>
+#include <net/netfilter/nf_conntrack.h>
+#include <net/netfilter/nf_conntrack_labels.h>
+#include <linux/netfilter/x_tables.h>
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Florian Westphal <fw@strlen.de>");
+MODULE_DESCRIPTION("Xtables: add/match connection trackling labels");
+
+static bool
+connlabel_mt(const struct sk_buff *skb, struct xt_action_param *par)
+{
+        const struct xt_connlabel_mtinfo *info = par->matchinfo;
+	enum ip_conntrack_info ctinfo;
+	struct nf_conn *ct;
+	bool invert = info->options & XT_CONNLABEL_OP_INVERT;
+
+	ct = nf_ct_get(skb, &ctinfo);
+	if (ct == NULL || nf_ct_is_untracked(ct))
+		return invert;
+
+	if (info->options & XT_CONNLABEL_OP_SET)
+		return (nf_connlabel_set(ct, info->bit) == 0) ^ invert;
+
+	return nf_connlabel_match(ct, info->bit) ^ invert;
+}
+
+static int connlabel_mt_check(const struct xt_mtchk_param *par)
+{
+	const int options = XT_CONNLABEL_OP_INVERT |
+			    XT_CONNLABEL_OP_SET;
+	struct xt_connlabel_mtinfo *info = par->matchinfo;
+	int ret;
+	size_t words;
+
+	if (info->bit > XT_CONNLABEL_MAXBIT)
+		return -ERANGE;
+
+	if (info->options & ~options) {
+		pr_err("Unknown options in mask %x\n", info->options);
+		return -EINVAL;
+	}
+
+	ret = nf_ct_l3proto_try_module_get(par->family);
+	if (ret < 0) {
+		pr_info("cannot load conntrack support for proto=%u\n", par->family);
+		return ret;
+	}
+
+	par->net->ct.labels_used++;
+	words = BITS_TO_LONGS(info->bit+1);
+	if (words > par->net->ct.label_words)
+		par->net->ct.label_words = words;
+
+	return ret;
+}
+
+static void connlabel_mt_destroy(const struct xt_mtdtor_param *par)
+{
+	par->net->ct.labels_used--;
+	if (par->net->ct.labels_used == 0)
+		par->net->ct.label_words = 0;
+	nf_ct_l3proto_module_put(par->family);
+}
+
+static struct xt_match connlabels_mt_reg __read_mostly = {
+        .name           = "connlabel",
+        .family         = NFPROTO_UNSPEC,
+        .checkentry     = connlabel_mt_check,
+        .match          = connlabel_mt,
+        .matchsize      = sizeof(struct xt_connlabel_mtinfo),
+        .destroy        = connlabel_mt_destroy,
+        .me             = THIS_MODULE,
+};
+
+static int __init connlabel_mt_init(void)
+{
+	return xt_register_match(&connlabels_mt_reg);
+}
+
+static void __exit connlabel_mt_exit(void)
+{
+	xt_unregister_match(&connlabels_mt_reg);
+}
+
+module_init(connlabel_mt_init);
+module_exit(connlabel_mt_exit);
-- 
1.7.8.6

[PATCH 2/3] netfilter: ctnetlink: deliver labels to userspace via CTA_LABELS attribute

[Florian Westphal]

Userspace receives a bit-vector of currently active labels.
Future patch will permit userspace to also set/delete active labels.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 include/uapi/linux/netfilter/nf_conntrack_common.h |    1 +
 include/uapi/linux/netfilter/nfnetlink_conntrack.h |    1 +
 net/netfilter/nf_conntrack_labels.c                |    2 +-
 net/netfilter/nf_conntrack_netlink.c               |   41 ++++++++++++++++++++
 4 files changed, 44 insertions(+), 1 deletions(-)

diff --git a/include/uapi/linux/netfilter/nf_conntrack_common.h b/include/uapi/linux/netfilter/nf_conntrack_common.h
index 1644cdd..d69483f 100644
--- a/include/uapi/linux/netfilter/nf_conntrack_common.h
+++ b/include/uapi/linux/netfilter/nf_conntrack_common.h
@@ -101,6 +101,7 @@ enum ip_conntrack_events {
 	IPCT_MARK,		/* new mark has been set */
 	IPCT_NATSEQADJ,		/* NAT is doing sequence adjustment */
 	IPCT_SECMARK,		/* new security mark has been set */
+	IPCT_LABEL,		/* new connlabel has been set */
 };
 
 enum ip_conntrack_expect_events {
diff --git a/include/uapi/linux/netfilter/nfnetlink_conntrack.h b/include/uapi/linux/netfilter/nfnetlink_conntrack.h
index 43bfe3e..41d0b6f 100644
--- a/include/uapi/linux/netfilter/nfnetlink_conntrack.h
+++ b/include/uapi/linux/netfilter/nfnetlink_conntrack.h
@@ -47,6 +47,7 @@ enum ctattr_type {
 	CTA_SECCTX,
 	CTA_TIMESTAMP,
 	CTA_MARK_MASK,
+	CTA_LABELS,
 	__CTA_MAX
 };
 #define CTA_MAX (__CTA_MAX - 1)
diff --git a/net/netfilter/nf_conntrack_labels.c b/net/netfilter/nf_conntrack_labels.c
index 22f4823..fe9c0c6 100644
--- a/net/netfilter/nf_conntrack_labels.c
+++ b/net/netfilter/nf_conntrack_labels.c
@@ -46,7 +46,7 @@ int nf_connlabel_set(struct nf_conn *ct, u16 bit)
 		return 0;
 
 	if (test_and_set_bit(bit, labels->bits))
-		return 0;
+		nf_conntrack_event_cache(IPCT_LABEL, ct);
 
 	return 0;
 }
diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
index cf22362..43a1247 100644
--- a/net/netfilter/nf_conntrack_netlink.c
+++ b/net/netfilter/nf_conntrack_netlink.c
@@ -324,6 +324,40 @@ nla_put_failure:
 #define ctnetlink_dump_secctx(a, b) (0)
 #endif
 
+#ifdef CONFIG_NF_CONNTRACK_LABELS
+static int ctnetlink_label_size(const struct nf_conn *ct)
+{
+	struct nf_conn_labels *labels = nf_ct_labels_find(ct);
+
+	if (!labels)
+		return 0;
+	return nla_total_size(labels->words * sizeof(long));
+}
+
+static int
+ctnetlink_dump_labels(struct sk_buff *skb, const struct nf_conn *ct)
+{
+	struct nf_conn_labels *labels = nf_ct_labels_find(ct);
+	unsigned int len, i;
+
+	if (!labels)
+		return 0;
+
+	len = labels->words * sizeof(long);
+	i = 0;
+	do {
+		if (labels->bits[i] != 0)
+			return nla_put(skb, CTA_LABELS, len, labels->bits);
+		i++;
+	} while (i < labels->words);
+
+	return 0;
+}
+#else
+#define ctnetlink_dump_labels(a, b) (0)
+#define ctnetlink_label_size(a)	(0)
+#endif
+
 #define master_tuple(ct) &(ct->master->tuplehash[IP_CT_DIR_ORIGINAL].tuple)
 
 static inline int
@@ -464,6 +498,7 @@ ctnetlink_fill_info(struct sk_buff *skb, u32 portid, u32 seq, u32 type,
 	    ctnetlink_dump_helpinfo(skb, ct) < 0 ||
 	    ctnetlink_dump_mark(skb, ct) < 0 ||
 	    ctnetlink_dump_secctx(skb, ct) < 0 ||
+	    ctnetlink_dump_labels(skb, ct) < 0 ||
 	    ctnetlink_dump_id(skb, ct) < 0 ||
 	    ctnetlink_dump_use(skb, ct) < 0 ||
 	    ctnetlink_dump_master(skb, ct) < 0 ||
@@ -562,6 +597,7 @@ ctnetlink_nlmsg_size(const struct nf_conn *ct)
 	       + nla_total_size(sizeof(u_int32_t)) /* CTA_MARK */
 #endif
 	       + ctnetlink_proto_size(ct)
+	       + ctnetlink_label_size(ct)
 	       ;
 }
 
@@ -663,6 +699,9 @@ ctnetlink_conntrack_event(unsigned int events, struct nf_ct_event *item)
 		    && ctnetlink_dump_secctx(skb, ct) < 0)
 			goto nla_put_failure;
 #endif
+		if (events & (1 << IPCT_LABEL) &&
+		     ctnetlink_dump_labels(skb, ct) < 0)
+			goto nla_put_failure;
 
 		if (events & (1 << IPCT_RELATED) &&
 		    ctnetlink_dump_master(skb, ct) < 0)
@@ -1877,6 +1916,8 @@ ctnetlink_nfqueue_build(struct sk_buff *skb, struct nf_conn *ct)
 	if (ct->mark && ctnetlink_dump_mark(skb, ct) < 0)
 		goto nla_put_failure;
 #endif
+	if (ctnetlink_dump_labels(skb, ct) < 0)
+		goto nla_put_failure;
 	rcu_read_unlock();
 	return 0;
 
-- 
1.7.8.6

[PATCH 3/3] netfilter: ctnetlink: allow userspace to set labels

[Florian Westphal]

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 include/net/netfilter/nf_conntrack_labels.h |    3 ++
 net/netfilter/nf_conntrack_labels.c         |   34 +++++++++++++++++++++++++++
 net/netfilter/nf_conntrack_netlink.c        |   23 ++++++++++++++++++
 3 files changed, 60 insertions(+), 0 deletions(-)

diff --git a/include/net/netfilter/nf_conntrack_labels.h b/include/net/netfilter/nf_conntrack_labels.h
index fdd88fb..e058b2b 100644
--- a/include/net/netfilter/nf_conntrack_labels.h
+++ b/include/net/netfilter/nf_conntrack_labels.h
@@ -52,3 +52,6 @@ static inline void nf_conntrack_labels_fini(struct net *net) {}
 
 bool nf_connlabel_match(const struct nf_conn *ct, u16 bit);
 int nf_connlabel_set(struct nf_conn *ct, u16 bit);
+
+int nfnetlink_connlabel_set(struct nf_conn *ct, const void *data,
+			    unsigned int length);
diff --git a/net/netfilter/nf_conntrack_labels.c b/net/netfilter/nf_conntrack_labels.c
index fe9c0c6..8bff33c 100644
--- a/net/netfilter/nf_conntrack_labels.c
+++ b/net/netfilter/nf_conntrack_labels.c
@@ -52,6 +52,40 @@ int nf_connlabel_set(struct nf_conn *ct, u16 bit)
 }
 EXPORT_SYMBOL_GPL(nf_connlabel_set);
 
+#if IS_ENABLED(CONFIG_NF_CT_NETLINK)
+int nfnetlink_connlabel_set(struct nf_conn *ct, const void *data, unsigned int length)
+{
+	const size_t maxblen = (1024 / BITS_PER_LONG) * sizeof(long);
+	struct nf_conn_labels *labels;
+	unsigned int size;
+
+	if (length > maxblen)
+		return -EMSGSIZE;
+
+	labels = nf_ct_labels_find(ct);
+	if (!labels)
+		return -ENOSPC;
+
+	size = labels->words * sizeof(long);
+
+	if (size < length)
+		length = size;
+
+	if (length)
+		memcpy(labels->bits, data, length);
+
+	if (size > length) {
+		unsigned int pad = size - length;
+		char *mem = (void *) labels->bits;
+		memset(mem + pad, 0, pad);
+	}
+
+	nf_conntrack_event_cache(IPCT_LABEL, ct);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(nfnetlink_connlabel_set);
+#endif
+
 static struct nf_ct_ext_type labels_extend __read_mostly = {
 	.len    = sizeof(struct nf_conn_labels),
 	.align  = __alignof__(struct nf_conn_labels),
diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
index 43a1247..834fe99 100644
--- a/net/netfilter/nf_conntrack_netlink.c
+++ b/net/netfilter/nf_conntrack_netlink.c
@@ -1229,6 +1229,16 @@ ctnetlink_change_nat(struct nf_conn *ct, const struct nlattr * const cda[])
 }
 
 static inline int
+ctnetlink_attach_label(struct nf_conn *ct, const struct nlattr * const cda[])
+{
+#ifdef CONFIG_NF_CONNTRACK_LABELS
+	return nfnetlink_connlabel_set(ct, nla_data(cda[CTA_LABELS]), nla_len(cda[CTA_LABELS]));
+#else
+	return -EOPNOTSUPP;
+#endif
+}
+
+static inline int
 ctnetlink_change_helper(struct nf_conn *ct, const struct nlattr * const cda[])
 {
 	struct nf_conntrack_helper *helper;
@@ -1441,6 +1451,11 @@ ctnetlink_change_conntrack(struct nf_conn *ct,
 			return err;
 	}
 #endif
+	if (cda[CTA_LABELS]) {
+		err = ctnetlink_attach_label(ct, cda);
+		if (err < 0)
+			return err;
+	}
 
 	return 0;
 }
@@ -1649,6 +1664,9 @@ ctnetlink_new_conntrack(struct sock *ctnl, struct sk_buff *skb,
 			else
 				events = IPCT_NEW;
 
+			if (cda[CTA_LABELS] && ctnetlink_attach_label(ct, cda) == 0)
+				events |= (1 << IPCT_LABEL);
+
 			nf_conntrack_eventmask_report((1 << IPCT_REPLY) |
 						      (1 << IPCT_ASSURED) |
 						      (1 << IPCT_HELPER) |
@@ -1946,6 +1964,11 @@ ctnetlink_nfqueue_parse_ct(const struct nlattr *cda[], struct nf_conn *ct)
 		if (err < 0)
 			return err;
 	}
+	if (cda[CTA_LABELS]) {
+		err = ctnetlink_attach_label(ct, cda);
+		if (err < 0)
+			return err;
+	}
 #if defined(CONFIG_NF_CONNTRACK_MARK)
 	if (cda[CTA_MARK])
 		ct->mark = ntohl(nla_get_be32(cda[CTA_MARK]));
-- 
1.7.8.6

Re: [PATCH 3/3] netfilter: ctnetlink: allow userspace to set labels

[Pablo Neira Ayuso]

Hi Florian,

On Thu, Nov 15, 2012 at 04:55:15PM +0100, Florian Westphal wrote:
> Signed-off-by: Florian Westphal <fw@strlen.de>
> ---
>  include/net/netfilter/nf_conntrack_labels.h |    3 ++
>  net/netfilter/nf_conntrack_labels.c         |   34 +++++++++++++++++++++++++++
>  net/netfilter/nf_conntrack_netlink.c        |   23 ++++++++++++++++++
>  3 files changed, 60 insertions(+), 0 deletions(-)
> 
> diff --git a/include/net/netfilter/nf_conntrack_labels.h b/include/net/netfilter/nf_conntrack_labels.h
> index fdd88fb..e058b2b 100644
> --- a/include/net/netfilter/nf_conntrack_labels.h
> +++ b/include/net/netfilter/nf_conntrack_labels.h
> @@ -52,3 +52,6 @@ static inline void nf_conntrack_labels_fini(struct net *net) {}
>  
>  bool nf_connlabel_match(const struct nf_conn *ct, u16 bit);
>  int nf_connlabel_set(struct nf_conn *ct, u16 bit);
> +
> +int nfnetlink_connlabel_set(struct nf_conn *ct, const void *data,
> +			    unsigned int length);
> diff --git a/net/netfilter/nf_conntrack_labels.c b/net/netfilter/nf_conntrack_labels.c
> index fe9c0c6..8bff33c 100644
> --- a/net/netfilter/nf_conntrack_labels.c
> +++ b/net/netfilter/nf_conntrack_labels.c
> @@ -52,6 +52,40 @@ int nf_connlabel_set(struct nf_conn *ct, u16 bit)
>  }
>  EXPORT_SYMBOL_GPL(nf_connlabel_set);
>  
> +#if IS_ENABLED(CONFIG_NF_CT_NETLINK)
> +int nfnetlink_connlabel_set(struct nf_conn *ct, const void *data, unsigned int length)
> +{
> +	const size_t maxblen = (1024 / BITS_PER_LONG) * sizeof(long);
> +	struct nf_conn_labels *labels;
> +	unsigned int size;
> +
> +	if (length > maxblen)
> +		return -EMSGSIZE;
> +
> +	labels = nf_ct_labels_find(ct);
> +	if (!labels)
> +		return -ENOSPC;
> +
> +	size = labels->words * sizeof(long);
> +
> +	if (size < length)
> +		length = size;
> +
> +	if (length)
> +		memcpy(labels->bits, data, length);
> +
> +	if (size > length) {
> +		unsigned int pad = size - length;
> +		char *mem = (void *) labels->bits;
> +		memset(mem + pad, 0, pad);
> +	}
> +
> +	nf_conntrack_event_cache(IPCT_LABEL, ct);
> +	return 0;
> +}

Via ctnetlink_new_conntrack, we should be able to create and set the
connlabel if we want to support state-sync of connlabels.

That requires calling _ext_add(...) to allocate the label, based on
cda[CTA_LABELS], and set it. In that case we're safe to memcpy without
interfering with any ongoing bit testing since that conntrack is not
in the hashes yet.

For the update case, I think we'll have to iterate over the mask and
use xchg to update words, thus, we avoid any interference ongoing bit
testing.

> +EXPORT_SYMBOL_GPL(nfnetlink_connlabel_set);
> +#endif
> +
>  static struct nf_ct_ext_type labels_extend __read_mostly = {
>  	.len    = sizeof(struct nf_conn_labels),
>  	.align  = __alignof__(struct nf_conn_labels),
> diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
> index 43a1247..834fe99 100644
> --- a/net/netfilter/nf_conntrack_netlink.c
> +++ b/net/netfilter/nf_conntrack_netlink.c
> @@ -1229,6 +1229,16 @@ ctnetlink_change_nat(struct nf_conn *ct, const struct nlattr * const cda[])
>  }
>  
>  static inline int
> +ctnetlink_attach_label(struct nf_conn *ct, const struct nlattr * const cda[])
> +{
> +#ifdef CONFIG_NF_CONNTRACK_LABELS
> +	return nfnetlink_connlabel_set(ct, nla_data(cda[CTA_LABELS]), nla_len(cda[CTA_LABELS]));
> +#else
> +	return -EOPNOTSUPP;
> +#endif
> +}
> +
> +static inline int
>  ctnetlink_change_helper(struct nf_conn *ct, const struct nlattr * const cda[])
>  {
>  	struct nf_conntrack_helper *helper;
> @@ -1441,6 +1451,11 @@ ctnetlink_change_conntrack(struct nf_conn *ct,
>  			return err;
>  	}
>  #endif
> +	if (cda[CTA_LABELS]) {
> +		err = ctnetlink_attach_label(ct, cda);
> +		if (err < 0)
> +			return err;
> +	}
>  
>  	return 0;
>  }
> @@ -1649,6 +1664,9 @@ ctnetlink_new_conntrack(struct sock *ctnl, struct sk_buff *skb,
>  			else
>  				events = IPCT_NEW;
>  
> +			if (cda[CTA_LABELS] && ctnetlink_attach_label(ct, cda) == 0)
> +				events |= (1 << IPCT_LABEL);
> +
>  			nf_conntrack_eventmask_report((1 << IPCT_REPLY) |
>  						      (1 << IPCT_ASSURED) |
>  						      (1 << IPCT_HELPER) |
> @@ -1946,6 +1964,11 @@ ctnetlink_nfqueue_parse_ct(const struct nlattr *cda[], struct nf_conn *ct)
>  		if (err < 0)
>  			return err;
>  	}
> +	if (cda[CTA_LABELS]) {
> +		err = ctnetlink_attach_label(ct, cda);
> +		if (err < 0)
> +			return err;
> +	}
>  #if defined(CONFIG_NF_CONNTRACK_MARK)
>  	if (cda[CTA_MARK])
>  		ct->mark = ntohl(nla_get_be32(cda[CTA_MARK]));
> -- 
> 1.7.8.6
> 
> --
> 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 3/3] netfilter: ctnetlink: allow userspace to set labels

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > +#if IS_ENABLED(CONFIG_NF_CT_NETLINK)
> > +int nfnetlink_connlabel_set(struct nf_conn *ct, const void *data, unsigned int length)
> > +{
[..]
> > +	labels = nf_ct_labels_find(ct);
> > +	if (!labels)
> > +		return -ENOSPC;
[..]

> Via ctnetlink_new_conntrack, we should be able to create and set the
> connlabel if we want to support state-sync of connlabels.

Right.   Good point.

> That requires calling _ext_add(...) to allocate the label, based on
> cda[CTA_LABELS], and set it. In that case we're safe to memcpy without
> interfering with any ongoing bit testing since that conntrack is not
> in the hashes yet.

True.  So we can't race with other _ext_add() callers either.
I'll add this functionality, thanks for pointing this out.

> For the update case, I think we'll have to iterate over the mask and
> use xchg to update words, thus, we avoid any interference ongoing bit
> testing.

Could you elaborate?
Why is memcpy not good enough here?

Re: [PATCH 3/3] netfilter: ctnetlink: allow userspace to set labels

[Pablo Neira Ayuso]

On Tue, Nov 27, 2012 at 12:50:00PM +0100, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > +#if IS_ENABLED(CONFIG_NF_CT_NETLINK)
> > > +int nfnetlink_connlabel_set(struct nf_conn *ct, const void *data, unsigned int length)
> > > +{
> [..]
> > > +	labels = nf_ct_labels_find(ct);
> > > +	if (!labels)
> > > +		return -ENOSPC;
> [..]
> 
> > Via ctnetlink_new_conntrack, we should be able to create and set the
> > connlabel if we want to support state-sync of connlabels.
> 
> Right.   Good point.
> 
> > That requires calling _ext_add(...) to allocate the label, based on
> > cda[CTA_LABELS], and set it. In that case we're safe to memcpy without
> > interfering with any ongoing bit testing since that conntrack is not
> > in the hashes yet.
> 
> True.  So we can't race with other _ext_add() callers either.
> I'll add this functionality, thanks for pointing this out.
> 
> > For the update case, I think we'll have to iterate over the mask and
> > use xchg to update words, thus, we avoid any interference ongoing bit
> > testing.
> 
> Could you elaborate?
> Why is memcpy not good enough here?

while updating the connlabel via memcpy, some test_bit on the
connlabel may be already happening. I was suggesting some way to avoid
racing with it.

Re: [PATCH 3/3] netfilter: ctnetlink: allow userspace to set labels

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > For the update case, I think we'll have to iterate over the mask and
> > > use xchg to update words, thus, we avoid any interference ongoing bit
> > > testing.
> > 
> > Could you elaborate?
> > Why is memcpy not good enough here?
> 
> while updating the connlabel via memcpy, some test_bit on the
> connlabel may be already happening. I was suggesting some way to avoid
> racing with it.

I don't understand why its racing.

Is there a case where we update a word, and test_bit can return
"bit is set", even if the bit in the word is neither currently
set nor about to be set?

If not, then I don't see the race; either the test happens
before we copied the word, or afterwards; regardless of copy vs.
xchg?

Re: [PATCH 3/3] netfilter: ctnetlink: allow userspace to set labels

[Pablo Neira Ayuso]

On Tue, Nov 27, 2012 at 02:09:04PM +0100, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > > For the update case, I think we'll have to iterate over the mask and
> > > > use xchg to update words, thus, we avoid any interference ongoing bit
> > > > testing.
> > > 
> > > Could you elaborate?
> > > Why is memcpy not good enough here?
> > 
> > while updating the connlabel via memcpy, some test_bit on the
> > connlabel may be already happening. I was suggesting some way to avoid
> > racing with it.
> 
> I don't understand why its racing.
> 
> Is there a case where we update a word, and test_bit can return
> "bit is set", even if the bit in the word is neither currently
> set nor about to be set?
> 
> If not, then I don't see the race; either the test happens
> before we copied the word, or afterwards; regardless of copy vs.
> xchg?

I was thinking on the case in which we are setting bits via the
connlabel extension and modifying this from ctnetlink at the same
time.

But I don't see any way to make it any better, I think your approach
is fine for the update case.

Re: [PATCH 3/3] netfilter: ctnetlink: allow userspace to set labels

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> I was thinking on the case in which we are setting bits via the
> connlabel extension and modifying this from ctnetlink at the same
> time.

Indeed, in that case we might scribble over a bit that has been set
the instant before.

And yes, this might be a problem.
The only way to fix it (AFAICS) would be to add a new interface to
allow (un)setting specific bits from userspace, so that userspace
could request "set this bit" or "clear that bit", rather than the
current "dump/modify/replace" cycle.

Re: [PATCH 3/3] netfilter: ctnetlink: allow userspace to set labels

[Pablo Neira Ayuso]

On Tue, Nov 27, 2012 at 03:24:33PM +0100, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > I was thinking on the case in which we are setting bits via the
> > connlabel extension and modifying this from ctnetlink at the same
> > time.
> 
> Indeed, in that case we might scribble over a bit that has been set
> the instant before.
> 
> And yes, this might be a problem.
> The only way to fix it (AFAICS) would be to add a new interface to
> allow (un)setting specific bits from userspace, so that userspace
> could request "set this bit" or "clear that bit", rather than the
> current "dump/modify/replace" cycle.

I see. That replacement operation still seems useful to me though.

Quick idea: I think we can also support atomic replacement at word
size using xchg, so the replacement operation can still happen at word
level. Setting many bits at once would be also faster with that.

Re: [PATCH 3/3] netfilter: ctnetlink: allow userspace to set labels

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> Quick idea: I think we can also support atomic replacement at word
> size using xchg, so the replacement operation can still happen at word
> level. Setting many bits at once would be also faster with that.

Unfortunately, no -- the interface is too rudimentary.
Example: You want to set bis 0, 2, 6; but leave all other
bit that are set intact.

So you first need to make a dump to fetch the current labels set.
Then, you set bits 0, 2, 6 and send the new state to the kernel.

But between the dump and the set operation, a new bit might
have been set.  So even when using xchg it will be un-set again...

Re: [PATCH 3/3] netfilter: ctnetlink: allow userspace to set labels

[Pablo Neira Ayuso]

On Fri, Nov 30, 2012 at 03:02:54PM +0100, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > Quick idea: I think we can also support atomic replacement at word
> > size using xchg, so the replacement operation can still happen at word
> > level. Setting many bits at once would be also faster with that.
> 
> Unfortunately, no -- the interface is too rudimentary.
> Example: You want to set bis 0, 2, 6; but leave all other
> bit that are set intact.
> 
> So you first need to make a dump to fetch the current labels set.
> Then, you set bits 0, 2, 6 and send the new state to the kernel.
> 
> But between the dump and the set operation, a new bit might
> have been set.  So even when using xchg it will be un-set again...

what about cmpxchg inside a loop? I think we can assume that the
probability of interference while updating a word is low.

Re: [PATCH 3/3] netfilter: ctnetlink: allow userspace to set labels

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Fri, Nov 30, 2012 at 03:02:54PM +0100, Florian Westphal wrote:
[..]
> > Unfortunately, no -- the interface is too rudimentary.
> > Example: You want to set bis 0, 2, 6; but leave all other
> > bit that are set intact.
> > 
> > So you first need to make a dump to fetch the current labels set.
> > Then, you set bits 0, 2, 6 and send the new state to the kernel.
> > 
> > But between the dump and the set operation, a new bit might
> > have been set.  So even when using xchg it will be un-set again...
> 
> what about cmpxchg inside a loop? I think we can assume that the
> probability of interference while updating a word is low.

I'll try to give an example.

Userspace tool wants to set bit 0 on all labels, and remove all
others.  Except label 1<<31, which should be left alone.

With the proposed interface, you would do something like

dump_conntracks();

for_each_ct_object(..) {
	u32 word = get_ct_labels(ct);

	word &= 0x80000000; /* clear all but bit 31 */
	word |= 1;	    /* set bit 0 */
	send_change_to_kernel(ct); /* tell kernel */
}

No matter what xchg tricks you do in the kernel: if 1<<31 was set
after the dump completed, it will be un-set again via
send_change_to_kernel(), i.e. we clear bit 1<<31, even though we didn't
want to.  I don't see how this can be solved; kernel has no idea that
userspace doesn't wish to alter 1<<31.

We would have to add explicit support for setting/clearing single bits
to make this work, then userspace could say 'set bit 0'. 'clear bit 1',
etc.

HOWEVER, i've failed to come up with a plausible usage scenario where
such an interface would be useful/required. 8-}

So, I'd propose to leave things as they are, i.e. userspace commits
the entire connlabel bitvector. The ruleset would presumably
re-set required labels anyway on the next packet.

If someone can come up with a usage scenario where this
isn't sufficient, we could always add such a 'clear/set bit' command
later.

What do you think?

Cheers,
Florian

Re: [PATCH 3/3] netfilter: ctnetlink: allow userspace to set labels

[Pablo Neira Ayuso]

Hi Florian,

On Fri, Nov 30, 2012 at 10:36:31PM +0100, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > On Fri, Nov 30, 2012 at 03:02:54PM +0100, Florian Westphal wrote:
> [..]
> > > Unfortunately, no -- the interface is too rudimentary.
> > > Example: You want to set bis 0, 2, 6; but leave all other
> > > bit that are set intact.
> > > 
> > > So you first need to make a dump to fetch the current labels set.
> > > Then, you set bits 0, 2, 6 and send the new state to the kernel.
> > > 
> > > But between the dump and the set operation, a new bit might
> > > have been set.  So even when using xchg it will be un-set again...
> > 
> > what about cmpxchg inside a loop? I think we can assume that the
> > probability of interference while updating a word is low.
> 
> I'll try to give an example.
> 
> Userspace tool wants to set bit 0 on all labels, and remove all
> others.  Except label 1<<31, which should be left alone.
> 
> With the proposed interface, you would do something like
> 
> dump_conntracks();
> 
> for_each_ct_object(..) {
> 	u32 word = get_ct_labels(ct);
> 
> 	word &= 0x80000000; /* clear all but bit 31 */
> 	word |= 1;	    /* set bit 0 */
> 	send_change_to_kernel(ct); /* tell kernel */
> }
>
> No matter what xchg tricks you do in the kernel: if 1<<31 was set
> after the dump completed, it will be un-set again via
> send_change_to_kernel(), i.e. we clear bit 1<<31, even though we didn't
> want to.  I don't see how this can be solved; kernel has no idea that
> userspace doesn't wish to alter 1<<31.

We can return -EAGAIN to userspace with cmpxchg. From kernel-space:

old = word;
word |= flags & mask; /* to set/unset a bunch of bits */
if (xchgcmp(&word, new, old) != old)
        return -EAGAIN;

So user-space has to refresh its connlabel and try again with the
fresh one. But that's too much for for user-space.

An alternative for this would be like:

do {
        old = word;
        word |= flags & mask;
} while (xchgcmp(&word, new, old) != old;

So we make sure that we are operating with the fresh word.

I'm assuming we're fine if kernel just set some bit and later on
we explicitly unset it.

Note that I'm also assuming that we pass flags and mask as attribute
as we do for nf_conntrack_tcp.c (see netlink attributes).

> We would have to add explicit support for setting/clearing single bits
> to make this work, then userspace could say 'set bit 0'. 'clear bit 1',
> etc.
> 
> HOWEVER, i've failed to come up with a plausible usage scenario where
> such an interface would be useful/required. 8-}
>
> So, I'd propose to leave things as they are, i.e. userspace commits
> the entire connlabel bitvector. The ruleset would presumably
> re-set required labels anyway on the next packet.
> 
> If someone can come up with a usage scenario where this
> isn't sufficient, we could always add such a 'clear/set bit' command
> later.
> 
> What do you think?

If you think we can make it with the suggestions above, good. If not,
we can keep this back and revisit it later once we get a clear
use-case. You have leave the libnetfilter_conntrack changes on some
branch.

Re: [PATCH 3/3] netfilter: ctnetlink: allow userspace to set labels

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > No matter what xchg tricks you do in the kernel: if 1<<31 was set
> > after the dump completed, it will be un-set again via
> > send_change_to_kernel(), i.e. we clear bit 1<<31, even though we didn't
> > want to.  I don't see how this can be solved; kernel has no idea that
> > userspace doesn't wish to alter 1<<31.
> 
> We can return -EAGAIN to userspace with cmpxchg. From kernel-space:
> 
> old = word;
> word |= flags & mask; /* to set/unset a bunch of bits */
> if (xchgcmp(&word, new, old) != old)
>         return -EAGAIN;

Ah.  you're pulling a mask parameter out of your hat :-)

> I'm assuming we're fine if kernel just set some bit and later on
> we explicitly unset it.

Right.

> Note that I'm also assuming that we pass flags and mask as attribute
> as we do for nf_conntrack_tcp.c (see netlink attributes).

Yes, there is no such thing at the moment.
Userspace just sends a CTA_LABEL attribute, which is a bit-vector
(u32 array).

I can add CTA_LABEL_MASK, of course, and change the memcpy accordingly.

In fact, I think I'll do so to see how much additional code it would be.

Thanks for your hint,
Florian

Re: [PATCH 3/3] netfilter: ctnetlink: allow userspace to set labels

[Pablo Neira Ayuso]

On Mon, Dec 03, 2012 at 12:13:32PM +0100, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > No matter what xchg tricks you do in the kernel: if 1<<31 was set
> > > after the dump completed, it will be un-set again via
> > > send_change_to_kernel(), i.e. we clear bit 1<<31, even though we didn't
> > > want to.  I don't see how this can be solved; kernel has no idea that
> > > userspace doesn't wish to alter 1<<31.
> > 
> > We can return -EAGAIN to userspace with cmpxchg. From kernel-space:
> > 
> > old = word;
> > word |= flags & mask; /* to set/unset a bunch of bits */
> > if (xchgcmp(&word, new, old) != old)
> >         return -EAGAIN;
> 
> Ah.  you're pulling a mask parameter out of your hat :-)

I did :-)

BTW, -EAGAIN already has a meaning for nfnetlink, so some other error
should be returned for the approach above.

Not sure if you checked the other approach I mentioned:

do {
        old = word;
        word |= flags & mask;
} while (xchgcmp(&word, new, old) != old);

So ctnetlink would keep trying until no interference happen.

Re: [PATCH 3/3] netfilter: ctnetlink: allow userspace to set labels

[Pablo Neira Ayuso]

While at it, can you also address a couple of nitpicking suggestions?
See below.

On Thu, Nov 15, 2012 at 04:55:15PM +0100, Florian Westphal wrote:
> Signed-off-by: Florian Westphal <fw@strlen.de>
> ---
>  include/net/netfilter/nf_conntrack_labels.h |    3 ++
>  net/netfilter/nf_conntrack_labels.c         |   34 +++++++++++++++++++++++++++
>  net/netfilter/nf_conntrack_netlink.c        |   23 ++++++++++++++++++
>  3 files changed, 60 insertions(+), 0 deletions(-)
> 
> diff --git a/include/net/netfilter/nf_conntrack_labels.h b/include/net/netfilter/nf_conntrack_labels.h
> index fdd88fb..e058b2b 100644
> --- a/include/net/netfilter/nf_conntrack_labels.h
> +++ b/include/net/netfilter/nf_conntrack_labels.h
> @@ -52,3 +52,6 @@ static inline void nf_conntrack_labels_fini(struct net *net) {}
>  
>  bool nf_connlabel_match(const struct nf_conn *ct, u16 bit);
>  int nf_connlabel_set(struct nf_conn *ct, u16 bit);
> +
> +int nfnetlink_connlabel_set(struct nf_conn *ct, const void *data,
> +			    unsigned int length);
> diff --git a/net/netfilter/nf_conntrack_labels.c b/net/netfilter/nf_conntrack_labels.c
> index fe9c0c6..8bff33c 100644
> --- a/net/netfilter/nf_conntrack_labels.c
> +++ b/net/netfilter/nf_conntrack_labels.c
> @@ -52,6 +52,40 @@ int nf_connlabel_set(struct nf_conn *ct, u16 bit)
>  }
>  EXPORT_SYMBOL_GPL(nf_connlabel_set);
>  
> +#if IS_ENABLED(CONFIG_NF_CT_NETLINK)
> +int nfnetlink_connlabel_set(struct nf_conn *ct, const void *data, unsigned int length)

Probably rename this to nf_connlabel_set? It is not specific of
nfnetlink (although it is the only client of it so far).

> +{
> +	const size_t maxblen = (1024 / BITS_PER_LONG) * sizeof(long);
> +	struct nf_conn_labels *labels;
> +	unsigned int size;
> +
> +	if (length > maxblen)
> +		return -EMSGSIZE;
> +
> +	labels = nf_ct_labels_find(ct);
> +	if (!labels)
> +		return -ENOSPC;
> +
> +	size = labels->words * sizeof(long);
> +
> +	if (size < length)
> +		length = size;
> +
> +	if (length)
> +		memcpy(labels->bits, data, length);
> +
> +	if (size > length) {
> +		unsigned int pad = size - length;
> +		char *mem = (void *) labels->bits;
> +		memset(mem + pad, 0, pad);
> +	}
> +
> +	nf_conntrack_event_cache(IPCT_LABEL, ct);
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(nfnetlink_connlabel_set);
> +#endif
> +
>  static struct nf_ct_ext_type labels_extend __read_mostly = {
>  	.len    = sizeof(struct nf_conn_labels),
>  	.align  = __alignof__(struct nf_conn_labels),
> diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
> index 43a1247..834fe99 100644
> --- a/net/netfilter/nf_conntrack_netlink.c
> +++ b/net/netfilter/nf_conntrack_netlink.c
> @@ -1229,6 +1229,16 @@ ctnetlink_change_nat(struct nf_conn *ct, const struct nlattr * const cda[])
>  }
>  
>  static inline int
> +ctnetlink_attach_label(struct nf_conn *ct, const struct nlattr * const cda[])
> +{
> +#ifdef CONFIG_NF_CONNTRACK_LABELS
> +	return nfnetlink_connlabel_set(ct, nla_data(cda[CTA_LABELS]), nla_len(cda[CTA_LABELS]));
> +#else
> +	return -EOPNOTSUPP;
> +#endif
> +}

Better move this function above to include/net/netfilter/nf_conntrack_labels.h.

> +
> +static inline int
>  ctnetlink_change_helper(struct nf_conn *ct, const struct nlattr * const cda[])
>  {
>  	struct nf_conntrack_helper *helper;
> @@ -1441,6 +1451,11 @@ ctnetlink_change_conntrack(struct nf_conn *ct,
>  			return err;
>  	}
>  #endif
> +	if (cda[CTA_LABELS]) {
> +		err = ctnetlink_attach_label(ct, cda);
> +		if (err < 0)
> +			return err;
> +	}
>  
>  	return 0;
>  }
> @@ -1649,6 +1664,9 @@ ctnetlink_new_conntrack(struct sock *ctnl, struct sk_buff *skb,
>  			else
>  				events = IPCT_NEW;
>  
> +			if (cda[CTA_LABELS] && ctnetlink_attach_label(ct, cda) == 0)
> +				events |= (1 << IPCT_LABEL);
> +
>  			nf_conntrack_eventmask_report((1 << IPCT_REPLY) |
>  						      (1 << IPCT_ASSURED) |
>  						      (1 << IPCT_HELPER) |
> @@ -1946,6 +1964,11 @@ ctnetlink_nfqueue_parse_ct(const struct nlattr *cda[], struct nf_conn *ct)
>  		if (err < 0)
>  			return err;
>  	}
> +	if (cda[CTA_LABELS]) {
> +		err = ctnetlink_attach_label(ct, cda);
> +		if (err < 0)
> +			return err;
> +	}
>  #if defined(CONFIG_NF_CONNTRACK_MARK)
>  	if (cda[CTA_MARK])
>  		ct->mark = ntohl(nla_get_be32(cda[CTA_MARK]));
> -- 
> 1.7.8.6
> 
> --
> 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 0/3] netfilter: add connlabel conntrack extension

[Pablo Neira Ayuso]

Hi Florian,

On Thu, Nov 15, 2012 at 04:55:12PM +0100, Florian Westphal wrote:
> The following three patches add the connlabel extension, plus
> ctnetlink support.
> 
> Connlabels are similar to connmarks, except labels are bit-based; i.e.
> all labels may be attached to a flow at the same time.
> 
> Up to 128 labels are supported at this time.
> 
> Mapping of bit-identifier to label name is done in userspace.
> 
> The extension is enabled at run-time once "-m connlabel" netfilter rules
> are added.
> 
> The '128-labels' limit could be increased, but we need to check that
> extension array won't wrap, when all conntrack extensions are being
> used (offsets are stored in 'u8' array).
> 
> Changes since RFCv2:
> - make it a variable-size extension and remove dynamic
>   reallocation of the label array
> - add ctnetlink support for receiving/setting labels
> - limit to 128 instead of 1k labels due to limited extension
>   space (128 is more than enough for now, so this is no problem).

At a quick glance I like this round, you got this simplified :-).

My only concern is that (if I'm not missing anything) it
inconditionally add the extension even if we don't need it. I can
think of two choices for this:

a) enable this via some /proc interface (which is something we have
tried to avoid in the past, although it's been used for cases in which
we had no choice, eg. conntrack helper compatibility behaviour).

b) use the CT target, which you were not convinced last time we
discussed this.

Re: [PATCH 0/3] netfilter: add connlabel conntrack extension

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Thu, Nov 15, 2012 at 04:55:12PM +0100, Florian Westphal wrote:
> > Changes since RFCv2:
> > - make it a variable-size extension and remove dynamic
> >   reallocation of the label array
> > - add ctnetlink support for receiving/setting labels
> > - limit to 128 instead of 1k labels due to limited extension
> >   space (128 is more than enough for now, so this is no problem).
> 
> At a quick glance I like this round, you got this simplified :-).
> 
> My only concern is that (if I'm not missing anything) it
> inconditionally add the extension even if we don't need it.

Its conditional:

words = ACCESS_ONCE(net->ct.label_words);
if (words == 0 || WARN_ON_ONCE(words > 8))
	return NULL;
cl_ext = nf_ct_ext_add_length(ct, NF_CT_EXT_LABELS, words * sizeof(long),..

xt_connlabel increases "label_words" to a non-zero
value based on the highest bit it saw so far.  So e.g. when bit 32 is
requested, a 32-bit machine kernel will set words to 2, and
a 64 bit kernel to 1, thus turning the extension on for new connections.

The only drawback is that when you add a connlabel rule for bits 127,
then for bit 1, then delete the first rule again words will be larger
than one.

But i think this ok (I don't want to add per-bit refcountr :-) )

Re: [PATCH 0/3] netfilter: add connlabel conntrack extension

[Pablo Neira Ayuso]

On Fri, Nov 16, 2012 at 12:31:09PM +0100, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > On Thu, Nov 15, 2012 at 04:55:12PM +0100, Florian Westphal wrote:
> > > Changes since RFCv2:
> > > - make it a variable-size extension and remove dynamic
> > >   reallocation of the label array
> > > - add ctnetlink support for receiving/setting labels
> > > - limit to 128 instead of 1k labels due to limited extension
> > >   space (128 is more than enough for now, so this is no problem).
> > 
> > At a quick glance I like this round, you got this simplified :-).
> > 
> > My only concern is that (if I'm not missing anything) it
> > inconditionally add the extension even if we don't need it.
> 
> Its conditional:
> 
> words = ACCESS_ONCE(net->ct.label_words);
> if (words == 0 || WARN_ON_ONCE(words > 8))
> 	return NULL;
> cl_ext = nf_ct_ext_add_length(ct, NF_CT_EXT_LABELS, words * sizeof(long),..
> 
> xt_connlabel increases "label_words" to a non-zero
> value based on the highest bit it saw so far.  So e.g. when bit 32 is
> requested, a 32-bit machine kernel will set words to 2, and
> a 64 bit kernel to 1, thus turning the extension on for new connections.
> 
> The only drawback is that when you add a connlabel rule for bits 127,
> then for bit 1, then delete the first rule again words will be larger
> than one.
> 
> But i think this ok (I don't want to add per-bit refcountr :-) )

I see. Let me give a more look in-depth, in any case this version
seems reasonable to be included mainstream :-).

Regards.