remaining libnetfilter_conntrack issues

remaining libnetfilter_conntrack issues

[Harald Welte]

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

Hi Pablo!

I've started to write a new ulogd2 plugin against the new
libnetfilter_conntrack, and found a number of issues, mostly related to
header files:

1) why do we need 'asm/types.h'?  this is unacceptable
2) why do we need 'linux/if.h'?
3) libnetfilter_conntrack needs to be split into two parts:
	a) what is visible to application programs
	b) what is visible to extensions
4) if we do '3', we can get rid of the ugly "linux_list.h" include
5) we'd ideally get rid of the ip_conntrack.h include, too
6) we should ship a copy of both nfnetlink.h and nfnetlink_conntrack.h with the library
   this way userspace programs can be built without any
   nfnetlink-enabled kernel headers installed.

I think everything but '5' should be fairly trivial to do.  Which
definitions do we really need from ip_conntrack.h?  The status bits?
Then we should also ship a copy of only those status bit definitions
with the library.  

All copies should be performed during the 'distrib' stage, i.e. every
time we make a release we tell the automake/autoconf system where to
take a current copy of those headers from.  They would not be present in
svn.

Cheers,

-- 
- Harald Welte <laforge@netfilter.org>                 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

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: remaining libnetfilter_conntrack issues

[Harald Welte]

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

On Fri, Oct 28, 2005 at 10:59:55PM +0200, Harald Welte wrote:
> Hi Pablo!
> 
> I've started to write a new ulogd2 plugin against the new
> libnetfilter_conntrack, and found a number of issues, mostly related to
> header files:

Another comment:

7) we put the definition of the 'nfct_handle' into a private header
   file. this is good practise for encapsulation, since it prevents the
   application from making any assumptions about its size and/or
   contents.

Thanks for your consideration,
	Harald
-- 
- Harald Welte <laforge@netfilter.org>                 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

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: remaining libnetfilter_conntrack issues

[Harald Welte]

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

On Fri, Oct 28, 2005 at 11:08:57PM +0200, Harald Welte wrote:
> [..]
> Another comment:

8) IPv6 addresses are 128bit, not 64!

-- 
- Harald Welte <laforge@netfilter.org>                 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

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: remaining libnetfilter_conntrack issues

[Pablo Neira]

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

Hi Harald!

Harald Welte wrote:
> I've started to write a new ulogd2 plugin against the new
> libnetfilter_conntrack, and found a number of issues, mostly related to
> header files:
> 
> 1) why do we need 'asm/types.h'?  this is unacceptable
> 2) why do we need 'linux/if.h'?

I just killed those. I'm not aware of having added those by myself, my 
guess is that they are remains of the primitive libctnetlink.

> 3) libnetfilter_conntrack needs to be split into two parts:
> 	a) what is visible to application programs
> 	b) what is visible to extensions
> 4) if we do '3', we can get rid of the ugly "linux_list.h" include

OK, now we have libnetfilter_conntrack.h and 
libnetfilter_conntrack_extensions.h, have a look at the patch below. I'm 
going to commit it asap.

> 5) we'd ideally get rid of the ip_conntrack.h include, too

I just did it, we just need the status bits. So I just took them from 
ip_conntrack.h

> 6) we should ship a copy of both nfnetlink.h and nfnetlink_conntrack.h with the library
>    this way userspace programs can be built without any
>    nfnetlink-enabled kernel headers installed.
> 
> All copies should be performed during the 'distrib' stage, i.e. every
> time we make a release we tell the automake/autoconf system where to
> take a current copy of those headers from.  They would not be present in
> svn.

OK, so we could just do it in autogen.sh, that would be quite straight 
forward. fine with it?

> 7) we put the definition of the 'nfct_handle' into a private header
>    file. this is good practise for encapsulation, since it prevents the
>    application from making any assumptions about its size and/or
>    contents.

Done

> 8) IPv6 addresses are 128bit, not 64!

Aiee, fixed as well.

I think we can release the first 1.0 version of conntrack this week, 
together with the first release of libnetfilter_conntrack. As soon as I 
finish the test for the expectation part of libnetfilter_conntrack (see 
utils) and fix some remaining problems reported on netfilter-devel. 
Then, I'll have a look back at your previous email detailing how to do 
releases, I didn't have time to do it up to now.

cheers,
Pablo

