ip_route_me_harder -> xfrm_lookup

Hi:

I've received a number of reports that the any packets that are modified by the PREROUTING mangle table will not be protected by IPsec.

The reason is that ip_route_me_harder which is called upon the exit of the mangle table does not set the proto field. This means that xfrm_lookup is never called.

The following patch sets the proto field so that the packet can be protected by IPsec.

Cheers,

Debian GNU/Linux 3.0 is out! ( http://www.debian.org/ )
Email:  Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Index: kernel-2.5/net/core/netfilter.c
===================================================================
RCS file: /home/gondolin/herbert/src/CVS/debian/kernel-source-2.5/net/core/netfilter.c,v retrieving revision 1.1.1.10

diff -u -r1.1.1.10 netfilter.c
@@ -639,6 +639,7 @@
 #ifdef CONFIG_IP_ROUTE_FWMARK
 		fl.nl_u.ip4_u.fwmark = (*pskb)->nfmark;
 #endif
+		fl.proto = iph->protocol;
 		if (ip_route_output_key(&rt, &fl) != 0)
 			return -1;
 

Hi Herbert,

I have been working on a set of patches for IPsec+Netfilter, the latest set has been posted to netfilter-devel last week. They will go in patch-o-matic for testing soon, but I will post them to netdev later today, so we won't waste time testing patches before Dave is fine with them.

Regards,
Patrick

Regardless, and I look forward to your work, Herbert's patch is absolutely correct so I'm going to apply it for now.

In fact, your work is less likely to be 2.6.4 material I imagine :) So best to get Herbert's simpler fix in for now.

Sorry for the late reply. You're right of course, Herbert's patch is best for now. The following five mails contain what I've currently got, it's still rough, but I hope you have some suggestions how to avoid some ugly things.

Best regards
Patrick

This patch adds a new function nf_reset to reset netfilter related skb-fields. It has no real relationship to netfilter+ipsec, but it's required for the follow-up patches.

# This is a BitKeeper generated diff -Nru style patch. #
# ChangeSet
# 2004/03/18 03:22:26+01:00 [email protected]
# [NETFILTER]: Add new function 'nf_reset' to reset netfilter related skb-fields
#

# net/ipv6/sit.c
#   2004/03/18 03:19:58+01:00 [email protected] +2 -14
#   [NETFILTER]: Add new function 'nf_reset' to reset netfilter related skb-fields
# 
# net/ipv6/ip6_tunnel.c
#   2004/03/18 03:19:58+01:00 [email protected] +1 -7
#   [NETFILTER]: Add new function 'nf_reset' to reset netfilter related skb-fields
# 
# net/ipv4/ipip.c
#   2004/03/18 03:19:58+01:00 [email protected] +2 -14
#   [NETFILTER]: Add new function 'nf_reset' to reset netfilter related skb-fields
# 
# net/ipv4/ip_input.c
#   2004/03/18 03:19:58+01:00 [email protected] +1 -5
#   [NETFILTER]: Add new function 'nf_reset' to reset netfilter related skb-fields
# 
# net/ipv4/ip_gre.c
#   2004/03/18 03:19:58+01:00 [email protected] +2 -14
#   [NETFILTER]: Add new function 'nf_reset' to reset netfilter related skb-fields
# 
# include/linux/skbuff.h
#   2004/03/18 03:19:58+01:00 [email protected] +12 -3
#   [NETFILTER]: Add new function 'nf_reset' to reset netfilter related skb-fields
# 
diff -Nru a/include/linux/skbuff.h b/include/linux/skbuff.h
--- a/include/linux/skbuff.h	Thu Mar 18 16:45:10 2004
+++ b/include/linux/skbuff.h	Thu Mar 18 16:45:10 2004
@@ -1201,6 +1201,14 @@
 	if (nfct)
 		atomic_inc(&nfct->master->use);
 }
+static inline void nf_reset(struct sk_buff *skb)
+{
+	nf_conntrack_put(skb->nfct);
+	skb->nfct = NULL;
+#ifdef CONFIG_NETFILTER_DEBUG
+	skb->nf_debug = 0;
+#endif
+}

#ifdef CONFIG_BRIDGE_NETFILTER
static inline void nf_bridge_put(struct nf_bridge_info *nf_bridge)
@@ -1213,9 +1221,10 @@
if (nf_bridge)
atomic_inc(&nf_bridge->use);
}
-#endif
-
-#endif
+#endif /* CONFIG_BRIDGE_NETFILTER */
+#else /* CONFIG_NETFILTER */
+static inline void nf_reset(struct sk_buff *skb) {} +#endif /* CONFIG_NETFILTER */

#endif /* __KERNEL__ */
#endif /* _LINUX_SKBUFF_H */

diff -Nru a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
@@ -643,13 +643,7 @@
 		skb->dev = tunnel->dev;
 		dst_release(skb->dst);
 		skb->dst = NULL;
-#ifdef CONFIG_NETFILTER
-		nf_conntrack_put(skb->nfct);
-		skb->nfct = NULL;
-#ifdef CONFIG_NETFILTER_DEBUG
-		skb->nf_debug = 0;
-#endif
-#endif
+		nf_reset(skb);
 		ipgre_ecn_decapsulate(iph, skb);
 		netif_rx(skb);
 		read_unlock(&ipgre_lock);

@@ -877,13 +871,7 @@
 		}
 	}
 
-#ifdef CONFIG_NETFILTER
-	nf_conntrack_put(skb->nfct);
-	skb->nfct = NULL;
-#ifdef CONFIG_NETFILTER_DEBUG
-	skb->nf_debug = 0;
-#endif
-#endif
+	nf_reset(skb);
 
 	IPTUNNEL_XMIT();
 	tunnel->recursion--;

diff -Nru a/net/ipv4/ip_input.c b/net/ipv4/ip_input.c
@@ -202,17 +202,13 @@
 
 #ifdef CONFIG_NETFILTER_DEBUG
 	nf_debug_ip_local_deliver(skb);
