[RFC][PATCH] kill the fake conntrack

[RFC][PATCH] kill the fake conntrack

[Pablo Neira]

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

Hi,

Well, the subject looks like good action film but it isn't :)

The patch attached kills the fake conntrack and propose a new logic to 
explicitely set connection as untracked. We set nfct to NULL and use a 
new flag called IP_CT_UNTRACKED for nfctinfo. I've slightely tested it 
here and works fine.

Comments welcome.

--
Pablo

[-- Attachment #2: new-untracked.patch --]
[-- Type: text/x-patch, Size: 6438 bytes --]

Index: linux-2.6/include/linux/netfilter_ipv4/ip_conntrack.h
===================================================================
--- linux-2.6.orig/include/linux/netfilter_ipv4/ip_conntrack.h	2005-06-24 23:47:19.000000000 +0200
+++ linux-2.6/include/linux/netfilter_ipv4/ip_conntrack.h	2005-06-25 13:17:21.000000000 +0200
@@ -20,7 +20,10 @@
 	IP_CT_IS_REPLY,
 
 	/* Number of distinct IP_CT types (no NEW in reply dirn). */
-	IP_CT_NUMBER = IP_CT_IS_REPLY * 2 - 1
+	IP_CT_NUMBER = IP_CT_IS_REPLY * 2 - 1,
+
+	/* This packet belongs to a untracked connection */
+	IP_CT_UNTRACKED = ~0U
 };
 
 /* Bitset representing status of connection. */
Index: linux-2.6/net/ipv4/netfilter/ip_nat_core.c
===================================================================
--- linux-2.6.orig/net/ipv4/netfilter/ip_nat_core.c	2005-06-24 23:47:19.000000000 +0200
+++ linux-2.6/net/ipv4/netfilter/ip_nat_core.c	2005-06-25 13:24:25.000000000 +0200
@@ -525,8 +525,6 @@
 	IP_NF_ASSERT(ip_conntrack_destroyed == NULL);
 	ip_conntrack_destroyed = &ip_nat_cleanup_conntrack;
 
-	/* Initialize fake conntrack so that NAT will skip it */
-	ip_conntrack_untracked.status |= IPS_NAT_DONE_MASK;
 	return 0;
 }
 
Index: linux-2.6/net/ipv4/netfilter/ip_conntrack_standalone.c
===================================================================
--- linux-2.6.orig/net/ipv4/netfilter/ip_conntrack_standalone.c	2005-06-24 23:47:19.000000000 +0200
+++ linux-2.6/net/ipv4/netfilter/ip_conntrack_standalone.c	2005-06-25 13:23:53.000000000 +0200
@@ -986,7 +986,6 @@
 EXPORT_SYMBOL(ip_conntrack_htable_size);
 EXPORT_SYMBOL(ip_conntrack_lock);
 EXPORT_SYMBOL(ip_conntrack_hash);
-EXPORT_SYMBOL(ip_conntrack_untracked);
 EXPORT_SYMBOL_GPL(ip_conntrack_find_get);
 EXPORT_SYMBOL_GPL(ip_conntrack_put);
 #ifdef CONFIG_IP_NF_NAT_NEEDED
Index: linux-2.6/net/ipv4/netfilter/ipt_conntrack.c
===================================================================
--- linux-2.6.orig/net/ipv4/netfilter/ipt_conntrack.c	2005-06-24 23:47:19.000000000 +0200
+++ linux-2.6/net/ipv4/netfilter/ipt_conntrack.c	2005-06-25 13:23:28.000000000 +0200
@@ -35,7 +35,7 @@
 
 #define FWINV(bool,invflg) ((bool) ^ !!(sinfo->invflags & invflg))
 
-	if (ct == &ip_conntrack_untracked)
+	if (ctinfo == IP_CT_UNTRACKED)
 		statebit = IPT_CONNTRACK_STATE_UNTRACKED;
 	else if (ct)
  		statebit = IPT_CONNTRACK_STATE_BIT(ctinfo);
