[PATCH net-next 0/5] Fixes to mlx5 IPsec implementation

[PATCH net-next 0/5] Fixes to mlx5 IPsec implementation

[Leon Romanovsky]

From: Leon Romanovsky <leonro@nvidia.com>

Hi,

This small patchset includes various fixes and one refactoring patch
which I collected for the features sent in this cycle, with one exception -
first patch.

First patch fixes code which was introduced in previous cycle, however I
was able to trigger FW error only in custom debug code, so don't see a
need to send it to net-rc.

Thanks

Leon Romanovsky (5):
  net/mlx5e: Fix FW error while setting IPsec policy block action
  net/mlx5e: Don't overwrite extack message returned from IPsec SA
    validator
  net/mlx5e: Compare all fields in IPv6 address
  net/mlx5e: Properly release work data structure
  net/mlx5e: Refactor duplicated code in mlx5e_ipsec_init_macs

 .../mellanox/mlx5/core/en_accel/ipsec.c       | 53 +++++++++----------
 .../mellanox/mlx5/core/en_accel/ipsec.h       |  2 +-
 .../mellanox/mlx5/core/en_accel/ipsec_fs.c    | 16 +++---
 3 files changed, 35 insertions(+), 36 deletions(-)

-- 
2.40.0

[PATCH net-next 1/5] net/mlx5e: Fix FW error while setting IPsec policy block action

[Leon Romanovsky]

From: Leon Romanovsky <leonro@nvidia.com>

When trying to set IPsec policy block action the following error is
generated:

 mlx5_cmd_out_err:803:(pid 3426): SET_FLOW_TABLE_ENTRY(0x936) op_mod(0x0) failed,
	status bad parameter(0x3), syndrome (0x8708c3), err(-22)

This error means that drop action is not allowed when modify action is
set, so update the code to skip modify header for XFRM_POLICY_BLOCK action.

Fixes: 6721239672fe ("net/mlx5e: Skip IPsec encryption for TX path without matching policy")
Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
 .../mellanox/mlx5/core/en_accel/ipsec_fs.c       | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

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 5a8fcd30fcb1..dbe87bf89c0d 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
@@ -1252,16 +1252,16 @@ static int tx_add_policy(struct mlx5e_ipsec_pol_entry *pol_entry)
 	setup_fte_no_frags(spec);
 	setup_fte_upper_proto_match(spec, &attrs->upspec);
 
