Skip to contentSkip to conversation

nf_conntrack_netlink port take#2

RFC PATCH In review Pablo Neira 4 participants 7 messages Nov 22, 2005 – Dec 4, 2005

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

RFC PATCH 1/1 nf_conntrack_netlink port take#2

Hi,

Sorry for the bad timing. Here attached the take#2 of the nf_conntrack_netlink port. It includes a layer-3 abstraction as suggested. Attached two patches:

- required modifications for nf_conntrack (modifs-take2.patch)
- a diff against ip_conntrack_netlink to make easy the review (x).

The whole nf_conntrack_netlink file is available at:

http://people.netfilter.org/pablo/nf_conntrack_netlink.c

Cheers.

Signature · 1 line
Pablo
include/net/netfilter/nf_conntrack.h +31 -0
index cc48256..64b82b7 100644
@@ -94,6 +94,9 @@ struct nf_conn
 	/* Current number of expected connections */
 	unsigned int expecting;
 
+	/* Unique ID that identifies this conntrack*/
+	unsigned int id;
+
 	/* Helper. if any */
 	struct nf_conntrack_helper *helper;
 
@@ -140,6 +143,9 @@ struct nf_conntrack_expect
 	/* Usage count. */
 	atomic_t use;
 
+	/* Unique ID */
+	unsigned int id;
+
 	/* Flags */
 	unsigned int flags;
 
@@ -190,6 +196,31 @@ static inline void nf_ct_put(struct nf_c
 	nf_conntrack_put(&ct->ct_general);
 }
 
+extern struct nf_conntrack_tuple_hash *
+__nf_conntrack_find(const struct nf_conntrack_tuple *tuple,
+		    const struct nf_conn *ignored_conntrack);
+
+extern void nf_conntrack_hash_insert(struct nf_conn *ct);
+
+extern struct nf_conntrack_expect *
+__nf_conntrack_expect_find(const struct nf_conntrack_tuple *tuple);
+
+extern struct nf_conntrack_expect *
+nf_conntrack_expect_find(const struct nf_conntrack_tuple *tuple);
+
+extern void nf_ct_unlink_expect(struct nf_conntrack_expect *exp);
+
+extern void nf_ct_remove_expectations(struct nf_conn *ct);
+
+extern void nf_conntrack_flush(void);
+
+extern struct nf_conntrack_helper *
+nf_ct_helper_find_get( const struct nf_conntrack_tuple *tuple);
+extern void nf_ct_helper_put(struct nf_conntrack_helper *helper);
+
+extern struct nf_conntrack_helper *
+__nf_conntrack_helper_find_byname(const char *name);
+
 /* call to create an explicit dependency on nf_conntrack. */
 extern void need_nf_conntrack(void);
 
include/net/netfilter/nf_conntrack_helper.h +2 -0
index 5a66b2a..86ec817 100644
@@ -33,6 +33,8 @@ struct nf_conntrack_helper
 		    unsigned int protoff,
 		    struct nf_conn *ct,
 		    enum ip_conntrack_info conntrackinfo);
+
+	int (*to_nfattr)(struct sk_buff *skb, const struct nf_conn *ct);
 };
 
 extern int nf_conntrack_helper_register(struct nf_conntrack_helper *);
include/net/netfilter/nf_conntrack_l3proto.h +14 -1
index 01663e5..67856eb 100644
@@ -14,6 +14,8 @@
 #include <linux/seq_file.h>
 #include <net/netfilter/nf_conntrack.h>
 
+struct nfattr;
+
 struct nf_conntrack_l3proto
 {
 	/* Next pointer. */
@@ -70,6 +72,12 @@ struct nf_conntrack_l3proto
 
 	u_int32_t (*get_features)(const struct nf_conntrack_tuple *tuple);
 
+	int (*tuple_to_nfattr)(struct sk_buff *skb,
+			       const struct nf_conntrack_tuple *t);
+
+	int (*nfattr_to_tuple)(struct nfattr *tb[],
+			       struct nf_conntrack_tuple *t);
+
 	/* Module (if any) which this is connected to. */
 	struct module *me;
 };
@@ -81,11 +89,16 @@ extern int nf_conntrack_l3proto_register
 extern void nf_conntrack_l3proto_unregister(struct nf_conntrack_l3proto *proto);
 
 static inline struct nf_conntrack_l3proto *
-nf_ct_find_l3proto(u_int16_t l3proto)
+__nf_ct_l3proto_find(u_int16_t l3proto)
 {
 	return nf_ct_l3protos[l3proto];
 }
 
+extern struct nf_conntrack_l3proto *
+nf_ct_l3proto_find_get(u_int16_t l3proto);
+
+extern void nf_ct_l3proto_put(struct nf_conntrack_l3proto *p);
+
 /* Existing built-in protocols */
 extern struct nf_conntrack_l3proto nf_conntrack_l3proto_ipv4;
 extern struct nf_conntrack_l3proto nf_conntrack_l3proto_ipv6;
include/net/netfilter/nf_conntrack_protocol.h +25 -1
index b3afda3..1f33737 100644
@@ -12,6 +12,7 @@
 #include <net/netfilter/nf_conntrack.h>
 
 struct seq_file;