[-- Attachment #2: x --]
[-- Type: text/plain, Size: 7395 bytes --]

Index: extensions/libnetfilter_conntrack_sctp.c
===================================================================
--- extensions/libnetfilter_conntrack_sctp.c	(revision 4394)
+++ extensions/libnetfilter_conntrack_sctp.c	(working copy)
@@ -14,6 +14,7 @@
 #include <netinet/in.h> /* For htons */
 #include <linux/netfilter/nfnetlink_conntrack.h>
 #include <libnetfilter_conntrack/libnetfilter_conntrack.h>
+#include <libnetfilter_conntrack/libnetfilter_conntrack_extensions.h>
 
 void parse_proto(struct nfattr *cda[], struct nfct_tuple *tuple)
 {
Index: extensions/libnetfilter_conntrack_tcp.c
===================================================================
--- extensions/libnetfilter_conntrack_tcp.c	(revision 4394)
+++ extensions/libnetfilter_conntrack_tcp.c	(working copy)
@@ -14,6 +14,7 @@
 #include <netinet/in.h> /* For htons */
 #include <linux/netfilter/nfnetlink_conntrack.h>
 #include <libnetfilter_conntrack/libnetfilter_conntrack.h>
+#include <libnetfilter_conntrack/libnetfilter_conntrack_extensions.h>
 
 static const char *states[] = {
 	"NONE",
Index: extensions/libnetfilter_conntrack_icmp.c
===================================================================
--- extensions/libnetfilter_conntrack_icmp.c	(revision 4394)
+++ extensions/libnetfilter_conntrack_icmp.c	(working copy)
@@ -14,6 +14,7 @@
 #include <netinet/in.h> /* For htons */
 #include <linux/netfilter/nfnetlink_conntrack.h>
 #include <libnetfilter_conntrack/libnetfilter_conntrack.h>
+#include <libnetfilter_conntrack/libnetfilter_conntrack_extensions.h>
 
 void parse_proto(struct nfattr *cda[], struct nfct_tuple *tuple)
 {
Index: extensions/libnetfilter_conntrack_udp.c
===================================================================
--- extensions/libnetfilter_conntrack_udp.c	(revision 4394)
+++ extensions/libnetfilter_conntrack_udp.c	(working copy)
@@ -14,6 +14,7 @@
 #include <netinet/in.h> /* For htons */
 #include <linux/netfilter/nfnetlink_conntrack.h>
 #include <libnetfilter_conntrack/libnetfilter_conntrack.h>
+#include <libnetfilter_conntrack/libnetfilter_conntrack_extensions.h>
 
 void parse_proto(struct nfattr *cda[], struct nfct_tuple *tuple)
 {
Index: include/libnetfilter_conntrack/libnetfilter_conntrack_extensions.h
===================================================================
--- include/libnetfilter_conntrack/libnetfilter_conntrack_extensions.h	(revision 0)
+++ include/libnetfilter_conntrack/libnetfilter_conntrack_extensions.h	(revision 0)
@@ -0,0 +1,28 @@
+/*
+ * (C) 2005 by Pablo Neira Ayuso <pablo@eurodev.net>
+ *
+ * This software may be used and distributed according to the terms
+ * of the GNU General Public License, incorporated herein by reference.
+ */
+
+#ifndef _LIBNETFILTER_CONNTRACK_EXTENSIONS_H_
+#define _LIBNETFILTER_CONNTRACK_EXTENSIONS_H_
+
+#include "linux_list.h"
+
+struct nfct_proto {
+	struct list_head head;
+	
+	char 		*name;
+	u_int8_t 	protonum;
+	char		*version;
+	
+	void (*parse_proto)(struct nfattr **, struct nfct_tuple *);
+	void (*parse_protoinfo)(struct nfattr **, struct nfct_conntrack *);
+	int (*print_protoinfo)(char *, union nfct_protoinfo *);
+	int (*print_proto)(char *, struct nfct_tuple *);
+};
+
+extern void nfct_register_proto(struct nfct_proto *h);
+
+#endif
Index: include/libnetfilter_conntrack/libnetfilter_conntrack.h
===================================================================
--- include/libnetfilter_conntrack/libnetfilter_conntrack.h	(revision 4394)
+++ include/libnetfilter_conntrack/libnetfilter_conntrack.h	(working copy)
@@ -9,13 +9,9 @@
 #define _LIBNETFILTER_CONNTRACK_H_
 
 #include <netinet/in.h>
-#include <asm/types.h>
-#include <linux/if.h>
 #include <linux/netfilter/nfnetlink.h>
 #include <linux/netfilter/nfnetlink_conntrack.h> 
 #include <libnfnetlink/libnfnetlink.h>
-#include <linux/netfilter_ipv4/ip_conntrack.h>
-#include "linux_list.h"
 
 #define LIBNETFILTER_CONNTRACK_VERSION "0.2.0"
 
@@ -57,7 +53,7 @@
 struct nfct_tuple {
 	union {
 		u_int32_t v4;
-		u_int64_t v6;
+		u_int32_t v6[4];
 	} src;
 
 	union {
@@ -112,18 +108,6 @@
 	unsigned int id;
 };
 
-struct nfct_proto {
-	struct list_head head;
-	
-	char 		*name;
-	u_int8_t 	protonum;
-	char		*version;
-	
-	void (*parse_proto)(struct nfattr **, struct nfct_tuple *);
-	void (*parse_protoinfo)(struct nfattr **, struct nfct_conntrack *);
-	int (*print_protoinfo)(char *, union nfct_protoinfo *);
-	int (*print_proto)(char *, struct nfct_tuple *);
-};
 
 enum {
 	NFCT_STATUS_BIT = 0,
@@ -151,6 +135,58 @@
 	NFCT_ID = (1 << NFCT_ID_BIT)
 };
 
+/* Bitset representing status of connection. Taken from ip_conntrack.h
+ * 
+ * Note: For backward compatibility this shouldn't ever change
+ * 	 in kernel space.
+ */
+enum ip_conntrack_status {
+	/* It's an expected connection: bit 0 set.  This bit never changed */
+	IPS_EXPECTED_BIT = 0,
+	IPS_EXPECTED = (1 << IPS_EXPECTED_BIT),
+
+	/* We've seen packets both ways: bit 1 set.  Can be set, not unset. */
+	IPS_SEEN_REPLY_BIT = 1,
+	IPS_SEEN_REPLY = (1 << IPS_SEEN_REPLY_BIT),
+
+	/* Conntrack should never be early-expired. */
+	IPS_ASSURED_BIT = 2,
+	IPS_ASSURED = (1 << IPS_ASSURED_BIT),
+
+	/* Connection is confirmed: originating packet has left box */
+	IPS_CONFIRMED_BIT = 3,
+	IPS_CONFIRMED = (1 << IPS_CONFIRMED_BIT),
+
+	/* Connection needs src nat in orig dir.  This bit never changed. */
+	IPS_SRC_NAT_BIT = 4,
+	IPS_SRC_NAT = (1 << IPS_SRC_NAT_BIT),
+
+	/* Connection needs dst nat in orig dir.  This bit never changed. */
+	IPS_DST_NAT_BIT = 5,
+	IPS_DST_NAT = (1 << IPS_DST_NAT_BIT),
+
+	/* Both together. */
+	IPS_NAT_MASK = (IPS_DST_NAT | IPS_SRC_NAT),
+
+	/* Connection needs TCP sequence adjusted. */
+	IPS_SEQ_ADJUST_BIT = 6,
+	IPS_SEQ_ADJUST = (1 << IPS_SEQ_ADJUST_BIT),
+
+	/* NAT initialization bits. */
+	IPS_SRC_NAT_DONE_BIT = 7,
+	IPS_SRC_NAT_DONE = (1 << IPS_SRC_NAT_DONE_BIT),
+
+	IPS_DST_NAT_DONE_BIT = 8,
+	IPS_DST_NAT_DONE = (1 << IPS_DST_NAT_DONE_BIT),
+
+	/* Both together */
+	IPS_NAT_DONE_MASK = (IPS_DST_NAT_DONE | IPS_SRC_NAT_DONE),
+
+	/* Connection is dying (removed from lists), can not be unset. */
+	IPS_DYING_BIT = 9,
+	IPS_DYING = (1 << IPS_DYING_BIT),
+};
+
 enum {
 	NFCT_MSG_UNKNOWN,
 	NFCT_MSG_NEW,
@@ -163,14 +199,6 @@
 typedef int (*nfct_handler)(struct nfct_handle *cth, struct nlmsghdr *nlh,
 			    void *arg);
 
-struct nfct_handle {
-	struct nfnl_handle nfnlh;
-	nfct_callback callback;		/* user callback */
-	nfct_handler handler;		/* netlink handler */
-};
-
-extern void nfct_register_proto(struct nfct_proto *h);
-
 /*
  * [Allocate|free] a conntrack
  */
Index: src/libnetfilter_conntrack.c
===================================================================
--- src/libnetfilter_conntrack.c	(revision 4397)
+++ src/libnetfilter_conntrack.c	(working copy)
@@ -17,6 +17,7 @@
 #include "linux_list.h"
 #include <libnfnetlink/libnfnetlink.h>
 #include <libnetfilter_conntrack/libnetfilter_conntrack.h>
+#include <libnetfilter_conntrack/libnetfilter_conntrack_extensions.h>
 
 #define NFCT_BUFSIZE 4096
 
@@ -26,6 +27,13 @@
 	((unsigned char *)&addr)[2], \
 	((unsigned char *)&addr)[3]
 
+/* Harald says: "better for encapsulation" ;) */
+struct nfct_handle {
+	struct nfnl_handle nfnlh;
+	nfct_callback callback;		/* user callback */
+	nfct_handler handler;		/* netlink handler */
+};
+
 static char *lib_dir = LIBNETFILTER_CONNTRACK_DIR;
 static LIST_HEAD(proto_list);
 static char *proto2str[IPPROTO_MAX] = {

Re: remaining libnetfilter_conntrack issues

[Harald Welte]

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

Hi Pablo!

On Sat, Oct 29, 2005 at 02:45:27PM +0200, Pablo Neira wrote:
 
> Harald Welte wrote:
> >I've started to write a new ulogd2 plugin against the new
> >libnetfilter_conntrack, and found a number of issues, mostly related to
> >header files:
> >1) why do we need 'asm/types.h'?  this is unacceptable
> >2) why do we need 'linux/if.h'?
> 
> I just killed those. 

thanks.

> I'm not aware of having added those by myself, my guess is that they are 
> remains of the primitive libctnetlink.

yes, probably of the very old one.

> >3) libnetfilter_conntrack needs to be split into two parts:
> >	a) what is visible to application programs
> >	b) what is visible to extensions
> >4) if we do '3', we can get rid of the ugly "linux_list.h" include
> 
> OK, now we have libnetfilter_conntrack.h and libnetfilter_conntrack_extensions.h, have a look at 
> the patch below. I'm going to commit it asap.

looks fine, thanks.

> >5) we'd ideally get rid of the ip_conntrack.h include, too
> 
> I just did it, we just need the status bits. So I just took them from ip_conntrack.h

ok.  Ideally (even in future kernel releases), we should have this
information in a dedicated 'include file for userspace apps'.

> >6) we should ship a copy of both nfnetlink.h and nfnetlink_conntrack.h with the library
> >   this way userspace programs can be built without any
> >   nfnetlink-enabled kernel headers installed.
> >All copies should be performed during the 'distrib' stage, i.e. every
> >time we make a release we tell the automake/autoconf system where to
> >take a current copy of those headers from.  They would not be present in
> >svn.
> 
> OK, so we could just do it in autogen.sh, that would be quite straight forward. fine with it?

