| Message ID | 269e24dc9fb30549d4f77895532603734f515650.1681976818.git.leon@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 3198ae7d42af844a7d30f6e432c079c134b03852 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Fixes to mlx5 IPsec implementation | expand |
On Thu, Apr 20, 2023 at 11:02:49AM +0300, Leon Romanovsky wrote: > From: Leon Romanovsky <leonro@nvidia.com> > > Fix size argument in memcmp to compare whole IPv6 address. > > Fixes: b3beba1fb404 ("net/mlx5e: Allow policies with reqid 0, to support IKE policy holes") > Reviewed-by: Raed Salem <raeds@nvidia.com> > Reviewed-by: Emeel Hakim <ehakim@nvidia.com> > Signed-off-by: Leon Romanovsky <leonro@nvidia.com> > --- > drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.h b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.h > index f7f7c09d2b32..4e9887171508 100644 > --- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.h > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.h > @@ -287,7 +287,7 @@ static inline bool addr6_all_zero(__be32 *addr6) > { > static const __be32 zaddr6[4] = {}; > > - return !memcmp(addr6, zaddr6, sizeof(*zaddr6)); > + return !memcmp(addr6, zaddr6, sizeof(zaddr6)); 1. Perhaps array_size() is appropriate here? 2. It's a shame that ipv6_addr_any() or some other common helper can't be used. > } > #else > static inline void mlx5e_ipsec_init(struct mlx5e_priv *priv) > -- > 2.40.0 >
On Thu, Apr 20, 2023 at 01:09:23PM +0200, Simon Horman wrote: > On Thu, Apr 20, 2023 at 11:02:49AM +0300, Leon Romanovsky wrote: > > From: Leon Romanovsky <leonro@nvidia.com> > > > > Fix size argument in memcmp to compare whole IPv6 address. > > > > Fixes: b3beba1fb404 ("net/mlx5e: Allow policies with reqid 0, to support IKE policy holes") > > Reviewed-by: Raed Salem <raeds@nvidia.com> > > Reviewed-by: Emeel Hakim <ehakim@nvidia.com> > > Signed-off-by: Leon Romanovsky <leonro@nvidia.com> > > --- > > drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.h | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.h b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.h > > index f7f7c09d2b32..4e9887171508 100644 > > --- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.h > > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.h > > @@ -287,7 +287,7 @@ static inline bool addr6_all_zero(__be32 *addr6) > > { > > static const __be32 zaddr6[4] = {}; > > > > - return !memcmp(addr6, zaddr6, sizeof(*zaddr6)); > > + return !memcmp(addr6, zaddr6, sizeof(zaddr6)); > > 1. Perhaps array_size() is appropriate here? It is overkill here, sizeof(zaddr6) is constant and can't overflow. 238 /** 239 * array_size() - Calculate size of 2-dimensional array. 240 * @a: dimension one 241 * @b: dimension two 242 * 243 * Calculates size of 2-dimensional array: @a * @b. 244 * 245 * Returns: number of bytes needed to represent the array or SIZE_MAX on 246 * overflow. 247 */ 248 #define array_size(a, b) size_mul(a, b) > 2. It's a shame that ipv6_addr_any() or some other common helper > can't be used. I didn't use ipv6_addr_any() as it required from me to cast "__be32 *addr6" to be "struct in6_addr *" just to replace one line memcmp to another one line function. Do you want me to post this code instead? diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c index 55b38544422f..a7c8e38658a0 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c @@ -945,7 +945,8 @@ static int mlx5e_xfrm_validate_policy(struct mlx5_core_dev *mdev, } if (!x->xfrm_vec[0].reqid && sel->proto == IPPROTO_IP && - addr6_all_zero(sel->saddr.a6) && addr6_all_zero(sel->daddr.a6)) { + ipv6_addr_any((struct in6_addr *)sel->saddr.a6) && + ipv6_addr_any((struct in6_addr *)sel->daddr.a6)) { NL_SET_ERR_MSG_MOD(extack, "Unsupported policy with reqid 0 without at least one of upper protocol or ip addr(s) different than 0"); return -EINVAL; } diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.h b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.h index 4e9887171508..097001ce5dc1 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.h +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.h @@ -283,12 +283,6 @@ mlx5e_ipsec_pol2dev(struct mlx5e_ipsec_pol_entry *pol_entry) return pol_entry->ipsec->mdev; } -static inline bool addr6_all_zero(__be32 *addr6) -{ - static const __be32 zaddr6[4] = {}; - - return !memcmp(addr6, zaddr6, sizeof(zaddr6)); -} #else static inline void mlx5e_ipsec_init(struct mlx5e_priv *priv) { diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec_fs.c b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec_fs.c index dbe87bf89c0d..e48113923c12 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec_fs.c +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec_fs.c @@ -721,7 +721,8 @@ static void setup_fte_addr4(struct mlx5_flow_spec *spec, __be32 *saddr, static void setup_fte_addr6(struct mlx5_flow_spec *spec, __be32 *saddr, __be32 *daddr) { - if (addr6_all_zero(saddr) && addr6_all_zero(daddr)) + if (ipv6_addr_any((struct in6_addr *)saddr) && + ipv6_addr_any((struct in6_addr *)daddr)) return; spec->match_criteria_enable |= MLX5_MATCH_OUTER_HEADERS; @@ -729,14 +730,14 @@ static void setup_fte_addr6(struct mlx5_flow_spec *spec, __be32 *saddr, MLX5_SET_TO_ONES(fte_match_param, spec->match_criteria, outer_headers.ip_version); MLX5_SET(fte_match_param, spec->match_value, outer_headers.ip_version, 6); - if (!addr6_all_zero(saddr)) { + if (!ipv6_addr_any((struct in6_addr *)saddr)) { memcpy(MLX5_ADDR_OF(fte_match_param, spec->match_value, outer_headers.src_ipv4_src_ipv6.ipv6_layout.ipv6), saddr, 16); memset(MLX5_ADDR_OF(fte_match_param, spec->match_criteria, outer_headers.src_ipv4_src_ipv6.ipv6_layout.ipv6), 0xff, 16); } - if (!addr6_all_zero(daddr)) { + if (!ipv6_addr_any((struct in6_addr *)daddr)) { memcpy(MLX5_ADDR_OF(fte_match_param, spec->match_value, outer_headers.dst_ipv4_dst_ipv6.ipv6_layout.ipv6), daddr, 16); memset(MLX5_ADDR_OF(fte_match_param, spec->match_criteria, Thanks > > > } > > #else > > static inline void mlx5e_ipsec_init(struct mlx5e_priv *priv) > > -- > > 2.40.0 > >
On Thu, Apr 20, 2023 at 02:52:43PM +0300, Leon Romanovsky wrote: > On Thu, Apr 20, 2023 at 01:09:23PM +0200, Simon Horman wrote: > > On Thu, Apr 20, 2023 at 11:02:49AM +0300, Leon Romanovsky wrote: > > > From: Leon Romanovsky <leonro@nvidia.com> > > > > > > Fix size argument in memcmp to compare whole IPv6 address. > > > > > > Fixes: b3beba1fb404 ("net/mlx5e: Allow policies with reqid 0, to support IKE policy holes") > > > Reviewed-by: Raed Salem <raeds@nvidia.com> > > > Reviewed-by: Emeel Hakim <ehakim@nvidia.com> > > > Signed-off-by: Leon Romanovsky <leonro@nvidia.com> > > > --- > > > drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.h | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.h b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.h > > > index f7f7c09d2b32..4e9887171508 100644 > > > --- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.h > > > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.h > > > @@ -287,7 +287,7 @@ static inline bool addr6_all_zero(__be32 *addr6) > > > { > > > static const __be32 zaddr6[4] = {}; > > > > > > - return !memcmp(addr6, zaddr6, sizeof(*zaddr6)); > > > + return !memcmp(addr6, zaddr6, sizeof(zaddr6)); > > > > 1. Perhaps array_size() is appropriate here? > > It is overkill here, sizeof(zaddr6) is constant and can't overflow. Maybe, but the original code had a bug because using sizeof() directly is error prone. > > 238 /** > 239 * array_size() - Calculate size of 2-dimensional array. > 240 * @a: dimension one > 241 * @b: dimension two > 242 * > 243 * Calculates size of 2-dimensional array: @a * @b. > 244 * > 245 * Returns: number of bytes needed to represent the array or SIZE_MAX on > 246 * overflow. > 247 */ > 248 #define array_size(a, b) size_mul(a, b) > > > 2. It's a shame that ipv6_addr_any() or some other common helper > > can't be used. > > I didn't use ipv6_addr_any() as it required from me to cast "__be32 *addr6" > to be "struct in6_addr *" just to replace one line memcmp to another one > line function. > > Do you want me to post this code instead? No :) I don't have a strong desire for churn. Just for correct code. As your patch is correct, it is fine by me in the current form. Reviewed-by: Simon Horman <simon.horman@corigine.com>
On Thu, Apr 20, 2023 at 02:05:01PM +0200, Simon Horman wrote: > On Thu, Apr 20, 2023 at 02:52:43PM +0300, Leon Romanovsky wrote: > > On Thu, Apr 20, 2023 at 01:09:23PM +0200, Simon Horman wrote: > > > On Thu, Apr 20, 2023 at 11:02:49AM +0300, Leon Romanovsky wrote: > > > > From: Leon Romanovsky <leonro@nvidia.com> > > > > > > > > Fix size argument in memcmp to compare whole IPv6 address. > > > > > > > > Fixes: b3beba1fb404 ("net/mlx5e: Allow policies with reqid 0, to support IKE policy holes") > > > > Reviewed-by: Raed Salem <raeds@nvidia.com> > > > > Reviewed-by: Emeel Hakim <ehakim@nvidia.com> > > > > Signed-off-by: Leon Romanovsky <leonro@nvidia.com> > > > > --- > > > > drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.h | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.h b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.h > > > > index f7f7c09d2b32..4e9887171508 100644 > > > > --- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.h > > > > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.h > > > > @@ -287,7 +287,7 @@ static inline bool addr6_all_zero(__be32 *addr6) > > > > { > > > > static const __be32 zaddr6[4] = {}; > > > > > > > > - return !memcmp(addr6, zaddr6, sizeof(*zaddr6)); > > > > + return !memcmp(addr6, zaddr6, sizeof(zaddr6)); > > > > > > 1. Perhaps array_size() is appropriate here? > > > > It is overkill here, sizeof(zaddr6) is constant and can't overflow. > > Maybe, but the original code had a bug because using sizeof() > directly is error prone. Sorry, just to clarify. I now realise that ARRAY_SIZE() is what I meant to suggest earlier. > > 238 /** > > 239 * array_size() - Calculate size of 2-dimensional array. > > 240 * @a: dimension one > > 241 * @b: dimension two > > 242 * > > 243 * Calculates size of 2-dimensional array: @a * @b. > > 244 * > > 245 * Returns: number of bytes needed to represent the array or SIZE_MAX on > > 246 * overflow. > > 247 */ > > 248 #define array_size(a, b) size_mul(a, b) > > > > > 2. It's a shame that ipv6_addr_any() or some other common helper > > > can't be used. > > > > I didn't use ipv6_addr_any() as it required from me to cast "__be32 *addr6" > > to be "struct in6_addr *" just to replace one line memcmp to another one > > line function. > > > > Do you want me to post this code instead? > > No :) > > I don't have a strong desire for churn. > Just for correct code. > > As your patch is correct, it is fine by me in the current form. > > Reviewed-by: Simon Horman <simon.horman@corigine.com> >
On Thu, Apr 20, 2023 at 04:35:53PM +0200, Simon Horman wrote: > On Thu, Apr 20, 2023 at 02:05:01PM +0200, Simon Horman wrote: > > On Thu, Apr 20, 2023 at 02:52:43PM +0300, Leon Romanovsky wrote: > > > On Thu, Apr 20, 2023 at 01:09:23PM +0200, Simon Horman wrote: > > > > On Thu, Apr 20, 2023 at 11:02:49AM +0300, Leon Romanovsky wrote: > > > > > From: Leon Romanovsky <leonro@nvidia.com> > > > > > > > > > > Fix size argument in memcmp to compare whole IPv6 address. > > > > > > > > > > Fixes: b3beba1fb404 ("net/mlx5e: Allow policies with reqid 0, to support IKE policy holes") > > > > > Reviewed-by: Raed Salem <raeds@nvidia.com> > > > > > Reviewed-by: Emeel Hakim <ehakim@nvidia.com> > > > > > Signed-off-by: Leon Romanovsky <leonro@nvidia.com> > > > > > --- > > > > > drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.h | 2 +- > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.h b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.h > > > > > index f7f7c09d2b32..4e9887171508 100644 > > > > > --- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.h > > > > > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.h > > > > > @@ -287,7 +287,7 @@ static inline bool addr6_all_zero(__be32 *addr6) > > > > > { > > > > > static const __be32 zaddr6[4] = {}; > > > > > > > > > > - return !memcmp(addr6, zaddr6, sizeof(*zaddr6)); > > > > > + return !memcmp(addr6, zaddr6, sizeof(zaddr6)); > > > > > > > > 1. Perhaps array_size() is appropriate here? > > > > > > It is overkill here, sizeof(zaddr6) is constant and can't overflow. > > > > Maybe, but the original code had a bug because using sizeof() > > directly is error prone. > > Sorry, just to clarify. > I now realise that ARRAY_SIZE() is what I meant to suggest earlier. ARRAY_SIZE(zaddr6) will give us 4, so we will need to multiple in sizeof(__be32) to get the right result (16 bytes). sizeof(zaddr6) == ARRAY_SIZE(zaddr6) * sizeof(__be32) Thanks
On Thu, Apr 20, 2023 at 08:13:19PM +0300, Leon Romanovsky wrote: > On Thu, Apr 20, 2023 at 04:35:53PM +0200, Simon Horman wrote: > > On Thu, Apr 20, 2023 at 02:05:01PM +0200, Simon Horman wrote: > > > On Thu, Apr 20, 2023 at 02:52:43PM +0300, Leon Romanovsky wrote: > > > > On Thu, Apr 20, 2023 at 01:09:23PM +0200, Simon Horman wrote: > > > > > On Thu, Apr 20, 2023 at 11:02:49AM +0300, Leon Romanovsky wrote: > > > > > > From: Leon Romanovsky <leonro@nvidia.com> > > > > > > > > > > > > Fix size argument in memcmp to compare whole IPv6 address. > > > > > > > > > > > > Fixes: b3beba1fb404 ("net/mlx5e: Allow policies with reqid 0, to support IKE policy holes") > > > > > > Reviewed-by: Raed Salem <raeds@nvidia.com> > > > > > > Reviewed-by: Emeel Hakim <ehakim@nvidia.com> > > > > > > Signed-off-by: Leon Romanovsky <leonro@nvidia.com> > > > > > > --- > > > > > > drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.h | 2 +- > > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > > > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.h b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.h > > > > > > index f7f7c09d2b32..4e9887171508 100644 > > > > > > --- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.h > > > > > > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.h > > > > > > @@ -287,7 +287,7 @@ static inline bool addr6_all_zero(__be32 *addr6) > > > > > > { > > > > > > static const __be32 zaddr6[4] = {}; > > > > > > > > > > > > - return !memcmp(addr6, zaddr6, sizeof(*zaddr6)); > > > > > > + return !memcmp(addr6, zaddr6, sizeof(zaddr6)); > > > > > > > > > > 1. Perhaps array_size() is appropriate here? > > > > > > > > It is overkill here, sizeof(zaddr6) is constant and can't overflow. > > > > > > Maybe, but the original code had a bug because using sizeof() > > > directly is error prone. > > > > Sorry, just to clarify. > > I now realise that ARRAY_SIZE() is what I meant to suggest earlier. > > ARRAY_SIZE(zaddr6) will give us 4, so we will need to multiple in > sizeof(__be32) to get the right result (16 bytes). > > sizeof(zaddr6) == ARRAY_SIZE(zaddr6) * sizeof(__be32) Let's retire this thread.
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.h b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.h index f7f7c09d2b32..4e9887171508 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.h +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.h @@ -287,7 +287,7 @@ static inline bool addr6_all_zero(__be32 *addr6) { static const __be32 zaddr6[4] = {}; - return !memcmp(addr6, zaddr6, sizeof(*zaddr6)); + return !memcmp(addr6, zaddr6, sizeof(zaddr6)); } #else static inline void mlx5e_ipsec_init(struct mlx5e_priv *priv)