-	skb->nf_debug = 0;
 #endif /*CONFIG_NETFILTER_DEBUG*/
 
 	__skb_pull(skb, ihl);
 
-#ifdef CONFIG_NETFILTER
 	/* Free reference early: we don't need it any more, and it may
            hold ip_conntrack module loaded indefinitely. */
-	nf_conntrack_put(skb->nfct);
-	skb->nfct = NULL;
-#endif /*CONFIG_NETFILTER*/
+	nf_reset(skb);
 
         /* Point into the IP datagram, just past the header. */
         skb->h.raw = skb->data;

diff -Nru a/net/ipv4/ipip.c b/net/ipv4/ipip.c
@@ -496,13 +496,7 @@
 		skb->dev = tunnel->dev;
 		dst_release(skb->dst);
 		skb->dst = NULL;
-#ifdef CONFIG_NETFILTER
-		nf_conntrack_put(skb->nfct);
-		skb->nfct = NULL;
-#ifdef CONFIG_NETFILTER_DEBUG
-		skb->nf_debug = 0;
-#endif
-#endif
+		nf_reset(skb);
 		ipip_ecn_decapsulate(iph, skb);
 		netif_rx(skb);
 		read_unlock(&ipip_lock);

@@ -647,13 +641,7 @@
 	if ((iph->ttl = tiph->ttl) == 0)
 		iph->ttl	=	old_iph->ttl;
 
-#ifdef CONFIG_NETFILTER
-	nf_conntrack_put(skb->nfct);
-	skb->nfct = NULL;
-#ifdef CONFIG_NETFILTER_DEBUG
-	skb->nf_debug = 0;
-#endif
-#endif
+	nf_reset(skb);
 
 	IPTUNNEL_XMIT();
 	tunnel->recursion--;

diff -Nru a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c
@@ -715,13 +715,7 @@
 	ipv6h->nexthdr = proto;
 	ipv6_addr_copy(&ipv6h->saddr, &fl.fl6_src);
 	ipv6_addr_copy(&ipv6h->daddr, &fl.fl6_dst);
-#ifdef CONFIG_NETFILTER
-	nf_conntrack_put(skb->nfct);
-	skb->nfct = NULL;
-#ifdef CONFIG_NETFILTER_DEBUG
-	skb->nf_debug = 0;
-#endif
-#endif
+	nf_reset(skb);
 	pkt_len = skb->len;
 	err = NF_HOOK(PF_INET6, NF_IP6_LOCAL_OUT, skb, NULL, 
 		      skb->dst->dev, dst_output);

diff -Nru a/net/ipv6/sit.c b/net/ipv6/sit.c
@@ -388,13 +388,7 @@
 		skb->dev = tunnel->dev;
 		dst_release(skb->dst);
 		skb->dst = NULL;
-#ifdef CONFIG_NETFILTER
-		nf_conntrack_put(skb->nfct);
-		skb->nfct = NULL;
-#ifdef CONFIG_NETFILTER_DEBUG
-		skb->nf_debug = 0;
-#endif
-#endif
+		nf_reset(skb);
 		ipip6_ecn_decapsulate(iph, skb);
 		netif_rx(skb);
 		read_unlock(&ipip6_lock);

@@ -580,13 +574,7 @@
 	if ((iph->ttl = tiph->ttl) == 0)
 		iph->ttl	=	iph6->hop_limit;
 
-#ifdef CONFIG_NETFILTER
-	nf_conntrack_put(skb->nfct);
-	skb->nfct = NULL;
-#ifdef CONFIG_NETFILTER_DEBUG
-	skb->nf_debug = 0;
-#endif
-#endif
+	nf_reset(skb);
 
 	IPTUNNEL_XMIT();
 	tunnel->recursion--;

I like and thus want this cleanup regardless of what happens to the rest of your patches :)

This patch adds new output-hooks. Packets with dst->xfrm != NULL traverse the POST_ROUTING hook before dst_output is called. The transformers mark the packets in the control buffer with a new flag IPSKB_XFRM_TRANSFORMED, these packets then traverse the LOCAL_OUT hook when they hit ip_output.

# This is a BitKeeper generated diff -Nru style patch. #
# ChangeSet
# 2004/03/18 14:59:24+01:00 [email protected]
# [NETFILTER}: Pass packets to POST_ROUTING hook before encryption and LOCAL_OUT afterwards
#

# net/ipv4/xfrm4_tunnel.c
#   2004/03/18 14:59:14+01:00 [email protected] +1 -0
#   [NETFILTER}: Pass packets to POST_ROUTING hook before encryption and LOCAL_OUT afterwards
# 
# net/ipv4/ipcomp.c
#   2004/03/18 14:59:14+01:00 [email protected] +1 -0
#   [NETFILTER}: Pass packets to POST_ROUTING hook before encryption and LOCAL_OUT afterwards
# 
# net/ipv4/ip_output.c
#   2004/03/18 14:59:14+01:00 [email protected] +20 -4
#   [NETFILTER}: Pass packets to POST_ROUTING hook before encryption and LOCAL_OUT afterwards
# 
# net/ipv4/ip_forward.c
#   2004/03/18 14:59:14+01:00 [email protected] +2 -1
#   [NETFILTER}: Pass packets to POST_ROUTING hook before encryption and LOCAL_OUT afterwards
# 
# net/ipv4/esp4.c
#   2004/03/18 14:59:14+01:00 [email protected] +1 -0
#   [NETFILTER}: Pass packets to POST_ROUTING hook before encryption and LOCAL_OUT afterwards
# 
# net/ipv4/ah4.c
#   2004/03/18 14:59:14+01:00 [email protected] +1 -0
#   [NETFILTER}: Pass packets to POST_ROUTING hook before encryption and LOCAL_OUT afterwards
# 
# include/net/ip.h
#   2004/03/18 14:59:14+01:00 [email protected] +1 -0
#   [NETFILTER}: Pass packets to POST_ROUTING hook before encryption and LOCAL_OUT afterwards
# 
# include/linux/netfilter.h
#   2004/03/18 14:59:14+01:00 [email protected] +9 -4
#   [NETFILTER}: Pass packets to POST_ROUTING hook before encryption and LOCAL_OUT afterwards
# 
diff -Nru a/include/linux/netfilter.h b/include/linux/netfilter.h
--- a/include/linux/netfilter.h	Thu Mar 18 16:45:22 2004
+++ b/include/linux/netfilter.h	Thu Mar 18 16:45:22 2004
@@ -119,12 +119,14 @@
 /* This is gross, but inline doesn't cut it for avoiding the function
    call in fast path: gcc doesn't inline (needs value tracking?). --RR */
 #ifdef CONFIG_NETFILTER_DEBUG
