| Message ID | 20240725131729.1729103-4-idosch@nvidia.com |
|---|---|
| State | RFC, archived |
| Headers | show |
| Series | Preparations for FIB rule DSCP selector | expand |
On Thu, Jul 25, 2024 at 04:17:29PM +0300, Ido Schimmel wrote: > The TOS field in the IPv4 flow information structure ('flowi4_tos') is > matched by the kernel against the TOS selector in IPv4 rules and routes. > The field is initialized differently by different call sites. Some treat > it as DSCP (RFC 2474) and initialize all six DSCP bits, some treat it as > RFC 1349 TOS and initialize it using RT_TOS() and some treat it as RFC > 791 TOS and initialize it using IPTOS_RT_MASK. > > What is common to all these call sites is that they all initialize the > lower three DSCP bits, which fits the TOS definition in the initial IPv4 > specification (RFC 791). > > Therefore, the kernel only allows configuring IPv4 FIB rules that match > on the lower three DSCP bits which are always guaranteed to be > initialized by all call sites: > > # ip -4 rule add tos 0x1c table 100 > # ip -4 rule add tos 0x3c table 100 > Error: Invalid tos. > > While this works, it is unlikely to be very useful. RFC 791 that > initially defined the TOS and IP precedence fields was updated by RFC > 2474 over twenty five years ago where these fields were replaced by a > single six bits DSCP field. > > Extending FIB rules to match on DSCP can be done by adding a new DSCP > selector while maintaining the existing semantics of the TOS selector > for applications that rely on that. > > A prerequisite for allowing FIB rules to match on DSCP is to adjust all > the call sites to initialize the high order DSCP bits and remove their > masking along the path to the core where the field is matched on. > > However, making this change alone will result in a behavior change. For > example, a forwarded IPv4 packet with a DS field of 0xfc will no longer > match a FIB rule that was configured with 'tos 0x1c'. > > This behavior change can be avoided by masking the upper three DSCP bits > in 'flowi4_tos' before comparing it against the TOS selectors in FIB > rules and routes. > > Implement the above by adding a new function that checks whether a given > DSCP value matches the one specified in the IPv4 flow information > structure and invoke it from the three places that currently match on > 'flowi4_tos'. > > Use RT_TOS() for the masking of 'flowi4_tos' instead of IPTOS_RT_MASK > since the latter is not uAPI and we should be able to remove it at some > point. > > No regressions in FIB tests: > > # ./fib_tests.sh > [...] > Tests passed: 218 > Tests failed: 0 > > And FIB rule tests: > > # ./fib_rule_tests.sh > [...] > Tests passed: 116 > Tests failed: 0 > > Signed-off-by: Ido Schimmel <idosch@nvidia.com> > --- > include/net/ip_fib.h | 7 +++++++ > net/ipv4/fib_rules.c | 2 +- > net/ipv4/fib_semantics.c | 3 +-- > net/ipv4/fib_trie.c | 3 +-- > 4 files changed, 10 insertions(+), 5 deletions(-) > > diff --git a/include/net/ip_fib.h b/include/net/ip_fib.h > index 72af2f223e59..967e4dc555fa 100644 > --- a/include/net/ip_fib.h > +++ b/include/net/ip_fib.h > @@ -22,6 +22,8 @@ > #include <linux/percpu.h> > #include <linux/notifier.h> > #include <linux/refcount.h> > +#include <linux/ip.h> Why including linux/ip.h? That doesn't seem necessary for this change. Appart from that, Reviewed-by: Guillaume Nault <gnault@redhat.com> Thanks a lot!
On Fri, Jul 26, 2024 at 03:17:15PM +0200, Guillaume Nault wrote: > On Thu, Jul 25, 2024 at 04:17:29PM +0300, Ido Schimmel wrote: > > diff --git a/include/net/ip_fib.h b/include/net/ip_fib.h > > index 72af2f223e59..967e4dc555fa 100644 > > --- a/include/net/ip_fib.h > > +++ b/include/net/ip_fib.h > > @@ -22,6 +22,8 @@ > > #include <linux/percpu.h> > > #include <linux/notifier.h> > > #include <linux/refcount.h> > > +#include <linux/ip.h> > > Why including linux/ip.h? That doesn't seem necessary for this change. RT_TOS() is defined in linux/in_route.h as ((tos)&IPTOS_TOS_MASK), but IPTOS_TOS_MASK is defined in liunx/ip.h which is not included by linux/in_route.h for some reason. This also works: diff --git a/include/net/ip_fib.h b/include/net/ip_fib.h index 967e4dc555fa..269ec10f63e4 100644 --- a/include/net/ip_fib.h +++ b/include/net/ip_fib.h @@ -22,7 +22,6 @@ #include <linux/percpu.h> #include <linux/notifier.h> #include <linux/refcount.h> -#include <linux/ip.h> #include <linux/in_route.h> struct fib_config { diff --git a/include/uapi/linux/in_route.h b/include/uapi/linux/in_route.h index 0cc2c23b47f8..10bdd7e7107f 100644 --- a/include/uapi/linux/in_route.h +++ b/include/uapi/linux/in_route.h @@ -2,6 +2,8 @@ #ifndef _LINUX_IN_ROUTE_H #define _LINUX_IN_ROUTE_H +#include <linux/ip.h> + /* IPv4 routing cache flags */ #define RTCF_DEAD RTNH_F_DEAD > > Appart from that, > > Reviewed-by: Guillaume Nault <gnault@redhat.com> Thanks!
On Sun, Jul 28, 2024 at 02:34:06PM +0300, Ido Schimmel wrote: > On Fri, Jul 26, 2024 at 03:17:15PM +0200, Guillaume Nault wrote: > > On Thu, Jul 25, 2024 at 04:17:29PM +0300, Ido Schimmel wrote: > > > diff --git a/include/net/ip_fib.h b/include/net/ip_fib.h > > > index 72af2f223e59..967e4dc555fa 100644 > > > --- a/include/net/ip_fib.h > > > +++ b/include/net/ip_fib.h > > > @@ -22,6 +22,8 @@ > > > #include <linux/percpu.h> > > > #include <linux/notifier.h> > > > #include <linux/refcount.h> > > > +#include <linux/ip.h> > > > > Why including linux/ip.h? That doesn't seem necessary for this change. > > RT_TOS() is defined in linux/in_route.h as ((tos)&IPTOS_TOS_MASK), but > IPTOS_TOS_MASK is defined in liunx/ip.h which is not included by > linux/in_route.h for some reason. > > This also works: > > diff --git a/include/net/ip_fib.h b/include/net/ip_fib.h > index 967e4dc555fa..269ec10f63e4 100644 > --- a/include/net/ip_fib.h > +++ b/include/net/ip_fib.h > @@ -22,7 +22,6 @@ > #include <linux/percpu.h> > #include <linux/notifier.h> > #include <linux/refcount.h> > -#include <linux/ip.h> > #include <linux/in_route.h> > > struct fib_config { > diff --git a/include/uapi/linux/in_route.h b/include/uapi/linux/in_route.h > index 0cc2c23b47f8..10bdd7e7107f 100644 > --- a/include/uapi/linux/in_route.h > +++ b/include/uapi/linux/in_route.h > @@ -2,6 +2,8 @@ > #ifndef _LINUX_IN_ROUTE_H > #define _LINUX_IN_ROUTE_H > > +#include <linux/ip.h> > + Thanks, I prefer this solution, which I find cleaner.
diff --git a/include/net/ip_fib.h b/include/net/ip_fib.h index 72af2f223e59..967e4dc555fa 100644 --- a/include/net/ip_fib.h +++ b/include/net/ip_fib.h @@ -22,6 +22,8 @@ #include <linux/percpu.h> #include <linux/notifier.h> #include <linux/refcount.h> +#include <linux/ip.h> +#include <linux/in_route.h> struct fib_config { u8 fc_dst_len; @@ -434,6 +436,11 @@ static inline bool fib4_rules_early_flow_dissect(struct net *net, #endif /* CONFIG_IP_MULTIPLE_TABLES */ +static inline bool fib_dscp_masked_match(dscp_t dscp, const struct flowi4 *fl4) +{ + return dscp == inet_dsfield_to_dscp(RT_TOS(fl4->flowi4_tos)); +} + /* Exported by fib_frontend.c */ extern const struct nla_policy rtm_ipv4_policy[]; void ip_fib_init(void); diff --git a/net/ipv4/fib_rules.c b/net/ipv4/fib_rules.c index 5bdd1c016009..c26776b71e97 100644 --- a/net/ipv4/fib_rules.c +++ b/net/ipv4/fib_rules.c @@ -186,7 +186,7 @@ INDIRECT_CALLABLE_SCOPE int fib4_rule_match(struct fib_rule *rule, ((daddr ^ r->dst) & r->dstmask)) return 0; - if (r->dscp && r->dscp != inet_dsfield_to_dscp(fl4->flowi4_tos)) + if (r->dscp && !fib_dscp_masked_match(r->dscp, fl4)) return 0; if (rule->ip_proto && (rule->ip_proto != fl4->flowi4_proto)) diff --git a/net/ipv4/fib_semantics.c b/net/ipv4/fib_semantics.c index 2b57cd2b96e2..0f70341cb8b5 100644 --- a/net/ipv4/fib_semantics.c +++ b/net/ipv4/fib_semantics.c @@ -2066,8 +2066,7 @@ static void fib_select_default(const struct flowi4 *flp, struct fib_result *res) if (fa->fa_slen != slen) continue; - if (fa->fa_dscp && - fa->fa_dscp != inet_dsfield_to_dscp(flp->flowi4_tos)) + if (fa->fa_dscp && !fib_dscp_masked_match(fa->fa_dscp, flp)) continue; if (fa->tb_id != tb->tb_id) continue; diff --git a/net/ipv4/fib_trie.c b/net/ipv4/fib_trie.c index 8f30e3f00b7f..09e31757e96c 100644 --- a/net/ipv4/fib_trie.c +++ b/net/ipv4/fib_trie.c @@ -1580,8 +1580,7 @@ int fib_table_lookup(struct fib_table *tb, const struct flowi4 *flp, if (index >= (1ul << fa->fa_slen)) continue; } - if (fa->fa_dscp && - inet_dscp_to_dsfield(fa->fa_dscp) != flp->flowi4_tos) + if (fa->fa_dscp && !fib_dscp_masked_match(fa->fa_dscp, flp)) continue; /* Paired with WRITE_ONCE() in fib_release_info() */ if (READ_ONCE(fi->fib_dead))
The TOS field in the IPv4 flow information structure ('flowi4_tos') is matched by the kernel against the TOS selector in IPv4 rules and routes. The field is initialized differently by different call sites. Some treat it as DSCP (RFC 2474) and initialize all six DSCP bits, some treat it as RFC 1349 TOS and initialize it using RT_TOS() and some treat it as RFC 791 TOS and initialize it using IPTOS_RT_MASK. What is common to all these call sites is that they all initialize the lower three DSCP bits, which fits the TOS definition in the initial IPv4 specification (RFC 791). Therefore, the kernel only allows configuring IPv4 FIB rules that match on the lower three DSCP bits which are always guaranteed to be initialized by all call sites: # ip -4 rule add tos 0x1c table 100 # ip -4 rule add tos 0x3c table 100 Error: Invalid tos. While this works, it is unlikely to be very useful. RFC 791 that initially defined the TOS and IP precedence fields was updated by RFC 2474 over twenty five years ago where these fields were replaced by a single six bits DSCP field. Extending FIB rules to match on DSCP can be done by adding a new DSCP selector while maintaining the existing semantics of the TOS selector for applications that rely on that. A prerequisite for allowing FIB rules to match on DSCP is to adjust all the call sites to initialize the high order DSCP bits and remove their masking along the path to the core where the field is matched on. However, making this change alone will result in a behavior change. For example, a forwarded IPv4 packet with a DS field of 0xfc will no longer match a FIB rule that was configured with 'tos 0x1c'. This behavior change can be avoided by masking the upper three DSCP bits in 'flowi4_tos' before comparing it against the TOS selectors in FIB rules and routes. Implement the above by adding a new function that checks whether a given DSCP value matches the one specified in the IPv4 flow information structure and invoke it from the three places that currently match on 'flowi4_tos'. Use RT_TOS() for the masking of 'flowi4_tos' instead of IPTOS_RT_MASK since the latter is not uAPI and we should be able to remove it at some point. No regressions in FIB tests: # ./fib_tests.sh [...] Tests passed: 218 Tests failed: 0 And FIB rule tests: # ./fib_rule_tests.sh [...] Tests passed: 116 Tests failed: 0 Signed-off-by: Ido Schimmel <idosch@nvidia.com> --- include/net/ip_fib.h | 7 +++++++ net/ipv4/fib_rules.c | 2 +- net/ipv4/fib_semantics.c | 3 +-- net/ipv4/fib_trie.c | 3 +-- 4 files changed, 10 insertions(+), 5 deletions(-)