Netdev Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH net] ip6_tunnel: set inner ipproto before ip6_tnl_encap.
@ 2020-10-16 11:11 Alexander Ovechkin
  2020-10-16 17:55 ` Willem de Bruijn
  0 siblings, 1 reply; 5+ messages in thread
From: Alexander Ovechkin @ 2020-10-16 11:11 UTC (permalink / raw)
  To: netdev

ip6_tnl_encap assigns to proto transport protocol which
encapsulates inner packet, but we must pass to set_inner_ipproto
protocol of that inner packet.

Calling set_inner_ipproto after ip6_tnl_encap might break gso.
For example, in case of encapsulating ipv6 packet in fou6 packet, inner_ipproto 
would be set to IPPROTO_UDP instead of IPPROTO_IPV6. This would lead to
incorrect calling sequence of gso functions:
ipv6_gso_segment -> udp6_ufo_fragment -> skb_udp_tunnel_segment -> udp6_ufo_fragment
instead of:
ipv6_gso_segment -> udp6_ufo_fragment -> skb_udp_tunnel_segment -> ip6ip6_gso_segment

Signed-off-by: Alexander Ovechkin <ovov@yandex-team.ru>
---
 net/ipv6/ip6_tunnel.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c
index a0217e5bf3bc..648db3fe508f 100644
--- a/net/ipv6/ip6_tunnel.c
+++ b/net/ipv6/ip6_tunnel.c
@@ -1271,6 +1271,8 @@ int ip6_tnl_xmit(struct sk_buff *skb, struct net_device *dev, __u8 dsfield,
 	if (max_headroom > dev->needed_headroom)
 		dev->needed_headroom = max_headroom;
 
+	skb_set_inner_ipproto(skb, proto);
+
 	err = ip6_tnl_encap(skb, t, &proto, fl6);
 	if (err)
 		return err;
@@ -1280,8 +1282,6 @@ int ip6_tnl_xmit(struct sk_buff *skb, struct net_device *dev, __u8 dsfield,
 		ipv6_push_frag_opts(skb, &opt.ops, &proto);
 	}
 
-	skb_set_inner_ipproto(skb, proto);
-
 	skb_push(skb, sizeof(struct ipv6hdr));
 	skb_reset_network_header(skb);
 	ipv6h = ipv6_hdr(skb);
-- 
2.17.1


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH net] ip6_tunnel: set inner ipproto before ip6_tnl_encap.
  2020-10-16 11:11 [PATCH net] ip6_tunnel: set inner ipproto before ip6_tnl_encap Alexander Ovechkin
@ 2020-10-16 17:55 ` Willem de Bruijn
  2020-10-17  0:59   ` Vadim Fedorenko
  2020-10-27 21:44   ` Alexander Ovechkin
  0 siblings, 2 replies; 5+ messages in thread
From: Willem de Bruijn @ 2020-10-16 17:55 UTC (permalink / raw)
  To: Alexander Ovechkin; +Cc: Network Development, vfedorenko

On Fri, Oct 16, 2020 at 7:14 AM Alexander Ovechkin <ovov@yandex-team.ru> wrote:
>
> ip6_tnl_encap assigns to proto transport protocol which
> encapsulates inner packet, but we must pass to set_inner_ipproto
> protocol of that inner packet.
>
> Calling set_inner_ipproto after ip6_tnl_encap might break gso.
> For example, in case of encapsulating ipv6 packet in fou6 packet, inner_ipproto
> would be set to IPPROTO_UDP instead of IPPROTO_IPV6. This would lead to
> incorrect calling sequence of gso functions:
> ipv6_gso_segment -> udp6_ufo_fragment -> skb_udp_tunnel_segment -> udp6_ufo_fragment
> instead of:
> ipv6_gso_segment -> udp6_ufo_fragment -> skb_udp_tunnel_segment -> ip6ip6_gso_segment
>
> Signed-off-by: Alexander Ovechkin <ovov@yandex-team.ru>

Commit 6c11fbf97e69 ("ip6_tunnel: add MPLS transmit support") moved
the call from ip6_tnl_encap's caller to inside ip6_tnl_encap.

It makes sense that that likely broke this behavior for UDP (L4) tunnels.

But it was moved on purpose to avoid setting the inner protocol to
IPPROTO_MPLS. That needs to use skb->inner_protocol to further
segment.

I suspect we need to set this before or after conditionally to avoid
breaking that use case.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH net] ip6_tunnel: set inner ipproto before ip6_tnl_encap.
  2020-10-16 17:55 ` Willem de Bruijn
@ 2020-10-17  0:59   ` Vadim Fedorenko
  2020-10-27 21:45     ` Alexander Ovechkin
  2020-10-27 21:44   ` Alexander Ovechkin
  1 sibling, 1 reply; 5+ messages in thread
From: Vadim Fedorenko @ 2020-10-17  0:59 UTC (permalink / raw)
  To: Willem de Bruijn, Alexander Ovechkin; +Cc: Network Development