-#define NF_HOOK(pf, hook, skb, indev, outdev, okfn)			\
- nf_hook_slow((pf), (hook), (skb), (indev), (outdev), (okfn), INT_MIN)
+#define NF_HOOK_COND(pf, hook, skb, indev, outdev, okfn, cond)		\
+(!(cond)									\
+ ? (okfn)(skb) 								\
+ : nf_hook_slow((pf), (hook), (skb), (indev), (outdev), (okfn), INT_MIN))
 #define NF_HOOK_THRESH nf_hook_slow
 #else
-#define NF_HOOK(pf, hook, skb, indev, outdev, okfn)			\
-(list_empty(&nf_hooks[(pf)][(hook)])					\
+#define NF_HOOK_COND(pf, hook, skb, indev, outdev, okfn, cond)		\
+(!(cond) || list_empty(&nf_hooks[(pf)][(hook)])				\
  ? (okfn)(skb)								\
  : nf_hook_slow((pf), (hook), (skb), (indev), (outdev), (okfn), INT_MIN))
 #define NF_HOOK_THRESH(pf, hook, skb, indev, outdev, okfn, thresh)	\
@@ -132,6 +134,8 @@
  ? (okfn)(skb)								\
  : nf_hook_slow((pf), (hook), (skb), (indev), (outdev), (okfn), (thresh)))
 #endif
+#define NF_HOOK(pf, hook, skb, indev, outdev, okfn)			\
+ NF_HOOK_COND((pf), (hook), (skb), (indev), (outdev), (okfn), 1)

int nf_hook_slow(int pf, unsigned int hook, struct sk_buff *skb,
struct net_device *indev, struct net_device *outdev,
@@ -164,6 +168,7 @@

#else /* !CONFIG_NETFILTER */
#define NF_HOOK(pf, hook, skb, indev, outdev, okfn) (okfn)(skb)
+#define NF_HOOK_COND NF_HOOK
#endif /*CONFIG_NETFILTER*/

#endif /*__KERNEL__*/

diff -Nru a/include/net/ip.h b/include/net/ip.h
@@ -48,6 +48,7 @@
 #define IPSKB_TRANSLATED	2
 #define IPSKB_FORWARDED		4
 #define IPSKB_XFRM_TUNNEL_SIZE	8
+#define IPSKB_XFRM_TRANSFORMED	16
 };
 
 struct ipcm_cookie

diff -Nru a/net/ipv4/ah4.c b/net/ipv4/ah4.c
@@ -145,6 +145,7 @@
 		err = -EHOSTUNREACH;
 		goto error_nolock;
 	}
+	IPCB(skb)->flags |= IPSKB_XFRM_TRANSFORMED;
 	return NET_XMIT_BYPASS;
 
 error:

diff -Nru a/net/ipv4/esp4.c b/net/ipv4/esp4.c
@@ -199,6 +199,7 @@
 		err = -EHOSTUNREACH;
 		goto error_nolock;
 	}
+	IPCB(skb)->flags |= IPSKB_XFRM_TRANSFORMED;
 	return NET_XMIT_BYPASS;
 
 error:

diff -Nru a/net/ipv4/ip_forward.c b/net/ipv4/ip_forward.c
@@ -51,7 +51,8 @@
 	if (unlikely(opt->optlen))
 		ip_forward_options(skb);
 
-	return dst_output(skb);
+	return NF_HOOK_COND(PF_INET, NF_IP_POST_ROUTING, skb, NULL,
+	                    skb->dst->dev, dst_output, skb->dst->xfrm != NULL);
 }
 
 int ip_forward(struct sk_buff *skb)

diff -Nru a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
@@ -123,6 +123,12 @@
 	return ttl;
 }
 
+static inline int ip_dst_output(struct sk_buff *skb)
+{
+	return NF_HOOK_COND(PF_INET, NF_IP_POST_ROUTING, skb, NULL,
+	                    skb->dst->dev, dst_output, skb->dst->xfrm != NULL);
+}
+
 /* 
  *		Add an ip header to a skbuff and send it out.
  *

@@ -165,7 +171,7 @@
 
 	/* Send it out. */
 	return NF_HOOK(PF_INET, NF_IP_LOCAL_OUT, skb, NULL, rt->u.dst.dev,
-		       dst_output);
+		       ip_dst_output);
 }
 
 static inline int ip_finish_output2(struct sk_buff *skb)

@@ -283,7 +289,7 @@
 		return ip_finish_output(skb);
 }
 
-int ip_output(struct sk_buff *skb)
+static inline int ip_output2(struct sk_buff *skb)
 {
 	IP_INC_STATS(IpOutRequests);
 

@@ -294,6 +300,16 @@
 		return ip_finish_output(skb);
 }
 