Index: linux-2.6/net/ipv4/netfilter/ipt_state.c
===================================================================
--- linux-2.6.orig/net/ipv4/netfilter/ipt_state.c	2005-06-24 23:47:19.000000000 +0200
+++ linux-2.6/net/ipv4/netfilter/ipt_state.c	2005-06-25 13:22:58.000000000 +0200
@@ -30,7 +30,7 @@
 	enum ip_conntrack_info ctinfo;
 	unsigned int statebit;
 
-	if (skb->nfct == &ip_conntrack_untracked.ct_general)
+	if (skb->nfctinfo == IP_CT_UNTRACKED)
 		statebit = IPT_STATE_UNTRACKED;
 	else if (!ip_conntrack_get(skb, &ctinfo))
 		statebit = IPT_STATE_INVALID;
Index: linux-2.6/net/ipv4/netfilter/ip_conntrack_core.c
===================================================================
--- linux-2.6.orig/net/ipv4/netfilter/ip_conntrack_core.c	2005-06-24 23:47:19.000000000 +0200
+++ linux-2.6/net/ipv4/netfilter/ip_conntrack_core.c	2005-06-25 13:59:28.000000000 +0200
@@ -71,7 +71,6 @@
 struct list_head *ip_conntrack_hash;
 static kmem_cache_t *ip_conntrack_cachep;
 static kmem_cache_t *ip_conntrack_expect_cachep;
-struct ip_conntrack ip_conntrack_untracked;
 unsigned int ip_ct_log_invalid;
 static LIST_HEAD(unconfirmed);
 static int ip_conntrack_vmalloc;
@@ -613,7 +612,7 @@
 	int ret;
 
 	/* Previously seen (loopback or untracked)?  Ignore. */
-	if ((*pskb)->nfct) {
+	if ((*pskb)->nfct || (*pskb)->nfctinfo == IP_CT_UNTRACKED) {
 		CONNTRACK_STAT_INC(ignore);
 		return NF_ACCEPT;
 	}
@@ -1203,12 +1202,6 @@
 	/* For use by ipt_REJECT */
 	ip_ct_attach = ip_conntrack_attach;
 
-	/* Set up fake conntrack:
-	    - to never be deleted, not in any hashes */
-	atomic_set(&ip_conntrack_untracked.ct_general.use, 1);
-	/*  - and look it like as a confirmed connection */
-	set_bit(IPS_CONFIRMED_BIT, &ip_conntrack_untracked.status);
-
 	return ret;
 
 err_free_conntrack_slab:
Index: linux-2.6/net/ipv4/netfilter/ip_nat_standalone.c
===================================================================
--- linux-2.6.orig/net/ipv4/netfilter/ip_nat_standalone.c	2005-06-25 14:04:47.000000000 +0200
+++ linux-2.6/net/ipv4/netfilter/ip_nat_standalone.c	2005-06-25 14:25:41.000000000 +0200
@@ -81,6 +81,14 @@
 			return NF_DROP;
 
 	ct = ip_conntrack_get(*pskb, &ctinfo);
+	/* This is freak. The user is mixing non-tracked stuff together
+	 * with NAT, this is not possible since we lack of the necessary
+	 * information to perform any handling. Since we don't want to 
+	 * break stupid settings <SIGH>, just let this packet continue 
+	 * its travel through the network stack */
+	if (ctinfo == IP_CT_UNTRACKED)
+		return NF_ACCEPT;
+
 	/* Can't track?  It's not due to stress, or conntrack would
 	   have dropped it.  Hence it's the user's responsibilty to
 	   packet filter it out, or implement conntrack/NAT for that
Index: linux-2.6/net/ipv4/netfilter/ipt_NOTRACK.c
===================================================================
--- linux-2.6.orig/net/ipv4/netfilter/ipt_NOTRACK.c	2005-06-25 13:49:52.000000000 +0200
+++ linux-2.6/net/ipv4/netfilter/ipt_NOTRACK.c	2005-06-25 14:01:32.000000000 +0200
@@ -1,5 +1,8 @@
 /* This is a module which is used for setting up fake conntracks
  * on packets so that they are not seen by the conntrack/NAT code.
+ *
+ * 05/06/25: Added new untracked logic. 
+ * 	     Pablo Neira Ayuso <pablo at eurodev dot net>
  */
 #include <linux/module.h>
 #include <linux/skbuff.h>
@@ -16,16 +19,18 @@
        void *userinfo)
 {
 	/* Previously seen (loopback)? Ignore. */
-	if ((*pskb)->nfct != NULL)
+	if ((*pskb)->nfct != NULL || (*pskb)->nfctinfo == IP_CT_UNTRACKED)
 		return IPT_CONTINUE;
 
 	/* Attach fake conntrack entry. 
 	   If there is a real ct entry correspondig to this packet, 
 	   it'll hang aroun till timing out. We don't deal with it
 	   for performance reasons. JK */
-	(*pskb)->nfct = &ip_conntrack_untracked.ct_general;
-	(*pskb)->nfctinfo = IP_CT_NEW;
-	nf_conntrack_get((*pskb)->nfct);
+	   
+	/* This packet hasn't got any conntrack associated. To 
+	   explicitely mark it as untracked, set the nfctinfo flag. */
+	(*pskb)->nfct = NULL;
+	(*pskb)->nfctinfo = IP_CT_UNTRACKED;
 
 	return IPT_CONTINUE;
 }

