mbox series

[net-next,00/12] Unmask upper DSCP bits - part 2

Message ID 20240827111813.2115285-1-idosch@nvidia.com (mailing list archive)
Headers show
Series Unmask upper DSCP bits - part 2 | expand

Message

Ido Schimmel Aug. 27, 2024, 11:18 a.m. UTC
tl;dr - This patchset continues to unmask the upper DSCP bits in the
IPv4 flow key in preparation for allowing IPv4 FIB rules to match on
DSCP. No functional changes are expected. Part 1 was merged in commit
("Merge branch 'unmask-upper-dscp-bits-part-1'").

The TOS field in the IPv4 flow key ('flowi4_tos') is used during FIB
lookup to match against the TOS selector in FIB rules and routes.

It is currently impossible for user space to configure FIB rules that
match on the DSCP value as the upper DSCP bits are either masked in the
various call sites that initialize the IPv4 flow key or along the path
to the FIB core.

In preparation for adding a DSCP selector to IPv4 and IPv6 FIB rules, we
need to make sure the entire DSCP value is present in the IPv4 flow key.
This patchset continues to unmask the upper DSCP bits, but this time in
the output route path.

Patches #1-#3 unmask the upper DSCP bits in the various places that
invoke the core output route lookup functions directly.

Patches #4-#6 do the same in three helpers that are widely used in the
output path to initialize the TOS field in the IPv4 flow key.

The rest of the patches continue to unmask these bits in call sites that
invoke the following wrappers around the core lookup functions:

Patch #7 - __ip_route_output_key()
Patches #8-#12 - ip_route_output_flow()

The next patchset will handle the callers of ip_route_output_ports() and
ip_route_output_key().

