Re: [PATCH net-next] net: DSCP in IPv4 routing v2

[Guillaume Nault <gnault@redhat.com>]

On Sat, Nov 21, 2020 at 06:24:46PM +1000, Russell Strong wrote:
> From 2f27f92d5a6f4dd69ac4af32cdb51ba8d2083606 Mon Sep 17 00:00:00 2001
> From: Russell Strong <russell@strong.id.au>
> Date: Sat, 21 Nov 2020 18:12:43 +1000
> Subject: [PATCH] DSCP in IPv4 routing v2
> 
> This patch allows the use of DSCP values in routing

Thanks. There are some problems with this patch though.

About the email:
  * Why did you duplicate email headers in the body?
  * For the subject, please put the "v2" in the "[PATCH ... ]" part.
  * You're modifying many files, but haven't Cc-ed any of their authors
    or maintainers.
  * The patch content is corrupted.

> Use of TOS macros are replaced with DSCP macros
> where the change does not change the user space API
> with one exception:
> 
> net/ipv4/fib_rules.c has been changed to accept a
> wider range of values ( dscp values ).  Previously
> this would have returned an error.

Have you really verified that replacing each of these RT_TOS calls had
no unwanted side effect?

RT_TOS didn't clear the second lowest bit, while the new IP_DSCP does.
Therefore, there's no guarantee that such a blanket replacement isn't
going to change existing behaviours. Replacements have to be done
step by step and accompanied by an explanation of why they're safe.

BTW, I think there are some problems with RT_TOS that need to be fixed
separately first.

For example some of the ip6_make_flowinfo() calls can probably
erroneously mark some packets with ECT(0). Instead of masking the
problem in this patch, I think it'd be better to have an explicit fix
that'd mask the ECN bits in ip6_make_flowinfo() and drop the buggy
RT_TOS() in the callers.

Another example is inet_rtm_getroute(). It calls
ip_route_output_key_hash_rcu() without masking the tos field first.
Therefore it can return a different route than what the routing code
would actually use. Like for the ip6_make_flowinfo() case, it might
be better to stop relying on the callers to mask ECN bits and do that
in ip_route_output_key_hash_rcu() instead.

I'll verify that these two problems can actually happen in practice
and will send patches if necessary.

> iproute2 already supports setting dscp values through
> ip route add dsfield <dscp value> lookup ......
> 
> Signed-off-by: Russell Strong <russell@strong.id.au>
> ---
>  .../ethernet/mellanox/mlx5/core/en/tc_tun.c   |  2 +-
>  drivers/net/geneve.c                          |  4 ++--
>  drivers/net/ipvlan/ipvlan_core.c              |  2 +-
>  drivers/net/ppp/pptp.c                        |  2 +-
>  drivers/net/vrf.c                             |  2 +-
>  drivers/net/vxlan.c                           |  4 ++--
>  include/net/ip.h                              |  2 +-
>  include/net/route.h                           |  6 ++----
>  include/uapi/linux/ip.h                       |  2 ++
>  net/bridge/br_netfilter_hooks.c               |  2 +-
>  net/core/filter.c                             |  4 ++--
>  net/core/lwt_bpf.c                            |  2 +-
>  net/ipv4/fib_frontend.c                       |  2 +-
>  net/ipv4/fib_rules.c                          |  2 +-
>  net/ipv4/icmp.c                               |  6 +++---
>  net/ipv4/ip_gre.c                             |  2 +-
>  net/ipv4/ip_output.c                          |  2 +-
>  net/ipv4/ip_tunnel.c                          |  6 +++---
>  net/ipv4/ipmr.c                               |  6 +++---
>  net/ipv4/netfilter.c                          |  2 +-
>  net/ipv4/netfilter/ipt_rpfilter.c             |  2 +-
>  net/ipv4/netfilter/nf_dup_ipv4.c              |  2 +-
>  net/ipv4/route.c                              | 20 +++++++++----------
>  net/ipv6/ip6_output.c                         |  2 +-
>  net/ipv6/ip6_tunnel.c                         |  4 ++--
>  net/ipv6/sit.c                                |  4 ++--
>  net/xfrm/xfrm_policy.c                        |  2 +-
>  27 files changed, 49 insertions(+), 49 deletions(-)