From mboxrd@z Thu Jan 1 00:00:00 1970 From: Patrick McHardy Subject: Re: [PATCH 05/13]: [IPV4/6]: Netfilter IPsec output hooks Date: Tue, 22 Nov 2005 05:53:35 +0100 Message-ID: <4382A44F.9000105@trash.net> References: <20051120163128.16666.38111.sendpatchset@localhost.localdomain> <20051120163134.16666.9265.sendpatchset@localhost.localdomain> <20051122044046.GA29166@gondor.apana.org.au> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: netdev@vger.kernel.org, netfilter-devel@lists.netfilter.org, davem@davemloft.net Return-path: To: Herbert Xu In-Reply-To: <20051122044046.GA29166@gondor.apana.org.au> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: netfilter-devel-bounces@lists.netfilter.org Errors-To: netfilter-devel-bounces@lists.netfilter.org List-Id: netdev.vger.kernel.org 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.