inet: Separate DSCP from ECN bits using new dscp_t type

Following my talk at LPC 2021 [1], here's a patch series whose objective is to start fixing the problems with how DSCP and ECN bits are handled in the kernel. This approach seemed to make consensus among the participants, although it implies a few behaviour changes for some corner cases of ip rule and ip route. Let's see if this consensus can survive a wider review :).

The problem to solve
====================

Currently, the networking stack generally doesn't clearly distinguish between DSCP and ECN bits. The entire DSCP+ECN bits are stored in a single u8 variable (or structure field), and each part of the stack handles them in their own way, using different macros.

This has been, and still is, the source of several bugs:

* Invalid use of RT_TOS(), leading to regression in VXLAN:
- a0dced17ad9d ("Revert "vxlan: fix tos value before xmit"")

* Other invalid uses of RT_TOS() in IPv6 code, where it doesn't make sense (RT_TOS() follows the old RFC 1349, which was never meant for IPv6):
- grep for 'ip6_make_flowinfo(RT_TOS(tos)' for several examples that need to be fixed.

* Failure to properly mask the ECN bits before doing IPv4 route
lookups, leading to returning a wrong route:
- 2e5a6266fbb1 ("netfilter: rpfilter: mask ecn bits before fib lookup"),
- 8d2b51b008c2 ("udp: mask TOS bits in udp_v4_early_demux()"),
- 21fdca22eb7d ("ipv4: Ignore ECN bits for fib lookups in fib_compute_spec_dst()"),
- 1ebf179037cb ("ipv4: Fix tos mask in inet_rtm_getroute()"),
- some more, uncommon, code paths still need to be fixed.

Also, this creates inconsistencies between subsystems:

* IPv4 routes accept tos/dsfield options that have ECN bits set, but no packet can actually match them, as their ECN bits are cleared before lookup.

* IPv4 rules accept tos/dsfield options that have the high order ECN bit set (but rejects those using the low order ECN bit). Like IPv4 routes, such rules can't match actual packets as the rule lookup is done after clearing the packets ECN bits.

* IPv6 routes accept the tos/dsfield options without any restriction, but silently ignores them.

* IPv6 rules also accept any value of tos/dsfield, but compares all 8 bits, including ECN. Therefore, a rule matching packets with a particular DSCP value stops working as soon as ECN is used (a work around is to define 4 rules for each DSCP value to match, one for each possible ECN code point).

* Modules that want to distinguish between the DSCP and ECN bits create their own local macros (Netfilter, SCTP).

What this RFC does
==================

This patch series creates a dscp_t type, meant to represent pure DSCP values, excluding ECN bits. Doing so allows to clearly separate the interpretation of DSCP and ECN bits and enables Sparse to detect invalid combinations of dscp_t with the plain u8 variables generally used to store the full TOS/Traffic Class/DS field.

Note, this patch series differs slightly from that of the original talk (slide 14 [2]). For the talk, I just cleared the ECN bits, while in this series, I do a bit-shift. This way dscp_t really represents DSCP values, as defined in RFCs. Also I've renamed the helper functions to replace "u8" by "dsfield", as I felt "u8" was ambiguous. Using "dsfield" makes it clear that dscp_t to u8 conversion isn't just a plain cast, but that a bit-shift happens and the result has the two ECN bits.

The new dscp_t type is then used to convert several field members:

