[PATCH] ipv6: Don't unset flowi6_proto in ipxip6_tnl_xmit()

[PATCH] ipv6: Don't unset flowi6_proto in ipxip6_tnl_xmit()

[Eli Cooper]

Commit 8eb30be0352d0916 ("ipv6: Create ip6_tnl_xmit") unsets
flowi6_proto in ip4ip6_tnl_xmit() and ip6ip6_tnl_xmit().
Since xfrm_selector_match() relies on this info, IPv6 packets
sent by an ip6tunnel cannot be properly selected by their
protocols after removing it. This patch puts flowi6_proto back.

Cc: stable@vger.kernel.org
Fixes: 8eb30be0352d ("ipv6: Create ip6_tnl_xmit")
Signed-off-by: Eli Cooper <elicooper@gmx.com>
---
 net/ipv6/ip6_tunnel.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c
index 2050217..5c57797 100644
--- a/net/ipv6/ip6_tunnel.c
+++ b/net/ipv6/ip6_tunnel.c
@@ -1174,6 +1174,7 @@ ip4ip6_tnl_xmit(struct sk_buff *skb, struct net_device *dev)
 		encap_limit = t->parms.encap_limit;
 
 	memcpy(&fl6, &t->fl.u.ip6, sizeof(fl6));
+	fl6.flowi6_proto = IPPROTO_IPIP;
 
 	dsfield = ipv4_get_dsfield(iph);
 
@@ -1233,6 +1234,7 @@ ip6ip6_tnl_xmit(struct sk_buff *skb, struct net_device *dev)
 		encap_limit = t->parms.encap_limit;
 
 	memcpy(&fl6, &t->fl.u.ip6, sizeof(fl6));
+	fl6.flowi6_proto = IPPROTO_IPV6;
 
 	dsfield = ipv6_get_dsfield(ipv6h);
 	if (t->parms.flags & IP6_TNL_F_USE_ORIG_TCLASS)
-- 
2.9.3

Re: [PATCH] ipv6: Don't unset flowi6_proto in ipxip6_tnl_xmit()

[Shmulik Ladkani]

Hi,

On Fri, 26 Aug 2016 23:52:29 +0800 Eli Cooper <elicooper@gmx.com> wrote:
> Commit 8eb30be0352d0916 ("ipv6: Create ip6_tnl_xmit") unsets
> flowi6_proto in ip4ip6_tnl_xmit() and ip6ip6_tnl_xmit().
> Since xfrm_selector_match() relies on this info, IPv6 packets
> sent by an ip6tunnel cannot be properly selected by their
> protocols after removing it. This patch puts flowi6_proto back.
> 
> Cc: stable@vger.kernel.org
> Fixes: 8eb30be0352d ("ipv6: Create ip6_tnl_xmit")
> Signed-off-by: Eli Cooper <elicooper@gmx.com>
> ---
>  net/ipv6/ip6_tunnel.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c
> index 2050217..5c57797 100644
> --- a/net/ipv6/ip6_tunnel.c
> +++ b/net/ipv6/ip6_tunnel.c
> @@ -1174,6 +1174,7 @@ ip4ip6_tnl_xmit(struct sk_buff *skb, struct net_device *dev)
>  		encap_limit = t->parms.encap_limit;
>  
>  	memcpy(&fl6, &t->fl.u.ip6, sizeof(fl6));
> +	fl6.flowi6_proto = IPPROTO_IPIP;
>  
>  	dsfield = ipv4_get_dsfield(iph);
>  
> @@ -1233,6 +1234,7 @@ ip6ip6_tnl_xmit(struct sk_buff *skb, struct net_device *dev)
>  		encap_limit = t->parms.encap_limit;
>  
>  	memcpy(&fl6, &t->fl.u.ip6, sizeof(fl6));
> +	fl6.flowi6_proto = IPPROTO_IPV6;
>  

I wonder if this is the right way to go.

This DOES fix the regression for ip4ip6_tnl_xmit and ip6ip6_tnl_xmit
users.

However new users of 'ip6_tnl_xmit' (e.g. __gre6_xmit callers) may still
have their 'fl6->flowi6_proto' not armed.

Maybe the right fix is assigning 'fl6->flowi6_proto = proto', either at
start of 'ip6_tnl_xmit' or just prior the ip6_route_output() call.