+struct nfattr;
 
 struct nf_conntrack_protocol
 {
@@ -66,6 +67,18 @@ struct nf_conntrack_protocol
 		     enum ip_conntrack_info *ctinfo,
 		     int pf, unsigned int hooknum);
 
+	/* convert protoinfo to nfnetink attributes */
+	int (*to_nfattr)(struct sk_buff *skb, struct nfattr *nfa,
+			 const struct nf_conn *ct);
+
+	/* convert nfnetlink attributes to protoinfo */
+	int (*from_nfattr)(struct nfattr *tb[], struct nf_conn *ct);
+
+	int (*tuple_to_nfattr)(struct sk_buff *skb,
+			       const struct nf_conntrack_tuple *t);
+	int (*nfattr_to_tuple)(struct nfattr *tb[],
+			       struct nf_conntrack_tuple *t);
+
 	/* Module (if any) which this is connected to. */
 	struct module *me;
 };
@@ -80,12 +93,23 @@ extern struct nf_conntrack_protocol nf_c
 extern struct nf_conntrack_protocol **nf_ct_protos[PF_MAX];
 
 extern struct nf_conntrack_protocol *
-nf_ct_find_proto(u_int16_t l3proto, u_int8_t protocol);
+__nf_ct_proto_find(u_int16_t l3proto, u_int8_t protocol);
+
+extern struct nf_conntrack_protocol *
+nf_ct_proto_find_get(u_int16_t l3proto, u_int8_t protocol);
+
+extern void nf_ct_proto_put(struct nf_conntrack_protocol *p);
 
 /* Protocol registration. */
 extern int nf_conntrack_protocol_register(struct nf_conntrack_protocol *proto);
 extern void nf_conntrack_protocol_unregister(struct nf_conntrack_protocol *proto);
 
+/* Generic netlink helpers */
+extern int nf_ct_port_tuple_to_nfattr(struct sk_buff *skb,
+				      const struct nf_conntrack_tuple *tuple);
+extern int nf_ct_port_nfattr_to_tuple(struct nfattr *tb[],
+				      struct nf_conntrack_tuple *t);
+
 /* Log invalid packets */
 extern unsigned int nf_ct_log_invalid;
 
include/net/netfilter/nf_conntrack_tuple.h +0 -0
net/ipv4/netfilter/ip_conntrack_netlink.c +0 -0
net/ipv4/netfilter/nf_conntrack_l3proto_ipv4.c +39 -0
index 8202c1c..fcefddf 100644
@@ -392,6 +392,40 @@ getorigdst(struct sock *sk, int optval, 
 	return -ENOENT;
 }
 
+#if defined(CONFIG_NF_CT_NETLINK) || \
+    defined(CONFIG_NF_CT_NETLINK_MODULE)
+
+#include <linux/netfilter/nfnetlink.h>
+#include <linux/netfilter/nfnetlink_conntrack.h>
+
+static int ipv4_tuple_to_nfattr(struct sk_buff *skb,
+				const struct nf_conntrack_tuple *tuple)
+{
+	NFA_PUT(skb, CTA_IP_V4_SRC, sizeof(u_int32_t),
+		&tuple->src.u3.ip);
+	NFA_PUT(skb, CTA_IP_V4_DST, sizeof(u_int32_t),
+		&tuple->dst.u3.ip);
+	return 0;
+
+nfattr_failure:
+	return -1;
+}
+
+static int ipv4_nfattr_to_tuple(struct nfattr *tb[],
+				struct nf_conntrack_tuple *t)
+{
+	if (!tb[CTA_IP_V4_SRC-1] || !tb[CTA_IP_V4_DST-1])
+		return -EINVAL;
+
+	t->src.u3.ip =
+		*(u_int32_t *)NFA_DATA(tb[CTA_IP_V4_SRC-1]);
+	t->dst.u3.ip =
+		*(u_int32_t *)NFA_DATA(tb[CTA_IP_V4_DST-1]);
+
+	return 0;
+}
+#endif
+
 static struct nf_sockopt_ops so_getorigdst = {
 	.pf		= PF_INET,
 	.get_optmin	= SO_ORIGINAL_DST,
@@ -408,6 +442,11 @@ struct nf_conntrack_l3proto nf_conntrack
 	.print_conntrack = ipv4_print_conntrack,
 	.prepare	 = ipv4_prepare,
 	.get_features	 = ipv4_get_features,
+#if defined(CONFIG_NF_CT_NETLINK) || \
+    defined(CONFIG_NF_CT_NETLINK_MODULE)
+	.tuple_to_nfattr = ipv4_tuple_to_nfattr,
+	.nfattr_to_tuple = ipv4_nfattr_to_tuple,
+#endif
 	.me		 = THIS_MODULE,
 };
 
net/ipv4/netfilter/nf_conntrack_proto_icmp.c +58 -8

94 lines not on this page · raw message

net/ipv6/netfilter/nf_conntrack_l3proto_ipv6.c +39 -0
index 753a3ae..a5dffe1 100644
@@ -401,6 +401,40 @@ static ctl_table nf_ct_net_table[] = {
 };
 #endif
 