+int ip_output(struct sk_buff *skb)
+{
+	int transformed = IPCB(skb)->flags & IPSKB_XFRM_TRANSFORMED;
+
+	if (transformed)
+		nf_reset(skb);
+	return NF_HOOK_COND(PF_INET, NF_IP_LOCAL_OUT, skb, NULL,
+	                    skb->dst->dev, ip_output2, transformed);
+}
+
 int ip_queue_xmit(struct sk_buff *skb, int ipfragok)
 {
 	struct sock *sk = skb->sk;

@@ -387,7 +403,7 @@
 	skb->priority = sk->sk_priority;
 
 	return NF_HOOK(PF_INET, NF_IP_LOCAL_OUT, skb, NULL, rt->u.dst.dev,
-		       dst_output);
+		       ip_dst_output);
 
 no_route:
 	IP_INC_STATS(IpOutNoRoutes);

@@ -1177,7 +1193,7 @@
 
 	/* Netfilter gets whole the not fragmented skb. */
 	err = NF_HOOK(PF_INET, NF_IP_LOCAL_OUT, skb, NULL, 
-		      skb->dst->dev, dst_output);
+		      skb->dst->dev, ip_dst_output);
 	if (err) {
 		if (err > 0)
 			err = inet->recverr ? net_xmit_errno(err) : 0;

diff -Nru a/net/ipv4/ipcomp.c b/net/ipv4/ipcomp.c
@@ -231,6 +231,7 @@
 		err = -EHOSTUNREACH;
 		goto error_nolock;
 	}
+	IPCB(skb)->flags |= IPSKB_XFRM_TRANSFORMED;
 	err = NET_XMIT_BYPASS;
 
 out_exit:

diff -Nru a/net/ipv4/xfrm4_tunnel.c b/net/ipv4/xfrm4_tunnel.c
@@ -76,6 +76,7 @@
 		err = -EHOSTUNREACH;
 		goto error_nolock;
 	}
+	IPCB(skb)->flags |= IPSKB_XFRM_TRANSFORMED;
 	return NET_XMIT_BYPASS;
 
 error_nolock:

This one isn't going to compile with netfilter disabled, I can tell just by looking at it :-)

The reason is that NF_HOOK_COND is defined to NF_HOOK in that case, and these two macros take a different number of args so the compiler is going to barf.

Thank you very much for your patches. This is definitely the biggest show stopper with the current IPsec stack.

I've just got a minor point about this one:

Debian GNU/Linux 3.0 is out! ( http://www.debian.org/ )
Email:  Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Hi

I don't have a full understanding of the code area we are talking about, having only had a peruse of it a while back.

But from my reading of this do I understand that packets that enter a box to be routed (forwarded on) that match a IPSEC transform policy

1) Are seen in mangle prerouting
2) nat prerouting
2) Encrypted
3) mangle Output
4) nat Output

Does this mean they also hit the filter input & output tables aswell or
just the filter forward table.

I would have thought for the whole process the packet (routed/forward packet) would only be seen in the prerouting/forward/postrouting flow. Weather or not it is going to be encrypted shouldn't make a difference I would have thought

This patch adds new hooks for input, it is the worst of all patches. Packets should traverse PRE_ROUTING+LOCAL_IN before decryption and PRE_ROUTING+LOCAL_IN/FORWARD afterwards. This is tricky because I didn't find an elegant way to determine when decapsulation is done. Currently, packets reposted into the stack don't traverse the hooks at the normal positions but at a later time when we know decapsulation if done:

- if a reposted packet has a non-local destination after input routing decapsulation must be done, it then traverses the PRE_ROUTING hook.

- otherwise, it continues until ip_local_deliver_finish but also skips the LOCAL_IN hook. All xfrm protocol-handlers are marked with xfrm_prot. If the protocol handler of a packet with a secpath pointer is a non-xfrm-protocol the packet was handled by ipsec and is done now, it traverses the PRE_ROUTING and LOCAL_IN hooks then. This catches packets from both tunnel-mode and transport-mode SAs.

Because the hooks are traversed at a later time than usual several not-so-nice things have to be done in the nf_postxfrm_* functions.

I don't see a better way currently, but I'm sure you do ;)

# This is a BitKeeper generated diff -Nru style patch. #
# ChangeSet
# 2004/03/18 16:41:37+01:00 [email protected]
# [NETFILTER]: post-ipsec input hooks
#

# net/ipv4/xfrm4_tunnel.c
#   2004/03/18 16:41:31+01:00 [email protected] +1 -0
#   [NETFILTER]: post-ipsec input hooks
# 
# net/ipv4/ipcomp.c
#   2004/03/18 16:41:31+01:00 [email protected] +1 -0
#   [NETFILTER]: post-ipsec input hooks
# 
# net/ipv4/ip_input.c
#   2004/03/18 16:41:31+01:00 [email protected] +15 -6
#   [NETFILTER]: post-ipsec input hooks
# 
# net/ipv4/esp4.c
#   2004/03/18 16:41:31+01:00 [email protected] +1 -0
#   [NETFILTER]: post-ipsec input hooks
# 
# net/ipv4/ah4.c
#   2004/03/18 16:41:31+01:00 [email protected] +1 -0
#   [NETFILTER]: post-ipsec input hooks
# 
# net/core/netfilter.c
#   2004/03/18 16:41:31+01:00 [email protected] +50 -3
#   [NETFILTER]: post-ipsec input hooks
# 
# include/net/protocol.h
#   2004/03/18 16:41:31+01:00 [email protected] +1 -0
#   [NETFILTER]: post-ipsec input hooks
# 
# include/linux/netfilter_ipv4.h
#   2004/03/18 16:41:31+01:00 [email protected] +15 -0
#   [NETFILTER]: post-ipsec input hooks
# 
diff -Nru a/include/linux/netfilter_ipv4.h b/include/linux/netfilter_ipv4.h
--- a/include/linux/netfilter_ipv4.h	Thu Mar 18 16:45:35 2004
+++ b/include/linux/netfilter_ipv4.h	Thu Mar 18 16:45:35 2004
@@ -83,6 +83,21 @@
    Returns true or false. */
 extern int skb_ip_make_writable(struct sk_buff **pskb,
 				unsigned int writable_len);
