On 2016/10/28 10:17, Tom Herbert wrote: > On Thu, Oct 27, 2016 at 6:52 PM, Eli Cooper <elicoo...@gmx.com> wrote: >> > skb->cb may contain data from previous layers. In the observed scenario, >> > the garbage data were misinterpreted as IP6CB(skb)->frag_max_size, so >> > that small packets sent through the tunnel are mistakenly fragmented. >> > >> > This patch clears the control buffer for the next layer, after an IPv6 >> > header is installed. >> > > Nice catch, but can you rectify this with what udp_tunnel6_xmit_skb is > doing. udp_tunnel6_xmit_skb calls ip6tunnel_xmit directly. Looks like > we do > > memset(&(IPCB(skb)->opt), 0, sizeof(IPCB(skb)->opt)); > IPCB(skb)->flags &= ~(IPSKB_XFRM_TUNNEL_SIZE | IPSKB_XFRM_TRANSFORMED > | IPSKB_REROUTED); > > Is this what we should be doing in ip6_tnl_xmit also, or is > udp_tunnel6_xmit_skb broken because it doesn't zero all the cb?
udp_tunnel6_xmit_skb() is also broken, I can confirm. Actually I found it pretty easy to reproduce by having a netns or router forwarding between two tunnels established with another two namespaces or hosts. The router sends out packets with IPv6 Fragment headers, even when the packet is too small to really get fragmented. The control buffer, by definition, is private to the layer having the skb queued at the moment. As David has pointed out in v1 of this patch, cb is either interpreted as IPCB or as IP6CB, and in this case, it will be IP6CB after ip6tunnel_xmit(). So I think it is best that all the IP6CB gets cleared before it is pushed to the next layer. Maybe we should clear IP6CB in ip6tunnel_xmit(), rather than in every tunnel's codes? By the way, I don't see any point in setting IPCB(skb)->flags in udp_tunnel6_xmit_skb(). It will not be interpreted as IPCB any further past ip6tunnel_xmit(), even if it were not cleared. Plus, nothing seems to use these flags anyway. Thanks, Eli
