[PATCH 0/2 net-next] geneve: fix TOS inheriting

[PATCH 0/2 net-next] geneve: fix TOS inheriting

[Matthias May]

Currently when the TOS of an encapsulated frame is inherited,
the 6 DSCP bits are cut down to 3 original TOS bits.
Compare to other L2 tunneling protocols (gretap, vxlan) this
is unexpected.
IPv4 and IPv6 have both this behaviour but for different reasons.

For IPv4 the bits are lost in the routing table lookup.
The patch copies the full tos out before the lookup and uses the copy.

For IPv6 the RT_TOS macro cuts off the 3 bits.
I'm not really familiar with the IPv6 code, but to me it seems as
if this part of the code only uses the TOS for the flowlabel.
Is there any reason why the flowlabel should be restricted to these
3 bits? The patch simply removes the usage of this macro, but i don't
know if there was a specific intention behind that. I can't find any
immediate breakage, but then again my IPv6 testing is fairly limited.

Matthias May (2):
  geneve: fix TOS inheriting for ipv4
  geneve: fix TOS inheriting for ipv6

 drivers/net/geneve.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

-- 
2.35.1

[PATCH 1/2 net-next] geneve: fix TOS inheriting for ipv4

[Matthias May]

The current code retrieves the TOS field after the lookup
on the ipv4 routing table. The routing process currently
only allows routing based on the original 3 TOS bits, and
not on the full 6 DSCP bits.
As a result the retrieved TOS is cut to the 3 bits.
However for inheriting purposes the full 6 bits should be used.

Extract the full 6 bits before the route lookup and use
that instead of the cut off 3 TOS bits.

Signed-off-by: Matthias May <matthias.may@westermo.com>
---
 drivers/net/geneve.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