+
+#ifdef CONFIG_XFRM
+extern int nf_postxfrm_input(struct sk_buff *skb);
+extern int nf_postxfrm_nonlocal(struct sk_buff *skb);
+#else /* CONFIG_XFRM */
+static inline int nf_postxfrm_input(struct sk_buff *skb)
+{
+	return 0;
+}
+
+static inline int nf_postxfrm_nonlocal(struct sk_buff *skb)
+{
+	return 0;
+}
+#endif /* CONFIG_XFRM */
 #endif /*__KERNEL__*/

#endif /*__LINUX_IP_NETFILTER_H*/

diff -Nru a/include/net/protocol.h b/include/net/protocol.h
@@ -39,6 +39,7 @@
 	int			(*handler)(struct sk_buff *skb);
 	void			(*err_handler)(struct sk_buff *skb, u32 info);
 	int			no_policy;
+	int			xfrm_prot;
 };
 
 #if defined(CONFIG_IPV6) || defined (CONFIG_IPV6_MODULE)

diff -Nru a/net/ipv4/ah4.c b/net/ipv4/ah4.c
@@ -344,6 +344,7 @@
 	.handler	=	xfrm4_rcv,
 	.err_handler	=	ah4_err,
 	.no_policy	=	1,
+	.xfrm_prot	=	1,
 };
 
 static int __init ah4_init(void)

diff -Nru a/net/ipv4/esp4.c b/net/ipv4/esp4.c
@@ -577,6 +577,7 @@
 	.handler	=	xfrm4_rcv,
 	.err_handler	=	esp4_err,
 	.no_policy	=	1,
+	.xfrm_prot	=	1,
 };
 
 static int __init esp4_init(void)

diff -Nru a/net/ipv4/ip_input.c b/net/ipv4/ip_input.c
@@ -224,6 +224,12 @@
 	resubmit:
 		hash = protocol & (MAX_INET_PROTOS - 1);
 		raw_sk = sk_head(&raw_v4_htable[hash]);
+		ipprot = inet_protos[hash];
+		smp_read_barrier_depends();
+
+		if (skb->sp && !ipprot->xfrm_prot)
+			if (nf_postxfrm_input(skb))
+				goto out;
 
 		/* If there maybe a raw socket we must check - if not we
 		 * don't care less

@@ -231,10 +237,9 @@
 		if (raw_sk)
 			raw_v4_input(skb, skb->nh.iph, hash);
 
-		if ((ipprot = inet_protos[hash]) != NULL) {
+		if (ipprot != NULL) {
 			int ret;
 
-			smp_read_barrier_depends();
 			if (!ipprot->no_policy &&
 			    !xfrm4_policy_check(NULL, XFRM_POLICY_IN, skb)) {
 				kfree_skb(skb);

@@ -279,8 +284,8 @@
 			return 0;
 	}
 
-	return NF_HOOK(PF_INET, NF_IP_LOCAL_IN, skb, skb->dev, NULL,
-		       ip_local_deliver_finish);
+	return NF_HOOK_COND(PF_INET, NF_IP_LOCAL_IN, skb, skb->dev, NULL,
+	                    ip_local_deliver_finish, !skb->sp);
 }
 
 static inline int ip_rcv_finish(struct sk_buff *skb)

@@ -346,6 +351,10 @@
 		}
 	}
 
+	if (skb->sp && !(((struct rtable *)skb->dst)->rt_flags&RTCF_LOCAL))
+		if (nf_postxfrm_nonlocal(skb))
+			goto drop;
+
 	return dst_input(skb);
 
 inhdr_error:

@@ -418,8 +427,8 @@
 		}
 	}
 
-	return NF_HOOK(PF_INET, NF_IP_PRE_ROUTING, skb, dev, NULL,
-		       ip_rcv_finish);
+	return NF_HOOK_COND(PF_INET, NF_IP_PRE_ROUTING, skb, dev, NULL,
+	                    ip_rcv_finish, !skb->sp);
 
 inhdr_error:
 	IP_INC_STATS_BH(IpInHdrErrors);

diff -Nru a/net/ipv4/ipcomp.c b/net/ipv4/ipcomp.c
@@ -408,6 +408,7 @@
 	.handler	=	xfrm4_rcv,
 	.err_handler	=	ipcomp4_err,
 	.no_policy	=	1,
+	.xfrm_prot	=	1,
 };
 
 static int __init ipcomp4_init(void)

diff -Nru a/net/ipv4/xfrm4_tunnel.c b/net/ipv4/xfrm4_tunnel.c
@@ -171,6 +171,7 @@
 	.handler	=	ipip_rcv,
 	.err_handler	=	ipip_err,
 	.no_policy	=	1,
+	.xfrm_prot	=	1,
 };
 
 static int __init ipip_init(void)

Be careful! xfrm4_tunnel handles both uncompressed ipcomp packets _and_ IPIP encapsulator device packets. Yet you will intepret usage of the ipprot as 'xfrm_prot==1' in all cases.

Yes this is ugly... if we added some kind of flag bit-mask to sk_buff, would that allow an easier implementation?

Good point.

I'm not sure if this'll help in the degenerate IPCOMP case. Perhaps we need a way to tell if it is a degenerate IPCOMP tunnel or an IPIP tunnel without actually processing the packet.

Debian GNU/Linux 3.0 is out! ( http://www.debian.org/ )
Email:  Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

I can't imagine how. Best would be to avoid the xfrm_prot flag completely. Maybe we can add a flag to xfrm_state which indicates that this is the last xfrm specified in the policy ?

The same state can be terminal in one policy but not another.

Debian GNU/Linux 3.0 is out! ( http://www.debian.org/ )
Email:  Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

If we also have a possibility to determine when decapsulation is done we could avoid the nf_postxfrm_* hacks. Without that, I don't think it helps.

You have good eyes ;) The packet needs to be rerouted in nf_postxfrm_nonlocal and needs to be special-cased in nf_postxfrm_input so it doesn't traverse PRE_ROUTING twice.

Thanks for your other suggestions as well, I'm going to make these changes.

Regards
Patrick

This patch adds policy lookups to ip_route_me_harder and makes NAT reroute for any change that affects route/policy lookups.

# This is a BitKeeper generated diff -Nru style patch. #
# ChangeSet
# 2004/03/18 16:43:22+01:00 [email protected]
# [NETFILTER]: Add policy lookups to ip_route_me_harder, make NAT reroute for any change in route/policy key
#

# net/ipv4/netfilter/ip_nat_standalone.c
#   2004/03/18 16:43:16+01:00 [email protected] +72 -8
#   [NETFILTER]: Add policy lookups to ip_route_me_harder, make NAT reroute for any change in route/policy key
# 
# net/ipv4/netfilter/ip_conntrack_standalone.c
#   2004/03/18 16:43:16+01:00 [email protected] +1 -0
#   [NETFILTER]: Add policy lookups to ip_route_me_harder, make NAT reroute for any change in route/policy key
# 
# net/core/netfilter.c
#   2004/03/18 16:43:16+01:00 [email protected] +15 -1
#   [NETFILTER]: Add policy lookups to ip_route_me_harder, make NAT reroute for any change in route/policy key
# 
diff -Nru a/net/core/netfilter.c b/net/core/netfilter.c
--- a/net/core/netfilter.c	Thu Mar 18 16:46:02 2004
+++ b/net/core/netfilter.c	Thu Mar 18 16:46:02 2004
@@ -27,6 +27,7 @@
 #include <net/sock.h>
 #include <net/route.h>
 #include <net/xfrm.h>
+#include <net/ip.h>
 #include <linux/ip.h>

/* In this code, we can be waiting indefinitely for userspace to
@@ -635,7 +636,6 @@
#ifdef CONFIG_IP_ROUTE_FWMARK
fl.nl_u.ip4_u.fwmark = (*pskb)->nfmark;
#endif
- fl.proto = iph->protocol;
if (ip_route_output_key(&rt, &fl) != 0)
return -1;

@@ -661,6 +661,20 @@

if ((*pskb)->dst->error)
return -1;
+
+#ifdef CONFIG_XFRM
+ if (!(IPCB(*pskb)->flags & IPSKB_XFRM_TRANSFORMED)) {
+ struct xfrm_policy_afinfo *afinfo;
+
+ afinfo = xfrm_policy_get_afinfo(AF_INET);
+ if (afinfo != NULL) {
+ afinfo->decode_session(*pskb, &fl);
+ xfrm_policy_put_afinfo(afinfo);
+ if (xfrm_lookup(&(*pskb)->dst, &fl, (*pskb)->sk, 0) != 0)
+ return -1;
+ }
+ }
+#endif

/* Change in oif may mean change in hh_len. */ hh_len = (*pskb)->dst->dev->hard_header_len;