+#if defined(CONFIG_NF_CT_NETLINK) || \
+    defined(CONFIG_NF_CT_NETLINK_MODULE)
+
+#include <linux/netfilter/nfnetlink.h>
+#include <linux/netfilter/nfnetlink_conntrack.h>
+
+static int ipv6_tuple_to_nfattr(struct sk_buff *skb,
+				const struct nf_conntrack_tuple *tuple)
+{
+	NFA_PUT(skb, CTA_IP_V6_SRC, sizeof(u_int32_t) * 4,
+		&tuple->src.u3.ip6);
+	NFA_PUT(skb, CTA_IP_V6_DST, sizeof(u_int32_t) * 4,
+		&tuple->dst.u3.ip6);
+	return 0;
+
+nfattr_failure:
+	return -1;
+}
+
+static int ipv6_nfattr_to_tuple(struct nfattr *tb[],
+				struct nf_conntrack_tuple *t)
+{
+	if (!tb[CTA_IP_V6_SRC-1] || !tb[CTA_IP_V6_DST-1])
+		return -EINVAL;
+
+	memcpy(t->src.u3.ip6, NFA_DATA(tb[CTA_IP_V6_SRC-1]), 
+	       sizeof(u_int32_t) * 4);
+	memcpy(t->dst.u3.ip6, NFA_DATA(tb[CTA_IP_V6_DST-1]),
+	       sizeof(u_int32_t) * 4);
+
+	return 0;
+}
+#endif
+
 struct nf_conntrack_l3proto nf_conntrack_l3proto_ipv6 = {
 	.l3proto		= PF_INET6,
 	.name			= "ipv6",
@@ -410,6 +444,11 @@ struct nf_conntrack_l3proto nf_conntrack
 	.print_conntrack	= ipv6_print_conntrack,
 	.prepare		= ipv6_prepare,
 	.get_features		= ipv6_get_features,
+#if defined(CONFIG_NF_CT_NETLINK) || \
+    defined(CONFIG_NF_CT_NETLINK_MODULE)
+	.tuple_to_nfattr 	= ipv6_tuple_to_nfattr,
+	.nfattr_to_tuple 	= ipv6_nfattr_to_tuple,
+#endif
 	.me			= THIS_MODULE,
 };
 
net/ipv6/netfilter/nf_conntrack_proto_icmpv6.c +1 -1
index c0f1da5..00574f7 100644
@@ -185,7 +185,7 @@ icmpv6_error_message(struct sk_buff *skb
 		return -NF_ACCEPT;
 	}
 
-	inproto = nf_ct_find_proto(PF_INET6, inprotonum);
+	inproto = __nf_ct_proto_find(PF_INET6, inprotonum);
 
 	/* Are they talking about one of our connections? */
 	if (!nf_ct_get_tuple(skb, inip6off, inprotoff, PF_INET6, inprotonum,
net/netfilter/Kconfig +7 -0
index a84f922..eaab979 100644
@@ -95,4 +95,11 @@ config NF_CONNTRACK_FTP
 
 	  To compile it as a module, choose M here.  If unsure, say N.
 
+config NF_CT_NETLINK
+	tristate 'Connection tracking netlink interface'
+	depends on NF_CONNTRACK && NETFILTER_NETLINK
+	depends on NF_CONNTRACK!=y || NETFILTER_NETLINK!=m
+	help
+	  This option enables support for a netlink-based userspace interface
+
 endmenu
net/netfilter/Makefile +3 -0
index 55f019a..cb21831 100644
@@ -13,3 +13,6 @@ obj-$(CONFIG_NF_CONNTRACK_FTP) += nf_con
 
 # SCTP protocol connection tracking
 obj-$(CONFIG_NF_CT_PROTO_SCTP) += nf_conntrack_proto_sctp.o
+
+# netlink interface for nf_conntrack
+obj-$(CONFIG_NF_CT_NETLINK) += nf_conntrack_netlink.o
net/netfilter/nf_conntrack_core.c +207 -28

394 lines not on this page · raw message

net/netfilter/nf_conntrack_proto_tcp.c +70 -0

88 lines not on this page · raw message

net/netfilter/nf_conntrack_proto_udp.c +10 -0
index 3cae7ce..1a592a5 100644
@@ -196,6 +196,11 @@ struct nf_conntrack_protocol nf_conntrac
 	.packet			= udp_packet,
 	.new			= udp_new,
 	.error			= udp_error4,
+#if defined(CONFIG_NF_CT_NETLINK) || \
+    defined(CONFIG_NF_CT_NETLINK_MODULE)
+	.tuple_to_nfattr	= nf_ct_port_tuple_to_nfattr,
+	.nfattr_to_tuple	= nf_ct_port_nfattr_to_tuple,
+#endif
 };
 
 struct nf_conntrack_protocol nf_conntrack_protocol_udp6 =
@@ -210,6 +215,11 @@ struct nf_conntrack_protocol nf_conntrac
 	.packet			= udp_packet,
 	.new			= udp_new,
 	.error			= udp_error6,
+#if defined(CONFIG_NF_CT_NETLINK) || \
+    defined(CONFIG_NF_CT_NETLINK_MODULE)
+	.tuple_to_nfattr	= nf_ct_port_tuple_to_nfattr,
+	.nfattr_to_tuple	= nf_ct_port_nfattr_to_tuple,
+#endif
 };
 
 EXPORT_SYMBOL(nf_conntrack_protocol_udp4);
