[PATCH nf-next,RFC 1/3] netfilter: nf_conntrack: add 64-bit conntrack ID extension

[PATCH nf-next,RFC 1/3] netfilter: nf_conntrack: add 64-bit conntrack ID extension

[Pablo Neira Ayuso]

This patch adds a 64-bit conntrack ID extension that allows userspace to
uniquely identify a conntrack object.

The existing 32-bit ID is not good to uniquely identify a conntrack
object. Long time ago, this used to be an incremental number that could
quickly wrap around. Someone suggested to use 64-bits, back then this
was considered to be too much memory for just an ID. So we usually
suggested to users that they should combine it with the conntrack tuple
to achieve a way to uniquely conntrack objects. This has always
generated a bit of controversy since userspace applications needed to
deal with extra work.

At some point, someone remove the explicit ct->id field that we used to
have to save memory space. This ID was modified to part of its memory
address. Howeover, this is a problem because objects can be quickly
recycled with the slab-by-rcu approach that we use these days. So even
combining this 32-bit ID with the tuple doesn't ensure that this is
unique. Moreover, this is leaking the pointer to userspace in 32-bit
arches, which is not good.

So let's introduce a 64-bit unique ID that ensures no overlaps. This is
only allocated once in the first packet, and never ever again from the
hot path, so let's keep this in a separated extension not to grab more
cachelines.

ID assignment is lockless: this patch divides the 64-bit space between
the existing CPUs, so they can freely allocate IDs in their space.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 include/net/netfilter/nf_conntrack_extend.h |  2 ++
 include/net/netfilter/nf_conntrack_id.h     | 51 +++++++++++++++++++++++++++++
 include/net/netns/conntrack.h               |  1 +
 net/netfilter/Makefile                      |  2 +-
 net/netfilter/nf_conntrack_core.c           | 18 +++++++++-
 net/netfilter/nf_conntrack_id.c             | 48 +++++++++++++++++++++++++++
 net/netfilter/nf_conntrack_netlink.c        |  2 ++
 7 files changed, 122 insertions(+), 2 deletions(-)
 create mode 100644 include/net/netfilter/nf_conntrack_id.h
 create mode 100644 net/netfilter/nf_conntrack_id.c

diff --git a/include/net/netfilter/nf_conntrack_extend.h b/include/net/netfilter/nf_conntrack_extend.h
index 21f887c5058c..274f9370c56a 100644
--- a/include/net/netfilter/nf_conntrack_extend.h
+++ b/include/net/netfilter/nf_conntrack_extend.h
@@ -28,6 +28,7 @@ enum nf_ct_ext_id {
 #if IS_ENABLED(CONFIG_NETFILTER_SYNPROXY)
 	NF_CT_EXT_SYNPROXY,
 #endif
+	NF_CT_EXT_ID,
 	NF_CT_EXT_NUM,
 };
 