On 16.10.2020 18:55, Willem de Bruijn wrote:
> On Fri, Oct 16, 2020 at 7:14 AM Alexander Ovechkin <ovov@yandex-team.ru> wrote:
>> ip6_tnl_encap assigns to proto transport protocol which
>> encapsulates inner packet, but we must pass to set_inner_ipproto
>> protocol of that inner packet.
>>
>> Calling set_inner_ipproto after ip6_tnl_encap might break gso.
>> For example, in case of encapsulating ipv6 packet in fou6 packet, inner_ipproto
>> would be set to IPPROTO_UDP instead of IPPROTO_IPV6. This would lead to
>> incorrect calling sequence of gso functions:
>> ipv6_gso_segment -> udp6_ufo_fragment -> skb_udp_tunnel_segment -> udp6_ufo_fragment
>> instead of:
>> ipv6_gso_segment -> udp6_ufo_fragment -> skb_udp_tunnel_segment -> ip6ip6_gso_segment
>>
>> Signed-off-by: Alexander Ovechkin <ovov@yandex-team.ru>
> Commit 6c11fbf97e69 ("ip6_tunnel: add MPLS transmit support") moved
> the call from ip6_tnl_encap's caller to inside ip6_tnl_encap.
>
> It makes sense that that likely broke this behavior for UDP (L4) tunnels.
>
> But it was moved on purpose to avoid setting the inner protocol to
> IPPROTO_MPLS. That needs to use skb->inner_protocol to further
> segment.
>
> I suspect we need to set this before or after conditionally to avoid
> breaking that use case.
I hope it could be fixed with something like this:

