[PATCH] Re: another PPTP conntrack problem (no fix this time)

[Philip Craig <philipc@snapgear.com>]

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

Philip Craig wrote:
> Paul Mielke wrote:
> 
>>Does this seem to make sense?  Please let me know if you think I am
>>mistaken in my analysis and that the previous ICMP patches would help.
> 
> 
> You've convinced me that there is a problem here.  I think
> that gre_pkt_to_tuple() needs to handle the case when
> gre_keymap_lookup() fails by trying to do an inverse
> lookup.  I haven't tried fixing this yet.
> 
> I suspect that you will still need the other ICMP patches
> as well once you have the gre lookup succeeding though.
> 

I've attached a patch that Paul has tested that fixes this
problem.  He had to use it in conjunction with an ICMP patch
posted by Balazs Scheidler.

The problem is that there are only gre keymaps for packets
in the state where they first arrive at a machine.  But when
we need to generate an ICMP error, the gre packet may have
been NATed, and so it no longer matches the keymaps.

The attached patch extends gre_keymap_lookup to also compare
against the inverted keymaps.

I'm not happy with this patch, but I don't have the time to
fix it properly at the moment, so I've post it here.  There
are 2 problems with the patch.

Firstly, it is combining two tasks into the one function.
That means that incoming packets are compared against the
inverted keymap, when they shouldn't be.  Similarly, when
generating ICMP errors, the packet is matched against the
original keymap when it only needs to be matched against
inverted one.

Secondly, matching against inverted tuples is not going to
work when we are doing both SNAT and DNAT.  This problem may
not be specific to the gre conntrack either; ip_conntrack_core.c
has the following comment in icmp_error_track():

/* FIXME: NAT code has to handle half-done double NAT --RR */

I think the fix to both of these problems is to add another
pkt_to_tuple function to the ip_conntrack_protocol structure
which converts a half-NATed packet to a tuple.

-- 
Philip Craig - philipc@snapgear.com - http://www.SnapGear.com
SnapGear - Custom Embedded Solutions and Security Appliances

[-- Attachment #2: gre-lookup.patch --]
[-- Type: text/plain, Size: 1301 bytes --]

diff -u -r1.4 ip_conntrack_proto_gre.c
--- ip_conntrack_proto_gre.c	4 Dec 2002 23:44:24 -0000	1.4
+++ ip_conntrack_proto_gre.c	6 Feb 2003 03:38:39 -0000
@@ -79,6 +79,17 @@
 	        (km->tuple.dst.u.all == t->dst.u.all));
 }
 
+static inline int gre_key_inverse_cmpfn(const struct ip_ct_gre_keymap *km,
+					const struct ip_conntrack_tuple *t)
+{
+	return ((km->tuple.dst.ip == t->src.ip) &&
+		(km->tuple.src.ip == t->dst.ip) &&
+	        (km->tuple.dst.protonum == t->dst.protonum) &&
+		(km->tuple.dst.u.gre.protocol == t->dst.u.gre.protocol) &&
+		(km->tuple.dst.u.gre.version == t->dst.u.gre.version) &&
+	        (km->tuple.src.u.gre.key == t->dst.u.gre.key));
+}
+
 /* look up the source key for a given tuple */
 static u_int32_t gre_keymap_lookup(struct ip_conntrack_tuple *t)
 {
@@ -88,12 +99,18 @@
 	READ_LOCK(&ip_ct_gre_lock);
 	km = LIST_FIND(&gre_keymap_list, gre_key_cmpfn,
 			struct ip_ct_gre_keymap *, t);
-	if (!km) {
-		READ_UNLOCK(&ip_ct_gre_lock);
-		return 0;
+	if (km) {
+		key = km->tuple.src.u.gre.key;
+	} else {
+		km = LIST_FIND(&gre_keymap_list, gre_key_inverse_cmpfn,
+				struct ip_ct_gre_keymap *, t);
+		if (km) {
+			key = km->tuple.dst.u.gre.key;
+		} else {
+			key = 0;
+		}
 	}
 
-	key = km->tuple.src.u.gre.key;
 	READ_UNLOCK(&ip_ct_gre_lock);
 
 	return key;