[PATCH v2] ip6_tunnel: Clear IP6CB in ip6_tnl_xmit() after encapsulation

[PATCH v2] ip6_tunnel: Clear IP6CB in ip6_tnl_xmit() after encapsulation

[Eli Cooper]

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.

Signed-off-by: Eli Cooper <elicooper@gmx.com>
---
v2: clears the whole IP6CB altogether and does it after encapsulation

 net/ipv6/ip6_tunnel.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c
index 202d16a..1487e17 100644
--- a/net/ipv6/ip6_tunnel.c
+++ b/net/ipv6/ip6_tunnel.c
@@ -1174,6 +1174,7 @@ int ip6_tnl_xmit(struct sk_buff *skb, struct net_device *dev, __u8 dsfield,
 
 	skb_push(skb, sizeof(struct ipv6hdr));
 	skb_reset_network_header(skb);
+	memset(skb->cb, 0, sizeof(struct inet6_skb_parm));
 	ipv6h = ipv6_hdr(skb);
 	ip6_flow_hdr(ipv6h, INET_ECN_encapsulate(0, dsfield),
 		     ip6_make_flowlabel(net, skb, fl6->flowlabel, true, fl6));
-- 
2.10.1

Re: [PATCH v2] ip6_tunnel: Clear IP6CB in ip6_tnl_xmit() after encapsulation

[Tom Herbert]

On Thu, Oct 27, 2016 at 6:52 PM, Eli Cooper <elicooper@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?

Thanks,
Tom

> Signed-off-by: Eli Cooper <elicooper@gmx.com>
> ---
> v2: clears the whole IP6CB altogether and does it after encapsulation
>
>  net/ipv6/ip6_tunnel.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c
> index 202d16a..1487e17 100644
> --- a/net/ipv6/ip6_tunnel.c
> +++ b/net/ipv6/ip6_tunnel.c
> @@ -1174,6 +1174,7 @@ int ip6_tnl_xmit(struct sk_buff *skb, struct net_device *dev, __u8 dsfield,
>
>         skb_push(skb, sizeof(struct ipv6hdr));
>         skb_reset_network_header(skb);
> +       memset(skb->cb, 0, sizeof(struct inet6_skb_parm));
>         ipv6h = ipv6_hdr(skb);
>         ip6_flow_hdr(ipv6h, INET_ECN_encapsulate(0, dsfield),
>                      ip6_make_flowlabel(net, skb, fl6->flowlabel, true, fl6));
> --
> 2.10.1
>

Re: [PATCH v2] ip6_tunnel: Clear IP6CB in ip6_tnl_xmit() after encapsulation

[Eli Cooper]

On 2016/10/28 10:17, Tom Herbert wrote:
> On Thu, Oct 27, 2016 at 6:52 PM, Eli Cooper <elicooper@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

Re: [PATCH v2] ip6_tunnel: Clear IP6CB in ip6_tnl_xmit() after encapsulation

[Shmulik Ladkani]

Hi,

On Fri, 28 Oct 2016 13:13:45 +0800 Eli Cooper <elicooper@gmx.com> wrote:
> So I think it is best that all the
> IP6CB gets cleared before it is pushed to the next layer.

Just a comparison to the ipv4 world:

All tunnels (udp/ip based) end up calling iptunnel_xmit(), which:
 - scrubs the skb
 - clears any IPCB residues
 - installs the iphdr
 - invokes ip_local_out()

OTOH ip6_tnl_xmit:
 - scrubs the skb
 - installs the ipv6hdr
 - invokes ip6tunnel_xmit() - a thin wrapper to ip6_local_out()
 * missing: clearing cb

And OTOH udp_tunnel6_xmit_skb:
 - clears IPCB(skb)->opt and some IPCB(skb)->flags
   (why these 2 explicitly? and why at this point? and IPCB is no longer
    relevant...)
 - installs the ipv6hdr
 - invokes ip6tunnel_xmit() - a thin wrapper to ip6_local_out()
 * missing: scrub, clearing cb

> Maybe we
> should clear IP6CB in ip6tunnel_xmit(), rather than in every tunnel's codes?

This seems reasonable.

A potential issue might be whether it needs to be done earlier, although
I've reviewed current versions of both 'ip6_tnl_xmit' and
'udp_tunnel6_xmit_skb' and it looks okay. But please verify.

> 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.

This seems right.

It was introduced in 6a93cc9052 "udp-tunnel: Add a few more UDP tunnel APIs".

If you checkout that tree, you'll notice same treatment to
IPCB(skb)->opt and IPCB(skb)->flags in l2tp_xmit_skb... maybe it was
just copied ;-)

Best,
Shmulik

Re: [PATCH v2] ip6_tunnel: Clear IP6CB in ip6_tnl_xmit() after encapsulation

[David Miller]

From: Shmulik Ladkani <shmulik.ladkani@gmail.com>
Date: Fri, 28 Oct 2016 19:07:57 +0300

> On Fri, 28 Oct 2016 13:13:45 +0800 Eli Cooper <elicooper@gmx.com> wrote:
>> Maybe we
>> should clear IP6CB in ip6tunnel_xmit(), rather than in every tunnel's codes?
> 
> This seems reasonable.
> 
> A potential issue might be whether it needs to be done earlier, although
> I've reviewed current versions of both 'ip6_tnl_xmit' and
> 'udp_tunnel6_xmit_skb' and it looks okay. But please verify.
> 
>> 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.
> 
> This seems right.
> 
> It was introduced in 6a93cc9052 "udp-tunnel: Add a few more UDP tunnel APIs".
> 
> If you checkout that tree, you'll notice same treatment to
> IPCB(skb)->opt and IPCB(skb)->flags in l2tp_xmit_skb... maybe it was
> just copied ;-)

I think for now, we should clear IP6CB in ip6tunnel_xmit()
unconditionally, and remove the IPCB() stuff from the UDP tunneling
code.

If we need some kind of preservation of some of the IP6CB(skb)->flag
bits for whatever reason, we can add that later.  Right now that
code was producing essentially garbage.