Re: [RFC][PATCH] kill the fake conntrack

[Patrick McHardy]

Pablo Neira wrote:
> The patch attached kills the fake conntrack and propose a new logic to
> explicitely set connection as untracked. We set nfct to NULL and use a
> new flag called IP_CT_UNTRACKED for nfctinfo. I've slightely tested it
> here and works fine.
> 
> Comments welcome.

What is the advantage of this patch? Its changing the assumption
that skb->nfctinfo is only valid if skb->nfct is set, so you probably
need to set nfctinfo to 0 in nf_reset as well. This alone is a reason
not to do it IMO.

Regards
Patrick

Re: [RFC][PATCH] kill the fake conntrack

[Pablo Neira]

Patrick McHardy wrote:
> Pablo Neira wrote:
> 
>>The patch attached kills the fake conntrack and propose a new logic to
>>explicitely set connection as untracked. We set nfct to NULL and use a
>>new flag called IP_CT_UNTRACKED for nfctinfo. I've slightely tested it
>>here and works fine.
>>
>>Comments welcome.
> 
> What is the advantage of this patch?

Whenever we work with conntracks, we always have to check if it's the 
fake or not previously. For example, a bad use of NOTRACK together with 
other targets, say ipt_CONNMARK, that doesn't currently check if it's 
working with the fake conntrack or not. Same thing for NAT handlings, I 
see that this can result in weirdness.

Setting nfct to NULL will make NAT targets, conntrack helpers and 
friends just ignore untracked packets, so we could forget about making 
sure that we aren't messing with the fake conntrack.

> Its changing the assumption
> that skb->nfctinfo is only valid if skb->nfct is set, so you probably
> need to set nfctinfo to 0 in nf_reset as well. This alone is a reason
> not to do it IMO.

why need to set nfctinfo to 0 in nf_reset? since skb->nfct is NULL 
nf_reset shouldn't be ever called. With the logic I'm proposing, this 
packet will be handle just like invalid ones, so it will be just ignored.

--
Pablo

Re: [RFC][PATCH] kill the fake conntrack

[Pablo Neira]

Pablo Neira wrote:
> why need to set nfctinfo to 0 in nf_reset? since skb->nfct is NULL 
> nf_reset shouldn't be ever called. 

Well, this isn't true at all. nf_reset can be called even if skb->nfct 
is NULL.

> With the logic I'm proposing, this 
> packet will be handle just like invalid ones, so it will be just ignored.

But I still support this this above, making untracked conntracks look 
ignore can simplify the logic AFAICS.

--
Pablo

Re: [RFC][PATCH] kill the fake conntrack

[Patrick McHardy]

Pablo Neira wrote:
> Patrick McHardy wrote:
> 
>> What is the advantage of this patch?
> 
> Whenever we work with conntracks, we always have to check if it's the
> fake or not previously. For example, a bad use of NOTRACK together with
> other targets, say ipt_CONNMARK, that doesn't currently check if it's
> working with the fake conntrack or not. Same thing for NAT handlings, I
> see that this can result in weirdness.
> 
> Setting nfct to NULL will make NAT targets, conntrack helpers and
> friends just ignore untracked packets, so we could forget about making
> sure that we aren't messing with the fake conntrack.

