ipv4: First steps toward removing RTO_ONLINK

All callers already initialise ->flowi4_scope with RT_SCOPE_UNIVERSE, either by manual field assignment, memset(0) of the whole structure or implicit structure initialisation of on-stack variables (RT_SCOPE_UNIVERSE actually equals 0).

Therefore, we don't need to always initialise ->flowi4_scope in ip_rt_fix_tos(). We only need to reduce the scope to RT_SCOPE_LINK when the special RTO_ONLINK flag is present in the tos.

This will allow some code simplification, like removing ip_rt_fix_tos(). Also, the long term idea is to remove RTO_ONLINK entirely by properly initialising ->flowi4_scope, instead of overloading ->flowi4_tos with a special flag. Eventually, this will allow to convert ->flowi4_tos to dscp_t.

Signed-off-by Guillaume Nault <[email protected]>

---
It's important for the correctness of this patch that all callers initialise ->flowi4_scope to 0 (in one way or another). Auditing all of them is long, although each case is pretty trivial.

If it helps, I can send a patch series that converts implicit initialisation of ->flowi4_scope with an explicit assignment to RT_SCOPE_UNIVERSE. This would also have the advantage of making it clear to future readers that ->flowi4_scope _has_ to be initialised. I haven't sent such patch series to not overwhelm reviewers with trivial and not technically-required changes (there are 40+ places to modify, scattered over 30+ different files). But if anyone prefers explicit initialisation everywhere, then just let me know and I'll send such patches.

---
 net/ipv4/route.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

net/ipv4/route.c +0 -0

index e839d424b861..d8f82c0ac132 100644

Quoted 21 lines · Guillaume Nault

All callers already initialise ->flowi4_scope with RT_SCOPE_UNIVERSE, either by manual field assignment, memset(0) of the whole structure or implicit structure initialisation of on-stack variables (RT_SCOPE_UNIVERSE actually equals 0).

Therefore, we don't need to always initialise ->flowi4_scope in ip_rt_fix_tos(). We only need to reduce the scope to RT_SCOPE_LINK when the special RTO_ONLINK flag is present in the tos.

This will allow some code simplification, like removing ip_rt_fix_tos(). Also, the long term idea is to remove RTO_ONLINK entirely by properly initialising ->flowi4_scope, instead of overloading ->flowi4_tos with a special flag. Eventually, this will allow to convert ->flowi4_tos to dscp_t.

Signed-off-by Guillaume Nault <[email protected]>

---
It's important for the correctness of this patch that all callers initialise ->flowi4_scope to 0 (in one way or another). Auditing all of them is long, although each case is pretty trivial.

Quoted 9 lines

If it helps, I can send a patch series that converts implicit initialisation of ->flowi4_scope with an explicit assignment to RT_SCOPE_UNIVERSE. This would also have the advantage of making it clear to future readers that ->flowi4_scope _has_ to be initialised. I haven't sent such patch series to not overwhelm reviewers with trivial and not technically-required changes (there are 40+ places to modify, scattered over 30+ different files). But if anyone prefers explicit initialisation everywhere, then just let me know and I'll send such patches.

There are a handful of places that open code the initialization of the flow struct. I *think* I found all of them in 40867d74c374.

Quoted 9 lines

---
 net/ipv4/route.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

net/ipv4/route.c +0 -0

index e839d424b861..d8f82c0ac132 100644

net/ipv4/route.c static void ip_rt_fix_tos(struct flowi4 *fl4) · line 510

Quoted 10 lines

net/ipv4/route.c +2 -2

@@ -503,8 +503,8 @@ static void ip_rt_fix_tos(struct flowi4 *fl4)
 	__u8 tos = RT_FL_TOS(fl4);
 
 	fl4->flowi4_tos = tos & IPTOS_RT_MASK;