-	if (attrs->reqid) {
+	switch (attrs->action) {
+	case XFRM_POLICY_ALLOW:
+		flow_act.action |= MLX5_FLOW_CONTEXT_ACTION_FWD_DEST;
+		if (!attrs->reqid)
+			break;
+
 		err = setup_modify_header(mdev, attrs->reqid,
 					  XFRM_DEV_OFFLOAD_OUT, &flow_act);
 		if (err)
 			goto err_mod_header;
-	}
-
-	switch (attrs->action) {
-	case XFRM_POLICY_ALLOW:
-		flow_act.action |= MLX5_FLOW_CONTEXT_ACTION_FWD_DEST;
 		break;
 	case XFRM_POLICY_BLOCK:
 		flow_act.action |= MLX5_FLOW_CONTEXT_ACTION_DROP |
@@ -1273,7 +1273,7 @@ static int tx_add_policy(struct mlx5e_ipsec_pol_entry *pol_entry)
 	default:
 		WARN_ON(true);
 		err = -EINVAL;
-		goto err_action;
+		goto err_mod_header;
 	}
 
 	flow_act.flags |= FLOW_ACT_NO_APPEND;
@@ -1293,7 +1293,7 @@ static int tx_add_policy(struct mlx5e_ipsec_pol_entry *pol_entry)
 	return 0;
 
 err_action:
-	if (attrs->reqid)
+	if (flow_act.modify_hdr)
 		mlx5_modify_header_dealloc(mdev, flow_act.modify_hdr);
 err_mod_header:
 	kvfree(spec);
-- 
2.40.0

Re: [PATCH net-next 1/5] net/mlx5e: Fix FW error while setting IPsec policy block action

[Simon Horman]

On Thu, Apr 20, 2023 at 11:02:47AM +0300, Leon Romanovsky wrote:
> From: Leon Romanovsky <leonro@nvidia.com>
> 
> When trying to set IPsec policy block action the following error is
> generated:
> 
>  mlx5_cmd_out_err:803:(pid 3426): SET_FLOW_TABLE_ENTRY(0x936) op_mod(0x0) failed,
> 	status bad parameter(0x3), syndrome (0x8708c3), err(-22)
> 
> This error means that drop action is not allowed when modify action is
> set, so update the code to skip modify header for XFRM_POLICY_BLOCK action.
> 
> Fixes: 6721239672fe ("net/mlx5e: Skip IPsec encryption for TX path without matching policy")
> Signed-off-by: Leon Romanovsky <leonro@nvidia.com>

Reviewed-by: Simon Horman <simon.horman@corigine.com>

[PATCH net-next 2/5] net/mlx5e: Don't overwrite extack message returned from IPsec SA validator

[Leon Romanovsky]

From: Leon Romanovsky <leonro@nvidia.com>

Addition of new err_xfrm label caused to error messages be overwritten.
Fix it by using proper NL_SET_ERR_MSG_WEAK_MOD macro together with change
in a default message.

Fixes: aa8bd0c9518c ("net/mlx5e: Support IPsec acquire default SA")
Reviewed-by: Raed Salem <raeds@nvidia.com>
Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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 5fd609d1120e..1547d8cda133 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c
@@ -712,7 +712,7 @@ static int mlx5e_xfrm_add_state(struct xfrm_state *x,
 	kfree(sa_entry->work);
 err_xfrm:
 	kfree(sa_entry);
-	NL_SET_ERR_MSG_MOD(extack, "Device failed to offload this policy");
+	NL_SET_ERR_MSG_WEAK_MOD(extack, "Device failed to offload this state");
 	return err;
 }
 
-- 
2.40.0

Re: [PATCH net-next 2/5] net/mlx5e: Don't overwrite extack message returned from IPsec SA validator

[Simon Horman]

On Thu, Apr 20, 2023 at 11:02:48AM +0300, Leon Romanovsky wrote:
> From: Leon Romanovsky <leonro@nvidia.com>
> 
> Addition of new err_xfrm label caused to error messages be overwritten.
> Fix it by using proper NL_SET_ERR_MSG_WEAK_MOD macro together with change
> in a default message.
> 
> Fixes: aa8bd0c9518c ("net/mlx5e: Support IPsec acquire default SA")
> Reviewed-by: Raed Salem <raeds@nvidia.com>
> Signed-off-by: Leon Romanovsky <leonro@nvidia.com>

Reviewed-by: Simon Horman <simon.horman@corigine.com>

[PATCH net-next 3/5] net/mlx5e: Compare all fields in IPv6 address

[Leon Romanovsky]

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));
 }
 #else
 static inline void mlx5e_ipsec_init(struct mlx5e_priv *priv)
-- 
2.40.0

Re: [PATCH net-next 3/5] net/mlx5e: Compare all fields in IPv6 address

[Simon Horman]

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
> 

Re: [PATCH net-next 3/5] net/mlx5e: Compare all fields in IPv6 address

[Leon Romanovsky]

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
> > 

Re: [PATCH net-next 3/5] net/mlx5e: Compare all fields in IPv6 address

[Simon Horman]

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>

Re: [PATCH net-next 3/5] net/mlx5e: Compare all fields in IPv6 address

[Simon Horman]

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>
> 

Re: [PATCH net-next 3/5] net/mlx5e: Compare all fields in IPv6 address

[Leon Romanovsky]

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

Re: [PATCH net-next 3/5] net/mlx5e: Compare all fields in IPv6 address

[Simon Horman]

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.

[PATCH net-next 4/5] net/mlx5e: Properly release work data structure

[Leon Romanovsky]

From: Leon Romanovsky <leonro@nvidia.com>