Sounds reasonable - but couldn't we simply change ip_conntrack_get
to return NULL for untracked connections?

>> Its changing the assumption
>> that skb->nfctinfo is only valid if skb->nfct is set, so you probably
>> need to set nfctinfo to 0 in nf_reset as well. This alone is a reason
>> not to do it IMO.
> 
> why need to set nfctinfo to 0 in nf_reset? since skb->nfct is NULL
> nf_reset shouldn't be ever called. With the logic I'm proposing, this
> packet will be handle just like invalid ones, so it will be just ignored.

nf_reset is called independant of the contents of skb->nfct. nfctinfo
needs to be set to 0 so a packet isn't untracked afterwards anymore.
Think of tunnel devices.

Regards
Patrick

Re: [RFC][PATCH] kill the fake conntrack

[Pablo Neira]

Patrick McHardy wrote:
> Pablo Neira wrote:
> 
>>Patrick McHardy wrote:
>>
>>
>>>What is the advantage of this patch?
>>
>>Whenever we work with conntracks, we always have to check if it's the
>>fake or not previously. For example, a bad use of NOTRACK together with
>>other targets, say ipt_CONNMARK, that doesn't currently check if it's
>>working with the fake conntrack or not. Same thing for NAT handlings, I
>>see that this can result in weirdness.
>>
>>Setting nfct to NULL will make NAT targets, conntrack helpers and
>>friends just ignore untracked packets, so we could forget about making
>>sure that we aren't messing with the fake conntrack.
> 
> 
> Sounds reasonable - but couldn't we simply change ip_conntrack_get
> to return NULL for untracked connections?

Then I won't be able to differenciate between invalid and untracked 
connections. So I'd need to return some extra info via ctinfo to achieve 
it, something in ip_conntrack_get like:

if (ct == &ip_conntrack_untracked)
	ctinfo = IP_CT_UNTRACKED

Well, but we'll still need the fake conntrack.

>>>Its changing the assumption
>>>that skb->nfctinfo is only valid if skb->nfct is set, so you probably
>>>need to set nfctinfo to 0 in nf_reset as well. This alone is a reason
>>>not to do it IMO.
>>
>>why need to set nfctinfo to 0 in nf_reset? since skb->nfct is NULL
>>nf_reset shouldn't be ever called. With the logic I'm proposing, this
>>packet will be handle just like invalid ones, so it will be just ignored.
> 
> 
> nf_reset is called independant of the contents of skb->nfct. nfctinfo
> needs to be set to 0 so a packet isn't untracked afterwards anymore.
> Think of tunnel devices.

OK. Hm, do you still consider that bad adding a line to nf_reset to 
reset skb->nfctinfo? This is the only drawback I see at the moment.

--
Pablo

Re: [RFC][PATCH] kill the fake conntrack

[Patrick McHardy]

Pablo Neira wrote:
>> nf_reset is called independant of the contents of skb->nfct. nfctinfo
>> needs to be set to 0 so a packet isn't untracked afterwards anymore.
>> Think of tunnel devices.
> 
> OK. Hm, do you still consider that bad adding a line to nf_reset to
> reset skb->nfctinfo? This is the only drawback I see at the moment.

Its not about the line, I'm not sure about changing the fact that
nfctinfo is only valid if nfct != NULL. Its too hot to think right
now, please give me until tommorrow :)

Regards
Patrick

Re: [RFC][PATCH] kill the fake conntrack

[Harald Welte]

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

On Sat, Jun 25, 2005 at 09:00:41PM +0200, Patrick McHardy wrote:
> Pablo Neira wrote:
> >> nf_reset is called independant of the contents of skb->nfct. nfctinfo
> >> needs to be set to 0 so a packet isn't untracked afterwards anymore.
> >> Think of tunnel devices.
> > 
> > OK. Hm, do you still consider that bad adding a line to nf_reset to
> > reset skb->nfctinfo? This is the only drawback I see at the moment.
> 
> Its not about the line, I'm not sure about changing the fact that
> nfctinfo is only valid if nfct != NULL. Its too hot to think right
> now, please give me until tommorrow :)

Looking through patchwork.netfilter.org, this thread never came to a
conclusion.  Any news on it?

-- 
- 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 --]