-	fl4->flowi4_scope = tos & RTO_ONLINK ?
-			    RT_SCOPE_LINK : RT_SCOPE_UNIVERSE;
+	if (tos & RTO_ONLINK)
+		fl4->flowi4_scope = RT_SCOPE_LINK;
 }
 
 static void __build_flow_key(const struct net *net, struct flowi4 *fl4,

Reviewed-by David Ahern <[email protected]>

Quoted 32 lines · David Ahern

Quoted 30 lines · Guillaume Nault

All callers already initialise ->flowi4_scope with RT_SCOPE_UNIVERSE, either by manual field assignment, memset(0) of the whole structure or implicit structure initialisation of on-stack variables (RT_SCOPE_UNIVERSE actually equals 0).

Therefore, we don't need to always initialise ->flowi4_scope in ip_rt_fix_tos(). We only need to reduce the scope to RT_SCOPE_LINK when the special RTO_ONLINK flag is present in the tos.

This will allow some code simplification, like removing ip_rt_fix_tos(). Also, the long term idea is to remove RTO_ONLINK entirely by properly initialising ->flowi4_scope, instead of overloading ->flowi4_tos with a special flag. Eventually, this will allow to convert ->flowi4_tos to dscp_t.

Signed-off-by Guillaume Nault <[email protected]>

---
It's important for the correctness of this patch that all callers initialise ->flowi4_scope to 0 (in one way or another). Auditing all of them is long, although each case is pretty trivial.

If it helps, I can send a patch series that converts implicit initialisation of ->flowi4_scope with an explicit assignment to RT_SCOPE_UNIVERSE. This would also have the advantage of making it clear to future readers that ->flowi4_scope _has_ to be initialised. I haven't sent such patch series to not overwhelm reviewers with trivial and not technically-required changes (there are 40+ places to modify, scattered over 30+ different files). But if anyone prefers explicit initialisation everywhere, then just let me know and I'll send such patches.

Quoted 2 lines

There are a handful of places that open code the initialization of the flow struct. I *think* I found all of them in 40867d74c374.

By open code, do you mean "doesn't use flowi4_init_output() nor ip_tunnel_init_flow()"? If so, I think there are many more.

Quoted 36 lines · Guillaume Nault

Quoted 34 lines · David Ahern

Quoted 21 lines · Guillaume Nault

All callers already initialise ->flowi4_scope with RT_SCOPE_UNIVERSE, either by manual field assignment, memset(0) of the whole structure or implicit structure initialisation of on-stack variables (RT_SCOPE_UNIVERSE actually equals 0).

Therefore, we don't need to always initialise ->flowi4_scope in ip_rt_fix_tos(). We only need to reduce the scope to RT_SCOPE_LINK when the special RTO_ONLINK flag is present in the tos.

This will allow some code simplification, like removing ip_rt_fix_tos(). Also, the long term idea is to remove RTO_ONLINK entirely by properly initialising ->flowi4_scope, instead of overloading ->flowi4_tos with a special flag. Eventually, this will allow to convert ->flowi4_tos to dscp_t.

Signed-off-by Guillaume Nault <[email protected]>

---
It's important for the correctness of this patch that all callers initialise ->flowi4_scope to 0 (in one way or another). Auditing all of them is long, although each case is pretty trivial.

Quoted 9 lines

If it helps, I can send a patch series that converts implicit initialisation of ->flowi4_scope with an explicit assignment to RT_SCOPE_UNIVERSE. This would also have the advantage of making it clear to future readers that ->flowi4_scope _has_ to be initialised. I haven't sent such patch series to not overwhelm reviewers with trivial and not technically-required changes (there are 40+ places to modify, scattered over 30+ different files). But if anyone prefers explicit initialisation everywhere, then just let me know and I'll send such patches.

There are a handful of places that open code the initialization of the flow struct. I *think* I found all of them in 40867d74c374.

Quoted 3 lines

By open code, do you mean "doesn't use flowi4_init_output() nor ip_tunnel_init_flow()"? If so, I think there are many more.

no, you made a comment about flow struct being initialized to 0 which implicitly initializes scope. My comment is that there are only a few places that do not use either memset(flow, 0, sizeof()) or struct flowi4 fl4 = {} to fully initialize the struct.

Now that ip_rt_fix_tos() doesn't reset ->flowi4_scope unconditionally, we don't have to rely on the RTO_ONLINK bit to properly set the scope of a flowi4 structure. We can just set ->flowi4_scope explicitly and avoid using RTO_ONLINK in ->flowi4_tos.

This patch converts callers of ip_route_connect(). Instead of setting the tos parameter with RT_CONN_FLAGS(sk), as all callers do, we can:

1- Drop the tos parameter from ip_route_connect(): its value was
entirely based on sk, which is also passed as parameter.

2- Set ->flowi4_scope depending on the SOCK_LOCALROUTE socket option
instead of always initialising it with RT_SCOPE_UNIVERSE (let's define ip_sock_rt_scope() for this purpose).

3- Avoid overloading ->flowi4_tos with RTO_ONLINK: since the scope is
now properly initialised, we don't need to tell ip_rt_fix_tos() to adjust ->flowi4_scope for us. So let's define ip_sock_rt_tos(), which is the same as RT_CONN_FLAGS() but without the RTO_ONLINK bit overload.

Note:
In the original ip_route_connect() code, __ip_route_output_key() might clear the RTO_ONLINK bit of fl4->flowi4_tos (because of ip_rt_fix_tos()). Therefore flowi4_update_output() had to reuse the original tos variable. Now that we don't set RTO_ONLINK any more, this is not a problem and we can use fl4->flowi4_tos in flowi4_update_output().

Signed-off-by Guillaume Nault <[email protected]>

---
 include/net/route.h | 36 ++++++++++++++++++++++++------------
 net/dccp/ipv4.c     |  5 ++---
 net/ipv4/af_inet.c  |  6 +++---
 net/ipv4/datagram.c |  7 +++----
 net/ipv4/tcp_ipv4.c |  5 ++---
 5 files changed, 34 insertions(+), 25 deletions(-)

All the *_redirect() and *_update_pmtu() functions initialise their struct flowi4 variable with either __build_flow_key() or build_sk_flow_key(). When sk is provided, these functions use RT_CONN_FLAGS() to set ->flowi4_tos and always use RT_SCOPE_UNIVERSE for ->flowi4_scope. Then they rely on ip_rt_fix_tos() to adjust the scope based on the RTO_ONLINK bit and to mask the tos with IPTOS_RT_MASK.

This patch modifies __build_flow_key() and build_sk_flow_key() to properly initialise ->flowi4_tos and ->flowi4_scope, so that the ICMP redirects and PMTU handlers don't need an extra call to ip_rt_fix_tos() before doing a fib lookup. That is, we:

* Drop RT_CONN_FLAGS(): use ip_sock_rt_tos() and ip_sock_rt_scope() instead, so that we don't have to rely on ip_rt_fix_tos() to adjust the scope anymore.

* Apply IPTOS_RT_MASK to the tos, so that we don't need ip_rt_fix_tos() to do it for us.

* Drop the ip_rt_fix_tos() calls that now become useless.

The only remaining ip_rt_fix_tos() caller is ip_route_output_key_hash() which needs it as long as external callers still use the RTO_ONLINK flag.

Note:
This patch also drops some useless RT_TOS() calls as IPTOS_RT_MASK is a stronger mask.

Signed-off-by Guillaume Nault <[email protected]>

---
 net/ipv4/route.c | 37 +++++++++++++++++--------------------
 1 file changed, 17 insertions(+), 20 deletions(-)

RTO_ONLINK is a flag that allows to reduce the scope of route lookups. It's stored in a normally unused bit of the ->flowi4_tos field, in struct flowi4. However it has several problems:

* This bit is also used by ECN. Although ECN bits are supposed to be cleared before doing a route lookup, it happened that some code paths didn't properly sanitise their ->flowi4_tos. So this mechanism is fragile and we had bugs in the past where ECN bits slipped in and could end up being erroneously interpreted as RTO_ONLINK.

* A dscp_t type was recently introduced to ensure ECN bits are cleared during route lookups. ->flowi4_tos is the most important structure field to convert, but RTO_ONLINK prevents such conversion, as dscp_t mandates that ECN bits (where RTO_ONLINK is stored) be zero.

Therefore we need to stop using RTO_ONLINK altogether. Fortunately RTO_ONLINK isn't a necessity. Instead of passing a flag in ->flowi4_tos to tell the route lookup function to restrict the scope, we can simply initialise the scope correctly.

Quoted 15 lines · Guillaume Nault

RTO_ONLINK is a flag that allows to reduce the scope of route lookups. It's stored in a normally unused bit of the ->flowi4_tos field, in struct flowi4. However it has several problems:

* This bit is also used by ECN. Although ECN bits are supposed to be cleared before doing a route lookup, it happened that some code paths didn't properly sanitise their ->flowi4_tos. So this mechanism is fragile and we had bugs in the past where ECN bits slipped in and could end up being erroneously interpreted as RTO_ONLINK.

* A dscp_t type was recently introduced to ensure ECN bits are cleared during route lookups. ->flowi4_tos is the most important structure field to convert, but RTO_ONLINK prevents such conversion, as dscp_t mandates that ECN bits (where RTO_ONLINK is stored) be zero.

Quoted 5 lines

Therefore we need to stop using RTO_ONLINK altogether. Fortunately RTO_ONLINK isn't a necessity. Instead of passing a flag in ->flowi4_tos to tell the route lookup function to restrict the scope, we can simply initialise the scope correctly.

I believe the set looks ok. I think the fib test coverage in selftests could use more tests to cover tos.