Re: [PATCH 05/13]: [IPV4/6]: Netfilter IPsec output hooks

[Patrick McHardy <kaber@trash.net>]

Herbert Xu wrote:
> On Sun, Nov 20, 2005 at 04:31:34PM +0000, Patrick McHardy wrote:
> 
>>diff --git a/net/ipv4/netfilter.c b/net/ipv4/netfilter.c
>>index ae0779d..b93e7cd 100644
>>--- a/net/ipv4/netfilter.c
>>+++ b/net/ipv4/netfilter.c
>>@@ -78,6 +79,34 @@ int ip_route_me_harder(struct sk_buff **
>> }
>> EXPORT_SYMBOL(ip_route_me_harder);
>> 
>>+#ifdef CONFIG_XFRM
>>+static inline int __ip_dst_output(struct sk_buff *skb)
> 
> 
> I'd like to suggest an alternative way of doing this that
> 
> 1) Keeps this XFRM stuff in xfrm*.c.
> 2) Removes the need for ip_dst_output.
> 
> Please see the attached patch.
> 
> 
>>+	do {
>>+		err = skb->dst->output(skb);
>>+
>>+		if (likely(err == 0))
>>+			return err;
>>+		if (unlikely(err != NET_XMIT_BYPASS))
>>+			return err;
>>+	} while (skb->dst->xfrm && !skb->dst->xfrm->props.mode);
>>+
>>+	return NF_HOOK(PF_INET, NF_IP_LOCAL_OUT, skb, NULL, skb->dst->dev,
>>+	               ip_dst_output);
> 
> 
> The idea is simply to put this stuff in xfrm[46]_output directly.
> So for your patch you would simply need to add the two NF_HOOK
> calls at the beginning and end of xfrm[46]_output once they've
> been modified in the way I outline below.

This looks nice, but placing the hooks at the end of the xfrm[46]
functions doesn't work with queueing without recursively calling
dst_output (as okfn) since we have to provide an okfn but also
have to return ownership of the skb back to dst_output.

>>diff --git a/net/ipv4/xfrm4_output.c b/net/ipv4/xfrm4_output.c
>>index 66620a9..c135746 100644
>>--- a/net/ipv4/xfrm4_output.c
>>+++ b/net/ipv4/xfrm4_output.c
>>@@ -133,6 +133,7 @@ int xfrm4_output(struct sk_buff *skb)
>> 		err = -EHOSTUNREACH;
>> 		goto error_nolock;
>> 	}
>>+	nf_reset(skb);
>> 	err = NET_XMIT_BYPASS;
> 
> 
> Shouldn't this sit after POST_ROUTING, i.e., once the connection
> has been confirmed?

This is after POST_ROUTING :) POST_ROUTING is called before the
transforms and LOCAL_OUT afterwards. But it could be moved to the
ip/ip6_dst_output functions to avoid unnecessarily trying to reset
the skb for transport mode transforms.