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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 24 Apr 2014 15:16:47 +0300
From:	Timo Teras <timo.teras@....fi>
To:	Tom Herbert <therbert@...gle.com>
Cc:	davem@...emloft.net, netdev@...r.kernel.org, eric.dumazet@...il.com
Subject: Re: [1/2,v3] ipv4: Cache dst in tunnels

On Thu, 2 Jan 2014 11:48:26 -0800 (PST)
Tom Herbert <therbert@...gle.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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html