RFC: NAT configuration over ctnetlink

RFC: NAT configuration over ctnetlink

[Patrick McHardy]

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

I added ctnetlink support to a SIP proxy (siproxd) yesterday and
stumbled over some problems with NAT. The CTA_NAT attribute only
allows to set up a single manip, since NAT mappings can't be
changed for existing conntracks there is no way to add both a
src- and a dst-manip. This patch removes the overloading of the
status bits with netlink-relevant semantic and changes the CTA_NAT
attribute to CTA_NAT_SRC and CTA_NAT_DST. It breaks compatiblity,
but I don't think its worth trying to keep it for this stupid
behaviour. Any comments?


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

[NETFILTER]: Fix ctnetlink NAT configuration

---
commit 420869deee91a3bb78701885c3fd616015e38024
tree 211d2fcc33a6f129e3eda854362d342f9d8c8109
parent 22748548e0e83899fddf877f556e5220569ed8fd
author Patrick McHardy <kaber@trash.net> Thu, 27 Apr 2006 19:22:18 +0200
committer Patrick McHardy <kaber@trash.net> Thu, 27 Apr 2006 19:22:18 +0200

 include/linux/netfilter/nfnetlink_conntrack.h |    3 +
 net/ipv4/netfilter/ip_conntrack_netlink.c     |   53 ++++++++++---------------
 2 files changed, 24 insertions(+), 32 deletions(-)

diff --git a/include/linux/netfilter/nfnetlink_conntrack.h b/include/linux/netfilter/nfnetlink_conntrack.h
index 668ec94..850526b 100644
--- a/include/linux/netfilter/nfnetlink_conntrack.h
+++ b/include/linux/netfilter/nfnetlink_conntrack.h
@@ -27,13 +27,14 @@ enum ctattr_type {
 	CTA_STATUS,
 	CTA_PROTOINFO,
 	CTA_HELP,
-	CTA_NAT,
+	CTA_NAT_SRC,
 	CTA_TIMEOUT,
 	CTA_MARK,
 	CTA_COUNTERS_ORIG,
 	CTA_COUNTERS_REPLY,
 	CTA_USE,
 	CTA_ID,
+	CTA_NAT_DST,
 	__CTA_MAX
 };
 #define CTA_MAX (__CTA_MAX - 1)
diff --git a/net/ipv4/netfilter/ip_conntrack_netlink.c b/net/ipv4/netfilter/ip_conntrack_netlink.c
index 01bd7ca..af152e3 100644
--- a/net/ipv4/netfilter/ip_conntrack_netlink.c
+++ b/net/ipv4/netfilter/ip_conntrack_netlink.c
@@ -629,7 +629,7 @@ static const size_t cta_min_nat[CTA_NAT_
 };
 
 static inline int