yes, is fine, even though having it at 'make distrib' stage would be
more intuitive.

> >7) we put the definition of the 'nfct_handle' into a private header
> >   file. this is good practise for encapsulation, since it prevents the
> >   application from making any assumptions about its size and/or
> >   contents.
> 
> Done

great.

> >8) IPv6 addresses are 128bit, not 64!
> 
> Aiee, fixed as well.

;) I hope this saves us from having to break the library/applicaiton ABI when
nf_conntrack gets nfnetlink_conntrack support.

> I think we can release the first 1.0 version of conntrack this week, together with the first 
> release of libnetfilter_conntrack. 

Yes, although I think it's fine if we still do some more review.
There's no reason to hurry with a 1.0 release as long as 2.6.14 has not
been released yet.  So I would still be happy if the 1.0 release happens
on the day of the 2.6.14 release ;)

-- 
- Harald Welte <laforge@netfilter.org>                 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

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: remaining libnetfilter_conntrack issues

[Harald Welte]

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

On Sat, Oct 29, 2005 at 04:01:19PM +0200, Harald Welte wrote:

> > I think we can release the first 1.0 version of conntrack this week, together with the first 
> > release of libnetfilter_conntrack. 
> 
> Yes, although I think it's fine if we still do some more review.
> There's no reason to hurry with a 1.0 release as long as 2.6.14 has not
> been released yet.  So I would still be happy if the 1.0 release happens
> on the day of the 2.6.14 release ;)

Oh, I just noticed now that 2.6.14 has been released.  I've been
incredibly busy with data protection, privacy and ePassport related
issues (including tv interviews) for the last couple of days, sorry ;)

-- 
- Harald Welte <laforge@netfilter.org>                 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

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]