* Patch 1 converts the tclass field of struct fib6_rule. It effectively forbids the use of ECN bits in the tos/dsfield option of ip -6 rule. Rules now match packets solely based on their DSCP bits, so ECN doesn't influence the result anymore. This contrasts with previous behaviour where all 8 bits of the Traffic Class field was used. It is believed this change is acceptable as matching ECN bits wasn't usable for IPv4, so only IPv6-only deployments could be depending on it (that is, it's unlikely enough that a someone uses ip6 rules matching ECN bits in production).

* Patch 2 converts the tos field of struct fib4_rule. This one too effectively forbids defining ECN bits, this time in ip -4 rule. Before that, setting ECN bit 1 was accepted, while ECN bit 0 was rejected. But even when accepted, the rule wouldn't match as the packets would normally have their ECN bits cleared while doing the rule lookup.

* Patch 3 converts the fc_tos field of struct fib_config. This is like patch 2, but for ip4 routes. Routes using a tos/dsfield option with any ECN bit set is now rejected. Before this patch, they were accepted but, as with ip4 rules, these routes couldn't match any real packet, since callers were supposed to clear their ECN bits beforehand.

* Patch 4 converts the fa_tos field of struct fib_alias. This one is pure internal u8 to dscp_t conversion. While patches 1-3 dealed with user facing consequences, this patch shouldn't have any side effect and is just there to give an overview of what such conversion patches will look like. These are quite mechanical, but imply some code churn.

Note that there's no equivalent of patch 3 for IPv6 (ip route), since the tos/dsfield option is silently ignored for IPv6 routes.

Future work
===========

This is a minimal series, the objective of this RFC is just to validate the dscp_t approach. More work will be needed to fully iron out the problem:

* Convert more internal structures to dscp_t (in particular the flowi4_tos field of struct flowi4).

* Slowly remove the uses of IPTOS_TOS_MASK, and hence RT_TOS(), in the kernel, as it clears only one of the ECN bits and the TOS is going to be masked again anyway by IPTOS_RT_MASK, which is stricter.

* Finally, start allowing high DSCP values in IPv4 routes and rules (the IPTOS_RT_MASK used in IPv4 clears the 3 high order bits of the DS field).

* Maybe find a way to warn users that use the ignored tos/dsfield option with ip -6 route.

Feedbacks welcome!

Thanks,

William

[1] LPC talk: https://linuxplumbersconf.org/event/11/contributions/943/
[2] LPC slides: https://linuxplumbersconf.org/event/11/contributions/943/attachments/901/1780/inet_tos_lpc2021.pdf

Guillaume Nault (4):
ipv6: Define dscp_t and stop taking ECN bits into account in ip6-rules
ipv4: Stop taking ECN bits into account in ip4-rules
ipv4: Reject routes specifying ECN bits in rtm_tos
ipv4: Use dscp_t in struct fib_alias

 include/net/inet_dscp.h  | 54 +++++++++++++++++++++++++++++++++++++
 include/net/ip_fib.h     |  3 ++-
 include/net/ipv6.h       |  6 +++++
 net/ipv4/fib_frontend.c  | 10 ++++++-
 net/ipv4/fib_lookup.h    |  3 ++-
 net/ipv4/fib_rules.c     | 17 ++++++------
 net/ipv4/fib_semantics.c | 14 +++++-----
 net/ipv4/fib_trie.c      | 58 +++++++++++++++++++++++-----------------
 net/ipv4/route.c         |  3 ++-
 net/ipv6/fib6_rules.c    | 18 ++++++++-----
 10 files changed, 138 insertions(+), 48 deletions(-)
 create mode 100644 include/net/inet_dscp.h

2.21.3

Define a dscp_t type and its appropriate helpers that ensure ECN bits are not taken into account when handling DSCP.

Use this new type to replace the tclass field of fib6_rule, so that ip6-rules don't get influenced by ECN bits anymore.

Before this patch, ip6-rules didn't make any distinction between the DSCP and ECN bits. Therefore, rules specifying a DSCP (tos or dsfield options in iproute2) stopped working as soon a packets had at least one of its ECN bits set (as a work around one could create four rules for each DSCP value to match, one for each possible ECN value).

After this patch ip6-rules only compare the DSCP bits. ECN doesn't influence the result anymore. Also, ip6-rules now must have the ECN bits cleared or they will be rejected.

---
 include/net/inet_dscp.h | 54 +++++++++++++++++++++++++++++++++++++++++
 include/net/ipv6.h      |  6 +++++
 net/ipv6/fib6_rules.c   | 18 +++++++++-----
 3 files changed, 72 insertions(+), 6 deletions(-)
 create mode 100644 include/net/inet_dscp.h

new file mode 100644
index 000000000000..e7f7e182529c
@@ -0,0 +1,54 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * inet_dscp.h: helpers for handling differentiated services codepoints (DSCP)
+ *
+ * DSCP is defined in RFC 2474:
+ *
+ *        0   1   2   3   4   5   6   7
+ *      +---+---+---+---+---+---+---+---+
+ *      |         DSCP          |  CU   |
+ *      +---+---+---+---+---+---+---+---+
+ *
+ *        DSCP: differentiated services codepoint
+ *        CU:   currently unused
+ *
+ * The whole DSCP + CU bits form the DS field.
+ * The DS field is also commonly called TOS or Traffic Class (for IPv6).
+ *
+ * Note: the CU bits are now used for Explicit Congestion Notification
+ *       (RFC 3168).
+ */
+
+#ifndef _INET_DSCP_H
+#define _INET_DSCP_H
+
+#include <linux/types.h>
+
+/* Special type for storing DSCP values.
+ *
+ * A dscp_t variable stores a DSCP value, without the CU (or ECN) bits.
+ * Using dscp_t allows to strictly separate DSCP and ECN bits, thus avoid bugs
+ * where ECN bits are erroneously taken into account during FIB lookups or
+ * policy routing.
+ */
+typedef u8 __bitwise dscp_t;
+
+#define INET_DSCP_MASK 0xfc
+#define INET_DSCP_SHIFT 2
+
+static inline dscp_t inet_dsfield_to_dscp(__u8 dsfield)
+{
+	return (__force dscp_t)(dsfield >> INET_DSCP_SHIFT);
+}
+
+static inline __u8 inet_dscp_to_dsfield(dscp_t dscp)
+{
+	return (__force __u8)dscp << INET_DSCP_SHIFT;
+}
+
+static inline bool inet_validate_dscp(__u8 val)
+{
+	return !(val & ~INET_DSCP_MASK);
+}
+
+#endif /* _INET_DSCP_H */

index 53ac7707ca70..3a457db7a4f6 100644
@@ -18,6 +18,7 @@
 #include <net/ndisc.h>
 #include <net/flow.h>
 #include <net/flow_dissector.h>
+#include <net/inet_dscp.h>
 #include <net/snmp.h>
 #include <net/netns/hash.h>
 

@@ -965,6 +966,11 @@ static inline u8 ip6_tclass(__be32 flowinfo)
 	return ntohl(flowinfo & IPV6_TCLASS_MASK) >> IPV6_TCLASS_SHIFT;
 }
 