No functional changes are expected as commit 1fa3314c14c6 ("ipv4:
Centralize TOS matching") moved the masking of the upper DSCP bits to
the core where 'flowi4_tos' is matched against the TOS selector.

Ido Schimmel (12):
  ipv4: Unmask upper DSCP bits in RTM_GETROUTE output route lookup
  ipv4: Unmask upper DSCP bits in ip_route_output_key_hash()
  ipv4: icmp: Unmask upper DSCP bits in icmp_route_lookup()
  ipv4: Unmask upper DSCP bits in ip_sock_rt_tos()
  ipv4: Unmask upper DSCP bits in get_rttos()
  ipv4: Unmask upper DSCP bits when building flow key
  xfrm: Unmask upper DSCP bits in xfrm_get_tos()
  ipv4: Unmask upper DSCP bits in ip_send_unicast_reply()
  ipv6: sit: Unmask upper DSCP bits in ipip6_tunnel_xmit()
  ipvlan: Unmask upper DSCP bits in ipvlan_process_v4_outbound()
  vrf: Unmask upper DSCP bits in vrf_process_v4_outbound()
  bpf: Unmask upper DSCP bits in __bpf_redirect_neigh_v4()

 drivers/net/ipvlan/ipvlan_core.c | 4 +++-
 drivers/net/vrf.c                | 3 ++-
 include/net/ip.h                 | 5 ++++-
 include/net/route.h              | 5 ++---
 net/core/filter.c                | 2 +-
 net/ipv4/icmp.c                  | 3 ++-
 net/ipv4/ip_output.c             | 3 ++-
 net/ipv4/route.c                 | 8 ++++----
 net/ipv6/sit.c                   | 5 +++--
 net/xfrm/xfrm_policy.c           | 3 ++-
 10 files changed, 25 insertions(+), 16 deletions(-)

Comments

Guillaume Nault Aug. 27, 2024, 1:47 p.m. UTC | #1
On Tue, Aug 27, 2024 at 02:18:01PM +0300, Ido Schimmel wrote:
> tl;dr - This patchset continues to unmask the upper DSCP bits in the
> IPv4 flow key in preparation for allowing IPv4 FIB rules to match on
> DSCP. No functional changes are expected. Part 1 was merged in commit
> ("Merge branch 'unmask-upper-dscp-bits-part-1'").
> 
> The TOS field in the IPv4 flow key ('flowi4_tos') is used during FIB
> lookup to match against the TOS selector in FIB rules and routes.
> 
> It is currently impossible for user space to configure FIB rules that
> match on the DSCP value as the upper DSCP bits are either masked in the
> various call sites that initialize the IPv4 flow key or along the path
> to the FIB core.
> 
> In preparation for adding a DSCP selector to IPv4 and IPv6 FIB rules, we

Hum, do you plan to add a DSCP selector for IPv6? That shouldn't be
necessary as IPv6 already takes all the DSCP bits into account. Also we
don't need to keep any compatibility with the legacy TOS interpretation,
as it has never been defined nor used in IPv6.

> need to make sure the entire DSCP value is present in the IPv4 flow key.
> This patchset continues to unmask the upper DSCP bits, but this time in
> the output route path.
Ido Schimmel Aug. 27, 2024, 3:45 p.m. UTC | #2
On Tue, Aug 27, 2024 at 03:47:05PM +0200, Guillaume Nault wrote:
> On Tue, Aug 27, 2024 at 02:18:01PM +0300, Ido Schimmel wrote:
> > tl;dr - This patchset continues to unmask the upper DSCP bits in the
> > IPv4 flow key in preparation for allowing IPv4 FIB rules to match on
> > DSCP. No functional changes are expected. Part 1 was merged in commit
> > ("Merge branch 'unmask-upper-dscp-bits-part-1'").
> > 
> > The TOS field in the IPv4 flow key ('flowi4_tos') is used during FIB
> > lookup to match against the TOS selector in FIB rules and routes.
> > 
> > It is currently impossible for user space to configure FIB rules that
> > match on the DSCP value as the upper DSCP bits are either masked in the
> > various call sites that initialize the IPv4 flow key or along the path
> > to the FIB core.
> > 
> > In preparation for adding a DSCP selector to IPv4 and IPv6 FIB rules, we
> 
> Hum, do you plan to add a DSCP selector for IPv6? That shouldn't be
> necessary as IPv6 already takes all the DSCP bits into account. Also we
> don't need to keep any compatibility with the legacy TOS interpretation,
> as it has never been defined nor used in IPv6.

Yes. I want to add the DSCP selector for both families so that user
space would not need to use different selectors for different families.
It's implemented in the patches I previously shared:

https://github.com/idosch/linux/commit/a3289a6838a0d0e6e0a30a61132bdce3d2f71a3c.patch
https://github.com/idosch/linux/commit/ff5dd634fb278431b58437654d7f65b57fd4ae4b.patch
https://github.com/idosch/linux/commit/3060ecb534475eadabfa1d419dd64804f0bd0148.patch
https://github.com/idosch/linux/commit/12ddbce4f519b42477ea1e130b6d2bab1cca137c.patch

> 
> > need to make sure the entire DSCP value is present in the IPv4 flow key.
> > This patchset continues to unmask the upper DSCP bits, but this time in
> > the output route path.
>
Ido Schimmel Aug. 28, 2024, 12:09 p.m. UTC | #3
On Tue, Aug 27, 2024 at 06:45:53PM +0300, Ido Schimmel wrote:
> On Tue, Aug 27, 2024 at 03:47:05PM +0200, Guillaume Nault wrote:
> > On Tue, Aug 27, 2024 at 02:18:01PM +0300, Ido Schimmel wrote:
> > > tl;dr - This patchset continues to unmask the upper DSCP bits in the
> > > IPv4 flow key in preparation for allowing IPv4 FIB rules to match on
> > > DSCP. No functional changes are expected. Part 1 was merged in commit
> > > ("Merge branch 'unmask-upper-dscp-bits-part-1'").
> > > 
> > > The TOS field in the IPv4 flow key ('flowi4_tos') is used during FIB
> > > lookup to match against the TOS selector in FIB rules and routes.
> > > 
> > > It is currently impossible for user space to configure FIB rules that
> > > match on the DSCP value as the upper DSCP bits are either masked in the
> > > various call sites that initialize the IPv4 flow key or along the path
> > > to the FIB core.
> > > 
> > > In preparation for adding a DSCP selector to IPv4 and IPv6 FIB rules, we
> > 
> > Hum, do you plan to add a DSCP selector for IPv6? That shouldn't be
> > necessary as IPv6 already takes all the DSCP bits into account. Also we
> > don't need to keep any compatibility with the legacy TOS interpretation,
> > as it has never been defined nor used in IPv6.
> 
> Yes. I want to add the DSCP selector for both families so that user
> space would not need to use different selectors for different families.

Another approach could be to add a mask to the existing tos/dsfield. For
example:

# ip -4 rule add dsfield 0x04/0xfc table 100
# ip -6 rule add dsfield 0xf8/0xfc table 100

The default IPv4 mask (when user doesn't specify one) would be 0x1c and
the default IPv6 mask would be 0xfc.

WDYT?

> It's implemented in the patches I previously shared:
> 
> https://github.com/idosch/linux/commit/a3289a6838a0d0e6e0a30a61132bdce3d2f71a3c.patch
> https://github.com/idosch/linux/commit/ff5dd634fb278431b58437654d7f65b57fd4ae4b.patch
> https://github.com/idosch/linux/commit/3060ecb534475eadabfa1d419dd64804f0bd0148.patch
> https://github.com/idosch/linux/commit/12ddbce4f519b42477ea1e130b6d2bab1cca137c.patch
> 
> > 
> > > need to make sure the entire DSCP value is present in the IPv4 flow key.
> > > This patchset continues to unmask the upper DSCP bits, but this time in
> > > the output route path.
> >
Guillaume Nault Aug. 29, 2024, 11:30 a.m. UTC | #4
On Tue, Aug 27, 2024 at 06:45:53PM +0300, Ido Schimmel wrote:
> On Tue, Aug 27, 2024 at 03:47:05PM +0200, Guillaume Nault wrote:
> > On Tue, Aug 27, 2024 at 02:18:01PM +0300, Ido Schimmel wrote:
> > > tl;dr - This patchset continues to unmask the upper DSCP bits in the
> > > IPv4 flow key in preparation for allowing IPv4 FIB rules to match on
> > > DSCP. No functional changes are expected. Part 1 was merged in commit
> > > ("Merge branch 'unmask-upper-dscp-bits-part-1'").
> > > 
> > > The TOS field in the IPv4 flow key ('flowi4_tos') is used during FIB
> > > lookup to match against the TOS selector in FIB rules and routes.
> > > 
> > > It is currently impossible for user space to configure FIB rules that
> > > match on the DSCP value as the upper DSCP bits are either masked in the
> > > various call sites that initialize the IPv4 flow key or along the path
> > > to the FIB core.
> > > 
> > > In preparation for adding a DSCP selector to IPv4 and IPv6 FIB rules, we
> > 
> > Hum, do you plan to add a DSCP selector for IPv6? That shouldn't be
> > necessary as IPv6 already takes all the DSCP bits into account. Also we
> > don't need to keep any compatibility with the legacy TOS interpretation,
> > as it has never been defined nor used in IPv6.
> 
> Yes. I want to add the DSCP selector for both families so that user
> space would not need to use different selectors for different families.
> It's implemented in the patches I previously shared:

Hum, I guess that was a misunderstanding on my side. I read
"adding a DSCP selector to [IPv4 and] IPv6 FIB rules" as "adding the
possibility to match only the 3-bits TOS in fib6_rules". But your
fib6_rule.c patch doesn't modify fib6_rule_match(), so I believe that
what you really meant was just to add the new FRA_DSCP netlink
attribute to IPv6. Am I getting it right?

> https://github.com/idosch/linux/commit/a3289a6838a0d0e6e0a30a61132bdce3d2f71a3c.patch
> https://github.com/idosch/linux/commit/ff5dd634fb278431b58437654d7f65b57fd4ae4b.patch
> https://github.com/idosch/linux/commit/3060ecb534475eadabfa1d419dd64804f0bd0148.patch
> https://github.com/idosch/linux/commit/12ddbce4f519b42477ea1e130b6d2bab1cca137c.patch


> > > need to make sure the entire DSCP value is present in the IPv4 flow key.
> > > This patchset continues to unmask the upper DSCP bits, but this time in
> > > the output route path.
> > 
>
Guillaume Nault Aug. 29, 2024, 11:54 a.m. UTC | #5
On Wed, Aug 28, 2024 at 03:09:19PM +0300, Ido Schimmel wrote:
> On Tue, Aug 27, 2024 at 06:45:53PM +0300, Ido Schimmel wrote:
> > On Tue, Aug 27, 2024 at 03:47:05PM +0200, Guillaume Nault wrote:
> > > On Tue, Aug 27, 2024 at 02:18:01PM +0300, Ido Schimmel wrote:
> > > > tl;dr - This patchset continues to unmask the upper DSCP bits in the
> > > > IPv4 flow key in preparation for allowing IPv4 FIB rules to match on
> > > > DSCP. No functional changes are expected. Part 1 was merged in commit
> > > > ("Merge branch 'unmask-upper-dscp-bits-part-1'").
> > > > 
> > > > The TOS field in the IPv4 flow key ('flowi4_tos') is used during FIB
> > > > lookup to match against the TOS selector in FIB rules and routes.
> > > > 
> > > > It is currently impossible for user space to configure FIB rules that
> > > > match on the DSCP value as the upper DSCP bits are either masked in the
> > > > various call sites that initialize the IPv4 flow key or along the path
> > > > to the FIB core.
> > > > 
> > > > In preparation for adding a DSCP selector to IPv4 and IPv6 FIB rules, we
> > > 
> > > Hum, do you plan to add a DSCP selector for IPv6? That shouldn't be
> > > necessary as IPv6 already takes all the DSCP bits into account. Also we
> > > don't need to keep any compatibility with the legacy TOS interpretation,
> > > as it has never been defined nor used in IPv6.
> > 
> > Yes. I want to add the DSCP selector for both families so that user
> > space would not need to use different selectors for different families.
> 
> Another approach could be to add a mask to the existing tos/dsfield. For
> example:
> 
> # ip -4 rule add dsfield 0x04/0xfc table 100
> # ip -6 rule add dsfield 0xf8/0xfc table 100
> 
> The default IPv4 mask (when user doesn't specify one) would be 0x1c and
> the default IPv6 mask would be 0xfc.
> 
> WDYT?

For internal implementation, I find the mask option elegant (to avoid
conditionals). But I don't really like the idea of letting user space
provide its own mask. This would let the user create non-standard
behaviours, likely by mistake (as nobody seem to ever have requested
that flexibility).

I think my favourite approach would be to have the new FRA_DSCP
attribute work identically on both IPv4 and IPv6 FIB rules and keep
the behaviour of the old "tos" field of struct fib_rule_hdr unchanged.

This "tos" field would still work differently for IPv4 and IPv6, as it
always did, but people wanting consistent behaviour could just use
FRA_DSCP instead. Also, FRA_DSCP accepts real DSCP values as defined in
RFCs, while "tos" requires the 2 bits shift. For all these reasons, I'm
tempted to just consider "tos" as a legacy option used only for
backward compatibility, while FRA_DSCP would be the "clean" interface.

Is that approach acceptable for you?
Ido Schimmel Aug. 29, 2024, 2:43 p.m. UTC | #6
On Thu, Aug 29, 2024 at 01:30:58PM +0200, Guillaume Nault wrote:
> On Tue, Aug 27, 2024 at 06:45:53PM +0300, Ido Schimmel wrote:
> > On Tue, Aug 27, 2024 at 03:47:05PM +0200, Guillaume Nault wrote:
> > > On Tue, Aug 27, 2024 at 02:18:01PM +0300, Ido Schimmel wrote:
> > > > tl;dr - This patchset continues to unmask the upper DSCP bits in the
> > > > IPv4 flow key in preparation for allowing IPv4 FIB rules to match on
> > > > DSCP. No functional changes are expected. Part 1 was merged in commit
> > > > ("Merge branch 'unmask-upper-dscp-bits-part-1'").
> > > > 
> > > > The TOS field in the IPv4 flow key ('flowi4_tos') is used during FIB
> > > > lookup to match against the TOS selector in FIB rules and routes.
> > > > 
> > > > It is currently impossible for user space to configure FIB rules that
> > > > match on the DSCP value as the upper DSCP bits are either masked in the
> > > > various call sites that initialize the IPv4 flow key or along the path
> > > > to the FIB core.
> > > > 
> > > > In preparation for adding a DSCP selector to IPv4 and IPv6 FIB rules, we
> > > 
> > > Hum, do you plan to add a DSCP selector for IPv6? That shouldn't be
> > > necessary as IPv6 already takes all the DSCP bits into account. Also we
> > > don't need to keep any compatibility with the legacy TOS interpretation,
> > > as it has never been defined nor used in IPv6.
> > 
> > Yes. I want to add the DSCP selector for both families so that user
> > space would not need to use different selectors for different families.
> > It's implemented in the patches I previously shared:
> 
> Hum, I guess that was a misunderstanding on my side. I read
> "adding a DSCP selector to [IPv4 and] IPv6 FIB rules" as "adding the
> possibility to match only the 3-bits TOS in fib6_rules". But your
> fib6_rule.c patch doesn't modify fib6_rule_match(), so I believe that
> what you really meant was just to add the new FRA_DSCP netlink
> attribute to IPv6. Am I getting it right?

Yes. To be clear, you will be able to use the new 'dscp' keyword exactly
the same way with both IPv4 and IPv6:

# ip -4 rule add dscp 63 table 100
# ip -6 rule add dscp 63 table 100

Mixing 'dscp' and 'tos' will not work:

# ip -4 rule add dscp 7 tos 0x1c table 100
Error: Cannot specify both TOS and DSCP.
# ip -6 rule add dscp 7 tos 0x1c table 100
Error: Cannot specify both TOS and DSCP.

> 
> > https://github.com/idosch/linux/commit/a3289a6838a0d0e6e0a30a61132bdce3d2f71a3c.patch
> > https://github.com/idosch/linux/commit/ff5dd634fb278431b58437654d7f65b57fd4ae4b.patch
> > https://github.com/idosch/linux/commit/3060ecb534475eadabfa1d419dd64804f0bd0148.patch
> > https://github.com/idosch/linux/commit/12ddbce4f519b42477ea1e130b6d2bab1cca137c.patch
> 
> 
> > > > need to make sure the entire DSCP value is present in the IPv4 flow key.
> > > > This patchset continues to unmask the upper DSCP bits, but this time in
> > > > the output route path.
> > > 
> > 
>
Ido Schimmel Aug. 29, 2024, 2:52 p.m. UTC | #7
On Thu, Aug 29, 2024 at 01:54:46PM +0200, Guillaume Nault wrote:
> On Wed, Aug 28, 2024 at 03:09:19PM +0300, Ido Schimmel wrote:
> > On Tue, Aug 27, 2024 at 06:45:53PM +0300, Ido Schimmel wrote:
> > > On Tue, Aug 27, 2024 at 03:47:05PM +0200, Guillaume Nault wrote:
> > > > On Tue, Aug 27, 2024 at 02:18:01PM +0300, Ido Schimmel wrote:
> > > > > tl;dr - This patchset continues to unmask the upper DSCP bits in the
> > > > > IPv4 flow key in preparation for allowing IPv4 FIB rules to match on
> > > > > DSCP. No functional changes are expected. Part 1 was merged in commit
> > > > > ("Merge branch 'unmask-upper-dscp-bits-part-1'").
> > > > > 
> > > > > The TOS field in the IPv4 flow key ('flowi4_tos') is used during FIB
> > > > > lookup to match against the TOS selector in FIB rules and routes.
> > > > > 
> > > > > It is currently impossible for user space to configure FIB rules that
> > > > > match on the DSCP value as the upper DSCP bits are either masked in the
> > > > > various call sites that initialize the IPv4 flow key or along the path
> > > > > to the FIB core.
> > > > > 
> > > > > In preparation for adding a DSCP selector to IPv4 and IPv6 FIB rules, we
> > > > 
> > > > Hum, do you plan to add a DSCP selector for IPv6? That shouldn't be
> > > > necessary as IPv6 already takes all the DSCP bits into account. Also we
> > > > don't need to keep any compatibility with the legacy TOS interpretation,
> > > > as it has never been defined nor used in IPv6.
> > > 
> > > Yes. I want to add the DSCP selector for both families so that user
> > > space would not need to use different selectors for different families.
> > 
> > Another approach could be to add a mask to the existing tos/dsfield. For
> > example:
> > 
> > # ip -4 rule add dsfield 0x04/0xfc table 100
> > # ip -6 rule add dsfield 0xf8/0xfc table 100
> > 
> > The default IPv4 mask (when user doesn't specify one) would be 0x1c and
> > the default IPv6 mask would be 0xfc.
> > 
> > WDYT?
> 
> For internal implementation, I find the mask option elegant (to avoid
> conditionals). But I don't really like the idea of letting user space
> provide its own mask. This would let the user create non-standard
> behaviours, likely by mistake (as nobody seem to ever have requested
> that flexibility).
> 
> I think my favourite approach would be to have the new FRA_DSCP
> attribute work identically on both IPv4 and IPv6 FIB rules and keep
> the behaviour of the old "tos" field of struct fib_rule_hdr unchanged.
> 
> This "tos" field would still work differently for IPv4 and IPv6, as it
> always did, but people wanting consistent behaviour could just use
> FRA_DSCP instead. Also, FRA_DSCP accepts real DSCP values as defined in
> RFCs, while "tos" requires the 2 bits shift. For all these reasons, I'm
> tempted to just consider "tos" as a legacy option used only for
> backward compatibility, while FRA_DSCP would be the "clean" interface.
> 
> Is that approach acceptable for you?

Yes. The patches I shared implement this approach :)
Guillaume Nault Aug. 29, 2024, 3:08 p.m. UTC | #8
On Thu, Aug 29, 2024 at 05:43:17PM +0300, Ido Schimmel wrote:
> On Thu, Aug 29, 2024 at 01:30:58PM +0200, Guillaume Nault wrote:
> > On Tue, Aug 27, 2024 at 06:45:53PM +0300, Ido Schimmel wrote:
> > > On Tue, Aug 27, 2024 at 03:47:05PM +0200, Guillaume Nault wrote:
> > > > On Tue, Aug 27, 2024 at 02:18:01PM +0300, Ido Schimmel wrote:
> > > > > tl;dr - This patchset continues to unmask the upper DSCP bits in the
> > > > > IPv4 flow key in preparation for allowing IPv4 FIB rules to match on
> > > > > DSCP. No functional changes are expected. Part 1 was merged in commit
> > > > > ("Merge branch 'unmask-upper-dscp-bits-part-1'").
> > > > > 
> > > > > The TOS field in the IPv4 flow key ('flowi4_tos') is used during FIB
> > > > > lookup to match against the TOS selector in FIB rules and routes.
> > > > > 
> > > > > It is currently impossible for user space to configure FIB rules that
> > > > > match on the DSCP value as the upper DSCP bits are either masked in the
> > > > > various call sites that initialize the IPv4 flow key or along the path
> > > > > to the FIB core.
> > > > > 
> > > > > In preparation for adding a DSCP selector to IPv4 and IPv6 FIB rules, we
> > > > 
> > > > Hum, do you plan to add a DSCP selector for IPv6? That shouldn't be
> > > > necessary as IPv6 already takes all the DSCP bits into account. Also we
> > > > don't need to keep any compatibility with the legacy TOS interpretation,
> > > > as it has never been defined nor used in IPv6.
> > > 
> > > Yes. I want to add the DSCP selector for both families so that user
> > > space would not need to use different selectors for different families.
> > > It's implemented in the patches I previously shared:
> > 
> > Hum, I guess that was a misunderstanding on my side. I read
> > "adding a DSCP selector to [IPv4 and] IPv6 FIB rules" as "adding the
> > possibility to match only the 3-bits TOS in fib6_rules". But your
> > fib6_rule.c patch doesn't modify fib6_rule_match(), so I believe that
> > what you really meant was just to add the new FRA_DSCP netlink
> > attribute to IPv6. Am I getting it right?
> 
> Yes. To be clear, you will be able to use the new 'dscp' keyword exactly
> the same way with both IPv4 and IPv6:
> 
> # ip -4 rule add dscp 63 table 100
> # ip -6 rule add dscp 63 table 100
> 
> Mixing 'dscp' and 'tos' will not work:
> 
> # ip -4 rule add dscp 7 tos 0x1c table 100
> Error: Cannot specify both TOS and DSCP.
> # ip -6 rule add dscp 7 tos 0x1c table 100
> Error: Cannot specify both TOS and DSCP.

Thanks, that's exactly what I had in mind.

> > 
> > > https://github.com/idosch/linux/commit/a3289a6838a0d0e6e0a30a61132bdce3d2f71a3c.patch
> > > https://github.com/idosch/linux/commit/ff5dd634fb278431b58437654d7f65b57fd4ae4b.patch
> > > https://github.com/idosch/linux/commit/3060ecb534475eadabfa1d419dd64804f0bd0148.patch
> > > https://github.com/idosch/linux/commit/12ddbce4f519b42477ea1e130b6d2bab1cca137c.patch
> > 
> > 
> > > > > need to make sure the entire DSCP value is present in the IPv4 flow key.
> > > > > This patchset continues to unmask the upper DSCP bits, but this time in
> > > > > the output route path.
> > > > 
> > > 
> > 
>
Guillaume Nault Aug. 29, 2024, 3:10 p.m. UTC | #9
On Thu, Aug 29, 2024 at 05:52:05PM +0300, Ido Schimmel wrote:
> On Thu, Aug 29, 2024 at 01:54:46PM +0200, Guillaume Nault wrote:
> > On Wed, Aug 28, 2024 at 03:09:19PM +0300, Ido Schimmel wrote:
> > > On Tue, Aug 27, 2024 at 06:45:53PM +0300, Ido Schimmel wrote:
> > > > On Tue, Aug 27, 2024 at 03:47:05PM +0200, Guillaume Nault wrote:
> > > > > On Tue, Aug 27, 2024 at 02:18:01PM +0300, Ido Schimmel wrote:
> > > > > > tl;dr - This patchset continues to unmask the upper DSCP bits in the
> > > > > > IPv4 flow key in preparation for allowing IPv4 FIB rules to match on
> > > > > > DSCP. No functional changes are expected. Part 1 was merged in commit
> > > > > > ("Merge branch 'unmask-upper-dscp-bits-part-1'").
> > > > > > 
> > > > > > The TOS field in the IPv4 flow key ('flowi4_tos') is used during FIB
> > > > > > lookup to match against the TOS selector in FIB rules and routes.
> > > > > > 
> > > > > > It is currently impossible for user space to configure FIB rules that
> > > > > > match on the DSCP value as the upper DSCP bits are either masked in the
> > > > > > various call sites that initialize the IPv4 flow key or along the path
> > > > > > to the FIB core.
> > > > > > 
> > > > > > In preparation for adding a DSCP selector to IPv4 and IPv6 FIB rules, we
> > > > > 
> > > > > Hum, do you plan to add a DSCP selector for IPv6? That shouldn't be
> > > > > necessary as IPv6 already takes all the DSCP bits into account. Also we
> > > > > don't need to keep any compatibility with the legacy TOS interpretation,
> > > > > as it has never been defined nor used in IPv6.
> > > > 
> > > > Yes. I want to add the DSCP selector for both families so that user
> > > > space would not need to use different selectors for different families.
> > > 
> > > Another approach could be to add a mask to the existing tos/dsfield. For
> > > example:
> > > 
> > > # ip -4 rule add dsfield 0x04/0xfc table 100
> > > # ip -6 rule add dsfield 0xf8/0xfc table 100
> > > 
> > > The default IPv4 mask (when user doesn't specify one) would be 0x1c and
> > > the default IPv6 mask would be 0xfc.
> > > 
> > > WDYT?
> > 
> > For internal implementation, I find the mask option elegant (to avoid
> > conditionals). But I don't really like the idea of letting user space
> > provide its own mask. This would let the user create non-standard
> > behaviours, likely by mistake (as nobody seem to ever have requested
> > that flexibility).
> > 
> > I think my favourite approach would be to have the new FRA_DSCP
> > attribute work identically on both IPv4 and IPv6 FIB rules and keep
> > the behaviour of the old "tos" field of struct fib_rule_hdr unchanged.
> > 
> > This "tos" field would still work differently for IPv4 and IPv6, as it
> > always did, but people wanting consistent behaviour could just use
> > FRA_DSCP instead. Also, FRA_DSCP accepts real DSCP values as defined in
> > RFCs, while "tos" requires the 2 bits shift. For all these reasons, I'm
> > tempted to just consider "tos" as a legacy option used only for
> > backward compatibility, while FRA_DSCP would be the "clean" interface.
> > 
> > Is that approach acceptable for you?
> 
> Yes. The patches I shared implement this approach :)

Thanks for confirming. And sorry for the misunderstanding in v1.