-ctnetlink_parse_nat(struct nfattr *cda[],
+ctnetlink_parse_nat(struct nfattr *nat,
 		    const struct ip_conntrack *ct, struct ip_nat_range *range)
 {
 	struct nfattr *tb[CTA_NAT_MAX];
@@ -639,7 +639,7 @@ ctnetlink_parse_nat(struct nfattr *cda[]
 
 	memset(range, 0, sizeof(*range));
 	
-	nfattr_parse_nested(tb, CTA_NAT_MAX, cda[CTA_NAT-1]);
+	nfattr_parse_nested(tb, CTA_NAT_MAX, nat);
 
 	if (nfattr_bad_size(tb, CTA_NAT_MAX, cta_min_nat))
 		return -EINVAL;
@@ -854,39 +854,30 @@ ctnetlink_change_status(struct ip_conntr
 		/* ASSURED bit can only be set */
 		return -EINVAL;
 
-	if (cda[CTA_NAT-1]) {
+	if (cda[CTA_NAT_SRC-1] || cda[CTA_NAT_DST-1]) {
 #ifndef CONFIG_IP_NF_NAT_NEEDED
 		return -EINVAL;
 #else
-		unsigned int hooknum;
 		struct ip_nat_range range;
 
-		if (ctnetlink_parse_nat(cda, ct, &range) < 0)
-			return -EINVAL;
-
-		DEBUGP("NAT: %u.%u.%u.%u-%u.%u.%u.%u:%u-%u\n", 
-		       NIPQUAD(range.min_ip), NIPQUAD(range.max_ip),
-		       htons(range.min.all), htons(range.max.all));
-		
-		/* This is tricky but it works. ip_nat_setup_info needs the
-		 * hook number as parameter, so let's do the correct 
-		 * conversion and run away */
-		if (status & IPS_SRC_NAT_DONE)
-			hooknum = NF_IP_POST_ROUTING; /* IP_NAT_MANIP_SRC */
-		else if (status & IPS_DST_NAT_DONE)
-			hooknum = NF_IP_PRE_ROUTING;  /* IP_NAT_MANIP_DST */
-		else 
-			return -EINVAL; /* Missing NAT flags */
-
-		DEBUGP("NAT status: %lu\n", 
-		       status & (IPS_NAT_MASK | IPS_NAT_DONE_MASK));
-		
-		if (ip_nat_initialized(ct, HOOK2MANIP(hooknum)))
-			return -EEXIST;
-		ip_nat_setup_info(ct, &range, hooknum);
-
-                DEBUGP("NAT status after setup_info: %lu\n",
-                       ct->status & (IPS_NAT_MASK | IPS_NAT_DONE_MASK));
+		if (cda[CTA_NAT_DST-1]) {
+			if (ctnetlink_parse_nat(cda[CTA_NAT_DST-1], ct,
+						&range) < 0)
+				return -EINVAL;
+			if (ip_nat_initialized(ct,
+					       HOOK2MANIP(NF_IP_PRE_ROUTING)))
+				return -EEXIST;
+			ip_nat_setup_info(ct, &range, NF_IP_PRE_ROUTING);
+		}
+		if (cda[CTA_NAT_SRC-1]) {
+			if (ctnetlink_parse_nat(cda[CTA_NAT_SRC-1], ct,
+						&range) < 0)
+				return -EINVAL;
+			if (ip_nat_initialized(ct,
+					       HOOK2MANIP(NF_IP_POST_ROUTING)))
+				return -EEXIST;
+			ip_nat_setup_info(ct, &range, NF_IP_POST_ROUTING);
+		}
 #endif
 	}
 
@@ -1106,7 +1097,7 @@ ctnetlink_new_conntrack(struct sock *ctn
 	/* implicit 'else' */
 
 	/* we only allow nat config for new conntracks */
-	if (cda[CTA_NAT-1]) {
+	if (cda[CTA_NAT_SRC-1] || cda[CTA_NAT_DST-1]) {
 		err = -EINVAL;
 		goto out_unlock;
 	}

Re: RFC: NAT configuration over ctnetlink

[Pablo Neira Ayuso]

Hi Patrick!

Patrick McHardy wrote:
> I added ctnetlink support to a SIP proxy (siproxd) yesterday and
> stumbled over some problems with NAT. The CTA_NAT attribute only
> allows to set up a single manip, since NAT mappings can't be
> changed for existing conntracks there is no way to add both a
> src- and a dst-manip. This patch removes the overloading of the
> status bits with netlink-relevant semantic and changes the CTA_NAT
> attribute to CTA_NAT_SRC and CTA_NAT_DST. It breaks compatiblity,
> but I don't think its worth trying to keep it for this stupid
> behaviour. Any comments?

Why not keep using the status bits logic? It doesn't break anything and 
you can still add support for source and destination NAT handlings at 
the same time. Is the status thing so ugly to break this?

-- 
The dawn of the fourth age of Linux firewalling is coming; a time of 
great struggle and heroic deeds -- J.Kadlecsik got inspired by J.Morris

Re: RFC: NAT configuration over ctnetlink

[Patrick McHardy]

Pablo Neira Ayuso wrote:
> Hi Patrick!
> 
> Patrick McHardy wrote:
> 
>> I added ctnetlink support to a SIP proxy (siproxd) yesterday and
>> stumbled over some problems with NAT. The CTA_NAT attribute only
>> allows to set up a single manip, since NAT mappings can't be
>> changed for existing conntracks there is no way to add both a
>> src- and a dst-manip. This patch removes the overloading of the
>> status bits with netlink-relevant semantic and changes the CTA_NAT
>> attribute to CTA_NAT_SRC and CTA_NAT_DST. It breaks compatiblity,
>> but I don't think its worth trying to keep it for this stupid
>> behaviour. Any comments?
> 
> 
> Why not keep using the status bits logic? It doesn't break anything and
> you can still add support for source and destination NAT handlings at
> the same time. Is the status thing so ugly to break this?


It is definitely ugly and broken. I'm really not happy that we added
this stuff in a hurry instead of just leaving it out and adding it
when it is in the proper shape.

I don't know. I would prefer to just remove it. Since the library
takes all parameters as arguments to a single function (one more
thing I definitely don't like), compatibility of the library will
break anyway when we add support for multiple manips. Do you really
think its worth keeping around?

Re: RFC: NAT configuration over ctnetlink

[Pablo Neira Ayuso]

Patrick McHardy wrote:
> Pablo Neira Ayuso wrote:
>>Patrick McHardy wrote:
>>
>>>I added ctnetlink support to a SIP proxy (siproxd) yesterday and
>>>stumbled over some problems with NAT. The CTA_NAT attribute only
>>>allows to set up a single manip, since NAT mappings can't be
>>>changed for existing conntracks there is no way to add both a
>>>src- and a dst-manip. This patch removes the overloading of the
>>>status bits with netlink-relevant semantic and changes the CTA_NAT
>>>attribute to CTA_NAT_SRC and CTA_NAT_DST. It breaks compatiblity,
>>>but I don't think its worth trying to keep it for this stupid
>>>behaviour. Any comments?
>>
>>Why not keep using the status bits logic? It doesn't break anything and
>>you can still add support for source and destination NAT handlings at
>>the same time. Is the status thing so ugly to break this?
> 
> It is definitely ugly and broken. I'm really not happy that we added
> this stuff in a hurry instead of just leaving it out and adding it
> when it is in the proper shape.
> 
> I don't know. I would prefer to just remove it. Since the library
> takes all parameters as arguments to a single function (one more
> thing I definitely don't like), compatibility of the library will
> break anyway when we add support for multiple manips. Do you really
> think its worth keeping around?

Not really, if it's trash better remove this sooner than later. Now that 
we are going to break backward compatibility, please let me know if you 
don't like anything else that we could change at this point.

Re: RFC: NAT configuration over ctnetlink

[Patrick McHardy]

Pablo Neira Ayuso wrote:
>> I don't know. I would prefer to just remove it. Since the library
>> takes all parameters as arguments to a single function (one more
>> thing I definitely don't like), compatibility of the library will
>> break anyway when we add support for multiple manips. Do you really
>> think its worth keeping around?
> 
> 
> Not really, if it's trash better remove this sooner than later. Now that
> we are going to break backward compatibility, please let me know if you
> don't like anything else that we could change at this point.

One thing that would make a lot of sense is to change or introduce a new
interface in libnetfilter_conntrack that allows to add netfilter
attributes to a conntrack "object" one at a time, so we don't have to
change function prototypes each time we're introducing something new.

Re: RFC: NAT configuration over ctnetlink

[Pablo Neira Ayuso]

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

Patrick McHardy wrote:
> Pablo Neira Ayuso wrote:
> 
>>>I don't know. I would prefer to just remove it. Since the library
>>>takes all parameters as arguments to a single function (one more
>>>thing I definitely don't like), compatibility of the library will
>>>break anyway when we add support for multiple manips. Do you really
>>>think its worth keeping around?
>>
>>Not really, if it's trash better remove this sooner than later. Now that
>>we are going to break backward compatibility, please let me know if you
>>don't like anything else that we could change at this point.
> 
> One thing that would make a lot of sense is to change or introduce a new
> interface in libnetfilter_conntrack that allows to add netfilter
> attributes to a conntrack "object" one at a time, so we don't have to
> change function prototypes each time we're introducing something new.

what do you think about the incomplete patch attached? Still missing the
getters and the expectations.

I think that with the setters/getters we can make private nfct_conntrack
and friends.

-- 
The dawn of the fourth age of Linux firewalling is coming; a time of
great struggle and heroic deeds -- J.Kadlecsik got inspired by J.Morris

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

Index: include/libnetfilter_conntrack/linux_nfnetlink_conntrack.h
===================================================================
--- include/libnetfilter_conntrack/linux_nfnetlink_conntrack.h	(revisión: 6554)
+++ include/libnetfilter_conntrack/linux_nfnetlink_conntrack.h	(copia de trabajo)
@@ -27,13 +27,14 @@
 	CTA_STATUS,
 	CTA_PROTOINFO,
 	CTA_HELP,
-	CTA_NAT,
+	CTA_NAT_SRC,
 	CTA_TIMEOUT,
 	CTA_MARK,
 	CTA_COUNTERS_ORIG,
 	CTA_COUNTERS_REPLY,
 	CTA_USE,
 	CTA_ID,
+	CTA_NAT_DST,
 	__CTA_MAX
 };
 #define CTA_MAX (__CTA_MAX - 1)
Index: include/libnetfilter_conntrack/libnetfilter_conntrack.h
===================================================================
--- include/libnetfilter_conntrack/libnetfilter_conntrack.h	(revisión: 6554)
+++ include/libnetfilter_conntrack/libnetfilter_conntrack.h	(copia de trabajo)
@@ -96,7 +96,8 @@
 
 	union nfct_protoinfo protoinfo;
 	struct nfct_counters counters[NFCT_DIR_MAX];
-	struct nfct_nat nat;
+	struct nfct_nat src_nat;
+	struct nfct_nat dst_nat;
 };
 
 struct nfct_expect {
@@ -193,7 +194,33 @@
 	IPS_DYING = (1 << IPS_DYING_BIT),
 };
 
+/* Attributes of a conntrack "object" */
 enum {
+	NFCT_ATTR_IPV4_SRC = 0,
+	NFCT_ATTR_IPV4_DST,
+	NFCT_ATTR_IPV6_SRC,
+	NFCT_ATTR_IPV6_DST,
+	NFCT_ATTR_L3PROTONUM,
+	NFCT_ATTR_PROTONUM,
+	NFCT_ATTR_PORT_SRC,
+	NFCT_ATTR_PORT_DST,
+	NFCT_ATTR_ICMP_TYPE,
+	NFCT_ATTR_ICMP_CODE,
+	NFCT_ATTR_ICMP_ID,
+	NFCT_ATTR_STATUS,
+	NFCT_ATTR_MARK,
+	NFCT_ATTR_ID,
+	NFCT_ATTR_TCP_STATE,
+	NFCT_ATTR_SRC_NAT,
+	NFCT_ATTR_DST_NAT,
+	NFCT_ATTR_SRC_MINIP_NAT,
+	NFCT_ATTR_SRC_MAXIP_NAT,
+	NFCT_ATTR_DST_MINIP_NAT,
+	NFCT_ATTR_DST_MAXIP_NAT,
+	NFCT_ATTR_MAX,
+};
+
+enum {
 	NFCT_MSG_UNKNOWN,
 	NFCT_MSG_NEW,
 	NFCT_MSG_UPDATE,
@@ -204,16 +231,19 @@
 typedef int (*nfct_callback)(void *arg, unsigned int flags, int, void *data);
 
 /*
- * [Allocate|free] a conntrack
+ * [Allocate|free] a conntrack "object"
  */
-extern struct nfct_conntrack *
-nfct_conntrack_alloc(struct nfct_tuple *orig, struct nfct_tuple *reply,
-		     u_int32_t timeout, union nfct_protoinfo *proto,
-		     u_int32_t status, u_int32_t mark,
-		     u_int32_t id, struct nfct_nat *range);
+extern struct nfct_conntrack *nfct_conntrack_alloc(struct nfct_tuple *orig, 
+						   struct nfct_tuple *r);
 extern void nfct_conntrack_free(struct nfct_conntrack *ct);
 
 /*
+ * Setter functions
+ */
+int nfct_tuple_set(struct nfct_tuple *, void *, unsigned int, int);
+int nfct_conntrack_set(struct nfct_conntrack *, void *, unsigned int, int);
+
+/*
  * [Allocate|free] an expectation
  */
 extern struct nfct_expect *
Index: src/libnetfilter_conntrack.c
===================================================================
--- src/libnetfilter_conntrack.c	(revisión: 6554)
+++ src/libnetfilter_conntrack.c	(copia de trabajo)
@@ -263,21 +263,37 @@
 }
 
 static void nfct_build_nat(struct nfnlhdr *req, int size,
-			   struct nfct_conntrack *ct)
+			   struct nfct_conntrack *ct, int attr)
 {
 	struct nfattr *nest;
 
-	nest = nfnl_nest(&req->nlh, size, CTA_NAT);
+	nest = nfnl_nest(&req->nlh, size, attr);
 
-	nfnl_addattr_l(&req->nlh, size, CTA_NAT_MINIP,
-		       &ct->nat.min_ip, sizeof(u_int32_t));
+	switch(attr) {
+	case CTA_NAT_SRC:
+		nfnl_addattr_l(&req->nlh, size, CTA_NAT_MINIP,
+			       &ct->src_nat.min_ip, sizeof(u_int32_t));
+
+		if (ct->src_nat.min_ip != ct->src_nat.max_ip)
+			nfnl_addattr_l(&req->nlh, size, CTA_NAT_MAXIP,
+				       &ct->src_nat.max_ip, sizeof(u_int32_t));
+		break;
+	case CTA_NAT_DST:
+		nfnl_addattr_l(&req->nlh, size, CTA_NAT_MINIP,
+			       &ct->dst_nat.max_ip, sizeof(u_int32_t));
+		if (ct->dst_nat.min_ip != ct->dst_nat.max_ip)
+			nfnl_addattr_l(&req->nlh, size, CTA_NAT_MAXIP,
+				       &ct->dst_nat.max_ip, sizeof(u_int32_t));
+		break;
+	default:
+		fprintf(stderr, "Unknown attribute to set up NAT\n");
+		return;
+	}
 	
-	if (ct->nat.min_ip != ct->nat.max_ip)
-		nfnl_addattr_l(&req->nlh, size, CTA_NAT_MAXIP,
-			       &ct->nat.max_ip, sizeof(u_int32_t));
-
+#if 0
 	if (ct->nat.l4min.all != ct->nat.l4max.all)
 		nfct_build_protonat(req, size, ct);
+#endif
 
 	nfnl_nest_end(&req->nlh, nest);
 }
@@ -865,11 +881,137 @@
 	return 0;
 }
 
+static size_t attrsize[NFCT_ATTR_MAX] = {
+	[NFCT_ATTR_IPV4_SRC]		=	sizeof(u_int32_t),
+	[NFCT_ATTR_IPV4_DST]		=	sizeof(u_int32_t),
+	[NFCT_ATTR_IPV6_SRC]		=	sizeof(u_int32_t)*4,
+	[NFCT_ATTR_IPV6_DST]		= 	sizeof(u_int32_t)*4,
+	[NFCT_ATTR_L3PROTONUM]		=	sizeof(u_int8_t),
+	[NFCT_ATTR_PROTONUM]		=	sizeof(u_int8_t),
+	[NFCT_ATTR_PORT_SRC]		=	sizeof(u_int16_t),
+	[NFCT_ATTR_PORT_DST]		= 	sizeof(u_int16_t),
+	[NFCT_ATTR_ICMP_TYPE]		=	sizeof(u_int8_t),
+	[NFCT_ATTR_ICMP_CODE]		= 	sizeof(u_int8_t),
+	[NFCT_ATTR_ICMP_ID]		=	sizeof(u_int16_t),
+	[NFCT_ATTR_STATUS]		=	sizeof(u_int32_t),
+	[NFCT_ATTR_MARK]		=	sizeof(u_int32_t),
+	[NFCT_ATTR_ID]			=	sizeof(u_int32_t),
+	[NFCT_ATTR_TCP_STATE]		=	sizeof(u_int8_t),
+	[NFCT_ATTR_SRC_NAT]		=	sizeof(u_int32_t),
+	[NFCT_ATTR_DST_NAT]		=	sizeof(u_int32_t),
+	[NFCT_ATTR_SRC_MINIP_NAT]	=	sizeof(u_int32_t),
+	[NFCT_ATTR_SRC_MAXIP_NAT]	=	sizeof(u_int32_t),
+	[NFCT_ATTR_DST_MINIP_NAT]	=	sizeof(u_int32_t),
+	[NFCT_ATTR_DST_MAXIP_NAT]	=	sizeof(u_int32_t),
+};
+
+int nfct_tuple_set(struct nfct_tuple *tuple, void *data, 
+		   unsigned int datasize, int attr)
+{
+	union {
+		u_int32_t unsig32;
+		u_int16_t unsig16;
+		u_int8_t unsig8;
+	} *attrdata = data;
+
+	if (datasize != attrsize[attr])
+		return -1;
+
+	switch(attr) {
+	case NFCT_ATTR_IPV4_SRC:
+		tuple->src.v4 = attrdata->unsig32;
+		break;
+	case NFCT_ATTR_IPV4_DST:
+		tuple->dst.v4 = attrdata->unsig32;
+		break;
+	case NFCT_ATTR_IPV6_SRC:
+		memcpy(tuple->src.v6, &attrdata->unsig32, sizeof(u_int32_t)*4);
+		break;
+	case NFCT_ATTR_IPV6_DST:
+		memcpy(tuple->dst.v6, &attrdata->unsig32, sizeof(u_int32_t)*4);
+		break;
+	case NFCT_ATTR_L3PROTONUM:
+		tuple->l3protonum = attrdata->unsig8;
+		break;
+	case NFCT_ATTR_PROTONUM:
+		tuple->protonum = attrdata->unsig8;
+		break;
+	case NFCT_ATTR_PORT_SRC:
+		tuple->l4src.all = attrdata->unsig16;
+		break;
+	case NFCT_ATTR_PORT_DST:
+		tuple->l4dst.all = attrdata->unsig16;
+		break;
+	case NFCT_ATTR_ICMP_TYPE:
+		tuple->l4dst.icmp.type = attrdata->unsig8;
+		break;
+	case NFCT_ATTR_ICMP_CODE:
+		tuple->l4dst.icmp.code = attrdata->unsig8;
+		break;
+	case NFCT_ATTR_ICMP_ID:
+		tuple->l4src.icmp.id = attrdata->unsig16;
+		break;
+	default:
+		fprintf(stderr, "invalid attribute %d\n", attr);
+		return -1;
+	}
+
+	return 0;
+}
+
+int nfct_conntrack_set(struct nfct_conntrack *ct, void *data,
+		       unsigned int datasize, int attr)
+{
+	union {
+		u_int32_t unsig32;
+		u_int16_t unsig16;
+		u_int8_t unsig8;
+	} *attrdata = data;
+
+	if (datasize != attrsize[attr])
+		return -1;
+
+	switch(attr) {
+	case NFCT_ATTR_STATUS:
+		ct->status = attrdata->unsig32;
+		break;
+	case NFCT_ATTR_MARK:
+		ct->mark = attrdata->unsig32;
+		break;
+	case NFCT_ATTR_ID:
+		ct->id = attrdata->unsig32;
+		break;
+	case NFCT_ATTR_TCP_STATE:
+		ct->protoinfo.tcp.state = attrdata->unsig8;
+	case NFCT_ATTR_SRC_NAT:
+		ct->src_nat.min_ip = attrdata->unsig32;
+		ct->src_nat.max_ip = attrdata->unsig32;
+		break;
+	case NFCT_ATTR_DST_NAT:
+		ct->dst_nat.min_ip = attrdata->unsig32;
+		ct->dst_nat.max_ip = attrdata->unsig32;
+		break;
+	case NFCT_ATTR_SRC_MINIP_NAT:
+		ct->src_nat.min_ip = attrdata->unsig32;
+		break;
+	case NFCT_ATTR_SRC_MAXIP_NAT:
+		ct->src_nat.max_ip = attrdata->unsig32;
+		break;
+	case NFCT_ATTR_DST_MINIP_NAT:
+		ct->dst_nat.min_ip = attrdata->unsig32;
+		break;
+	case NFCT_ATTR_DST_MAXIP_NAT:
+		ct->dst_nat.max_ip = attrdata->unsig32;
+		break;
+	default:
+		fprintf(stderr, "invalid attribute %d", attr);
+		return -1;
+	}
+	return 0;
+}
+
 struct nfct_conntrack *
-nfct_conntrack_alloc(struct nfct_tuple *orig, struct nfct_tuple *reply,
-		     u_int32_t timeout, union nfct_protoinfo *proto,
-		     u_int32_t status, u_int32_t mark, 
-		     u_int32_t id, struct nfct_nat *range)
+nfct_conntrack_alloc(struct nfct_tuple *orig, struct nfct_tuple *reply)
 {
 	struct nfct_conntrack *ct;
 
@@ -880,14 +1022,6 @@
 
 	ct->tuple[NFCT_DIR_ORIGINAL] = *orig;
 	ct->tuple[NFCT_DIR_REPLY] = *reply;
-	ct->timeout = timeout;
-	ct->status = status;
-	ct->protoinfo = *proto;
-	ct->mark = mark;
-	if (id != NFCT_ANY_ID)
-		ct->id = id;
-	if (range)
-		ct->nat = *range;
 
 	return ct;
 }
@@ -981,8 +1115,10 @@
 			       sizeof(u_int32_t));
 
 	nfct_build_protoinfo(req, sizeof(buf), ct);
-	if (ct->nat.min_ip != 0)
-		nfct_build_nat(req, sizeof(buf), ct);
+	if (ct->src_nat.min_ip != 0)
+		nfct_build_nat(req, sizeof(buf), ct, CTA_NAT_SRC);
+	if (ct->dst_nat.min_ip != 0)
+		nfct_build_nat(req, sizeof(buf), ct, CTA_NAT_DST);
 
 	return nfnl_talk(cth->nfnlh, &req->nlh, 0, 0, NULL, NULL, NULL);
 }

Re: RFC: NAT configuration over ctnetlink

[Patrick McHardy]

Pablo Neira Ayuso wrote:
> Patrick McHardy wrote:
> 
>>Pablo Neira Ayuso wrote:
>>
>>
>>>>I don't know. I would prefer to just remove it. Since the library
>>>>takes all parameters as arguments to a single function (one more
>>>>thing I definitely don't like), compatibility of the library will
>>>>break anyway when we add support for multiple manips. Do you really
>>>>think its worth keeping around?
>>>
>>>Not really, if it's trash better remove this sooner than later. Now that
>>>we are going to break backward compatibility, please let me know if you
>>>don't like anything else that we could change at this point.
>>
>>One thing that would make a lot of sense is to change or introduce a new
>>interface in libnetfilter_conntrack that allows to add netfilter
>>attributes to a conntrack "object" one at a time, so we don't have to
>>change function prototypes each time we're introducing something new.
> 
> 
> what do you think about the incomplete patch attached? Still missing the
> getters and the expectations.
> 
> I think that with the setters/getters we can make private nfct_conntrack
> and friends.

This goes even further than what I meant :) I'll have a closer look,
but probably going to take a bit, I'm quite busy currently.

Another item I would like to bring up for discussion is whether we
want to continue with (some) of these libraries at all. For a small
pet-project of mine I've been working a bit on Thomas Graf's libnl,
which provides a very nice netlink infrastructure, support for
basically all kernel netlink interfaces, nice object representation
of the individual items, cache management for replication of kernel
databases, and quite a few things more. Besides lots of nice
infrastructure, one main advantage of using libnl would be that
there is a single library which can be used for communication with
any kernel subsystem using netlink in a uniform way.

Comments?

Re: RFC: NAT configuration over ctnetlink

[Harald Welte]

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

On Wed, May 03, 2006 at 03:40:11PM +0200, Patrick McHardy wrote:

> Another item I would like to bring up for discussion is whether we
> want to continue with (some) of these libraries at all. For a small
> pet-project of mine I've been working a bit on Thomas Graf's libnl,
> which provides a very nice netlink infrastructure, support for
> basically all kernel netlink interfaces, nice object representation
> of the individual items, cache management for replication of kernel
> databases, and quite a few things more. Besides lots of nice
> infrastructure, one main advantage of using libnl would be that
> there is a single library which can be used for communication with
> any kernel subsystem using netlink in a uniform way.

well, certainly that work is interesting.  However, I don't really see
how this fits into the picture.  First of all, nfnetlink is different in
that it has its peculiar byteorder.  Also, we specifically divided the
libraries into separate packages so they can be updated independently.
It is unlikely that conntrack bugfixes affect logging and vice versa.

-- 
- 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: RFC: NAT configuration over ctnetlink

[Patrick McHardy]

Harald Welte wrote:
> On Wed, May 03, 2006 at 03:40:11PM +0200, Patrick McHardy wrote:
> 
> 
>>Another item I would like to bring up for discussion is whether we
>>want to continue with (some) of these libraries at all. For a small
>>pet-project of mine I've been working a bit on Thomas Graf's libnl,
>>which provides a very nice netlink infrastructure, support for
>>basically all kernel netlink interfaces, nice object representation
>>of the individual items, cache management for replication of kernel
>>databases, and quite a few things more. Besides lots of nice
>>infrastructure, one main advantage of using libnl would be that
>>there is a single library which can be used for communication with
>>any kernel subsystem using netlink in a uniform way.
> 
> 
> well, certainly that work is interesting.  However, I don't really see
> how this fits into the picture.  First of all, nfnetlink is different in
> that it has its peculiar byteorder.


Unfortunately that is true, but it shouldn't be that hard to adapt
libnl. But you're certainly right that this needs to be looked into
before reasonably continuing this discussion.


  Also, we specifically divided the
> libraries into separate packages so they can be updated independently.
> It is unlikely that conntrack bugfixes affect logging and vice versa.


I don't see that as a problem, what does it matter whether I need
to update one library dealing with logging or another library dealing
with everything .. given that it is a pure bugfix update. Using that
library would IMO not only give us lots of nice features (at least
parts of which we would need to duplicate otherwise), but also benefit
users by providing uniform access to the kernel.

Re: RFC: NAT configuration over ctnetlink

[Patrick McHardy]

Pablo Neira Ayuso wrote:
> Patrick McHardy wrote:
> 
>>Pablo Neira Ayuso wrote:
>>
>>
>>>>I don't know. I would prefer to just remove it. Since the library
>>>>takes all parameters as arguments to a single function (one more
>>>>thing I definitely don't like), compatibility of the library will
>>>>break anyway when we add support for multiple manips. Do you really
>>>>think its worth keeping around?
>>>
>>>Not really, if it's trash better remove this sooner than later. Now that
>>>we are going to break backward compatibility, please let me know if you
>>>don't like anything else that we could change at this point.
>>
>>One thing that would make a lot of sense is to change or introduce a new
>>interface in libnetfilter_conntrack that allows to add netfilter
>>attributes to a conntrack "object" one at a time, so we don't have to
>>change function prototypes each time we're introducing something new.
> 
> 
> what do you think about the incomplete patch attached? Still missing the
> getters and the expectations.
> 
> I think that with the setters/getters we can make private nfct_conntrack
> and friends.


I talked to Harald about this and we decided how to proceed. Harald
made some changes to libnfnetlink which break libnetfilter_conntrack
and others, so he wants to fix them and do a new release of all the
libraries first. After that your patch should go in, but we decided
not to change the existing create_conntrack prototype, but add a few
new functions instead for allocating and sending the conntrack message.
This way old code will at least compile, and everything besides
setting up NAT mappings will keep working.

Re: RFC: NAT configuration over ctnetlink

[Pablo Neira Ayuso]

Hi,

Patrick McHardy wrote:
> Pablo Neira Ayuso wrote:
> 
>>Patrick McHardy wrote:
>>
>>>Pablo Neira Ayuso wrote:
>>>
>>>>>I don't know. I would prefer to just remove it. Since the library
>>>>>takes all parameters as arguments to a single function (one more
>>>>>thing I definitely don't like), compatibility of the library will
>>>>>break anyway when we add support for multiple manips. Do you really
>>>>>think its worth keeping around?
>>>>
>>>>Not really, if it's trash better remove this sooner than later. Now that
>>>>we are going to break backward compatibility, please let me know if you
>>>>don't like anything else that we could change at this point.
>>>
>>>One thing that would make a lot of sense is to change or introduce a new
>>>interface in libnetfilter_conntrack that allows to add netfilter
>>>attributes to a conntrack "object" one at a time, so we don't have to
>>>change function prototypes each time we're introducing something new.
>>
>>
>>what do you think about the incomplete patch attached? Still missing the
>>getters and the expectations.
>>
>>I think that with the setters/getters we can make private nfct_conntrack
>>and friends.
> 
> 
> I talked to Harald about this and we decided how to proceed. Harald
> made some changes to libnfnetlink which break libnetfilter_conntrack
> and others, so he wants to fix them and do a new release of all the
> libraries first.

BTW: @Harald, do you plan to include part of this patch in the changes?
http://patchwork.netfilter.org/netfilter-devel/patch.pl?id=3315

> After that your patch should go in, but we decided
> not to change the existing create_conntrack prototype, but add a few
> new functions instead for allocating and sending the conntrack message.
> This way old code will at least compile, and everything besides
> setting up NAT mappings will keep working.

OK, I'll finish the patch then. I'm thinking about hiding the definition
of nfct_conntrack/nfct_tuple/... and provide the getters to access the
attributes of the object. Since this structure could evolve in time, we
wouldn't have any problems to introduce new changes in future. But that
would definitely break code that initialize the structure by setting up
the structure fields. Ideas?

-- 
The dawn of the fourth age of Linux firewalling is coming; a time of
great struggle and heroic deeds -- J.Kadlecsik got inspired by J.Morris

Re: RFC: NAT configuration over ctnetlink

[Patrick McHardy]

Pablo Neira Ayuso wrote:
> OK, I'll finish the patch then. I'm thinking about hiding the definition
> of nfct_conntrack/nfct_tuple/... and provide the getters to access the
> attributes of the object. Since this structure could evolve in time, we
> wouldn't have any problems to introduce new changes in future. But that
> would definitely break code that initialize the structure by setting up
> the structure fields. Ideas?

I don't think thats necessary (and I'm no fan of hiding things unless
there's a good reason), basically all of these structures are similar
to semantically well-defined structures in the kernel that are very
unlikely to change in an incompatible way.