| Message ID | a0ba792bbbf088882a55507c932f1abec915c3b6.1641407336.git.gnault@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | ipv4: Fix accidental RTO_ONLINK flags passed to ip_route_output_key_hash() | expand |
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 ? 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.
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.
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; attr.fl.fl4.daddr = tun_key->u.ipv4.dst; attr.fl.fl4.saddr = tun_key->u.ipv4.src; attr.ttl = tun_key->ttl; @@ -350,7 +351,7 @@ int mlx5e_tc_tun_update_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; attr.fl.fl4.daddr = tun_key->u.ipv4.dst; attr.fl.fl4.saddr = tun_key->u.ipv4.src; attr.ttl = tun_key->ttl;
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(-)