There are some flows in which work structure is not allocated at all
and it is needed to be checked prior release of data structure.

 general protection fault, probably for non-canonical address 0xdffffc000000000a: 0000 [#1] SMP KASAN
 KASAN: null-ptr-deref in range [0x0000000000000050-0x0000000000000057]
 CPU: 6 PID: 3486 Comm: kworker/6:0 Not tainted 6.3.0-rc5_for_upstream_debug_2023_04_06_11_01 #1
 Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.13.0-0-gf21b5a4aeb02-prebuilt.qemu.org 04/01/2014
 Workqueue: events xfrm_state_gc_task
 RIP: 0010:mlx5e_xfrm_free_state+0x177/0x260 [mlx5_core]
 Code: c1 ea 03 80 3c 02 00 0f 85 f5 00 00 00 4c 8b a5 08 01 00 00 48 b8 00 00 00 00 00 fc ff df 49 8d 7c 24 50 48 89 fa 48 c1 ea 03 <80> 3c 02 00 0f 85 b7 00 00 00 49 8b 7c 24 50 e8 85 7c 09 e0 4c 89
 RSP: 0018:ffff888137a8fc50 EFLAGS: 00010206
 RAX: dffffc0000000000 RBX: ffff888180398000 RCX: 0000000000000000
 RDX: 000000000000000a RSI: ffffffffa1878227 RDI: 0000000000000050
 RBP: ffff88812a0c8000 R08: ffff888137a8fb60 R09: 0000000000000000
 R10: fffffbfff09aba0c R11: 0000000000000001 R12: 0000000000000000
 R13: ffff88812a0c8108 R14: ffffffff84c63480 R15: ffff8881acb63118
 FS:  0000000000000000(0000) GS:ffff88881eb00000(0000) knlGS:0000000000000000
 CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
 CR2: 00007f667e8bc000 CR3: 0000000004693006 CR4: 0000000000370ea0
 DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
 DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
 Call Trace:

  ___xfrm_state_destroy+0x3c8/0x5e0
  xfrm_state_gc_task+0xf6/0x140
  ? ___xfrm_state_destroy+0x5e0/0x5e0
  process_one_work+0x7c2/0x1340
  ? lockdep_hardirqs_on_prepare+0x3f0/0x3f0
  ? pwq_dec_nr_in_flight+0x230/0x230
  ? spin_bug+0x1d0/0x1d0
  worker_thread+0x59d/0xec0
  ? __kthread_parkme+0xd9/0x1d0
  ? process_one_work+0x1340/0x1340
  kthread+0x28f/0x330
  ? kthread_complete_and_exit+0x20/0x20
  ret_from_fork+0x1f/0x30

 Modules linked in: sch_ingress openvswitch nsh mlx5_vdpa vringh vhost_iotlb vdpa mlx5_ib mlx5_core xt_conntrack xt_MASQUERADE nf_conntrack_netlink nfnetlink xt_addrtype iptable_nat nf_nat br_netfilter rpcrdma rdma_ucm ib_iser libiscsi scsi_transport_iscsi rdma_cm iw_cm ib_umad ib_ipoib ib_cm ib_uverbs ib_core vfio_pci vfio_pci_core vfio_iommu_type1 vfio cuse overlay zram zsmalloc fuse [last unloaded: mlx5_core]
 ---[ end trace 0000000000000000 ]---

Fixes: 4562116f8a56 ("net/mlx5e: Generalize IPsec work structs")
Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

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 1547d8cda133..59b9927ac90f 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c
@@ -708,7 +708,8 @@ static int mlx5e_xfrm_add_state(struct xfrm_state *x,
 release_dwork:
 	kfree(sa_entry->dwork);
 release_work:
-	kfree(sa_entry->work->data);
+	if (sa_entry->work)
+		kfree(sa_entry->work->data);
 	kfree(sa_entry->work);
 err_xfrm:
 	kfree(sa_entry);
@@ -752,7 +753,8 @@ static void mlx5e_xfrm_free_state(struct xfrm_state *x)
 	mlx5e_accel_ipsec_fs_del_rule(sa_entry);
 	mlx5_ipsec_free_sa_ctx(sa_entry);
 	kfree(sa_entry->dwork);
-	kfree(sa_entry->work->data);
+	if (sa_entry->work)
+		kfree(sa_entry->work->data);
 	kfree(sa_entry->work);
 sa_entry_free:
 	kfree(sa_entry);
-- 
2.40.0

Re: [PATCH net-next 4/5] net/mlx5e: Properly release work data structure

[Simon Horman]

On Thu, Apr 20, 2023 at 11:02:50AM +0300, Leon Romanovsky wrote:
> From: Leon Romanovsky <leonro@nvidia.com>
> 
> There are some flows in which work structure is not allocated at all
> and it is needed to be checked prior release of data structure.
> 
>  general protection fault, probably for non-canonical address 0xdffffc000000000a: 0000 [#1] SMP KASAN
>  KASAN: null-ptr-deref in range [0x0000000000000050-0x0000000000000057]
>  CPU: 6 PID: 3486 Comm: kworker/6:0 Not tainted 6.3.0-rc5_for_upstream_debug_2023_04_06_11_01 #1
>  Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.13.0-0-gf21b5a4aeb02-prebuilt.qemu.org 04/01/2014
>  Workqueue: events xfrm_state_gc_task
>  RIP: 0010:mlx5e_xfrm_free_state+0x177/0x260 [mlx5_core]
>  Code: c1 ea 03 80 3c 02 00 0f 85 f5 00 00 00 4c 8b a5 08 01 00 00 48 b8 00 00 00 00 00 fc ff df 49 8d 7c 24 50 48 89 fa 48 c1 ea 03 <80> 3c 02 00 0f 85 b7 00 00 00 49 8b 7c 24 50 e8 85 7c 09 e0 4c 89
>  RSP: 0018:ffff888137a8fc50 EFLAGS: 00010206
>  RAX: dffffc0000000000 RBX: ffff888180398000 RCX: 0000000000000000
>  RDX: 000000000000000a RSI: ffffffffa1878227 RDI: 0000000000000050
>  RBP: ffff88812a0c8000 R08: ffff888137a8fb60 R09: 0000000000000000
>  R10: fffffbfff09aba0c R11: 0000000000000001 R12: 0000000000000000
>  R13: ffff88812a0c8108 R14: ffffffff84c63480 R15: ffff8881acb63118
>  FS:  0000000000000000(0000) GS:ffff88881eb00000(0000) knlGS:0000000000000000
>  CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>  CR2: 00007f667e8bc000 CR3: 0000000004693006 CR4: 0000000000370ea0
>  DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
>  DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
>  Call Trace:
> 
>   ___xfrm_state_destroy+0x3c8/0x5e0
>   xfrm_state_gc_task+0xf6/0x140
>   ? ___xfrm_state_destroy+0x5e0/0x5e0
>   process_one_work+0x7c2/0x1340
>   ? lockdep_hardirqs_on_prepare+0x3f0/0x3f0
>   ? pwq_dec_nr_in_flight+0x230/0x230
>   ? spin_bug+0x1d0/0x1d0
>   worker_thread+0x59d/0xec0
>   ? __kthread_parkme+0xd9/0x1d0
>   ? process_one_work+0x1340/0x1340
>   kthread+0x28f/0x330
>   ? kthread_complete_and_exit+0x20/0x20
>   ret_from_fork+0x1f/0x30
> 
>  Modules linked in: sch_ingress openvswitch nsh mlx5_vdpa vringh vhost_iotlb vdpa mlx5_ib mlx5_core xt_conntrack xt_MASQUERADE nf_conntrack_netlink nfnetlink xt_addrtype iptable_nat nf_nat br_netfilter rpcrdma rdma_ucm ib_iser libiscsi scsi_transport_iscsi rdma_cm iw_cm ib_umad ib_ipoib ib_cm ib_uverbs ib_core vfio_pci vfio_pci_core vfio_iommu_type1 vfio cuse overlay zram zsmalloc fuse [last unloaded: mlx5_core]
>  ---[ end trace 0000000000000000 ]---
> 
> Fixes: 4562116f8a56 ("net/mlx5e: Generalize IPsec work structs")
> Signed-off-by: Leon Romanovsky <leonro@nvidia.com>

Reviewed-by: Simon Horman <simon.horman@corigine.com>

[PATCH net-next 5/5] net/mlx5e: Refactor duplicated code in mlx5e_ipsec_init_macs

[Leon Romanovsky]

From: Leon Romanovsky <leonro@nvidia.com>

ARP discovery code has same logic for RX and TX flows, but with
different source and destination fields. Instead of duplicating
same code in mlx5e_ipsec_init_macs, let's refactor.

Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
 .../mellanox/mlx5/core/en_accel/ipsec.c       | 45 +++++++++----------
 1 file changed, 21 insertions(+), 24 deletions(-)

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 59b9927ac90f..55b38544422f 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c
@@ -252,6 +252,8 @@ static void mlx5e_ipsec_init_macs(struct mlx5e_ipsec_sa_entry *sa_entry,
 	struct net_device *netdev;
 	struct neighbour *n;
 	u8 addr[ETH_ALEN];
+	const void *pkey;
+	u8 *dst, *src;
 
 	if (attrs->mode != XFRM_MODE_TUNNEL ||
 	    attrs->type != XFRM_DEV_OFFLOAD_PACKET)
@@ -262,36 +264,31 @@ static void mlx5e_ipsec_init_macs(struct mlx5e_ipsec_sa_entry *sa_entry,
 	mlx5_query_mac_address(mdev, addr);
 	switch (attrs->dir) {
 	case XFRM_DEV_OFFLOAD_IN:
-		ether_addr_copy(attrs->dmac, addr);
-		n = neigh_lookup(&arp_tbl, &attrs->saddr.a4, netdev);
-		if (!n) {
-			n = neigh_create(&arp_tbl, &attrs->saddr.a4, netdev);
-			if (IS_ERR(n))
-				return;
-			neigh_event_send(n, NULL);
-			attrs->drop = true;
-			break;
-		}
-		neigh_ha_snapshot(addr, n, netdev);
-		ether_addr_copy(attrs->smac, addr);
+		src = attrs->dmac;
+		dst = attrs->smac;
+		pkey = &attrs->saddr.a4;
 		break;
 	case XFRM_DEV_OFFLOAD_OUT:
-		ether_addr_copy(attrs->smac, addr);
-		n = neigh_lookup(&arp_tbl, &attrs->daddr.a4, netdev);
-		if (!n) {
-			n = neigh_create(&arp_tbl, &attrs->daddr.a4, netdev);
-			if (IS_ERR(n))
-				return;
-			neigh_event_send(n, NULL);
-			attrs->drop = true;
-			break;
-		}
-		neigh_ha_snapshot(addr, n, netdev);
-		ether_addr_copy(attrs->dmac, addr);
+		src = attrs->smac;
+		dst = attrs->dmac;
+		pkey = &attrs->daddr.a4;
 		break;
 	default:
 		return;
 	}
+
+	ether_addr_copy(src, addr);
+	n = neigh_lookup(&arp_tbl, pkey, netdev);
+	if (!n) {
+		n = neigh_create(&arp_tbl, pkey, netdev);
+		if (IS_ERR(n))
+			return;
+		neigh_event_send(n, NULL);
+		attrs->drop = true;
+	} else {
+		neigh_ha_snapshot(addr, n, netdev);
+		ether_addr_copy(dst, addr);
+	}
 	neigh_release(n);
 }
 
-- 
2.40.0

Re: [PATCH net-next 5/5] net/mlx5e: Refactor duplicated code in mlx5e_ipsec_init_macs

[Simon Horman]

On Thu, Apr 20, 2023 at 11:02:51AM +0300, Leon Romanovsky wrote:
> From: Leon Romanovsky <leonro@nvidia.com>
> 
> ARP discovery code has same logic for RX and TX flows, but with
> different source and destination fields. Instead of duplicating
> same code in mlx5e_ipsec_init_macs, let's refactor.
> 
> Signed-off-by: Leon Romanovsky <leonro@nvidia.com>

Reviewed-by: Simon Horman <simon.horman@corigine.com>

Re: [PATCH net-next 0/5] Fixes to mlx5 IPsec implementation

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by David S. Miller <davem@davemloft.net>:

On Thu, 20 Apr 2023 11:02:46 +0300 you wrote:
> From: Leon Romanovsky <leonro@nvidia.com>
> 
> Hi,
> 
> This small patchset includes various fixes and one refactoring patch
> which I collected for the features sent in this cycle, with one exception -
> first patch.
> 
> [...]

Here is the summary with links:
  - [net-next,1/5] net/mlx5e: Fix FW error while setting IPsec policy block action
    https://git.kernel.org/netdev/net-next/c/e239e31ae802
  - [net-next,2/5] net/mlx5e: Don't overwrite extack message returned from IPsec SA validator
    https://git.kernel.org/netdev/net-next/c/697b3518ebfd
  - [net-next,3/5] net/mlx5e: Compare all fields in IPv6 address
    https://git.kernel.org/netdev/net-next/c/3198ae7d42af
  - [net-next,4/5] net/mlx5e: Properly release work data structure
    https://git.kernel.org/netdev/net-next/c/94edec448479
  - [net-next,5/5] net/mlx5e: Refactor duplicated code in mlx5e_ipsec_init_macs
    https://git.kernel.org/netdev/net-next/c/45fd01f2fbf1

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html