Re: [PATCH net 4/4] mlx5: Don't accidentally set RTO_ONLINK before mlx5e_route_lookup_ipv4_get()

[Guillaume Nault <gnault@redhat.com>]

On Wed, Jan 05, 2022 at 07:57:23PM -0800, Saeed Mahameed wrote:
> On Wed, Jan 05, 2022 at 08:56:28PM +0100, Guillaume Nault wrote:
> > Mask the ECN bits before calling mlx5e_route_lookup_ipv4_get(). The
> > tunnel key might have the last ECN bit set. This interferes with the
> > route lookup process as ip_route_output_key_hash() interpretes this bit
> > specially (to restrict the route scope).
> > 
> > Found by code inspection, compile tested only.
> > 
> > Fixes: c7b9038d8af6 ("net/mlx5e: TC preparation refactoring for routing update event")
> > Fixes: 9a941117fb76 ("net/mlx5e: Maximize ip tunnel key usage on the TC offloading path")
> > Signed-off-by: Guillaume Nault <gnault@redhat.com>
> > ---
> > drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun.c | 5 +++--
> > 1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun.c b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun.c
> > index a5e450973225..bc5f1dcb75e1 100644
> > --- a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun.c
> > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun.c
> > @@ -1,6 +1,7 @@
> > /* SPDX-License-Identifier: GPL-2.0 OR Linux-OpenIB */
> > /* Copyright (c) 2018 Mellanox Technologies. */
> > 
> > +#include <net/inet_ecn.h>
> > #include <net/vxlan.h>
> > #include <net/gre.h>
> > #include <net/geneve.h>
> > @@ -235,7 +236,7 @@ int mlx5e_tc_tun_create_header_ipv4(struct mlx5e_priv *priv,
> > 	int err;
> > 
> > 	/* add the IP fields */
> > -	attr.fl.fl4.flowi4_tos = tun_key->tos;
> > +	attr.fl.fl4.flowi4_tos = tun_key->tos & ~INET_ECN_MASK;
> 
> This is TC control path, why would ecn bits be ON in TC act->tunnel info ?

As far as I understand, the value of tun_key->tos can be set by
act_tunnel_key.c, which doesn't impose any restriction on the tos value
(TCA_TUNNEL_KEY_ENC_TOS). Unless I've missed something (I'm not
familiar with the hardware offload infrastructure), tun_key->tos is
effectively under user control.

> I don't believe these bits are on and if they were, TC tunnels layer should
> clear them before calling the driver's offload callback.

We could reject TOS values that have the ECN bits set in
act_tunnel_key.c, but that'd be a much broader change, and a user
visible one. At the very least it'd be something for net-next, while
this series tries to fix bugs in net.

Also, from a logical point of view, callers of ip_route_output_key()
are responsible for properly setting or clearing the RTO_ONLINK flag.
We shouldn't have to change act_tunnel_key.c because one of the drivers
offload path might call ip_route_output_key().

Even though I agree that act_tunnel_key should ideally not accept ECN
bits in TCA_TUNNEL_KEY_ENC_TOS, it should refuse them for user
understandable reasons (decouple DSCP and ECN), not because of some
drivers implementation details.