diff -Nru a/net/ipv4/netfilter/ip_conntrack_standalone.c b/net/ipv4/netfilter/ip_conntrack_standalone.c
@@ -583,6 +583,7 @@
 EXPORT_SYMBOL(ip_conntrack_alter_reply);
 EXPORT_SYMBOL(ip_conntrack_destroyed);
 EXPORT_SYMBOL(ip_conntrack_get);
+EXPORT_SYMBOL(__ip_conntrack_confirm);
 EXPORT_SYMBOL(need_ip_conntrack);
 EXPORT_SYMBOL(ip_conntrack_helper_register);
 EXPORT_SYMBOL(ip_conntrack_helper_unregister);

Why are you deleting that "fl.proto = iph->protocol;" line in net/core/netfilter.c? Is something else going to set it properly?

The patch adds a call to decode_session/xfrm_lookup below. This handles packets with local and non-local source, setting fl.proto only handles packets with local source. Also we must check if the packet was already transformed to prevent loops.

Regards
Patrick

I don't understand the relationship to transport mode packets. I used an explicit call to xfrm_lookup so packets with non-local source are also handled. We also need to protect against loops, packets which are already transformed should not be transformed again.

Regards
Patrick

Right, you're calling decode_session below which is much better.

Actually it was me who was confused. ip_route_me_harder can be called on both incoming/outgoing packets. That's what the if clause is trying to determine. You should only call xfrm_lookup on the outgoing path.

So this should be moved back to the if clause above:

		fl.proto = iph->protocol;
		lookup = __ip_route_output_key;
#ifdef CONFIG_XFRM
		if (!(IPCB(*pskb)->flags & IPSKB_XFRM_TRANSFORMED)) {
			lookup = ip_route_output_key;
			do_decode
		}
#endif
		if (lookup(&rt, &fl) != 0)
			return -1;