+static inline dscp_t ip6_dscp(__be32 flowinfo)
+{
+	return inet_dsfield_to_dscp(ip6_tclass(flowinfo));
+}
+
 static inline __be32 ip6_make_flowinfo(unsigned int tclass, __be32 flowlabel)
 {
 	return htonl(tclass << IPV6_TCLASS_SHIFT) | flowlabel;

index dcedfe29d9d9..82d95d00214c 100644
@@ -16,6 +16,7 @@
 #include <linux/indirect_call_wrapper.h>
 
 #include <net/fib_rules.h>
+#include <net/inet_dscp.h>
 #include <net/ipv6.h>
 #include <net/addrconf.h>
 #include <net/ip6_route.h>

@@ -25,14 +26,14 @@ struct fib6_rule {
 	struct fib_rule		common;
 	struct rt6key		src;
 	struct rt6key		dst;
-	u8			tclass;
+	dscp_t			dscp;
 };
 
 static bool fib6_rule_matchall(const struct fib_rule *rule)
 {
 	struct fib6_rule *r = container_of(rule, struct fib6_rule, common);
 
-	if (r->dst.plen || r->src.plen || r->tclass)
+	if (r->dst.plen || r->src.plen || r->dscp)
 		return false;
 	return fib_rule_matchall(rule);
 }

@@ -323,7 +324,7 @@ INDIRECT_CALLABLE_SCOPE int fib6_rule_match(struct fib_rule *rule,
 			return 0;
 	}
 
-	if (r->tclass && r->tclass != ip6_tclass(fl6->flowlabel))
+	if (r->dscp && r->dscp != ip6_dscp(fl6->flowlabel))
 		return 0;
 
 	if (rule->ip_proto && (rule->ip_proto != fl6->flowi6_proto))

@@ -353,6 +354,12 @@ static int fib6_rule_configure(struct fib_rule *rule, struct sk_buff *skb,
 	struct net *net = sock_net(skb->sk);
 	struct fib6_rule *rule6 = (struct fib6_rule *) rule;
 
+	if (!inet_validate_dscp(frh->tos)) {
+		NL_SET_ERR_MSG(extack, "Invalid dsfield (tos): ECN bits must be 0");
+		goto errout;
+	}
+	rule6->dscp = inet_dsfield_to_dscp(frh->tos);
+
 	if (rule->action == FR_ACT_TO_TBL && !rule->l3mdev) {
 		if (rule->table == RT6_TABLE_UNSPEC) {
 			NL_SET_ERR_MSG(extack, "Invalid table");

@@ -373,7 +380,6 @@ static int fib6_rule_configure(struct fib_rule *rule, struct sk_buff *skb,
 
 	rule6->src.plen = frh->src_len;
 	rule6->dst.plen = frh->dst_len;
-	rule6->tclass = frh->tos;
 
 	if (fib_rule_requires_fldissect(rule))
 		net->ipv6.fib6_rules_require_fldissect++;

@@ -406,7 +412,7 @@ static int fib6_rule_compare(struct fib_rule *rule, struct fib_rule_hdr *frh,
 	if (frh->dst_len && (rule6->dst.plen != frh->dst_len))
 		return 0;
 
-	if (frh->tos && (rule6->tclass != frh->tos))
+	if (frh->tos && inet_dscp_to_dsfield(rule6->dscp) != frh->tos)
 		return 0;
 
 	if (frh->src_len &&

@@ -427,7 +433,7 @@ static int fib6_rule_fill(struct fib_rule *rule, struct sk_buff *skb,
 
 	frh->dst_len = rule6->dst.plen;
 	frh->src_len = rule6->src.plen;
-	frh->tos = rule6->tclass;
+	frh->tos = inet_dscp_to_dsfield(rule6->dscp);
 
 	if ((rule6->dst.plen &&
 	     nla_put_in6_addr(skb, FRA_DST, &rule6->dst.addr)) ||

2.21.3

Use the new dscp_t type to replace the tos field of fib4_rule, so that ip4-rules consistently ignore ECN bits.

Before this patch, ip4-rules did accept rules with the high order ECN bit set (but not the low order one). Also, it relied on its callers masking the ECN bits of ->flowi4_tos to prevent those from influencing the result. This was brittle and a few call paths still do the lookup without masking the ECN bits first.

After this patch ip4-rules only compare the DSCP bits. ECN can't influence the result anymore, even if the caller didn't mask these bits. Also, ip4-rules now must have both ECN bits cleared or they will be rejected.

---
 net/ipv4/fib_rules.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

index d279cb8ac158..d4529350a2e8 100644
@@ -23,6 +23,7 @@
 #include <linux/list.h>
 #include <linux/rcupdate.h>
 #include <linux/export.h>
+#include <net/inet_dscp.h>
 #include <net/ip.h>
 #include <net/route.h>
 #include <net/tcp.h>

@@ -35,7 +36,7 @@ struct fib4_rule {
 	struct fib_rule		common;
 	u8			dst_len;
 	u8			src_len;
-	u8			tos;
+	dscp_t			dscp;
 	__be32			src;
 	__be32			srcmask;
 	__be32			dst;

@@ -49,7 +50,7 @@ static bool fib4_rule_matchall(const struct fib_rule *rule)
 {
 	struct fib4_rule *r = container_of(rule, struct fib4_rule, common);
 
-	if (r->dst_len || r->src_len || r->tos)
+	if (r->dst_len || r->src_len || r->dscp)
 		return false;
 	return fib_rule_matchall(rule);
 }

@@ -185,7 +186,7 @@ INDIRECT_CALLABLE_SCOPE int fib4_rule_match(struct fib_rule *rule,
 	    ((daddr ^ r->dst) & r->dstmask))
 		return 0;
 
-	if (r->tos && (r->tos != fl4->flowi4_tos))
+	if (r->dscp && r->dscp != inet_dsfield_to_dscp(fl4->flowi4_tos))
 		return 0;
 
 	if (rule->ip_proto && (rule->ip_proto != fl4->flowi4_proto))

@@ -230,10 +231,11 @@ static int fib4_rule_configure(struct fib_rule *rule, struct sk_buff *skb,
 	int err = -EINVAL;
 	struct fib4_rule *rule4 = (struct fib4_rule *) rule;
 
-	if (frh->tos & ~IPTOS_TOS_MASK) {
-		NL_SET_ERR_MSG(extack, "Invalid tos");
+	if (!inet_validate_dscp(frh->tos)) {
+		NL_SET_ERR_MSG(extack, "Invalid dsfield (tos): ECN bits must be 0");
 		goto errout;
 	}
+	rule4->dscp = inet_dsfield_to_dscp(frh->tos);
 
 	/* split local/main if they are not already split */
 	err = fib_unmerge(net);

@@ -275,7 +277,6 @@ static int fib4_rule_configure(struct fib_rule *rule, struct sk_buff *skb,
 	rule4->srcmask = inet_make_mask(rule4->src_len);
 	rule4->dst_len = frh->dst_len;
 	rule4->dstmask = inet_make_mask(rule4->dst_len);
-	rule4->tos = frh->tos;
 
 	net->ipv4.fib_has_custom_rules = true;
 

@@ -318,7 +319,7 @@ static int fib4_rule_compare(struct fib_rule *rule, struct fib_rule_hdr *frh,
 	if (frh->dst_len && (rule4->dst_len != frh->dst_len))
 		return 0;
 
-	if (frh->tos && (rule4->tos != frh->tos))
+	if (frh->tos && inet_dscp_to_dsfield(rule4->dscp) != frh->tos)
 		return 0;
 
 #ifdef CONFIG_IP_ROUTE_CLASSID

@@ -342,7 +343,7 @@ static int fib4_rule_fill(struct fib_rule *rule, struct sk_buff *skb,
 
 	frh->dst_len = rule4->dst_len;
 	frh->src_len = rule4->src_len;
-	frh->tos = rule4->tos;
+	frh->tos = inet_dscp_to_dsfield(rule4->dscp);
 
 	if ((rule4->dst_len &&
 	     nla_put_in_addr(skb, FRA_DST, rule4->dst)) ||

2.21.3

Use the new dscp_t type to replace the fc_tos field of fib_config, to ensure IPv4 routes aren't influenced by ECN bits when configured with non-zero rtm_tos.

Before this patch, IPv4 routes specifying an rtm_tos with some of the ECN bits set were accepted. However they wouldn't work (never match) as IPv4 normally clears the ECN bits with IPTOS_RT_MASK before doing a FIB lookup (although a few buggy code paths don't).

After this patch, IPv4 routes specifying an rtm_tos with any ECN bit set is rejected.

Note, IPv6 routes ignore rtm_tos altogether: any rtm_tos is accepted, but treated as if it were 0.

---
 include/net/ip_fib.h    |  3 ++-
 net/ipv4/fib_frontend.c | 10 +++++++++-
 net/ipv4/fib_trie.c     |  7 +++++--
 3 files changed, 16 insertions(+), 4 deletions(-)

index 3417ba2d27ad..920091064731 100644
@@ -17,6 +17,7 @@
 #include <linux/rcupdate.h>
 #include <net/fib_notifier.h>
 #include <net/fib_rules.h>
+#include <net/inet_dscp.h>
 #include <net/inetpeer.h>
 #include <linux/percpu.h>
 #include <linux/notifier.h>

@@ -24,7 +25,7 @@
 
 struct fib_config {
 	u8			fc_dst_len;
-	u8			fc_tos;
+	dscp_t			fc_dscp;
 	u8			fc_protocol;
 	u8			fc_scope;
 	u8			fc_type;

index 4d61ddd8a0ec..0cc6dacabead 100644
@@ -32,6 +32,7 @@
 #include <linux/list.h>
 #include <linux/slab.h>
 
+#include <net/inet_dscp.h>
 #include <net/ip.h>
 #include <net/protocol.h>
 #include <net/route.h>

@@ -735,8 +736,15 @@ static int rtm_to_fib_config(struct net *net, struct sk_buff *skb,
 	memset(cfg, 0, sizeof(*cfg));
 
 	rtm = nlmsg_data(nlh);
+
+	if (!inet_validate_dscp(rtm->rtm_tos)) {
+		NL_SET_ERR_MSG(extack, "Invalid dsfield (tos): ECN bits must be 0");
+		err = -EINVAL;
+		goto errout;
+	}
+	cfg->fc_dscp = inet_dsfield_to_dscp(rtm->rtm_tos);
+
 	cfg->fc_dst_len = rtm->rtm_dst_len;
-	cfg->fc_tos = rtm->rtm_tos;
 	cfg->fc_table = rtm->rtm_table;
 	cfg->fc_protocol = rtm->rtm_protocol;
 	cfg->fc_scope = rtm->rtm_scope;

index 8060524f4256..d937eeebb812 100644
@@ -61,6 +61,7 @@
 #include <linux/vmalloc.h>
 #include <linux/notifier.h>
 #include <net/net_namespace.h>
+#include <net/inet_dscp.h>
 #include <net/ip.h>
 #include <net/protocol.h>
 #include <net/route.h>

@@ -1210,9 +1211,9 @@ int fib_table_insert(struct net *net, struct fib_table *tb,
 	struct fib_info *fi;
 	u8 plen = cfg->fc_dst_len;
 	u8 slen = KEYLENGTH - plen;
-	u8 tos = cfg->fc_tos;
 	u32 key;
 	int err;
+	u8 tos;
 
 	key = ntohl(cfg->fc_dst);
 

@@ -1227,6 +1228,7 @@ int fib_table_insert(struct net *net, struct fib_table *tb,
 		goto err;
 	}
 
+	tos = inet_dscp_to_dsfield(cfg->fc_dscp);
 	l = fib_find_node(t, &tp, key);
 	fa = l ? fib_find_alias(&l->leaf, slen, tos, fi->fib_priority,
 				tb->tb_id, false) : NULL;

@@ -1703,8 +1705,8 @@ int fib_table_delete(struct net *net, struct fib_table *tb,
 	struct key_vector *l, *tp;
 	u8 plen = cfg->fc_dst_len;
 	u8 slen = KEYLENGTH - plen;
-	u8 tos = cfg->fc_tos;
 	u32 key;
+	u8 tos;
 
 	key = ntohl(cfg->fc_dst);
 

@@ -1715,6 +1717,7 @@ int fib_table_delete(struct net *net, struct fib_table *tb,
 	if (!l)
 		return -ESRCH;
 
+	tos = inet_dscp_to_dsfield(cfg->fc_dscp);
 	fa = fib_find_alias(&l->leaf, slen, tos, 0, tb->tb_id, false);
 	if (!fa)
 		return -ESRCH;

2.21.3

Use the new dscp_t type to replace the fa_tos field of fib_alias. This ensures ECN bits are ignored and makes the field compatible with fc_dscp (struct fib_config).

Converting old *tos variables and fields to dscp_t, allows sparse to flag incorrect uses of DSCP and ECN bits.

---
 net/ipv4/fib_lookup.h    |  3 +-
 net/ipv4/fib_semantics.c | 14 ++++++----
 net/ipv4/fib_trie.c      | 59 ++++++++++++++++++++++------------------
 net/ipv4/route.c         |  3 +-
 4 files changed, 45 insertions(+), 34 deletions(-)

index e184bcb19943..a63014b54809 100644
@@ -4,13 +4,14 @@
 
 #include <linux/types.h>
 #include <linux/list.h>
+#include <net/inet_dscp.h>
 #include <net/ip_fib.h>
 #include <net/nexthop.h>
 
 struct fib_alias {
 	struct hlist_node	fa_list;
 	struct fib_info		*fa_info;
-	u8			fa_tos;
+	dscp_t			fa_dscp;
 	u8			fa_type;
 	u8			fa_state;
 	u8			fa_slen;

index fde7797b5806..872adeee9e5b 100644
@@ -31,6 +31,7 @@
 #include <linux/netlink.h>
 
 #include <net/arp.h>
+#include <net/inet_dscp.h>
 #include <net/ip.h>
 #include <net/protocol.h>
 #include <net/route.h>

@@ -515,7 +516,7 @@ void rtmsg_fib(int event, __be32 key, struct fib_alias *fa,
 	fri.tb_id = tb_id;
 	fri.dst = key;
 	fri.dst_len = dst_len;
-	fri.tos = fa->fa_tos;
+	fri.tos = inet_dscp_to_dsfield(fa->fa_dscp);
 	fri.type = fa->fa_type;
 	fri.offload = fa->offload;
 	fri.trap = fa->trap;

@@ -2016,7 +2017,7 @@ static void fib_select_default(const struct flowi4 *flp, struct fib_result *res)
 	int order = -1, last_idx = -1;
 	struct fib_alias *fa, *fa1 = NULL;
 	u32 last_prio = res->fi->fib_priority;
-	u8 last_tos = 0;
+	dscp_t last_dscp = 0;
 
 	hlist_for_each_entry_rcu(fa, fa_head, fa_list) {
 		struct fib_info *next_fi = fa->fa_info;

@@ -2024,19 +2025,20 @@ static void fib_select_default(const struct flowi4 *flp, struct fib_result *res)
 
 		if (fa->fa_slen != slen)
 			continue;
-		if (fa->fa_tos && fa->fa_tos != flp->flowi4_tos)
+		if (fa->fa_dscp &&
+		    fa->fa_dscp != inet_dsfield_to_dscp(flp->flowi4_tos))
 			continue;
 		if (fa->tb_id != tb->tb_id)
 			continue;
 		if (next_fi->fib_priority > last_prio &&
-		    fa->fa_tos == last_tos) {
-			if (last_tos)
+		    fa->fa_dscp == last_dscp) {
+			if (last_dscp)
 				continue;
 			break;
 		}
 		if (next_fi->fib_flags & RTNH_F_DEAD)
 			continue;
-		last_tos = fa->fa_tos;
+		last_dscp = fa->fa_dscp;
 		last_prio = next_fi->fib_priority;
 
 		if (next_fi->fib_scope != res->scope ||

index 243a0c52be42..8432bc066839 100644
@@ -84,6 +84,7 @@
 #include <linux/jhash.h>
 #include <net/dst.h>
 #include <net/dst_metadata.h>
+#include <net/inet_dscp.h>
 #include <net/net_namespace.h>
 #include <net/ip.h>
 #include <net/route.h>

@@ -3390,7 +3391,7 @@ static int inet_rtm_getroute(struct sk_buff *in_skb, struct nlmsghdr *nlh,
 
 				if (fa->fa_slen == slen &&
 				    fa->tb_id == fri.tb_id &&
-				    fa->fa_tos == fri.tos &&
+				    fa->fa_dscp == inet_dsfield_to_dscp(fri.tos) &&
 				    fa->fa_info == res.fi &&
 				    fa->fa_type == fri.type) {
 					fri.offload = fa->offload;

2.21.3

This is of course an RFC patch series, not meant for merging. Sorry for the missing RFC tag in the subject (used the wrong script :/).

I like the approach, although I must admit to not being too familiar with the parts of the code you're touching in this series. But I think the typedefs make sense, and I (still) think it's a good idea to do the conversion. I think the main thing to ensure from a backwards compatibility PoV is that we don't silently change behaviour in a way that is hard to detect. I.e., rejecting invalid configuration is fine even if it was "allowed" before, but, say, changing the matching behaviour so an existing rule set will still run unchanged but behave differently is best avoided.

I like the names, but why do the bitshift? I get that it's conceptually "cleaner", but AFAICT the shifted values are not actually used for anything other than being shifted back again? In which case you're just adding operations in the fast path for no reason...

I think this is OK, cf the "break explicitly" thing I wrote above.

As above.

Didn't work at all, so also fine.

This is reasonable, and I think the code churn is worth the extra clarity. You should probably spell out in the commit message that it's not intended to change behaviour, though.

Shouldn't we just start rejecting them, like for v4?

-Toke

That's right, the value is always shifted back again because all current APIs work with the full dsfield (well all the ones I'm aware of at least).

I switched to a bit shift when I tried writing down what dscp_t was representing: I found it a bit clumsy to explain that it actually wasn't exactly the DSCP. Also I didn't expect the bit shift to have any mesurable impact.

Anyway, I don't mind reverting to a simple bit mask.

Thanks for these feedbacks. These are the main points I wanted to discuss with this RFC.

Will do.

I had some thoughs about that, but didn't talk about them in the cover letter since I felt there was already enough edge cases to discuss, and this one wasn't directly related to this series (the problem is there regardless of this RFC).

So, on the one hand, we have this old policy of ignoring unknown netlink attributes, so it looks consistent to also ignore unused structure fields.

On the other hand, ignoring rtm_tos leads to a different behaviour than what was requested. So it certainly makes sense to at least warn the user. But a hard fail may break existing programs that don't clear rtm_tos by mistake.

I'm not too sure which approach is better.

I look forward to a final toss of TOS.

So I guess you could argue that those applications were broken in the first place, and so an explicit reject would only expose this? Do you know of any applications that actually *function* while doing what you describe?

One thought could be to add the rejection but be prepared to back it out if it does turn out (during the -rc phase) that it breaks something?

-Toke

I don't know of any existing application that actually does. But it's easy to imagine a developer setting only parts of the rtmsg structure and leaving the rest uninitialised. Exposing the problem might not help the end user, who may have no way to modify the broken program.

Also, for people using ifupdown (/etc/network/interfaces on Debian and derivatives), rejecting a command can cancel the configuration of an entire device section. So a stray tos option on an ip -6 route command would now leave the network interface entirely unconfigured.

I'm not saying these situations exist, just trying to anticipate all possible side effects.

Given that it's something that'd be easy to revert, maybe we can try this approach.