net/netfilter/nf_conntrack_standalone.c +29 -10
index 5af381f..4c82f37 100644
@@ -161,14 +161,14 @@ static int ct_seq_show(struct seq_file *
 	if (NF_CT_DIRECTION(hash))
 		return 0;
 
-	l3proto = nf_ct_find_l3proto(conntrack->tuplehash[IP_CT_DIR_ORIGINAL]
-				     .tuple.src.l3num);
+	l3proto = __nf_ct_l3proto_find(conntrack->tuplehash[IP_CT_DIR_ORIGINAL]
+				       .tuple.src.l3num);
 
 	NF_CT_ASSERT(l3proto);
-	proto = nf_ct_find_proto(conntrack->tuplehash[IP_CT_DIR_ORIGINAL]
-				 .tuple.src.l3num,
-				 conntrack->tuplehash[IP_CT_DIR_ORIGINAL]
-				 .tuple.dst.protonum);
+	proto = __nf_ct_proto_find(conntrack->tuplehash[IP_CT_DIR_ORIGINAL]
+				   .tuple.src.l3num,
+				   conntrack->tuplehash[IP_CT_DIR_ORIGINAL]
+				   .tuple.dst.protonum);
 	NF_CT_ASSERT(proto);
 
 	if (seq_printf(s, "%-8s %u %-8s %u %ld ",
@@ -307,9 +307,9 @@ static int exp_seq_show(struct seq_file 
 		   expect->tuple.src.l3num,
 		   expect->tuple.dst.protonum);
 	print_tuple(s, &expect->tuple,
-		    nf_ct_find_l3proto(expect->tuple.src.l3num),
-		    nf_ct_find_proto(expect->tuple.src.l3num,
-				     expect->tuple.dst.protonum));
+		    __nf_ct_l3proto_find(expect->tuple.src.l3num),
+		    __nf_ct_proto_find(expect->tuple.src.l3num,
+				       expect->tuple.dst.protonum));
 	return seq_putc(s, '\n');
 }
 
@@ -847,7 +847,11 @@ EXPORT_SYMBOL(nf_conntrack_helper_unregi
 EXPORT_SYMBOL(nf_ct_iterate_cleanup);
 EXPORT_SYMBOL(__nf_ct_refresh_acct);
 EXPORT_SYMBOL(nf_ct_protos);
-EXPORT_SYMBOL(nf_ct_find_proto);
+EXPORT_SYMBOL(__nf_ct_proto_find);
+EXPORT_SYMBOL(nf_ct_proto_find_get);
+EXPORT_SYMBOL(nf_ct_proto_put);
+EXPORT_SYMBOL(nf_ct_l3proto_find_get);
+EXPORT_SYMBOL(nf_ct_l3proto_put);
 EXPORT_SYMBOL(nf_ct_l3protos);
 EXPORT_SYMBOL(nf_conntrack_expect_alloc);
 EXPORT_SYMBOL(nf_conntrack_expect_put);
@@ -867,3 +871,18 @@ EXPORT_SYMBOL(nf_ct_get_tuple);
 EXPORT_SYMBOL(nf_ct_invert_tuple);
 EXPORT_SYMBOL(nf_conntrack_in);
 EXPORT_SYMBOL(__nf_conntrack_attach);
+EXPORT_SYMBOL(nf_conntrack_alloc);
+EXPORT_SYMBOL(nf_conntrack_free);
+EXPORT_SYMBOL(nf_conntrack_flush);
+EXPORT_SYMBOL(nf_ct_remove_expectations);
+EXPORT_SYMBOL(nf_ct_helper_find_get);
+EXPORT_SYMBOL(nf_ct_helper_put);
+EXPORT_SYMBOL(__nf_conntrack_helper_find_byname);
+EXPORT_SYMBOL(__nf_conntrack_find);
+EXPORT_SYMBOL(nf_ct_unlink_expect);
+EXPORT_SYMBOL(nf_conntrack_hash_insert);
+EXPORT_SYMBOL(__nf_conntrack_expect_find);
+EXPORT_SYMBOL(nf_conntrack_expect_find);
+EXPORT_SYMBOL(nf_conntrack_expect_list);
+EXPORT_SYMBOL(nf_ct_port_tuple_to_nfattr);
+EXPORT_SYMBOL(nf_ct_port_nfattr_to_tuple);
net/netfilter/nf_conntrack_netlink.c renamed from net/ipv4/netfilter/ip_conntrack_netlink.c +217 -194

1,095 lines not on this page · raw message

#1 lore raw
Quoted 5 lines · Pablo Neira

Hi,

Sorry for the bad timing. Here attached the take#2 of the nf_conntrack_netlink port. It includes a layer-3 abstraction as suggested. Attached two patches:

ok, I didn't have the time for a detailed review (and chances are undlikely that I will have anytime soon). However, if noone opposes, I would like to submit this for 2.6.16 quite soon. But not too soon (dave has a open net-2.6.16 tree, but we don't want to flood him with incremental fixes, at least not intentionally). So I'd say Pablo keeps the patch update out-of-tree until 2.6.15 is released, and at that time we push it to mainline immediately.

Signature · 5 lines
- Harald Welte <[email protected]>                 http://netfilter.org/
============================================================================
  "Fragmentation is like classful addressing -- an interesting early
   architectural error that shows how much experimentation was going
   on while IP was being designed."                    -- Paul Vixie
#2 lore raw
Quoted 6 lines · Harald Welte
Quoted 4 lines · Pablo Neira

Sorry for the bad timing. Here attached the take#2 of the nf_conntrack_netlink port. It includes a layer-3 abstraction as suggested. Attached two patches:

Quoted 7 lines

ok, I didn't have the time for a detailed review (and chances are undlikely that I will have anytime soon). However, if noone opposes, I would like to submit this for 2.6.16 quite soon. But not too soon (dave has a open net-2.6.16 tree, but we don't want to flood him with incremental fixes, at least not intentionally). So I'd say Pablo keeps the patch update out-of-tree until 2.6.15 is released, and at that time we push it to mainline immediately.

