Re: [1/2,v3] ipv4: Cache dst in tunnels

[Timo Teras <timo.teras@iki.fi>]

On Thu, 2 Jan 2014 11:48:26 -0800 (PST)
Tom Herbert <therbert@google.com> wrote:

> Avoid doing a route lookup on every packet being tunneled.
> 
> In ip_tunnel.c cache the route returned from ip_route_output if
> the tunnel is "connected" so that all the rouitng parameters are
> taken from tunnel parms for a packet. Specifically, not NBMA tunnel
> and tos is from tunnel parms (not inner packet).

Sorry for late reply. The 'connected' check you do is not sufficient.
It breaks NBMA mode gre. See below.

> @@ -528,10 +574,11 @@ void ip_tunnel_xmit(struct sk_buff *skb, struct
> net_device *dev, struct flowi4 fl4;
>  	u8     tos, ttl;
>  	__be16 df;
> -	struct rtable *rt;		/* Route to the other host
> */
> +	struct rtable *rt = NULL;	/* Route to the other host
> */ unsigned int max_headroom;	/* The extra header space needed
> */ __be32 dst;
>  	int err;
> +	bool connected = true;
>  
>  	inner_iph = (const struct iphdr
> *)skb_inner_network_header(skb); 
> @@ -581,27 +628,39 @@ void ip_tunnel_xmit(struct sk_buff *skb, struct
> net_device *dev, #endif
>  		else
>  			goto tx_error;
> +
> +		connected = false;
>  	}

The assumption in these two hunks is wrong. Even if tnl_params->daddr is
set, it might be an NBMA mode tunnel.

This happens (at least) when ipgre_header() is used and it pushes the
modified iphdr to skb. In that case ipgre_xmit just pulls it, and gives
that as the tnl_params argument.

I don't remember fully if tnl_params->daddr can be zero in
ip_tunnel_xmit. It *might* be a leftover dead-code path.

Any ideas how to fix this in a proper manner?

Thanks. Timo.