Re: [PATCH] net: ip/tnl: Set iph->id only when don't fragment is not set

[Eric Dumazet <eric.dumazet@gmail.com>]

On 11/23/19 6:58 AM, Oliver Herms wrote:
> In IPv4 the identification field ensures that fragments of different datagrams
> are not mixed by the receiver. Packets with Don't Fragment (DF) flag set are not
> to be fragmented in transit and thus don't need an identification.

Official sources for this assertion please, so that we can double check if you
implemented the proper avoidance ?

> Calculating the identification takes significant CPU time.
> This patch will increase IP tunneling performance by ~10% unless DF is not set.
> However, DF is set by default which is best practice.
> 
> Signed-off-by: Oliver Herms <oliver.peter.herms@gmail.com>
> ---
>  net/ipv4/ip_tunnel_core.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/net/ipv4/ip_tunnel_core.c b/net/ipv4/ip_tunnel_core.c
> index 1452a97914a0..8636c1e0e7b7 100644
> --- a/net/ipv4/ip_tunnel_core.c
> +++ b/net/ipv4/ip_tunnel_core.c
> @@ -73,7 +73,9 @@ void iptunnel_xmit(struct sock *sk, struct rtable *rt, struct sk_buff *skb,
>  	iph->daddr	=	dst;
>  	iph->saddr	=	src;
>  	iph->ttl	=	ttl;
> -	__ip_select_ident(net, iph, skb_shinfo(skb)->gso_segs ?: 1);
> +
> +	if (unlikely((iph->frag_off & htons(IP_DF)) == false))

This unlikely() seems wrong to me.

You do not know what are the odds of IP_DF being set or not.



> +		__ip_select_ident(net, iph, skb_shinfo(skb)->gso_segs ?: 1);
>  
>  	err = ip_local_out(net, sk, skb);
>  
> 

So we are going to send 2 bytes with garbage if we do not call __ip_select_ident()

This would cause various security threats, since the garbage might reveal a secret.