Fine with it. I'll keep such patch updated at my people.netfilter.org place.

Signature · 1 line
Pablo
#3 lore raw
In reply to Pablo Neira

Hi, Pablo,

From: Pablo Neira <[email protected]>
Date: Tue, 22 Nov 2005 20:22:38 +0100

Quoted 6 lines

Hi,

Sorry for the bad timing. Here attached the take#2 of the nf_conntrack_netlink port. It includes a layer-3 abstraction as suggested. Attached two patches:

Quoted 6 lines

- required modifications for nf_conntrack (modifs-take2.patch)
- a diff against ip_conntrack_netlink to make easy the review (x).

The whole nf_conntrack_netlink file is available at:

http://people.netfilter.org/pablo/nf_conntrack_netlink.c

As far as I read roughly, it looks good. I'll read this deeply and write a patch for ICMPv6 this weekend.

Regards,

-- Yasuyuki Kozakai

#4 lore raw

[Sorry for long message]

Hi, Pablo,

I've succeeded to run 'conntrack -L' with your patch and I could see informations about IPv4 connections!

And I have some comments for fix or cleanup. Some of them can be said about ip_conntrack_netlink, too. Then I marked [nfct] or [both] on all comments.

[nfct] means 'comment on only nf_conntrack_netlink'. [both] means 'comment on both of {ip,nf}_conntrack_netlink'.

include/net/netfilter/nf_conntrack_protocol.h extern struct nf_conntrack_protocol nf_c · line 96

Quoted 3 lines

extern struct nf_conntrack_protocol *

include/net/netfilter/nf_conntrack_protocol.h +1 -1
-nf_ct_find_proto(u_int16_t l3proto, u_int8_t protocol);
+__nf_ct_proto_find(u_int16_t l3proto, u_int8_t protocol);

[nfct] Thanks for these cleanup :)

net/ipv4/netfilter/nf_conntrack_proto_icmp.c checksum_skipped: · line 302