diff --git a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c
index a0217e5..87368b0 100644
--- a/net/ipv6/ip6_tunnel.c
+++ b/net/ipv6/ip6_tunnel.c
@@ -1121,6 +1121,7 @@ int ip6_tnl_xmit(struct sk_buff *skb, struct net_device 
*dev, __u8 dsfield,
         bool use_cache = false;
         u8 hop_limit;
         int err = -1;
+       __u8 pproto = proto;

         if (t->parms.collect_md) {
                 hop_limit = skb_tunnel_info(skb)->key.ttl;
@@ -1280,7 +1281,7 @@ int ip6_tnl_xmit(struct sk_buff *skb, struct net_device 
*dev, __u8 dsfield,
                 ipv6_push_frag_opts(skb, &opt.ops, &proto);
         }

-       skb_set_inner_ipproto(skb, proto);
+       skb_set_inner_ipproto(skb, pproto == IPPROTO_MPLS ? proto : pproto);

         skb_push(skb, sizeof(struct ipv6hdr));
         skb_reset_network_header(skb);


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH net] ip6_tunnel: set inner ipproto before ip6_tnl_encap.
  2020-10-16 17:55 ` Willem de Bruijn
  2020-10-17  0:59   ` Vadim Fedorenko
@ 2020-10-27 21:44   ` Alexander Ovechkin
  1 sibling, 0 replies; 5+ messages in thread
From: Alexander Ovechkin @ 2020-10-27 21:44 UTC (permalink / raw)
  To: Willem de Bruijn, vfedorenko; +Cc: netdev

> But it was moved on purpose to avoid setting the inner protocol to IPPROTO_MPLS. That needs to use skb->inner_protocol to further segment.
And why do we need to avoid setting the inner protocol to IPPROTO_MPLS? Currently skb->inner_protocol is used before call of ip6_tnl_xmit.
Can you please give example when this patch breaks MPLS segmentation?

> On 16 Oct 2020, at 20:55, Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote:
> 
> On Fri, Oct 16, 2020 at 7:14 AM Alexander Ovechkin <ovov@yandex-team.ru> wrote:
>> 
>> ip6_tnl_encap assigns to proto transport protocol which
>> encapsulates inner packet, but we must pass to set_inner_ipproto
>> protocol of that inner packet.
>> 
>> Calling set_inner_ipproto after ip6_tnl_encap might break gso.
>> For example, in case of encapsulating ipv6 packet in fou6 packet, inner_ipproto
>> would be set to IPPROTO_UDP instead of IPPROTO_IPV6. This would lead to
>> incorrect calling sequence of gso functions:
>> ipv6_gso_segment -> udp6_ufo_fragment -> skb_udp_tunnel_segment -> udp6_ufo_fragment
>> instead of:
>> ipv6_gso_segment -> udp6_ufo_fragment -> skb_udp_tunnel_segment -> ip6ip6_gso_segment
>> 
>> Signed-off-by: Alexander Ovechkin <ovov@yandex-team.ru>
> 
> Commit 6c11fbf97e69 ("ip6_tunnel: add MPLS transmit support") moved
> the call from ip6_tnl_encap's caller to inside ip6_tnl_encap.
> 
> It makes sense that that likely broke this behavior for UDP (L4) tunnels.
> 
> But it was moved on purpose to avoid setting the inner protocol to
> IPPROTO_MPLS. That needs to use skb->inner_protocol to further
> segment.
> 
> I suspect we need to set this before or after conditionally to avoid
> breaking that use case.


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH net] ip6_tunnel: set inner ipproto before ip6_tnl_encap.
  2020-10-17  0:59   ` Vadim Fedorenko
@ 2020-10-27 21:45     ` Alexander Ovechkin
  0 siblings, 0 replies; 5+ messages in thread
From: Alexander Ovechkin @ 2020-10-27 21:45 UTC (permalink / raw)
  To: Vadim Fedorenko; +Cc: Willem de Bruijn, Network Development

But ip6_tnl_encap assigns to proto number of outer protocol (i.e. =
protocol that encapsulates our original packet). Setting inner_ipproto =
to this value makes no sense.=20

For example in case of ipv6 inside MPLS inside fou6 encapsulation we =
have following packet structure:
+--------------+ <---+
|    ipv6      |     |
+--------------+     +- fou6-encap
|     udp      |     |
+--------------+ <---+
|     mpls     | <---   mpls-enacp
+--------------+ <---+
|  inner-ipv6  |     |
+--------------+     +- original packet
|     ...      |     |
+--------------+ <---+
After ip6_tnl_encap proto will be equal to IPPROTO_UDP, that is =
obviously is not inner ipproto.

Actually if pproto =3D=3D IPPROTO_MPLS than we have two layers of =
encapsulation and it is meaningless to set inner ipproto, cause =
currently there is no support for segmentation of packets with two =
layers of encapsulation.


> On 17 Oct 2020, at 03:59, Vadim Fedorenko <vfedorenko@novek.ru> wrote:
> 
> On 16.10.2020 18:55, Willem de Bruijn wrote:
>> On Fri, Oct 16, 2020 at 7:14 AM Alexander Ovechkin <ovov@yandex-team.ru> wrote:
>>> ip6_tnl_encap assigns to proto transport protocol which
>>> encapsulates inner packet, but we must pass to set_inner_ipproto
>>> protocol of that inner packet.
>>> 
>>> Calling set_inner_ipproto after ip6_tnl_encap might break gso.
>>> For example, in case of encapsulating ipv6 packet in fou6 packet, inner_ipproto
>>> would be set to IPPROTO_UDP instead of IPPROTO_IPV6. This would lead to
>>> incorrect calling sequence of gso functions:
>>> ipv6_gso_segment -> udp6_ufo_fragment -> skb_udp_tunnel_segment -> udp6_ufo_fragment
>>> instead of:
>>> ipv6_gso_segment -> udp6_ufo_fragment -> skb_udp_tunnel_segment -> ip6ip6_gso_segment
>>> 
>>> Signed-off-by: Alexander Ovechkin <ovov@yandex-team.ru>
>> Commit 6c11fbf97e69 ("ip6_tunnel: add MPLS transmit support") moved
>> the call from ip6_tnl_encap's caller to inside ip6_tnl_encap.
>> 
>> It makes sense that that likely broke this behavior for UDP (L4) tunnels.
>> 
>> But it was moved on purpose to avoid setting the inner protocol to
>> IPPROTO_MPLS. That needs to use skb->inner_protocol to further
>> segment.
>> 
>> I suspect we need to set this before or after conditionally to avoid
>> breaking that use case.
> I hope it could be fixed with something like this:
> 
> diff --git a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c
> index a0217e5..87368b0 100644
> --- a/net/ipv6/ip6_tunnel.c
> +++ b/net/ipv6/ip6_tunnel.c
> @@ -1121,6 +1121,7 @@ int ip6_tnl_xmit(struct sk_buff *skb, struct net_device *dev, __u8 dsfield,
>         bool use_cache = false;
>         u8 hop_limit;
>         int err = -1;
> +       __u8 pproto = proto;
> 
>         if (t->parms.collect_md) {
>                 hop_limit = skb_tunnel_info(skb)->key.ttl;
> @@ -1280,7 +1281,7 @@ int ip6_tnl_xmit(struct sk_buff *skb, struct net_device *dev, __u8 dsfield,
>                 ipv6_push_frag_opts(skb, &opt.ops, &proto);
>         }
> 
> -       skb_set_inner_ipproto(skb, proto);
> +       skb_set_inner_ipproto(skb, pproto == IPPROTO_MPLS ? proto : pproto);
> 
>         skb_push(skb, sizeof(struct ipv6hdr));
>         skb_reset_network_header(skb);
> 


^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-16 11:11 [PATCH net] ip6_tunnel: set inner ipproto before ip6_tnl_encap Alexander Ovechkin
2020-10-16 17:55 ` Willem de Bruijn
2020-10-17  0:59   ` Vadim Fedorenko
2020-10-27 21:45     ` Alexander Ovechkin
2020-10-27 21:44   ` Alexander Ovechkin

Netdev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/netdev/0 netdev/git/0.git
	git clone --mirror https://lore.kernel.org/netdev/1 netdev/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 netdev netdev/ https://lore.kernel.org/netdev \
		netdev@vger.kernel.org
	public-inbox-index netdev

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.netdev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git