@@ -40,6 +41,7 @@ enum nf_ct_ext_id {
 #define NF_CT_EXT_TIMEOUT_TYPE struct nf_conn_timeout
 #define NF_CT_EXT_LABELS_TYPE struct nf_conn_labels
 #define NF_CT_EXT_SYNPROXY_TYPE struct nf_conn_synproxy
+#define NF_CT_EXT_ID_TYPE struct nf_conn_id
 
 /* Extensions: optional stuff which isn't permanently in struct. */
 struct nf_ct_ext {
diff --git a/include/net/netfilter/nf_conntrack_id.h b/include/net/netfilter/nf_conntrack_id.h
new file mode 100644
index 000000000000..4dfd2d2fff6c
--- /dev/null
+++ b/include/net/netfilter/nf_conntrack_id.h
@@ -0,0 +1,51 @@
+#ifndef _NF_CONNTRACK_ID_H
+#define _NF_CONNTRACK_ID_H
+
+#include <linux/types.h>
+#include <net/netfilter/nf_conntrack.h>
+#include <net/netfilter/nf_conntrack_core.h>
+#include <net/netfilter/nf_conntrack_extend.h>
+
+struct nf_conn_id {
+	u64	id;
+};
+
+static inline u64 nf_ct_id(const struct nf_conn *ct)
+{
+	struct nf_conn_id *conn;
+
+	conn = nf_ct_ext_find(ct, NF_CT_EXT_ID);
+	if (!conn)
+		return 0;
+
+	return conn->id;
+}
+
+/* Needs to be called with preemption disabled. */
+static inline u64 nf_ct_id_alloc(struct net *net)
+{
+	u64 *id_cpu = this_cpu_ptr(net->ct.ids);
+
+	return (*id_cpu)++;
+}
+
+static inline struct nf_conn_id *
+nf_ct_id_ext_add(struct net *net, struct nf_conn *ct)
+{
+	struct nf_conn_id *conn;
+
+	conn = nf_ct_ext_add(ct, NF_CT_EXT_ID, GFP_ATOMIC);
+	if (!conn)
+		return NULL;
+
+	conn->id = nf_ct_id_alloc(net);
+
+	return conn;
+}
+
+int nf_conntrack_id_pernet_init(struct net *net);
+void nf_conntrack_id_pernet_fini(struct net *net);
+int nf_conntrack_id_init(void);
+void nf_conntrack_id_fini(void);
+
+#endif /* _NF_CONNTRACK_ID_H */
diff --git a/include/net/netns/conntrack.h b/include/net/netns/conntrack.h
index 9795d628a127..1675c9601c9d 100644
--- a/include/net/netns/conntrack.h
+++ b/include/net/netns/conntrack.h
@@ -114,6 +114,7 @@ struct netns_ct {
 
 	struct ct_pcpu __percpu *pcpu_lists;
 	struct ip_conntrack_stat __percpu *stat;
+	u64			__percpu *ids;
 	struct nf_ct_event_notifier __rcu *nf_conntrack_event_cb;
 	struct nf_exp_event_notifier __rcu *nf_expect_event_cb;
 	struct nf_ip_net	nf_ct_proto;
diff --git a/net/netfilter/Makefile b/net/netfilter/Makefile
index f78ed2470831..ea95e7e79a7e 100644
--- a/net/netfilter/Makefile
+++ b/net/netfilter/Makefile
@@ -1,7 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0
 netfilter-objs := core.o nf_log.o nf_queue.o nf_sockopt.o
 
-nf_conntrack-y	:= nf_conntrack_core.o nf_conntrack_standalone.o nf_conntrack_expect.o nf_conntrack_helper.o nf_conntrack_proto.o nf_conntrack_l3proto_generic.o nf_conntrack_proto_generic.o nf_conntrack_proto_tcp.o nf_conntrack_proto_udp.o nf_conntrack_extend.o nf_conntrack_acct.o nf_conntrack_seqadj.o
+nf_conntrack-y	:= nf_conntrack_core.o nf_conntrack_standalone.o nf_conntrack_expect.o nf_conntrack_helper.o nf_conntrack_proto.o nf_conntrack_l3proto_generic.o nf_conntrack_proto_generic.o nf_conntrack_proto_tcp.o nf_conntrack_proto_udp.o nf_conntrack_extend.o nf_conntrack_acct.o nf_conntrack_seqadj.o nf_conntrack_id.o
 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
diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
index 5749fcaa2770..85129bcf38e4 100644
--- a/net/netfilter/nf_conntrack_core.c
+++ b/net/netfilter/nf_conntrack_core.c
@@ -51,6 +51,7 @@
 #include <net/netfilter/nf_conntrack_timeout.h>
 #include <net/netfilter/nf_conntrack_labels.h>
 #include <net/netfilter/nf_conntrack_synproxy.h>
+#include <net/netfilter/nf_conntrack_id.h>
 #include <net/netfilter/nf_nat.h>
 #include <net/netfilter/nf_nat_core.h>
 #include <net/netfilter/nf_nat_helper.h>
@@ -1241,6 +1242,8 @@ init_conntrack(struct net *net, struct nf_conn *tmpl,
 			     GFP_ATOMIC);
 
 	local_bh_disable();
+	nf_ct_id_ext_add(net, ct);
+
 	if (net->ct.expect_count) {
 		spin_lock(&nf_conntrack_expect_lock);
 		exp = nf_ct_find_expectation(net, zone, tuple);
@@ -1856,6 +1859,7 @@ void nf_conntrack_cleanup_net_list(struct list_head *net_exit_list)
 		nf_conntrack_expect_pernet_fini(net);
 		free_percpu(net->ct.stat);
 		free_percpu(net->ct.pcpu_lists);
+		nf_conntrack_id_pernet_fini(net);
 	}
 }
 
@@ -1971,7 +1975,7 @@ module_param_call(hashsize, nf_conntrack_set_hashsize, param_get_uint,
 static __always_inline unsigned int total_extension_size(void)
 {
 	/* remember to add new extensions below */
-	BUILD_BUG_ON(NF_CT_EXT_NUM > 9);
+	BUILD_BUG_ON(NF_CT_EXT_NUM > 10);
 
 	return sizeof(struct nf_ct_ext) +
 	       sizeof(struct nf_conn_help)
@@ -1995,6 +1999,7 @@ static __always_inline unsigned int total_extension_size(void)
 #if IS_ENABLED(CONFIG_NETFILTER_SYNPROXY)
 		+ sizeof(struct nf_conn_synproxy)
 #endif
+		+ sizeof(struct nf_conn_id)
 	;
 };
 
@@ -2052,6 +2057,10 @@ int nf_conntrack_init_start(void)
 	       NF_CONNTRACK_VERSION, nf_conntrack_htable_size,
 	       nf_conntrack_max);
 
+	ret = nf_conntrack_id_init();
+	if (ret < 0)
+		goto err_id;
+
 	ret = nf_conntrack_expect_init();
 	if (ret < 0)
 		goto err_expect;
@@ -2110,6 +2119,8 @@ int nf_conntrack_init_start(void)
 err_acct:
 	nf_conntrack_expect_fini();
 err_expect:
+	nf_conntrack_id_fini();
+err_id:
 	kmem_cache_destroy(nf_conntrack_cachep);
 err_cachep:
 	nf_ct_free_hashtable(nf_conntrack_hash, nf_conntrack_htable_size);
@@ -2154,6 +2165,9 @@ int nf_conntrack_init_net(struct net *net)
 	if (!net->ct.stat)
 		goto err_pcpu_lists;
 
+	ret = nf_conntrack_id_pernet_init(net);
+	if (ret < 0)
+		goto err_id;
 	ret = nf_conntrack_expect_pernet_init(net);
 	if (ret < 0)
 		goto err_expect;
@@ -2185,6 +2199,8 @@ int nf_conntrack_init_net(struct net *net)
 err_acct:
 	nf_conntrack_expect_pernet_fini(net);
 err_expect:
+	nf_conntrack_id_pernet_fini(net);
+err_id:
 	free_percpu(net->ct.stat);
 err_pcpu_lists:
 	free_percpu(net->ct.pcpu_lists);
diff --git a/net/netfilter/nf_conntrack_id.c b/net/netfilter/nf_conntrack_id.c
new file mode 100644
index 000000000000..8ec535c33aa4
--- /dev/null
+++ b/net/netfilter/nf_conntrack_id.c
@@ -0,0 +1,48 @@
+#include <linux/types.h>
+#include <linux/netfilter.h>
+#include <linux/module.h>
+#include <linux/kernel.h>
+
+#include <net/netfilter/nf_conntrack_id.h>
+
+static const struct nf_ct_ext_type nf_ct_id_extend = {
+	.len    = sizeof(struct nf_conn_id),
+	.align  = __alignof__(struct nf_conn_id),
+	.id     = NF_CT_EXT_ID,
+};
+
+int nf_conntrack_id_pernet_init(struct net *net)
+{
+	int i = 0, cpu;
+
+	net->ct.ids = alloc_percpu(u64);
+	if (!net->ct.ids)
+		return -ENOMEM;
+
+	/* Divide u64 conntrack id space between existing CPUs, so we can
+	 * assign them locklessly.
+	 */
+	for_each_possible_cpu(cpu) {
+		u64 *id_base = per_cpu_ptr(net->ct.ids, cpu);
+
+		*id_base = (U64_MAX / nr_cpu_ids) * i;
+		i++;
+	}
+
+	return 0;
+}
+
+void nf_conntrack_id_pernet_fini(struct net *net)
+{
+	free_percpu(net->ct.ids);
+}
+
+int nf_conntrack_id_init(void)
+{
+	return nf_ct_extend_register(&nf_ct_id_extend);
+}
+
+void nf_conntrack_id_fini(void)
+{
+	nf_ct_extend_unregister(&nf_ct_id_extend);
+}
diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
index 71a43ed19a0f..b3b8249ced4a 100644
--- a/net/netfilter/nf_conntrack_netlink.c
+++ b/net/netfilter/nf_conntrack_netlink.c
@@ -47,6 +47,7 @@
 #include <net/netfilter/nf_conntrack_labels.h>
 #include <net/netfilter/nf_conntrack_seqadj.h>
 #include <net/netfilter/nf_conntrack_synproxy.h>
+#include <net/netfilter/nf_conntrack_id.h>
 #ifdef CONFIG_NF_NAT_NEEDED
 #include <net/netfilter/nf_nat_core.h>
 #include <net/netfilter/nf_nat_l4proto.h>
@@ -1839,6 +1840,7 @@ ctnetlink_create_conntrack(struct net *net,
 	nf_ct_labels_ext_add(ct);
 	nfct_seqadj_ext_add(ct);
 	nfct_synproxy_ext_add(ct);
+	nf_ct_id_ext_add(net, ct);
 
 	/* we must add conntrack extensions before confirmation. */
 	ct->status |= IPS_CONFIRMED;
-- 
2.11.0

[PATCH nf-next,RFC 2/3] netfilter: ctnetlink: use 64-bit conntrack ID

[Pablo Neira Ayuso]

The older 32-bit conntrack ID is still exposed for backward
compatibility reasons, add new CTA_ID64 attribute.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 include/uapi/linux/netfilter/nfnetlink_conntrack.h |  2 ++
 net/netfilter/nf_conntrack_netlink.c               | 14 ++++++++++++--
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/include/uapi/linux/netfilter/nfnetlink_conntrack.h b/include/uapi/linux/netfilter/nfnetlink_conntrack.h
index 7397e022ce6e..dcd7b97eeeac 100644
--- a/include/uapi/linux/netfilter/nfnetlink_conntrack.h
+++ b/include/uapi/linux/netfilter/nfnetlink_conntrack.h
@@ -54,6 +54,8 @@ enum ctattr_type {
 	CTA_MARK_MASK,
 	CTA_LABELS,
 	CTA_LABELS_MASK,
+	CTA_ID64,
+	CTA_PAD,
 	__CTA_MAX
 };
 #define CTA_MAX (__CTA_MAX - 1)
diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
index b3b8249ced4a..7aecb8ae5ecc 100644
--- a/net/netfilter/nf_conntrack_netlink.c
+++ b/net/netfilter/nf_conntrack_netlink.c
@@ -446,7 +446,8 @@ static int ctnetlink_dump_ct_seq_adj(struct sk_buff *skb, struct nf_conn *ct)
 
 static int ctnetlink_dump_id(struct sk_buff *skb, const struct nf_conn *ct)
 {
-	if (nla_put_be32(skb, CTA_ID, htonl((unsigned long)ct)))
+	if (nla_put_be32(skb, CTA_ID, htonl((unsigned long)ct)) ||
+	    nla_put_be64(skb, CTA_ID64, cpu_to_be64(nf_ct_id(ct)), CTA_PAD))
 		goto nla_put_failure;
 	return 0;
 
@@ -600,6 +601,7 @@ static size_t ctnetlink_nlmsg_size(const struct nf_conn *ct)
 	       + 3 * nla_total_size(0) /* CTA_TUPLE_PROTO */
 	       + 3 * nla_total_size(sizeof(u_int8_t)) /* CTA_PROTO_NUM */
 	       + nla_total_size(sizeof(u_int32_t)) /* CTA_ID */
+	       + nla_total_size(sizeof(u64)) /* CTA_ID64 */
 	       + nla_total_size(sizeof(u_int32_t)) /* CTA_STATUS */
 	       + ctnetlink_acct_size(ct)
 	       + ctnetlink_timestamp_size(ct)
@@ -1108,6 +1110,7 @@ static const struct nla_policy ct_nla_policy[CTA_MAX+1] = {
 				    .len = NF_CT_LABELS_MAX_SIZE },
 	[CTA_LABELS_MASK]	= { .type = NLA_BINARY,
 				    .len = NF_CT_LABELS_MAX_SIZE },
+	[CTA_ID64]		= { .type = NLA_U64 },
 };
 
 static int ctnetlink_flush_conntrack(struct net *net,
@@ -1174,6 +1177,13 @@ static int ctnetlink_del_conntrack(struct net *net, struct sock *ctnl,
 			nf_ct_put(ct);
 			return -ENOENT;
 		}
+	} else if (cda[CTA_ID64]) {
+		u64 id = ntohl(nla_get_be64(cda[CTA_ID64]));
+
+		if (id != nf_ct_id(ct)) {
+			nf_ct_put(ct);
+			return -ENOENT;
+		}
 	}
 
 	nf_ct_delete(ct, NETLINK_CB(skb).portid, nlmsg_report(nlh));
@@ -1319,7 +1329,7 @@ ctnetlink_dump_list(struct sk_buff *skb, struct netlink_callback *cb, bool dying
 				if (!atomic_inc_not_zero(&ct->ct_general.use))
 					continue;
 				cb->args[0] = cpu;
-				cb->args[1] = (unsigned long)ct;
+				cb->args[1] = nf_ct_id(ct);
 				spin_unlock_bh(&pcpu->lock);
 				goto out;
 			}
-- 
2.11.0

Re: [PATCH nf-next,RFC 2/3] netfilter: ctnetlink: use 64-bit conntrack ID

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>  static int ctnetlink_flush_conntrack(struct net *net,
> @@ -1174,6 +1177,13 @@ static int ctnetlink_del_conntrack(struct net *net, struct sock *ctnl,
>  			nf_ct_put(ct);
>  			return -ENOENT;
>  		}
> +	} else if (cda[CTA_ID64]) {
> +		u64 id = ntohl(nla_get_be64(cda[CTA_ID64]));

be64_to_cpu()?

But at this point we already uniquely identified the conntrack entry
so the ID check appears to be unneeded?

I never understood existing test either, so this remark isn't specific
to your patch.

Re: [PATCH nf-next,RFC 2/3] netfilter: ctnetlink: use 64-bit conntrack ID

[Pablo Neira Ayuso]

On Tue, Nov 28, 2017 at 01:12:06PM +0100, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> >  static int ctnetlink_flush_conntrack(struct net *net,
> > @@ -1174,6 +1177,13 @@ static int ctnetlink_del_conntrack(struct net *net, struct sock *ctnl,
> >  			nf_ct_put(ct);
> >  			return -ENOENT;
> >  		}
> > +	} else if (cda[CTA_ID64]) {
> > +		u64 id = ntohl(nla_get_be64(cda[CTA_ID64]));
> 
> be64_to_cpu()?
> 
> But at this point we already uniquely identified the conntrack entry
> so the ID check appears to be unneeded?
> 
> I never understood existing test either, so this remark isn't specific
> to your patch.

When the ID was incremental, not a memory address, you could use it to
specifically refer to a conntrack through tuple + id.

If a conntrack with tuple X is gone, then created again, you refer to
the right object.

Re: [PATCH nf-next,RFC 2/3] netfilter: ctnetlink: use 64-bit conntrack ID

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Tue, Nov 28, 2017 at 01:12:06PM +0100, Florian Westphal wrote:
> > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > >  static int ctnetlink_flush_conntrack(struct net *net,
> > > @@ -1174,6 +1177,13 @@ static int ctnetlink_del_conntrack(struct net *net, struct sock *ctnl,
> > >  			nf_ct_put(ct);
> > >  			return -ENOENT;
> > >  		}
> > > +	} else if (cda[CTA_ID64]) {
> > > +		u64 id = ntohl(nla_get_be64(cda[CTA_ID64]));
> > 
> > be64_to_cpu()?
> > 
> > But at this point we already uniquely identified the conntrack entry
> > so the ID check appears to be unneeded?
> > 
> > I never understood existing test either, so this remark isn't specific
> > to your patch.
> 
> When the ID was incremental, not a memory address, you could use it to
> specifically refer to a conntrack through tuple + id.
>
> If a conntrack with tuple X is gone, then created again, you refer to
> the right object.

Yes, but why was that needed?!

I understand what it does, I don't understand the use case :)

[PATCH nf-next,RFC 3/3] netfilter: ctnetlink: randomize 32-bit ID

[Pablo Neira Ayuso]

Don't leak the kernel pointer to userspace by adding a random seed. If
you want a unique conntrack ID, used the new CTA_ID64 attribute.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/nf_conntrack_netlink.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
index 7aecb8ae5ecc..9a3357eeecdc 100644
--- a/net/netfilter/nf_conntrack_netlink.c
+++ b/net/netfilter/nf_conntrack_netlink.c
@@ -60,6 +60,7 @@
 MODULE_LICENSE("GPL");
 
 static char __initdata version[] = "0.93";
+static u32 nf_ct_seed __read_mostly;
 
 static int ctnetlink_dump_tuples_proto(struct sk_buff *skb,
 				const struct nf_conntrack_tuple *tuple,
@@ -444,9 +445,19 @@ static int ctnetlink_dump_ct_seq_adj(struct sk_buff *skb, struct nf_conn *ct)
 	return -1;
 }
 
+static u32 nf_ct_id32(const struct nf_conn *ct)
+{
+	u32 id = (u32)(unsigned long)ct;
+
+	if (!nf_ct_seed)
+		nf_ct_seed = get_random_u32();
+
+	return id + nf_ct_seed;
+}
+
 static int ctnetlink_dump_id(struct sk_buff *skb, const struct nf_conn *ct)
 {
-	if (nla_put_be32(skb, CTA_ID, htonl((unsigned long)ct)) ||
+	if (nla_put_be32(skb, CTA_ID, htonl(nf_ct_id32(ct))) ||
 	    nla_put_be64(skb, CTA_ID64, cpu_to_be64(nf_ct_id(ct)), CTA_PAD))
 		goto nla_put_failure;
 	return 0;
@@ -1173,7 +1184,7 @@ static int ctnetlink_del_conntrack(struct net *net, struct sock *ctnl,
 
 	if (cda[CTA_ID]) {
 		u_int32_t id = ntohl(nla_get_be32(cda[CTA_ID]));
-		if (id != (u32)(unsigned long)ct) {
+		if (id != nf_ct_id32(ct)) {
 			nf_ct_put(ct);
 			return -ENOENT;
 		}
-- 
2.11.0

Re: [PATCH nf-next,RFC 3/3] netfilter: ctnetlink: randomize 32-bit ID

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> Don't leak the kernel pointer to userspace by adding a random seed. If
> you want a unique conntrack ID, used the new CTA_ID64 attribute.
> 
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> ---
>  net/netfilter/nf_conntrack_netlink.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
> index 7aecb8ae5ecc..9a3357eeecdc 100644
> --- a/net/netfilter/nf_conntrack_netlink.c
> +++ b/net/netfilter/nf_conntrack_netlink.c
> @@ -60,6 +60,7 @@
>  MODULE_LICENSE("GPL");
>  
>  static char __initdata version[] = "0.93";
> +static u32 nf_ct_seed __read_mostly;
>  
>  static int ctnetlink_dump_tuples_proto(struct sk_buff *skb,
>  				const struct nf_conntrack_tuple *tuple,
> @@ -444,9 +445,19 @@ static int ctnetlink_dump_ct_seq_adj(struct sk_buff *skb, struct nf_conn *ct)
>  	return -1;
>  }
>  
> +static u32 nf_ct_id32(const struct nf_conn *ct)
> +{
> +	u32 id = (u32)(unsigned long)ct;
> +
> +	if (!nf_ct_seed)
> +		nf_ct_seed = get_random_u32();
> +
> +	return id + nf_ct_seed;
> +}

If we have the 64bit id, why not use it instead of ct address?

Re: [PATCH nf-next,RFC 3/3] netfilter: ctnetlink: randomize 32-bit ID

[Pablo Neira Ayuso]

On Tue, Nov 28, 2017 at 01:18:37PM +0100, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > Don't leak the kernel pointer to userspace by adding a random seed. If
> > you want a unique conntrack ID, used the new CTA_ID64 attribute.
> > 
> > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> > ---
> >  net/netfilter/nf_conntrack_netlink.c | 15 +++++++++++++--
> >  1 file changed, 13 insertions(+), 2 deletions(-)
> > 
> > diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
> > index 7aecb8ae5ecc..9a3357eeecdc 100644
> > --- a/net/netfilter/nf_conntrack_netlink.c
> > +++ b/net/netfilter/nf_conntrack_netlink.c
> > @@ -60,6 +60,7 @@
> >  MODULE_LICENSE("GPL");
> >  
> >  static char __initdata version[] = "0.93";
> > +static u32 nf_ct_seed __read_mostly;
> >  
> >  static int ctnetlink_dump_tuples_proto(struct sk_buff *skb,
> >  				const struct nf_conntrack_tuple *tuple,
> > @@ -444,9 +445,19 @@ static int ctnetlink_dump_ct_seq_adj(struct sk_buff *skb, struct nf_conn *ct)
> >  	return -1;
> >  }
> >  
> > +static u32 nf_ct_id32(const struct nf_conn *ct)
> > +{
> > +	u32 id = (u32)(unsigned long)ct;
> > +
> > +	if (!nf_ct_seed)
> > +		nf_ct_seed = get_random_u32();
> > +
> > +	return id + nf_ct_seed;
> > +}
> 
> If we have the 64bit id, why not use it instead of ct address?

What part of the 64bit would you take? The upper 32-bits are likely to
move not very often with the approach this patchset describes. The
lower 32-bits will bump quickly, but will likely overlap with the same
ID in other CPU.

Re: [PATCH nf-next,RFC 1/3] netfilter: nf_conntrack: add 64-bit conntrack ID extension

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> This patch adds a 64-bit conntrack ID extension that allows userspace to
> uniquely identify a conntrack object.
> 
> The existing 32-bit ID is not good to uniquely identify a conntrack
> object. Long time ago, this used to be an incremental number that could
> quickly wrap around. Someone suggested to use 64-bits, back then this
> was considered to be too much memory for just an ID. So we usually
> suggested to users that they should combine it with the conntrack tuple
> to achieve a way to uniquely conntrack objects. This has always
> generated a bit of controversy since userspace applications needed to
> deal with extra work.
> 
> At some point, someone remove the explicit ct->id field that we used to
> have to save memory space. This ID was modified to part of its memory
> address. Howeover, this is a problem because objects can be quickly
> recycled with the slab-by-rcu approach that we use these days. So even
> combining this 32-bit ID with the tuple doesn't ensure that this is
> unique. Moreover, this is leaking the pointer to userspace in 32-bit
> arches, which is not good.
> 
> So let's introduce a 64-bit unique ID that ensures no overlaps. This is
> only allocated once in the first packet, and never ever again from the
> hot path, so let's keep this in a separated extension not to grab more
> cachelines.
> 
> ID assignment is lockless: this patch divides the 64-bit space between

Why do we need an id in the first place?
Can you elaborate?

As far as I know there is no need for this ID at all,
the netlink attribute is only provided for backwards compat.

And for that the suggested approach (hash of tuple and memory
addresses) will work just fine.

Re: [PATCH nf-next,RFC 1/3] netfilter: nf_conntrack: add 64-bit conntrack ID extension

[Pablo Neira Ayuso]

On Tue, Nov 28, 2017 at 11:54:51AM +0100, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > This patch adds a 64-bit conntrack ID extension that allows userspace to
> > uniquely identify a conntrack object.
> > 
> > The existing 32-bit ID is not good to uniquely identify a conntrack
> > object. Long time ago, this used to be an incremental number that could
> > quickly wrap around. Someone suggested to use 64-bits, back then this
> > was considered to be too much memory for just an ID. So we usually
> > suggested to users that they should combine it with the conntrack tuple
> > to achieve a way to uniquely conntrack objects. This has always
> > generated a bit of controversy since userspace applications needed to
> > deal with extra work.
> > 
> > At some point, someone remove the explicit ct->id field that we used to
> > have to save memory space. This ID was modified to part of its memory
> > address. Howeover, this is a problem because objects can be quickly
> > recycled with the slab-by-rcu approach that we use these days. So even
> > combining this 32-bit ID with the tuple doesn't ensure that this is
> > unique. Moreover, this is leaking the pointer to userspace in 32-bit
> > arches, which is not good.
> > 
> > So let's introduce a 64-bit unique ID that ensures no overlaps. This is
> > only allocated once in the first packet, and never ever again from the
> > hot path, so let's keep this in a separated extension not to grab more
> > cachelines.
> > 
> > ID assignment is lockless: this patch divides the 64-bit space between
> 
> Why do we need an id in the first place?
> Can you elaborate?

It's a userspace thing, for statistics and manipulation via ctnetlink,
eg. delete an entry. To make sure you refer to the same conntrack
entry when polling for statistics and also for events, in case of
event loss, you keep context around in case tear-down and re-creation
happens in little time.

> As far as I know there is no need for this ID at all,
> the netlink attribute is only provided for backwards compat.
> 
> And for that the suggested approach (hash of tuple and memory
> addresses) will work just fine.

It's still possible to land a conntrack with exactly the same tuple on
the same memory address.

Think of a conntrack that is created and teared down very quickly,
then exactly the same conntrack, with same tuples, may be created
again.

I can just keep this patchset back and push the one that randomizes
what we dump to userspace (ie. 3/3), I don't need this now, it's just
a patchset that it's been stuck in my tree for a while.

Re: [PATCH nf-next,RFC 1/3] netfilter: nf_conntrack: add 64-bit conntrack ID extension

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > ID assignment is lockless: this patch divides the 64-bit space between
> > 
> > Why do we need an id in the first place?
> > Can you elaborate?
> 
> It's a userspace thing, for statistics and manipulation via ctnetlink,
> eg. delete an entry. To make sure you refer to the same conntrack
> entry when polling for statistics and also for events, in case of
> event loss, you keep context around in case tear-down and re-creation
> happens in little time.

Hmm.  I think for that use case (e.g. do not delete an entry unless
it is the one we initally saw via NEW in case we missed a DELETE in
between even my jhash based patch in patch work is good enough.

However, in most realistic cases there never was such a gurantee, see
below.

> > As far as I know there is no need for this ID at all,
> > the netlink attribute is only provided for backwards compat.
> > 
> > And for that the suggested approach (hash of tuple and memory
> > addresses) will work just fine.
> 
> It's still possible to land a conntrack with exactly the same tuple on
> the same memory address.

Sure, but the chances are already pretty much close to 0.
Remember that we can have connection re-use while conntrack object
remains unchanged (e.g. new SYN while contrack is in TW).

The ID doesn't change in this case even with the old approach.

ANd what are the chances of a full tuple reuse *right* at the moment
of expiry (i.e. before any other new allocation)?

And why would we treat it differently than syn-in-tw case?

> I can just keep this patchset back and push the one that randomizes
> what we dump to userspace (ie. 3/3), I don't need this now, it's just
> a patchset that it's been stuck in my tree for a while.

I am fine with 3/3.  You might want to use hash_ptr anyway just in case
though (and perhaps include ct->ext).

Re: [PATCH nf-next,RFC 1/3] netfilter: nf_conntrack: add 64-bit conntrack ID extension

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> This patch adds a 64-bit conntrack ID extension that allows userspace to
> uniquely identify a conntrack object.
> 
> The existing 32-bit ID is not good to uniquely identify a conntrack
> object. Long time ago, this used to be an incremental number that could
> quickly wrap around. Someone suggested to use 64-bits, back then this
> was considered to be too much memory for just an ID. So we usually
> suggested to users that they should combine it with the conntrack tuple
> to achieve a way to uniquely conntrack objects. This has always
> generated a bit of controversy since userspace applications needed to
> deal with extra work.
> 
> At some point, someone remove the explicit ct->id field that we used to
> have to save memory space. This ID was modified to part of its memory
> address. Howeover, this is a problem because objects can be quickly
> recycled with the slab-by-rcu approach that we use these days. So even
> combining this 32-bit ID with the tuple doesn't ensure that this is
> unique. Moreover, this is leaking the pointer to userspace in 32-bit
> arches, which is not good.
> 
> So let's introduce a 64-bit unique ID that ensures no overlaps. This is
> only allocated once in the first packet, and never ever again from the
> hot path, so let's keep this in a separated extension not to grab more
> cachelines.
> 
> ID assignment is lockless: this patch divides the 64-bit space between
> the existing CPUs, so they can freely allocate IDs in their space.
> 
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> ---
>  include/net/netfilter/nf_conntrack_extend.h |  2 ++
>  include/net/netfilter/nf_conntrack_id.h     | 51 +++++++++++++++++++++++++++++
>  include/net/netns/conntrack.h               |  1 +
>  net/netfilter/Makefile                      |  2 +-
>  net/netfilter/nf_conntrack_core.c           | 18 +++++++++-
>  net/netfilter/nf_conntrack_id.c             | 48 +++++++++++++++++++++++++++
>  net/netfilter/nf_conntrack_netlink.c        |  2 ++
>  7 files changed, 122 insertions(+), 2 deletions(-)
>  create mode 100644 include/net/netfilter/nf_conntrack_id.h
>  create mode 100644 net/netfilter/nf_conntrack_id.c
> 
> diff --git a/include/net/netfilter/nf_conntrack_extend.h b/include/net/netfilter/nf_conntrack_extend.h
> index 21f887c5058c..274f9370c56a 100644
> --- a/include/net/netfilter/nf_conntrack_extend.h
> +++ b/include/net/netfilter/nf_conntrack_extend.h
> @@ -28,6 +28,7 @@ enum nf_ct_ext_id {
>  #if IS_ENABLED(CONFIG_NETFILTER_SYNPROXY)
>  	NF_CT_EXT_SYNPROXY,
>  #endif
> +	NF_CT_EXT_ID,

I think, if we decide that this new id is needed,
we might as well place this directly in the extension
struct itself rather than an id.

AFAIU this id is always active/set so we never have a
conntrack without this.

> +static inline u64 nf_ct_id(const struct nf_conn *ct)
> +{
> +	struct nf_conn_id *conn;
> +
> +	conn = nf_ct_ext_find(ct, NF_CT_EXT_ID);
> +	if (!conn)
> +		return 0;

so this could just return ct->ext->id.

Re: [PATCH nf-next,RFC 1/3] netfilter: nf_conntrack: add 64-bit conntrack ID extension

[Pablo Neira Ayuso]

On Tue, Nov 28, 2017 at 01:16:52PM +0100, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > This patch adds a 64-bit conntrack ID extension that allows userspace to
> > uniquely identify a conntrack object.
> > 
> > The existing 32-bit ID is not good to uniquely identify a conntrack
> > object. Long time ago, this used to be an incremental number that could
> > quickly wrap around. Someone suggested to use 64-bits, back then this
> > was considered to be too much memory for just an ID. So we usually
> > suggested to users that they should combine it with the conntrack tuple
> > to achieve a way to uniquely conntrack objects. This has always
> > generated a bit of controversy since userspace applications needed to
> > deal with extra work.
> > 
> > At some point, someone remove the explicit ct->id field that we used to
> > have to save memory space. This ID was modified to part of its memory
> > address. Howeover, this is a problem because objects can be quickly
> > recycled with the slab-by-rcu approach that we use these days. So even
> > combining this 32-bit ID with the tuple doesn't ensure that this is
> > unique. Moreover, this is leaking the pointer to userspace in 32-bit
> > arches, which is not good.
> > 
> > So let's introduce a 64-bit unique ID that ensures no overlaps. This is
> > only allocated once in the first packet, and never ever again from the
> > hot path, so let's keep this in a separated extension not to grab more
> > cachelines.
> > 
> > ID assignment is lockless: this patch divides the 64-bit space between
> > the existing CPUs, so they can freely allocate IDs in their space.
> > 
> > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> > ---
> >  include/net/netfilter/nf_conntrack_extend.h |  2 ++
> >  include/net/netfilter/nf_conntrack_id.h     | 51 +++++++++++++++++++++++++++++
> >  include/net/netns/conntrack.h               |  1 +
> >  net/netfilter/Makefile                      |  2 +-
> >  net/netfilter/nf_conntrack_core.c           | 18 +++++++++-
> >  net/netfilter/nf_conntrack_id.c             | 48 +++++++++++++++++++++++++++
> >  net/netfilter/nf_conntrack_netlink.c        |  2 ++
> >  7 files changed, 122 insertions(+), 2 deletions(-)
> >  create mode 100644 include/net/netfilter/nf_conntrack_id.h
> >  create mode 100644 net/netfilter/nf_conntrack_id.c
> > 
> > diff --git a/include/net/netfilter/nf_conntrack_extend.h b/include/net/netfilter/nf_conntrack_extend.h
> > index 21f887c5058c..274f9370c56a 100644
> > --- a/include/net/netfilter/nf_conntrack_extend.h
> > +++ b/include/net/netfilter/nf_conntrack_extend.h
> > @@ -28,6 +28,7 @@ enum nf_ct_ext_id {
> >  #if IS_ENABLED(CONFIG_NETFILTER_SYNPROXY)
> >  	NF_CT_EXT_SYNPROXY,
> >  #endif
> > +	NF_CT_EXT_ID,
> 
> I think, if we decide that this new id is needed,
> we might as well place this directly in the extension
> struct itself rather than an id.
> 
> AFAIU this id is always active/set so we never have a
> conntrack without this.

I understand your goal would be to make this run faster for the first
packet that is part of the connection. This would fit into the 128 bytes
that we preallocate, so you're just trying to remove one level of
indirection, right?

Re: [PATCH nf-next,RFC 1/3] netfilter: nf_conntrack: add 64-bit conntrack ID extension

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > index 21f887c5058c..274f9370c56a 100644
> > > --- a/include/net/netfilter/nf_conntrack_extend.h
> > > +++ b/include/net/netfilter/nf_conntrack_extend.h
> > > @@ -28,6 +28,7 @@ enum nf_ct_ext_id {
> > >  #if IS_ENABLED(CONFIG_NETFILTER_SYNPROXY)
> > >  	NF_CT_EXT_SYNPROXY,
> > >  #endif
> > > +	NF_CT_EXT_ID,
> > 
> > I think, if we decide that this new id is needed,
> > we might as well place this directly in the extension
> > struct itself rather than an id.
> > 
> > AFAIU this id is always active/set so we never have a
> > conntrack without this.
> 
> I understand your goal would be to make this run faster for the first
> packet that is part of the connection. This would fit into the 128 bytes
> that we preallocate, so you're just trying to remove one level of
> indirection, right?

Yes, and save the space in the len/data array for the new extension id.

There is nothing else to it.
We currently do not have an "always on" extension, this would be the
first.