Quoted 13 lines
net/ipv4/netfilter/nf_conntrack_proto_icmp.c +13 -0
+static int icmp_tuple_to_nfattr(struct sk_buff *skb,
+				const struct nf_conntrack_tuple *t)
+{
+	NFA_PUT(skb, CTA_PROTO_ICMP_ID, sizeof(u_int16_t),
+		&t->src.u.icmp.id);
+	NFA_PUT(skb, CTA_PROTO_ICMP_TYPE, sizeof(u_int8_t),
+		&t->dst.u.icmp.type);
+	NFA_PUT(skb, CTA_PROTO_ICMP_CODE, sizeof(u_int8_t),
+		&t->dst.u.icmp.code);
+
+	if (t->dst.u.icmp.type >= sizeof(valid_new) 
+	    || !valid_new[t->dst.u.icmp.type])
+		return -EINVAL;

[both] Why this check isn't done before putting attributes ?

Quoted 7 lines
net/ipv4/netfilter/nf_conntrack_proto_icmp.c +7 -0
+static int icmp_nfattr_to_tuple(struct nfattr *tb[],
+				struct nf_conntrack_tuple *tuple)
+{
+	if (!tb[CTA_PROTO_ICMP_TYPE-1]
+	    || !tb[CTA_PROTO_ICMP_CODE-1]
+	    || !tb[CTA_PROTO_ICMP_ID-1])
+		return -1;

net/ipv4/netfilter/nf_conntrack_proto_icmp.c checksum_skipped: · line 326

Quoted 10 lines
net/ipv4/netfilter/nf_conntrack_proto_icmp.c +10 -0
+
+	tuple->dst.u.icmp.type = 
+			*(u_int8_t *)NFA_DATA(tb[CTA_PROTO_ICMP_TYPE-1]);
+	tuple->dst.u.icmp.code =
+			*(u_int8_t *)NFA_DATA(tb[CTA_PROTO_ICMP_CODE-1]);
+	tuple->src.u.icmp.id =
+			*(u_int16_t *)NFA_DATA(tb[CTA_PROTO_ICMP_ID-1]);
+
+	return 0;
+}

[both] I think that it's necessary to check with valid_new[], to avoid to create the connection which nf_conntrack cannot track.

net/ipv6/netfilter/nf_conntrack_l3proto_ipv6.c static ctl_table nf_ct_net_table[] = { · line 432

Quoted 10 lines
net/ipv6/netfilter/nf_conntrack_l3proto_ipv6.c +10 -0
+static int ipv6_nfattr_to_tuple(struct nfattr *tb[],
+				struct nf_conntrack_tuple *t)
+{
+	if (!tb[CTA_IP_V6_SRC-1] || !tb[CTA_IP_V6_DST-1])
+		return -EINVAL;
+
+	memcpy(t->src.u3.ip6, NFA_DATA(tb[CTA_IP_V6_SRC-1]), 
+	       sizeof(u_int32_t) * 4);
+	memcpy(t->dst.u3.ip6, NFA_DATA(tb[CTA_IP_V6_DST-1]),
+	       sizeof(u_int32_t) * 4);

[nfct] cosmetic: Here you use t->src.u3.ip6, but you use &t->src.u3.ip6 in ipv6_tuple_to_nfattr().

net/netfilter/nf_conntrack_core.c static inline int helper_cmp(const struc · line 816

Quoted 7 lines
net/netfilter/nf_conntrack_core.c +7 -0
+struct nf_conntrack_helper *
+nf_ct_helper_find_get( const struct nf_conntrack_tuple *tuple)
+{
+	struct nf_conntrack_helper *helper;
+
+	/* need ip_conntrack_lock to assure that helper exists until
+	 * try_module_get() is called */

[nfct] cosmetic: Please sed s/ip_conntrack_lock/nf_conntrack_lock/.

Quoted 9 lines
net/netfilter/nf_conntrack_core.c +1 -3
-void nf_conntrack_cleanup(void)
+void nf_conntrack_flush()
 {
-	int i;
-
 	/* This makes sure all current packets have passed through
 	   netfilter framework.  Roll on, two-stage module
 	   delete... */

net/netfilter/nf_conntrack_core.c void nf_conntrack_cleanup(void) · line 1556

Quoted 7 lines
net/netfilter/nf_conntrack_core.c +4 -0
@@ -1383,6 +1550,18 @@ void nf_conntrack_cleanup(void)
 		schedule();
 		goto i_see_dead_people;
 	}
+	/* wait until all references to ip_conntrack_untracked are dropped */
+	while (atomic_read(&nf_conntrack_untracked.ct_general.use) > 1)
+		schedule();
+}

[nfct] nice catch! I think this would better to be applied to mainline ASAP. Could you re-send this in other patch ?

OK, next is nf_conntrack_netlink.c

Quoted 13 lines

static inline int
ctnetlink_dump_tuples(struct sk_buff *skb,
const struct nf_conntrack_tuple *tuple)

{
	struct nfattr *nest_parms;
	struct nf_conntrack_l3proto *l3proto;
	int ret = 0;
	
	l3proto = nf_ct_l3proto_find_get(tuple->src.l3num);
	
	nest_parms = NFA_NEST(skb, CTA_TUPLE_IP);
	if (likely(l3proto->tuple_to_nfattr))
		ret = l3proto->tuple_to_nfattr(skb, tuple);

[nfct] if this function failed, can this stop dumping and return -1 ?

Quoted 6 lines
	NFA_NEST_END(skb, nest_parms);

	nf_ct_l3proto_put(l3proto);

	nest_parms = NFA_NEST(skb, CTA_TUPLE_PROTO);
	ctnetlink_dump_tuples_proto(skb, tuple);

[both] Is there any reason that return value has to be ignored ?

Quoted 10 lines

static inline int
ctnetlink_dump_helpinfo(struct sk_buff *skb, const struct nf_conn *ct)

{
	struct nfattr *nest_helper;

	if (!ct->helper)
		return 0;
		
	nest_helper = NFA_NEST(skb, CTA_HELP);
	NFA_PUT(skb, CTA_HELP_NAME, CTA_HELP_MAXNAMESIZE, &ct->helper->name);

[both] Oh, &ct->helper->name doesn't point intended area. Because ct->helper->name isn't declared as array.

[both] And its size may be less than CTA_HELP_MAXNAMESIZE.

Quoted 4 lines

static inline int
ctnetlink_dump_use(struct sk_buff *skb, const struct nf_conn *ct)

{
	unsigned int use = htonl(atomic_read(&ct->ct_general.use));

[both] cosmetic(?): Why 'unsigned int' is used here ? other functions seems to use 'u_int32_t'.

Quoted 3 lines

static int ctnetlink_conntrack_event(struct notifier_block *this,
unsigned long events, void *ptr)
{

<snip>

Quoted 25 lines
	if (events & IPCT_DESTROY) {
		type = IPCTNL_MSG_CT_DELETE;
		group = NFNLGRP_CONNTRACK_DESTROY;
		goto alloc_skb;
	}
	if (events & (IPCT_NEW | IPCT_RELATED)) {
		type = IPCTNL_MSG_CT_NEW;
		flags = NLM_F_CREATE|NLM_F_EXCL;
		/* dump everything */
		events = ~0UL;
		group = NFNLGRP_CONNTRACK_NEW;
		goto alloc_skb;
	}
	if (events & (IPCT_STATUS |
		      IPCT_PROTOINFO |
		      IPCT_HELPER |
		      IPCT_HELPINFO |
		      IPCT_NATINFO)) {
		type = IPCTNL_MSG_CT_NEW;
		group = NFNLGRP_CONNTRACK_UPDATE;
		goto alloc_skb;
	} 
	
	return NOTIFY_DONE;
Quoted 1 line

alloc_skb:

[both] cleanup: Why don't we use 'else if ... else' instead of 'goto' ?

Quoted 4 lines
static const size_t cta_min_ip[CTA_IP_MAX] = {
	[CTA_IP_V4_SRC-1]	= sizeof(u_int32_t),
	[CTA_IP_V4_DST-1]	= sizeof(u_int32_t),
};

[nfct] Can CTA_IP_V6_{SRC/DST} join them ? :)

[both] cleanup: Or, it might be better that this size check goes to l3proto->nfattr_to_tuple(), to generalize more.

Quoted 14 lines

static inline int ctnetlink_parse_tuple_proto(struct nfattr *attr,
struct nf_conntrack_tuple *tuple)

{
	struct nfattr *tb[CTA_PROTO_MAX];
	struct nf_conntrack_protocol *proto;
	int ret = 0;

	DEBUGP("entered %s\n", __FUNCTION__);

	nfattr_parse_nested(tb, CTA_PROTO_MAX, attr);

	if (nfattr_bad_size(tb, CTA_PROTO_MAX, cta_min_proto))
		return -EINVAL;

[both] cleanup: again, it might be better that this goes to proto->nfattr_to_tuple().

Quoted 13 lines

static int ctnetlink_parse_nat_proto(struct nfattr *attr,
const struct nf_conn *ct, struct ip_nat_range *range)

{
	struct nfattr *tb[CTA_PROTONAT_MAX];
	struct ip_nat_protocol *npt;

	DEBUGP("entered %s\n", __FUNCTION__);

	nfattr_parse_nested(tb, CTA_PROTONAT_MAX, attr);

	if (nfattr_bad_size(tb, CTA_PROTONAT_MAX, cta_min_protonat))
		return -EINVAL;

[both] cleanup: I think this can go to ip_nat_port_nfattr_to_range(), too.

Quoted 5 lines

static int
ctnetlink_create_conntrack(struct nfattr *cda[],
struct nf_conntrack_tuple *otuple, struct nf_conntrack_tuple *rtuple)
{

<snip>

Quoted 8 lines
	ct->helper = nf_ct_helper_find_get(rtuple);

	add_timer(&ct->timeout);
	nf_conntrack_hash_insert(ct);

	if (ct->helper)
		nf_ct_helper_put(ct->helper);
Quoted 4 lines
#if defined(CONFIG_IP_NF_CONNTRACK_MARK)
	if (cda[CTA_MARK-1])
		ct->mark = ntohl(*(u_int32_t *)NFA_DATA(cda[CTA_MARK-1]));

#endif

[both] this should go before nf_conntrack_hash_insert(). Here is not protected with nf_conntrack_lock.

Quoted 6 lines

static int
ctnetlink_exp_fill_info(struct sk_buff *skb, u32 pid, u32 seq,
int event,
int nowait,
const struct nf_conntrack_expect *exp)
{

<snip>

Quoted 5 lines
	nlh    = NLMSG_PUT(skb, pid, seq, event, sizeof(struct nfgenmsg));
	nfmsg  = NLMSG_DATA(nlh);

	nlh->nlmsg_flags    = (nowait && pid) ? NLM_F_MULTI : 0;
	nfmsg->nfgen_family = AF_INET;

[nfct] exp->tuple.src.l3num ?

Quoted 3 lines

static int ctnetlink_expect_event(struct notifier_block *this,
unsigned long events, void *ptr)
{

<snip>

Quoted 6 lines
	type |= NFNL_SUBSYS_CTNETLINK << 8;
	nlh   = NLMSG_PUT(skb, 0, 0, type, sizeof(struct nfgenmsg));
	nfmsg = NLMSG_DATA(nlh);

	nlh->nlmsg_flags    = flags;
	nfmsg->nfgen_family = AF_INET;

[nfct] again, exp->tuple.src.l3num ?

Quoted 8 lines
	nfmsg->version	    = NFNETLINK_V0;
	nfmsg->res_id	    = 0;

	if (ctnetlink_exp_dump_expect(skb, exp) < 0)
		goto nfattr_failure;

	nlh->nlmsg_len = skb->tail - b;
	proto = exp->tuple.dst.protonum;

[both] cleanup: proto is set only here, but it isn't used.

-- Yasuyuki Kozakai

#5 lore raw
Quoted 8 lines · Yasuyuki KOZAKAI
Quoted 8 lines
net/ipv4/netfilter/nf_conntrack_proto_icmp.c +8 -0
+static int icmp_tuple_to_nfattr(struct sk_buff *skb,
+				const struct nf_conntrack_tuple *t)
+{
+	NFA_PUT(skb, CTA_PROTO_ICMP_ID, sizeof(u_int16_t),
+		&t->src.u.icmp.id);
+	NFA_PUT(skb, CTA_PROTO_ICMP_TYPE, sizeof(u_int8_t),
+		&t->dst.u.icmp.type);
+	NFA_PUT(skb, CTA_PROTO_ICMP_CODE, sizeof(u_int8_t),
Quoted 8 lines

net/ipv4/netfilter/nf_conntrack_proto_icmp.c checksum_skipped: · line 302

Quoted 5 lines
net/ipv4/netfilter/nf_conntrack_proto_icmp.c +5 -0
+		&t->dst.u.icmp.code);
+
+	if (t->dst.u.icmp.type >= sizeof(valid_new) 
+	    || !valid_new[t->dst.u.icmp.type])
+		return -EINVAL;

[both] Why this check isn't done before putting attributes ?

Why is this check done at all? We're just dumping the tuples the kernel has already created and validated.

Quoted 12 lines
Quoted 12 lines
net/netfilter/nf_conntrack_core.c +1 -1
-void nf_conntrack_cleanup(void)
+void nf_conntrack_flush()

{

net/netfilter/nf_conntrack_core.c +0 -2
-	int i;
-
	/* This makes sure all current packets have passed through
	   netfilter framework.  Roll on, two-stage module
	   delete... */
net/netfilter/nf_conntrack_core.c +0 -0
@@ -1383,6 +1550,18 @@ void nf_conntrack_cleanup(void)
 		schedule();
 		goto i_see_dead_people;
 	}
Quoted 8 lines

net/netfilter/nf_conntrack_core.c void nf_conntrack_cleanup(void) · line 1556

Quoted 4 lines
net/netfilter/nf_conntrack_core.c +4 -0
+	/* wait until all references to ip_conntrack_untracked are dropped */
+	while (atomic_read(&nf_conntrack_untracked.ct_general.use) > 1)
+		schedule();
+}

[nfct] nice catch! I think this would better to be applied to mainline ASAP. Could you re-send this in other patch ?

I agree to the fix, but disagree about abusing this function for ctnetlink. ctnetlink doesn't care about references and doesn't need to wait for untracked conntracks, it just needs to clean the table. I have a patch here for ctnetlink that replaces the use of this function by ip_ct_iterate_cleanup, which significantly speeds up flushing the table on a busy (conntrack-wise) system.

Quoted 8 lines
Quoted 8 lines

static inline int
ctnetlink_dump_helpinfo(struct sk_buff *skb, const struct nf_conn *ct)

{
	struct nfattr *nest_helper;

	if (!ct->helper)
		return 0;
Quoted 8 lines
Quoted 2 lines
	nest_helper = NFA_NEST(skb, CTA_HELP);
	NFA_PUT(skb, CTA_HELP_NAME, CTA_HELP_MAXNAMESIZE, &ct->helper->name);

[both] Oh, &ct->helper->name doesn't point intended area. Because ct->helper->name isn't declared as array.

[both] And its size may be less than CTA_HELP_MAXNAMESIZE.

And also larger. I don't think we need to limit the length at all, but we should make sure to stop at the trailing \0 byte.

#6 lore raw
In reply to Yasuyuki KOZAKAI
Quoted 11 lines · Yasuyuki KOZAKAI

Review void nf_conntrack_cleanup(void) · line 1556

Quoted 8 lines
(quoted diff) +4 -0
@@ -1383,6 +1550,18 @@ void nf_conntrack_cleanup(void)
 		schedule();
 		goto i_see_dead_people;
 	}
+	/* wait until all references to ip_conntrack_untracked are dropped */
+	while (atomic_read(&nf_conntrack_untracked.ct_general.use) > 1)
+		schedule();
+}

[nfct] nice catch! I think this would better to be applied to mainline ASAP.

Agreed. I've added this patch to my tree.

[NETFILTER]: Wait for untracked references in nf_conntrack module unload

Noticed by Pablo Neira <[email protected]>.

---
commit 96c75906027f008ed3a4058a606938901e9c6d99
tree 87b8bb22d35f61383fbb5f4ee6164a16cc256bb3
parent aea6c7fa24378c7f38026177c0b524719789b4be
author Patrick McHardy <[email protected]> Sun, 04 Dec 2005 17:22:02 +0100 committer Patrick McHardy <[email protected]> Sun, 04 Dec 2005 17:22:02 +0100

 net/netfilter/nf_conntrack_core.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)
net/netfilter/nf_conntrack_core.c +3 -0
index 1da6783..a99285d 100644
@@ -1383,6 +1383,9 @@ void nf_conntrack_cleanup(void)
 		schedule();
 		goto i_see_dead_people;
 	}
+	/* wait until all references to nf_conntrack_untracked are dropped */
+	while (atomic_read(&nf_conntrack_untracked.ct_general.use) > 1)
+		break;
 
 	for (i = 0; i < NF_CT_F_NUM; i++) {
 		if (nf_ct_cache[i].use == 0)