index 2495a5719e1c..4c380c06f178 100644
--- a/drivers/net/geneve.c
+++ b/drivers/net/geneve.c
@@ -797,7 +797,8 @@ static struct rtable *geneve_get_v4_rt(struct sk_buff *skb,
 				       struct geneve_sock *gs4,
 				       struct flowi4 *fl4,
 				       const struct ip_tunnel_info *info,
-				       __be16 dport, __be16 sport)
+				       __be16 dport, __be16 sport,
+				       __u8 *full_tos)
 {
 	bool use_cache = ip_tunnel_dst_cache_usable(skb, info);
 	struct geneve_dev *geneve = netdev_priv(dev);
@@ -822,6 +823,7 @@ static struct rtable *geneve_get_v4_rt(struct sk_buff *skb,
 		use_cache = false;
 	}
 	fl4->flowi4_tos = RT_TOS(tos);
+	*full_tos = tos;
 
 	dst_cache = (struct dst_cache *)&info->dst_cache;
 	if (use_cache) {
@@ -910,6 +912,7 @@ static int geneve_xmit_skb(struct sk_buff *skb, struct net_device *dev,
 	const struct ip_tunnel_key *key = &info->key;
 	struct rtable *rt;
 	struct flowi4 fl4;
+	__u8 full_tos;
 	__u8 tos, ttl;
 	__be16 df = 0;
 	__be16 sport;
@@ -920,7 +923,7 @@ static int geneve_xmit_skb(struct sk_buff *skb, struct net_device *dev,
 
 	sport = udp_flow_src_port(geneve->net, skb, 1, USHRT_MAX, true);
 	rt = geneve_get_v4_rt(skb, dev, gs4, &fl4, info,
-			      geneve->cfg.info.key.tp_dst, sport);
+			      geneve->cfg.info.key.tp_dst, sport, &full_tos);
 	if (IS_ERR(rt))
 		return PTR_ERR(rt);
 
@@ -964,7 +967,7 @@ static int geneve_xmit_skb(struct sk_buff *skb, struct net_device *dev,
 
 		df = key->tun_flags & TUNNEL_DONT_FRAGMENT ? htons(IP_DF) : 0;
 	} else {
-		tos = ip_tunnel_ecn_encap(fl4.flowi4_tos, ip_hdr(skb), skb);
+		tos = ip_tunnel_ecn_encap(full_tos, ip_hdr(skb), skb);
 		if (geneve->cfg.ttl_inherit)
 			ttl = ip_tunnel_get_ttl(ip_hdr(skb), skb);
 		else
@@ -1137,6 +1140,7 @@ static int geneve_fill_metadata_dst(struct net_device *dev, struct sk_buff *skb)
 {
 	struct ip_tunnel_info *info = skb_tunnel_info(skb);
 	struct geneve_dev *geneve = netdev_priv(dev);
+	__u8 full_tos;
 	__be16 sport;
 
 	if (ip_tunnel_info_af(info) == AF_INET) {
@@ -1148,7 +1152,8 @@ static int geneve_fill_metadata_dst(struct net_device *dev, struct sk_buff *skb)
 					  1, USHRT_MAX, true);
 
 		rt = geneve_get_v4_rt(skb, dev, gs4, &fl4, info,
-				      geneve->cfg.info.key.tp_dst, sport);
+				      geneve->cfg.info.key.tp_dst, sport,
+				      &full_tos);
 		if (IS_ERR(rt))
 			return PTR_ERR(rt);
 
-- 
2.35.1

[PATCH 2/2 net-next] geneve: fix TOS inheriting for ipv6

[Matthias May]

The current code uses the RT_TOS macro to cut off the 6 DSCP
bits, down to the original 3 TOS bits.

Do not use this macro to get the prio for inheriting purposes.

Signed-off-by: Matthias May <matthias.may@westermo.com>
---
 drivers/net/geneve.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
index 4c380c06f178..e1a4480e6f17 100644
--- a/drivers/net/geneve.c
+++ b/drivers/net/geneve.c
@@ -877,8 +877,7 @@ static struct dst_entry *geneve_get_v6_dst(struct sk_buff *skb,
 		use_cache = false;
 	}
 
-	fl6->flowlabel = ip6_make_flowinfo(RT_TOS(prio),
-					   info->key.label);
+	fl6->flowlabel = ip6_make_flowinfo(prio, info->key.label);
 	dst_cache = (struct dst_cache *)&info->dst_cache;
 	if (use_cache) {
 		dst = dst_cache_get_ip6(dst_cache, &fl6->saddr);
-- 
2.35.1

Re: [PATCH 2/2 net-next] geneve: fix TOS inheriting for ipv6

[Guillaume Nault]

On Sun, Jul 24, 2022 at 02:37:41AM +0200, Matthias May wrote:
> The current code uses the RT_TOS macro to cut off the 6 DSCP
> bits, down to the original 3 TOS bits.
> 
> Do not use this macro to get the prio for inheriting purposes.

Honestly, this patch is a bug fix and is suitable for the net tree
(with appropriate 'Fixes' tag).

Ideally, we'd also fix ip6_dst_lookup_tunnel() (used by bareudp
tunnels) and vxlan6_get_route().

Also, mlx5e_tc_tun_update_header_ipv6() and
mlx5e_tc_tun_create_header_ipv6() both call RT_TOS() inside
ip6_make_flowinfo() and certainly need to be fixed too.

> Signed-off-by: Matthias May <matthias.may@westermo.com>
> ---
>  drivers/net/geneve.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
> index 4c380c06f178..e1a4480e6f17 100644
> --- a/drivers/net/geneve.c
> +++ b/drivers/net/geneve.c
> @@ -877,8 +877,7 @@ static struct dst_entry *geneve_get_v6_dst(struct sk_buff *skb,
>  		use_cache = false;
>  	}
>  
> -	fl6->flowlabel = ip6_make_flowinfo(RT_TOS(prio),
> -					   info->key.label);
> +	fl6->flowlabel = ip6_make_flowinfo(prio, info->key.label);
>  	dst_cache = (struct dst_cache *)&info->dst_cache;
>  	if (use_cache) {
>  		dst = dst_cache_get_ip6(dst_cache, &fl6->saddr);
> -- 
> 2.35.1
> 

Re: [PATCH 2/2 net-next] geneve: fix TOS inheriting for ipv6

[Matthias May]

[-- Attachment #1.1: Type: text/plain, Size: 1019 bytes --]

On 25/07/2022 19:05, Guillaume Nault wrote:
> On Sun, Jul 24, 2022 at 02:37:41AM +0200, Matthias May wrote:
>> The current code uses the RT_TOS macro to cut off the 6 DSCP
>> bits, down to the original 3 TOS bits.
>>
>> Do not use this macro to get the prio for inheriting purposes.
> 
> Honestly, this patch is a bug fix and is suitable for the net tree
> (with appropriate 'Fixes' tag).
> 
> Ideally, we'd also fix ip6_dst_lookup_tunnel() (used by bareudp
> tunnels) and vxlan6_get_route().
> 
> Also, mlx5e_tc_tun_update_header_ipv6() and
> mlx5e_tc_tun_create_header_ipv6() both call RT_TOS() inside
> ip6_make_flowinfo() and certainly need to be fixed too.
> 

Hi Guillaume
How would i do that?
Send a v2 to net with the fixes tag on 95caf6f71a999?
Or just resend to net with the fixes tag on 95caf6f71a999?
Since there are no actual changes to the patch.
This kind of contradicts the statement that IPv4 and IPv6 should behave the same.
--> v6 would be fixed, but v4 not.

BR
Matthias

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

Re: [PATCH 2/2 net-next] geneve: fix TOS inheriting for ipv6

[Guillaume Nault]

On Tue, Jul 26, 2022 at 06:29:56PM +0200, Matthias May wrote:
> On 25/07/2022 19:05, Guillaume Nault wrote:
> > On Sun, Jul 24, 2022 at 02:37:41AM +0200, Matthias May wrote:
> > > The current code uses the RT_TOS macro to cut off the 6 DSCP
> > > bits, down to the original 3 TOS bits.
> > > 
> > > Do not use this macro to get the prio for inheriting purposes.
> > 
> > Honestly, this patch is a bug fix and is suitable for the net tree
> > (with appropriate 'Fixes' tag).
> > 
> > Ideally, we'd also fix ip6_dst_lookup_tunnel() (used by bareudp
> > tunnels) and vxlan6_get_route().
> > 
> > Also, mlx5e_tc_tun_update_header_ipv6() and
> > mlx5e_tc_tun_create_header_ipv6() both call RT_TOS() inside
> > ip6_make_flowinfo() and certainly need to be fixed too.
> > 
> 
> Hi Guillaume
> How would i do that?
> Send a v2 to net with the fixes tag on 95caf6f71a999?
> Or just resend to net with the fixes tag on 95caf6f71a999?
> Since there are no actual changes to the patch.

Hi Matthias,

Ideally, send a patch series to net that'd removes RT_TOS() from the
ip6_make_flowinfo() calls in geneve, vxlan and bareudp (one patch for
each protocol, with the appropriate Fixes tag). You can add the IPv4
patch in that series or send it separately, as you see fit.

Alternatively you can just repost this series to net, with a proper
Fixes tag for each patch (and I'll take care of vxlan and bareudp in
a future series).

> This kind of contradicts the statement that IPv4 and IPv6 should behave the same.
> --> v6 would be fixed, but v4 not.

I personally consider the current IPv4 behaviour for TOS inherit option
to be a bug, so, in this case, we can have both IPv4 and IPv6 fixed in
the same tree.

But generally speaking, we have some divergence in how IPv4 and IPv6
treat tos/dsfield. That's because of some historical reasons and it's
not easy to reconciliate both implementations (because of backward
compatibility).

> BR
> Matthias

Re: [PATCH 0/2 net-next] geneve: fix TOS inheriting

[Guillaume Nault]

On Sun, Jul 24, 2022 at 02:37:39AM +0200, Matthias May wrote:
> Is there any reason why the flowlabel should be restricted to these
> 3 bits?

It's a bug, likely a copy/paste mistake.

> The patch simply removes the usage of this macro, but i don't
> know if there was a specific intention behind that.

RT_TOS() is an old macro used to interprete IPv4 TOS as described in
the obsolete RFC 1349. It's conceptually wrong to use it even in IPv4
code, although, given the current state of the code, most of the
existing calls have no consequence.

But using RT_TOS() in IPv6 code is always a bug: IPv6 never had a "TOS"
field to be interpreted the RFC 1349 way. There's no historical
compatibility to worry about.