Tom, can you acknowledge?

Thanks,
Shmulik

Re: [PATCH] ipv6: Don't unset flowi6_proto in ipxip6_tnl_xmit()

[Eli Cooper]

Hi,

On 2016/8/27 3:49, Shmulik Ladkani wrote:
> However new users of 'ip6_tnl_xmit' (e.g. __gre6_xmit callers) may still
> have their 'fl6->flowi6_proto' not armed.
>
> Maybe the right fix is assigning 'fl6->flowi6_proto = proto', either at
> start of 'ip6_tnl_xmit' or just prior the ip6_route_output() call.

The only other user of ip6_tnl_xmit() is ip6gre, and it has its
fl6->flowi6_proto set in ip6gre_tnl_link_config() as in commit
252f3f5a1189 ("ip6_gre: Set flowi6_proto as IPPROTO_GRE in xmit path.")

Assigning flowi6_proto at the start of ip6_tnl_xmit() was also my first
idea, but I was unsure which fix would be better.

Thanks,
Eli

Re: [PATCH] ipv6: Don't unset flowi6_proto in ipxip6_tnl_xmit()

[David Miller]

From: Eli Cooper <elicooper@gmx.com>
Date: Fri, 26 Aug 2016 23:52:29 +0800

> @@ -1174,6 +1174,7 @@ ip4ip6_tnl_xmit(struct sk_buff *skb, struct net_device *dev)
>  		encap_limit = t->parms.encap_limit;
>  
>  	memcpy(&fl6, &t->fl.u.ip6, sizeof(fl6));
> +	fl6.flowi6_proto = IPPROTO_IPIP;

Let's just simply have t->fl have the proto setup properly, just like
in GRE.

Assigning it explicitly every packet transmit doesn't make much sense.

Re: [PATCH] ipv6: Don't unset flowi6_proto in ipxip6_tnl_xmit()

[Eli Cooper]

Hello,

On 2016/9/1 4:56, David Miller wrote:
> From: Eli Cooper <elicooper@gmx.com>
> Date: Fri, 26 Aug 2016 23:52:29 +0800
>
>> @@ -1174,6 +1174,7 @@ ip4ip6_tnl_xmit(struct sk_buff *skb, struct net_device *dev)
>>  		encap_limit = t->parms.encap_limit;
>>  
>>  	memcpy(&fl6, &t->fl.u.ip6, sizeof(fl6));
>> +	fl6.flowi6_proto = IPPROTO_IPIP;
> Let's just simply have t->fl have the proto setup properly, just like
> in GRE.
>
> Assigning it explicitly every packet transmit doesn't make much sense.

I doubt that. Unlike GRE, where the proto must be IPPROTO_GRE, the proto
here can be either IPPROTO_IPV6 or IPPROTO_IPIP for a single tunnel, and
t->fl is shared by them. Thus it has to be assigned for every packet.

Thanks,
Eli

Re: [PATCH] ipv6: Don't unset flowi6_proto in ipxip6_tnl_xmit()

[David Miller]

From: Eli Cooper <elicooper@gmx.com>
Date: Thu, 1 Sep 2016 09:18:04 +0800

> Hello,
> 
> On 2016/9/1 4:56, David Miller wrote:
>> From: Eli Cooper <elicooper@gmx.com>
>> Date: Fri, 26 Aug 2016 23:52:29 +0800
>>
>>> @@ -1174,6 +1174,7 @@ ip4ip6_tnl_xmit(struct sk_buff *skb, struct net_device *dev)
>>>  		encap_limit = t->parms.encap_limit;
>>>  
>>>  	memcpy(&fl6, &t->fl.u.ip6, sizeof(fl6));
>>> +	fl6.flowi6_proto = IPPROTO_IPIP;
>> Let's just simply have t->fl have the proto setup properly, just like
>> in GRE.
>>
>> Assigning it explicitly every packet transmit doesn't make much sense.
> 
> I doubt that. Unlike GRE, where the proto must be IPPROTO_GRE, the proto
> here can be either IPPROTO_IPV6 or IPPROTO_IPIP for a single tunnel, and
> t->fl is shared by them. Thus it has to be assigned for every packet.

You're right.  Patch applied, thanks for explaining.