Debian GNU/Linux 3.0 is out! ( http://www.debian.org/ )
Email:  Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

No, ip_route_me_harder is currently (without the patches) only called for outgoing packets. The if-clause is there because ip_route_output doesn't handle packets with non-local source, and we don't want to set the source to 0 (as was done before) because it prevents policy routing from working properly. That's why we need the xfrm_lookup for both cases.

Regards
Patrick

You're right. Sorry for the confusion.

Debian GNU/Linux 3.0 is out! ( http://www.debian.org/ )
Email:  Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Is it correct that __xfrm_route_forward will use NULL for the sock parameter to xfrm_lookup even if the packet is from a local socket ?

Regards
Patrick

No that would be wrong as socket policies won't be applied correctly. Forget about that idea :)

Debian GNU/Linux 3.0 is out! ( http://www.debian.org/ )
Email:  Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Thanks anyway, it reminded me to check for !(dst->flags & DST_NOXFRM) before xfrm_lookup, I'm going to change this now.

Regards
Patrick

Hi

Think their might be a problem with this patch.

Potientially a packet could traverse the pre, forward and the post routing, at which point it can be SNAT'ed or MASQ'ed and then re injected into route_me_harder. This potiential could allow packets to be rerouted based on the new src/dst addresses differently to the intail packet but this new packet doesn't traverse any of the chains with the new information.

Alex

This is just as without the patches, SNAT in POST_ROUTING never causes a packet to re-traverse the hooks. There is one minor difference, packets which match a policy after NAT stop traversing the hooks at NF_IP_PRI_NAT_SRC priority. I will fix this this for the final version.

Regards
Patrick

Sorry might not have made myself clear, after an SNAT with your patch the packet is re injected into route_me_harder, thus the packet is able to be rerouted (sent out another interface for example)

What this would mean is a packet could meet your iptable rules with is PRE-SNAT details and then actually behave differently once it has been SNAT'ed (and not get checked)

Alex

This patch makes xfrm_policy_check locate the correct policy after NAT. For protocols which do policy checks in their receive routines the reference to nfct has to be kept until policy checks are done, the other ones still drop it in ip_local_deliver_finish.

# This is a BitKeeper generated diff -Nru style patch. #
# ChangeSet
# 2004/03/18 16:44:21+01:00 [email protected]
# [NETFILTER]: Make policy checks find correct policy after NAT
#

# net/xfrm/xfrm_policy.c
#   2004/03/18 16:44:12+01:00 [email protected] +2 -0
#   [NETFILTER]: Make policy checks find correct policy after NAT
# 
# net/ipv4/udp.c
#   2004/03/18 16:44:12+01:00 [email protected] +2 -0
#   [NETFILTER]: Make policy checks find correct policy after NAT
# 
# net/ipv4/tcp_ipv4.c
#   2004/03/18 16:44:12+01:00 [email protected] +1 -0
#   [NETFILTER]: Make policy checks find correct policy after NAT
# 
# net/ipv4/raw.c
#   2004/03/18 16:44:12+01:00 [email protected] +1 -0
#   [NETFILTER]: Make policy checks find correct policy after NAT
# 
# net/ipv4/ip_input.c
#   2004/03/18 16:44:12+01:00 [email protected] +6 -8
#   [NETFILTER]: Make policy checks find correct policy after NAT
# 
# net/core/netfilter.c
#   2004/03/18 16:44:12+01:00 [email protected] +43 -0
#   [NETFILTER]: Make policy checks find correct policy after NAT
# 
# include/linux/netfilter.h
#   2004/03/18 16:44:12+01:00 [email protected] +16 -0
#   [NETFILTER]: Make policy checks find correct policy after NAT
# 
diff -Nru a/include/linux/netfilter.h b/include/linux/netfilter.h
--- a/include/linux/netfilter.h	Thu Mar 18 16:46:27 2004
+++ b/include/linux/netfilter.h	Thu Mar 18 16:46:27 2004
@@ -171,5 +171,21 @@
 #define NF_HOOK_COND NF_HOOK
 #endif /*CONFIG_NETFILTER*/

+#ifdef CONFIG_XFRM
+#ifdef CONFIG_IP_NF_NAT_NEEDED
+struct flowi;
+extern void nf_nat_decode_session4(struct sk_buff *skb, struct flowi *fl); +
+static inline void
+nf_nat_decode_session(struct sk_buff *skb, struct flowi *fl, int family) +{
+ if (family == AF_INET)
+ nf_nat_decode_session4(skb, fl);
+}
+#else /* CONFIG_IP_NF_NAT_NEEDED */
+#define nf_nat_decode_session(skb,fl,family)
+#endif /* CONFIG_IP_NF_NAT_NEEDED */
+#endif /* CONFIG_XFRM */
+
#endif /*__KERNEL__*/
#endif /*__LINUX_NETFILTER_H*/

diff -Nru a/net/core/netfilter.c b/net/core/netfilter.c
@@ -739,6 +739,49 @@
 	return NF_HOOK(PF_INET, NF_IP_PRE_ROUTING, skb, skb->dev, NULL,
 	               nf_postxfrm_done);
 }
+
+#ifdef CONFIG_IP_NF_NAT_NEEDED
+#include <linux/netfilter_ipv4/ip_conntrack.h>
+#include <linux/netfilter_ipv4/ip_nat.h>
+
+void nf_nat_decode_session4(struct sk_buff *skb, struct flowi *fl)
+{
+	struct ip_conntrack *ct;
+	struct ip_conntrack_tuple *t;
+	struct ip_nat_info_manip *m;
+	unsigned int i;
+
+	if (skb->nfct == NULL)
+		return;
+	ct = (struct ip_conntrack *)skb->nfct->master;
+
+	for (i = 0; i < ct->nat.info.num_manips; i++) {
+		m = &ct->nat.info.manips[i];
+		t = &ct->tuplehash[m->direction].tuple;
+
+		switch (m->hooknum) {
+		case NF_IP_PRE_ROUTING:
+			if (m->maniptype != IP_NAT_MANIP_DST)
+				break;
+			fl->fl4_dst = t->dst.ip;
+			if (t->dst.protonum == IPPROTO_TCP ||
+			    t->dst.protonum == IPPROTO_UDP)
+				fl->fl_ip_dport = t->dst.u.tcp.port;
+			break;
+#ifdef CONFIG_IP_NF_NAT_LOCAL
+		case NF_IP_LOCAL_IN:
+			if (m->maniptype != IP_NAT_MANIP_SRC)
+				break;
+			fl->fl4_src = t->src.ip;
+			if (t->dst.protonum == IPPROTO_TCP ||
+			    t->dst.protonum == IPPROTO_UDP)
+				fl->fl_ip_sport = t->src.u.tcp.port;
+			break;
+#endif
+		}
+	}
+}
+#endif /* CONFIG_IP_NF_NAT_NEEDED */
 #endif /* CONFIG_XFRM */
 
 int skb_ip_make_writable(struct sk_buff **pskb, unsigned int writable_len)

diff -Nru a/net/ipv4/ip_input.c b/net/ipv4/ip_input.c
@@ -206,10 +206,6 @@
 
 	__skb_pull(skb, ihl);
 
-	/* Free reference early: we don't need it any more, and it may
-           hold ip_conntrack module loaded indefinitely. */
-	nf_reset(skb);
-
         /* Point into the IP datagram, just past the header. */
         skb->h.raw = skb->data;
 

@@ -240,10 +236,12 @@
 		if (ipprot != NULL) {
 			int ret;
 
-			if (!ipprot->no_policy &&
-			    !xfrm4_policy_check(NULL, XFRM_POLICY_IN, skb)) {
-				kfree_skb(skb);
-				goto out;
+			if (!ipprot->no_policy) {
+				if (!xfrm4_policy_check(NULL, XFRM_POLICY_IN, skb)) {
+					kfree_skb(skb);
+					goto out;
+				}
+				nf_reset(skb);
 			}
 			ret = ipprot->handler(skb);
 			if (ret < 0) {

diff -Nru a/net/ipv4/raw.c b/net/ipv4/raw.c
@@ -249,6 +249,7 @@
 		kfree_skb(skb);
 		return NET_RX_DROP;
 	}
+	nf_reset(skb);
 
 	skb_push(skb, skb->data - skb->nh.raw);
 

diff -Nru a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
@@ -1785,6 +1785,7 @@
 
 	if (!xfrm4_policy_check(sk, XFRM_POLICY_IN, skb))
 		goto discard_and_relse;
+	nf_reset(skb);
 
 	if (sk_filter(sk, skb, 0))
 		goto discard_and_relse;

diff -Nru a/net/ipv4/udp.c b/net/ipv4/udp.c
@@ -1030,6 +1030,7 @@
 		kfree_skb(skb);
 		return -1;
 	}
+	nf_reset(skb);
 
 	if (up->encap_type) {
 		/*

@@ -1195,6 +1196,7 @@
 
 	if (!xfrm4_policy_check(NULL, XFRM_POLICY_IN, skb))
 		goto drop;
+	nf_reset(skb);
 
 	/* No socket. Drop packet silently, if checksum is wrong */
 	if (udp_checksum_complete(skb))

diff -Nru a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
@@ -21,6 +21,7 @@
 #include <linux/workqueue.h>
 #include <linux/notifier.h>
 #include <linux/netdevice.h>
+#include <linux/netfilter.h>
 #include <net/xfrm.h>
 #include <net/ip.h>
 

@@ -908,6 +909,7 @@
 
 	if (_decode_session(skb, &fl, family) < 0)
 		return 0;
+	nf_nat_decode_session(skb, &fl, family);
 
 	/* First, check used SA against their selectors. */
 	if (skb->sp) {

This patch looks fine to me.

Other than the minor comments I've made the most unhappy I am with the input patch, and you agree it's grotty too. Let's look for a better solution, perhaps with new top-level SKB state, and then we can put all of your work in after you're made the other minor fixes I've asked for as well.

Thanks Patrick.

I have applied these to 2.6.4 (debian source) and compiles okay and seems to work okay, ie the NAT + IPSEC

my 2c

Done some more testing and there seems to be a problem with the NAT table

Chain PREROUTING (policy ACCEPT 0 packets, 0 bytes)
pkts bytes target prot opt in out source
destination

   12   720            all  --  *      *       0.0.0.0/0 192.168.5.2
    0     0 DROP       all  --  *      *       169.254.0.0/16 0.0.0.0/0
54378 5744K ACCEPT     all  --  *      *       0.0.0.0/0 0.0.0.0/0           state NEW

Chain POSTROUTING (policy ACCEPT 6 packets, 240 bytes)

 pkts bytes target     prot opt in     out     source destination
    0     0            all  --  *      *       0.0.0.0/0 192.168.5.2
 2872  537K MASQUERADE  all  --  *      eth0    192.168.8.0/22 0.0.0.0/0
    0     0 MASQUERADE  all  --  *      ppp+    192.168.8.0/22 0.0.0.0/0

165K 25M ACCEPT all -- * * 0.0.0.0/0 0.0.0.0/0 state NEW

Chain OUTPUT (policy ACCEPT 0 packets, 0 bytes)

 pkts bytes target     prot opt in     out     source destination
    0     0            all  --  *      *       0.0.0.0/0 192.168.5.2
 169K   25M ACCEPT     all  --  *      *       0.0.0.0/0 0.0.0.0/0           state NEW

There is an ipsec tunnel out of eth0 setup like

conn abcd

	leftsubnet 192.168.8.0/22
	rightsubnet 192.168.5.0/24

when I ping from 192.168.10.244 through to 192.168.5.2 it doesn't seem to it through to POSTROUTING, the counter in PREROUTING increase.

My initial search for this was why I did not need a rule to stop the MASQ from happening

This is using the 2.6 stack (2.6.4 debain)

Alex

Thanks for the report, does this patch fix it ?

Regards
Patrick

===== net/ipv4/raw.c 1.44 vs edited =====

@@ -257,6 +257,12 @@
 	return 0;
 }
 
+static inline int ip_dst_output(struct sk_buff *skb)
+{
+	return NF_HOOK_COND(PF_INET, NF_IP_POST_ROUTING, skb, NULL,
+	                    skb->dst->dev, dst_output, skb->dst->xfrm != NULL);
+}
+
 static int raw_send_hdrinc(struct sock *sk, void *from, int length,
 			struct rtable *rt, 
 			unsigned int flags)

@@ -308,7 +314,7 @@
 	}
 
 	err = NF_HOOK(PF_INET, NF_IP_LOCAL_OUT, skb, NULL, rt->u.dst.dev,
-		      dst_output);
+		      ip_dst_output);
 	if (err > 0)
 		err = inet->recverr ? net_xmit_errno(err) : 0;
 	if (err)

I'm going to make the changes you suggested. Can you please explain what you imagine with the new top-level skb state ?

Regards
Patrick