[pull request][net 00/15] mlx5 fixes 2022-11-24

[pull request][net 00/15] mlx5 fixes 2022-11-24

[Saeed Mahameed]

From: Saeed Mahameed <saeedm@nvidia.com>

This series provides bug fixes to mlx5 driver.

Focusing on error handling and proper memory management in mlx5, in
general and in the newly added macsec module.

I still have few fixes left in my queue and I hope those will be the
last ones for mlx5 for this cycle.

Please pull and let me know if there is any problem.

Happy thanksgiving.

Thanks,
Saeed.


The following changes since commit cd07eadd5147ffdae11b6fd28b77a3872f2a2484:

  octeontx2-pf: Add check for devm_kcalloc (2022-11-24 08:34:45 +0100)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/saeed/linux.git tags/mlx5-fixes-2022-11-24

for you to fetch changes up to 9034b29251818ab7b4b38bf6ca860cee45d2726a:

  net/mlx5e: MACsec, block offload requests with encrypt off (2022-11-24 00:03:23 -0800)

----------------------------------------------------------------
mlx5-fixes-2022-11-24

----------------------------------------------------------------
Chris Mi (2):
      net/mlx5: E-switch, Destroy legacy fdb table when needed
      net/mlx5: E-switch, Fix duplicate lag creation

Dan Carpenter (1):
      net/mlx5e: Fix a couple error codes

Emeel Hakim (3):
      net/mlx5e: MACsec, fix add Rx security association (SA) rule memory leak
      net/mlx5e: MACsec, remove replay window size limitation in offload path
      net/mlx5e: MACsec, block offload requests with encrypt off

Raed Salem (5):
      net/mlx5e: MACsec, fix RX data path 16 RX security channel limit
      net/mlx5e: MACsec, fix memory leak when MACsec device is deleted
      net/mlx5e: MACsec, fix update Rx secure channel active field
      net/mlx5e: MACsec, fix mlx5e_macsec_update_rxsa bail condition and functionality
      net/mlx5e: MACsec, fix Tx SA active field update

Roi Dayan (1):
      net/mlx5e: Fix use-after-free when reverting termination table

YueHaibing (3):
      net/mlx5: DR, Fix uninitialized var warning
      net/mlx5: Fix uninitialized variable bug in outlen_write()
      net/mlx5e: Use kvfree() in mlx5e_accel_fs_tcp_create()

 drivers/net/ethernet/mellanox/mlx5/core/cmd.c      |   4 +-
 .../ethernet/mellanox/mlx5/core/en_accel/fs_tcp.c  |   4 +-
 .../ethernet/mellanox/mlx5/core/en_accel/macsec.c  | 138 +++++++++------------
 .../ethernet/mellanox/mlx5/core/en_accel/macsec.h  |   6 +-
 .../mellanox/mlx5/core/en_accel/macsec_fs.c        |  17 ++-
 drivers/net/ethernet/mellanox/mlx5/core/eswitch.c  |   3 +
 drivers/net/ethernet/mellanox/mlx5/core/eswitch.h  |   8 ++
 .../ethernet/mellanox/mlx5/core/eswitch_offloads.c |   7 ++
 .../mellanox/mlx5/core/eswitch_offloads_termtbl.c  |   2 +
 drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c  |   5 +-
 .../mellanox/mlx5/core/steering/dr_table.c         |   5 +-
 include/linux/mlx5/mlx5_ifc.h                      |   7 --
 12 files changed, 105 insertions(+), 101 deletions(-)

[net 01/15] net/mlx5: DR, Fix uninitialized var warning

[Saeed Mahameed]

From: YueHaibing <yuehaibing@huawei.com>

Smatch warns this:

drivers/net/ethernet/mellanox/mlx5/core/steering/dr_table.c:81
 mlx5dr_table_set_miss_action() error: uninitialized symbol 'ret'.

Initializing ret with -EOPNOTSUPP and fix missing action case.

Fixes: 7838e1725394 ("net/mlx5: DR, Expose steering table functionality")
Signed-off-by: YueHaibing <yuehaibing@huawei.com>
Reviewed-by: Roi Dayan <roid@nvidia.com>
Signed-off-by: Saeed Mahameed <saeedm@nvidia.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/steering/dr_table.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/steering/dr_table.c b/drivers/net/ethernet/mellanox/mlx5/core/steering/dr_table.c
index 31d443dd8386..f68461b13391 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/steering/dr_table.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/steering/dr_table.c
@@ -46,7 +46,7 @@ static int dr_table_set_miss_action_nic(struct mlx5dr_domain *dmn,
 int mlx5dr_table_set_miss_action(struct mlx5dr_table *tbl,
 				 struct mlx5dr_action *action)
 {
-	int ret;
+	int ret = -EOPNOTSUPP;
 
 	if (action && action->action_type != DR_ACTION_TYP_FT)
 		return -EOPNOTSUPP;
@@ -67,6 +67,9 @@ int mlx5dr_table_set_miss_action(struct mlx5dr_table *tbl,
 			goto out;
 	}
 
+	if (ret)
+		goto out;
+
 	/* Release old action */
 	if (tbl->miss_action)
 		refcount_dec(&tbl->miss_action->refcount);
-- 
2.38.1

[net 02/15] net/mlx5: E-switch, Destroy legacy fdb table when needed

[Saeed Mahameed]

From: Chris Mi <cmi@nvidia.com>

The cited commit removes eswitch mode none. But when disabling
sriov in legacy mode or changing from switchdev to legacy mode
without sriov enabled, the legacy fdb table is not destroyed.

It is not the right behavior. Destroy legacy fdb table in above
two caes.

Fixes: f019679ea5f2 ("net/mlx5: E-switch, Remove dependency between sriov and eswitch mode")
Signed-off-by: Chris Mi <cmi@nvidia.com>
Reviewed-by: Roi Dayan <roid@nvidia.com>
Reviewed-by: Eli Cohen <elic@nvidia.com>
Reviewed-by: Mark Bloch <mbloch@nvidia.com>
Reviewed-by: Vlad Buslov <vladbu@nvidia.com>
Signed-off-by: Saeed Mahameed <saeedm@nvidia.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/eswitch.c          | 3 +++
 drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c | 7 +++++++
 2 files changed, 10 insertions(+)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
index 2169486c4bfb..374e3fbdc2cf 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
@@ -1362,6 +1362,9 @@ void mlx5_eswitch_disable_sriov(struct mlx5_eswitch *esw, bool clear_vf)
 
 		devl_rate_nodes_destroy(devlink);
 	}
+	/* Destroy legacy fdb when disabling sriov in legacy mode. */
+	if (esw->mode == MLX5_ESWITCH_LEGACY)
+		mlx5_eswitch_disable_locked(esw);
 
 	esw->esw_funcs.num_vfs = 0;
 
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
index 3fda75fe168c..8c6c9bcb3dc3 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
@@ -3387,6 +3387,13 @@ static int esw_offloads_stop(struct mlx5_eswitch *esw,
 	int err;
 
 	esw->mode = MLX5_ESWITCH_LEGACY;
+
+	/* If changing from switchdev to legacy mode without sriov enabled,
+	 * no need to create legacy fdb.
+	 */
+	if (!mlx5_sriov_is_enabled(esw->dev))
+		return 0;
+
 	err = mlx5_eswitch_enable_locked(esw, MLX5_ESWITCH_IGNORE_NUM_VFS);
 	if (err)
 		NL_SET_ERR_MSG_MOD(extack, "Failed setting eswitch to legacy");
-- 
2.38.1

[net 03/15] net/mlx5: E-switch, Fix duplicate lag creation

[Saeed Mahameed]

From: Chris Mi <cmi@nvidia.com>

If creating bond first and then enabling sriov in switchdev mode,
will hit the following syndrome:

mlx5_core 0000:08:00.0: mlx5_cmd_out_err:778:(pid 25543): CREATE_LAG(0x840) op_mod(0x0) failed, status bad parameter(0x3), syndrome (0x7d49cb), err(-22)

The reason is because the offending patch removes eswitch mode
none. In vf lag, the checking of eswitch mode none is replaced
by checking if sriov is enabled. But when driver enables sriov,
it triggers the bond workqueue task first and then setting sriov
number in pci_enable_sriov(). So the check fails.

Fix it by checking if sriov is enabled using eswitch internal
counter that is set before triggering the bond workqueue task.

Fixes: f019679ea5f2 ("net/mlx5: E-switch, Remove dependency between sriov and eswitch mode")
Signed-off-by: Chris Mi <cmi@nvidia.com>
Reviewed-by: Roi Dayan <roid@nvidia.com>
Reviewed-by: Mark Bloch <mbloch@nvidia.com>
Reviewed-by: Vlad Buslov <vladbu@nvidia.com>
Signed-off-by: Saeed Mahameed <saeedm@nvidia.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/eswitch.h | 8 ++++++++
 drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c | 5 +++--
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
index f68dc2d0dbe6..3029bc1c0dd0 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
@@ -736,6 +736,14 @@ void mlx5_eswitch_offloads_destroy_single_fdb(struct mlx5_eswitch *master_esw,
 					      struct mlx5_eswitch *slave_esw);
 int mlx5_eswitch_reload_reps(struct mlx5_eswitch *esw);
 
+static inline int mlx5_eswitch_num_vfs(struct mlx5_eswitch *esw)
+{
+	if (mlx5_esw_allowed(esw))
+		return esw->esw_funcs.num_vfs;
+
+	return 0;
+}
+
 #else  /* CONFIG_MLX5_ESWITCH */
 /* eswitch API stubs */
 static inline int  mlx5_eswitch_init(struct mlx5_core_dev *dev) { return 0; }
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c b/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
index be1307a63e6d..4070dc1d17cb 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
@@ -701,8 +701,9 @@ static bool mlx5_lag_check_prereq(struct mlx5_lag *ldev)
 
 #ifdef CONFIG_MLX5_ESWITCH
 	dev = ldev->pf[MLX5_LAG_P1].dev;
-	if ((mlx5_sriov_is_enabled(dev)) && !is_mdev_switchdev_mode(dev))
-		return false;
+	for (i = 0; i  < ldev->ports; i++)
+		if (mlx5_eswitch_num_vfs(dev->priv.eswitch) && !is_mdev_switchdev_mode(dev))
+			return false;
 
 	mode = mlx5_eswitch_mode(dev);
 	for (i = 0; i < ldev->ports; i++)
-- 
2.38.1

[net 04/15] net/mlx5: Fix uninitialized variable bug in outlen_write()

[Saeed Mahameed]

From: YueHaibing <yuehaibing@huawei.com>

If sscanf() return 0, outlen is uninitialized and used in kzalloc(),
this is unexpected. We should return -EINVAL if the string is invalid.

Fixes: e126ba97dba9 ("mlx5: Add driver for Mellanox Connect-IB adapters")
Signed-off-by: YueHaibing <yuehaibing@huawei.com>
Reviewed-by: Leon Romanovsky <leonro@nvidia.com>
Signed-off-by: Saeed Mahameed <saeedm@nvidia.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/cmd.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/cmd.c b/drivers/net/ethernet/mellanox/mlx5/core/cmd.c
index 74bd05e5dda2..e7a894ba5c3e 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/cmd.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/cmd.c
@@ -1497,8 +1497,8 @@ static ssize_t outlen_write(struct file *filp, const char __user *buf,
 		return -EFAULT;
 
 	err = sscanf(outlen_str, "%d", &outlen);
-	if (err < 0)
-		return err;
+	if (err != 1)
+		return -EINVAL;
 
 	ptr = kzalloc(outlen, GFP_KERNEL);
 	if (!ptr)
-- 
2.38.1

[net 05/15] net/mlx5e: Fix use-after-free when reverting termination table

[Saeed Mahameed]

From: Roi Dayan <roid@nvidia.com>

When having multiple dests with termination tables and second one
or afterwards fails the driver reverts usage of term tables but
doesn't reset the assignment in attr->dests[num_vport_dests].termtbl
which case a use-after-free when releasing the rule.
Fix by resetting the assignment of termtbl to null.

Fixes: 10caabdaad5a ("net/mlx5e: Use termination table for VLAN push actions")
Signed-off-by: Roi Dayan <roid@nvidia.com>
Reviewed-by: Maor Dickman <maord@nvidia.com>
Signed-off-by: Saeed Mahameed <saeedm@nvidia.com>
---
 .../net/ethernet/mellanox/mlx5/core/eswitch_offloads_termtbl.c  | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads_termtbl.c b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads_termtbl.c
index 108a3503f413..edd910258314 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads_termtbl.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads_termtbl.c
@@ -312,6 +312,8 @@ mlx5_eswitch_add_termtbl_rule(struct mlx5_eswitch *esw,
 	for (curr_dest = 0; curr_dest < num_vport_dests; curr_dest++) {
 		struct mlx5_termtbl_handle *tt = attr->dests[curr_dest].termtbl;
 
+		attr->dests[curr_dest].termtbl = NULL;
+
 		/* search for the destination associated with the
 		 * current term table
 		 */
-- 
2.38.1

[net 06/15] net/mlx5e: Fix a couple error codes

[Saeed Mahameed]

From: Dan Carpenter <dan.carpenter@oracle.com>

If kvzalloc() fails then return -ENOMEM.  Don't return success.

Fixes: 3b20949cb21b ("net/mlx5e: Add MACsec RX steering rules")
Fixes: e467b283ffd5 ("net/mlx5e: Add MACsec TX steering rules")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Saeed Mahameed <saeedm@nvidia.com>
---
 .../ethernet/mellanox/mlx5/core/en_accel/macsec_fs.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec_fs.c b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec_fs.c
index 1ac0cf04e811..96cec6d826c2 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec_fs.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec_fs.c
@@ -250,7 +250,7 @@ static int macsec_fs_tx_create(struct mlx5e_macsec_fs *macsec_fs)
 	struct mlx5_flow_handle *rule;
 	struct mlx5_flow_spec *spec;
 	u32 *flow_group_in;
-	int err = 0;
+	int err;
 
 	ns = mlx5_get_flow_namespace(macsec_fs->mdev, MLX5_FLOW_NAMESPACE_EGRESS_MACSEC);
 	if (!ns)
@@ -261,8 +261,10 @@ static int macsec_fs_tx_create(struct mlx5e_macsec_fs *macsec_fs)
 		return -ENOMEM;
 
 	flow_group_in = kvzalloc(inlen, GFP_KERNEL);
-	if (!flow_group_in)
+	if (!flow_group_in) {
+		err = -ENOMEM;
 		goto out_spec;
+	}
 
 	tx_tables = &tx_fs->tables;
 	ft_crypto = &tx_tables->ft_crypto;
@@ -898,7 +900,7 @@ static int macsec_fs_rx_create(struct mlx5e_macsec_fs *macsec_fs)
 	struct mlx5_flow_handle *rule;
 	struct mlx5_flow_spec *spec;
 	u32 *flow_group_in;
-	int err = 0;
+	int err;
 
 	ns = mlx5_get_flow_namespace(macsec_fs->mdev, MLX5_FLOW_NAMESPACE_KERNEL_RX_MACSEC);
 	if (!ns)
@@ -909,8 +911,10 @@ static int macsec_fs_rx_create(struct mlx5e_macsec_fs *macsec_fs)
 		return -ENOMEM;
 
 	flow_group_in = kvzalloc(inlen, GFP_KERNEL);
-	if (!flow_group_in)
+	if (!flow_group_in) {
+		err = -ENOMEM;
 		goto free_spec;
+	}
 
 	rx_tables = &rx_fs->tables;
 	ft_crypto = &rx_tables->ft_crypto;
-- 
2.38.1

[net 07/15] net/mlx5e: Use kvfree() in mlx5e_accel_fs_tcp_create()

[Saeed Mahameed]

From: YueHaibing <yuehaibing@huawei.com>

'accel_tcp' is allocated by kvzalloc(), which should freed by kvfree().

Fixes: f52f2faee581 ("net/mlx5e: Introduce flow steering API")
Signed-off-by: YueHaibing <yuehaibing@huawei.com>
Reviewed-by: Tariq Toukan <tariqt@nvidia.com>
Signed-off-by: Saeed Mahameed <saeedm@nvidia.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/en_accel/fs_tcp.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/fs_tcp.c b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/fs_tcp.c
index 285d32d2fd08..d7c020f72401 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/fs_tcp.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/fs_tcp.c
@@ -365,7 +365,7 @@ void mlx5e_accel_fs_tcp_destroy(struct mlx5e_flow_steering *fs)
 	for (i = 0; i < ACCEL_FS_TCP_NUM_TYPES; i++)
 		accel_fs_tcp_destroy_table(fs, i);
 
-	kfree(accel_tcp);
+	kvfree(accel_tcp);
 	mlx5e_fs_set_accel_tcp(fs, NULL);
 }
 
@@ -397,7 +397,7 @@ int mlx5e_accel_fs_tcp_create(struct mlx5e_flow_steering *fs)
 err_destroy_tables:
 	while (--i >= 0)
 		accel_fs_tcp_destroy_table(fs, i);
-	kfree(accel_tcp);
+	kvfree(accel_tcp);
 	mlx5e_fs_set_accel_tcp(fs, NULL);
 	return err;
 }
-- 
2.38.1

[net 08/15] net/mlx5e: MACsec, fix RX data path 16 RX security channel limit

[Saeed Mahameed]

From: Raed Salem <raeds@nvidia.com>

Currently the data path metadata flow id mask wrongly limits the
number of different RX security channels (SC) to 16, whereas in
adding RX SC the limit is "2^16 - 1" this cause an overlap in
metadata flow id once more than 16 RX SCs is added, this corrupts
MACsec RX offloaded flow handling.

Fix by using the correct mask, while at it improve code to use this
mask when adding the Rx rule and improve visibility of such errors
by adding debug massage.

Fixes: b7c9400cbc48 ("net/mlx5e: Implement MACsec Rx data path using MACsec skb_metadata_dst")
Signed-off-by: Raed Salem <raeds@nvidia.com>
Reviewed-by: Emeel Hakim <ehakim@nvidia.com>
Signed-off-by: Saeed Mahameed <saeedm@nvidia.com>
---
 .../net/ethernet/mellanox/mlx5/core/en_accel/macsec.c | 11 ++++++++---
 .../net/ethernet/mellanox/mlx5/core/en_accel/macsec.h |  6 ++++--
 .../ethernet/mellanox/mlx5/core/en_accel/macsec_fs.c  |  4 ++--
 3 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c
index 3dc6c987b8da..96fa553ef93a 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c
@@ -736,9 +736,14 @@ static int mlx5e_macsec_add_rxsc(struct macsec_context *ctx)
 
 	sc_xarray_element->rx_sc = rx_sc;
 	err = xa_alloc(&macsec->sc_xarray, &sc_xarray_element->fs_id, sc_xarray_element,
-		       XA_LIMIT(1, USHRT_MAX), GFP_KERNEL);
-	if (err)
+		       XA_LIMIT(1, MLX5_MACEC_RX_FS_ID_MAX), GFP_KERNEL);
+	if (err) {
+		if (err == -EBUSY)
+			netdev_err(ctx->netdev,
+				   "MACsec offload: unable to create entry for RX SC (%d Rx SCs already allocated)\n",
+				   MLX5_MACEC_RX_FS_ID_MAX);
 		goto destroy_sc_xarray_elemenet;
+	}
 
 	rx_sc->md_dst = metadata_dst_alloc(0, METADATA_MACSEC, GFP_KERNEL);
 	if (!rx_sc->md_dst) {
@@ -1748,7 +1753,7 @@ void mlx5e_macsec_offload_handle_rx_skb(struct net_device *netdev,
 	if (!macsec)
 		return;
 
-	fs_id = MLX5_MACSEC_METADATA_HANDLE(macsec_meta_data);
+	fs_id = MLX5_MACSEC_RX_METADAT_HANDLE(macsec_meta_data);
 
 	rcu_read_lock();
 	sc_xarray_element = xa_load(&macsec->sc_xarray, fs_id);
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.h b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.h
index d580b4a91253..347380a2cd9c 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.h
@@ -10,9 +10,11 @@
 #include <net/macsec.h>
 #include <net/dst_metadata.h>
 
-/* Bit31 - 30: MACsec marker, Bit3-0: MACsec id */
+/* Bit31 - 30: MACsec marker, Bit15-0: MACsec id */
+#define MLX5_MACEC_RX_FS_ID_MAX USHRT_MAX /* Must be power of two */
+#define MLX5_MACSEC_RX_FS_ID_MASK MLX5_MACEC_RX_FS_ID_MAX
 #define MLX5_MACSEC_METADATA_MARKER(metadata)  ((((metadata) >> 30) & 0x3)  == 0x1)
-#define MLX5_MACSEC_METADATA_HANDLE(metadata)  ((metadata) & GENMASK(3, 0))
+#define MLX5_MACSEC_RX_METADAT_HANDLE(metadata)  ((metadata) & MLX5_MACSEC_RX_FS_ID_MASK)
 
 struct mlx5e_priv;
 struct mlx5e_macsec;
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec_fs.c b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec_fs.c
index 96cec6d826c2..f87a1c4021bc 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec_fs.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec_fs.c
@@ -1146,10 +1146,10 @@ macsec_fs_rx_add_rule(struct mlx5e_macsec_fs *macsec_fs,
 	ft_crypto = &rx_tables->ft_crypto;
 
 	/* Set bit[31 - 30] macsec marker - 0x01 */
-	/* Set bit[3-0] fs id */
+	/* Set bit[15-0] fs id */
 	MLX5_SET(set_action_in, action, action_type, MLX5_ACTION_TYPE_SET);
 	MLX5_SET(set_action_in, action, field, MLX5_ACTION_IN_FIELD_METADATA_REG_B);
-	MLX5_SET(set_action_in, action, data, fs_id | BIT(30));
+	MLX5_SET(set_action_in, action, data, MLX5_MACSEC_RX_METADAT_HANDLE(fs_id) | BIT(30));
 	MLX5_SET(set_action_in, action, offset, 0);
 	MLX5_SET(set_action_in, action, length, 32);
 
-- 
2.38.1

[net 09/15] net/mlx5e: MACsec, fix memory leak when MACsec device is deleted

[Saeed Mahameed]

From: Raed Salem <raeds@nvidia.com>

When the MACsec netdevice is deleted, all related Rx/Tx HW/SW
states should be released/deallocated, however currently part
of the Rx security channel association data is not cleaned
properly, hence the memory leaks.

Fix by make sure all related Rx Sc resources are cleaned/freed,
while at it improve code by grouping release SC context in a
function so it can be used in both delete MACsec device and
delete Rx SC operations.

Fixes: 5a39816a75e5 ("net/mlx5e: Add MACsec offload SecY support")
Signed-off-by: Raed Salem <raeds@nvidia.com>
Reviewed-by: Emeel Hakim <ehakim@nvidia.com>
Signed-off-by: Saeed Mahameed <saeedm@nvidia.com>
---
 .../mellanox/mlx5/core/en_accel/macsec.c      | 77 ++++++++-----------
 1 file changed, 33 insertions(+), 44 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c
index 96fa553ef93a..b51de07d5bad 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c
@@ -823,16 +823,43 @@ static int mlx5e_macsec_upd_rxsc(struct macsec_context *ctx)
 	return err;
 }
 
+static void macsec_del_rxsc_ctx(struct mlx5e_macsec *macsec, struct mlx5e_macsec_rx_sc *rx_sc)
+{
+	struct mlx5e_macsec_sa *rx_sa;
+	int i;
+
+	for (i = 0; i < MACSEC_NUM_AN; ++i) {
+		rx_sa = rx_sc->rx_sa[i];
+		if (!rx_sa)
+			continue;
+
+		mlx5e_macsec_cleanup_sa(macsec, rx_sa, false);
+		mlx5_destroy_encryption_key(macsec->mdev, rx_sa->enc_key_id);
+
+		kfree(rx_sa);
+		rx_sc->rx_sa[i] = NULL;
+	}
+
+	/* At this point the relevant MACsec offload Rx rule already removed at
+	 * mlx5e_macsec_cleanup_sa need to wait for datapath to finish current
+	 * Rx related data propagating using xa_erase which uses rcu to sync,
+	 * once fs_id is erased then this rx_sc is hidden from datapath.
+	 */
+	list_del_rcu(&rx_sc->rx_sc_list_element);
+	xa_erase(&macsec->sc_xarray, rx_sc->sc_xarray_element->fs_id);
+	metadata_dst_free(rx_sc->md_dst);
+	kfree(rx_sc->sc_xarray_element);
+	kfree_rcu(rx_sc);
+}
+
 static int mlx5e_macsec_del_rxsc(struct macsec_context *ctx)
 {
 	struct mlx5e_priv *priv = netdev_priv(ctx->netdev);
 	struct mlx5e_macsec_device *macsec_device;
 	struct mlx5e_macsec_rx_sc *rx_sc;
-	struct mlx5e_macsec_sa *rx_sa;
 	struct mlx5e_macsec *macsec;
 	struct list_head *list;
 	int err = 0;
-	int i;
 
 	mutex_lock(&priv->macsec->lock);
 
@@ -854,31 +881,7 @@ static int mlx5e_macsec_del_rxsc(struct macsec_context *ctx)
 		goto out;
 	}
 
-	for (i = 0; i < MACSEC_NUM_AN; ++i) {
-		rx_sa = rx_sc->rx_sa[i];
-		if (!rx_sa)
-			continue;
-
-		mlx5e_macsec_cleanup_sa(macsec, rx_sa, false);
-		mlx5_destroy_encryption_key(macsec->mdev, rx_sa->enc_key_id);
-
-		kfree(rx_sa);
-		rx_sc->rx_sa[i] = NULL;
-	}
-
-/*
- * At this point the relevant MACsec offload Rx rule already removed at
- * mlx5e_macsec_cleanup_sa need to wait for datapath to finish current
- * Rx related data propagating using xa_erase which uses rcu to sync,
- * once fs_id is erased then this rx_sc is hidden from datapath.
- */
-	list_del_rcu(&rx_sc->rx_sc_list_element);
-	xa_erase(&macsec->sc_xarray, rx_sc->sc_xarray_element->fs_id);
-	metadata_dst_free(rx_sc->md_dst);
-	kfree(rx_sc->sc_xarray_element);
-
-	kfree_rcu(rx_sc);
-
+	macsec_del_rxsc_ctx(macsec, rx_sc);
 out:
 	mutex_unlock(&macsec->lock);
 
@@ -1239,7 +1242,6 @@ static int mlx5e_macsec_del_secy(struct macsec_context *ctx)
 	struct mlx5e_priv *priv = netdev_priv(ctx->netdev);
 	struct mlx5e_macsec_device *macsec_device;
 	struct mlx5e_macsec_rx_sc *rx_sc, *tmp;
-	struct mlx5e_macsec_sa *rx_sa;
 	struct mlx5e_macsec_sa *tx_sa;
 	struct mlx5e_macsec *macsec;
 	struct list_head *list;
@@ -1268,28 +1270,15 @@ static int mlx5e_macsec_del_secy(struct macsec_context *ctx)
 	}
 
 	list = &macsec_device->macsec_rx_sc_list_head;
-	list_for_each_entry_safe(rx_sc, tmp, list, rx_sc_list_element) {
-		for (i = 0; i < MACSEC_NUM_AN; ++i) {
-			rx_sa = rx_sc->rx_sa[i];
-			if (!rx_sa)
-				continue;
-
-			mlx5e_macsec_cleanup_sa(macsec, rx_sa, false);
-			mlx5_destroy_encryption_key(macsec->mdev, rx_sa->enc_key_id);
-			kfree(rx_sa);
-			rx_sc->rx_sa[i] = NULL;
-		}
-
-		list_del_rcu(&rx_sc->rx_sc_list_element);
-
-		kfree_rcu(rx_sc);
-	}
+	list_for_each_entry_safe(rx_sc, tmp, list, rx_sc_list_element)
+		macsec_del_rxsc_ctx(macsec, rx_sc);
 
 	kfree(macsec_device->dev_addr);
 	macsec_device->dev_addr = NULL;
 
 	list_del_rcu(&macsec_device->macsec_device_list_element);
 	--macsec->num_of_devices;
+	kfree(macsec_device);
 
 out:
 	mutex_unlock(&macsec->lock);
-- 
2.38.1

[net 10/15] net/mlx5e: MACsec, fix update Rx secure channel active field

[Saeed Mahameed]

From: Raed Salem <raeds@nvidia.com>

The main functionality for this operation is to update the
active state of the Rx security channel (SC) if the new
active setting is different from the current active state
of this Rx SC, however the relevant active state check is
done post updating the current active state to match the
new active state, effectively blocks any offload state
update for the Rx SC in question.

Fix by delay the assignment to be post the relevant check.

Fixes: aae3454e4d4c ("net/mlx5e: Add MACsec offload Rx command support")
Signed-off-by: Raed Salem <raeds@nvidia.com>
Reviewed-by: Emeel Hakim <ehakim@nvidia.com>
Signed-off-by: Saeed Mahameed <saeedm@nvidia.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c
index b51de07d5bad..9c891a877998 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c
@@ -803,10 +803,10 @@ static int mlx5e_macsec_upd_rxsc(struct macsec_context *ctx)
 		goto out;
 	}
 
-	rx_sc->active = ctx_rx_sc->active;
 	if (rx_sc->active == ctx_rx_sc->active)
 		goto out;
 
+	rx_sc->active = ctx_rx_sc->active;
 	for (i = 0; i < MACSEC_NUM_AN; ++i) {
 		rx_sa = rx_sc->rx_sa[i];
 		if (!rx_sa)
-- 
2.38.1

[net 11/15] net/mlx5e: MACsec, fix mlx5e_macsec_update_rxsa bail condition and functionality

[Saeed Mahameed]

From: Raed Salem <raeds@nvidia.com>

Fix update Rx SA wrong bail condition, naturally update functionality
needs to check that something changed otherwise bailout currently the
active state check does just the opposite, furthermore unlike deactivate
path which remove the macsec rules to deactivate the offload, the
activation path does not include the counter part installation of the
macsec rules.

Fix by using correct bailout condition and when Rx SA changes state to
active then add the relevant macsec rules.

While at it, refine function name to reflect more precisely its role.

Fixes: aae3454e4d4c ("net/mlx5e: Add MACsec offload Rx command support")
Signed-off-by: Raed Salem <raeds@nvidia.com>
Reviewed-by: Emeel Hakim <ehakim@nvidia.com>
Signed-off-by: Saeed Mahameed <saeedm@nvidia.com>
---
 .../mellanox/mlx5/core/en_accel/macsec.c      | 24 +++++++++----------
 1 file changed, 11 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c
index 9c891a877998..c19581f1f733 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c
@@ -427,15 +427,15 @@ mlx5e_macsec_get_rx_sc_from_sc_list(const struct list_head *list, sci_t sci)
 	return NULL;
 }
 
-static int mlx5e_macsec_update_rx_sa(struct mlx5e_macsec *macsec,
-				     struct mlx5e_macsec_sa *rx_sa,
-				     bool active)
+static int macsec_rx_sa_active_update(struct macsec_context *ctx,
+				      struct mlx5e_macsec_sa *rx_sa,
+				      bool active)
 {
-	struct mlx5_core_dev *mdev = macsec->mdev;
-	struct mlx5_macsec_obj_attrs attrs = {};
+	struct mlx5e_priv *priv = netdev_priv(ctx->netdev);
+	struct mlx5e_macsec *macsec = priv->macsec;
 	int err = 0;
 
-	if (rx_sa->active != active)
+	if (rx_sa->active == active)
 		return 0;
 
 	rx_sa->active = active;
@@ -444,13 +444,11 @@ static int mlx5e_macsec_update_rx_sa(struct mlx5e_macsec *macsec,
 		return 0;
 	}
 
-	attrs.sci = cpu_to_be64((__force u64)rx_sa->sci);
-	attrs.enc_key_id = rx_sa->enc_key_id;
-	err = mlx5e_macsec_create_object(mdev, &attrs, false, &rx_sa->macsec_obj_id);
+	err = mlx5e_macsec_init_sa(ctx, rx_sa, true, false);
 	if (err)
-		return err;
+		rx_sa->active = false;
 
-	return 0;
+	return err;
 }
 
 static bool mlx5e_macsec_secy_features_validate(struct macsec_context *ctx)
@@ -812,7 +810,7 @@ static int mlx5e_macsec_upd_rxsc(struct macsec_context *ctx)
 		if (!rx_sa)
 			continue;
 
-		err = mlx5e_macsec_update_rx_sa(macsec, rx_sa, rx_sa->active && ctx_rx_sc->active);
+		err = macsec_rx_sa_active_update(ctx, rx_sa, rx_sa->active && ctx_rx_sc->active);
 		if (err)
 			goto out;
 	}
@@ -1023,7 +1021,7 @@ static int mlx5e_macsec_upd_rxsa(struct macsec_context *ctx)
 		goto out;
 	}
 
-	err = mlx5e_macsec_update_rx_sa(macsec, rx_sa, ctx_rx_sa->active);
+	err = macsec_rx_sa_active_update(ctx, rx_sa, ctx_rx_sa->active);
 out:
 	mutex_unlock(&macsec->lock);
 
-- 
2.38.1

[net 12/15] net/mlx5e: MACsec, fix add Rx security association (SA) rule memory leak

[Saeed Mahameed]

From: Emeel Hakim <ehakim@nvidia.com>

Currently MACsec's add Rx SA flow steering (fs) rule routine
uses a spec object which is dynamically allocated and do
not free it upon leaving. The above led to a memory leak.

Fix by freeing dynamically allocated objects.

Fixes: 3b20949cb21b ("net/mlx5e: Add MACsec RX steering rules")
Signed-off-by: Emeel Hakim <ehakim@nvidia.com>
Reviewed-by: Raed Salem <raeds@nvidia.com>
Signed-off-by: Saeed Mahameed <saeedm@nvidia.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec_fs.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec_fs.c b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec_fs.c
index f87a1c4021bc..5b658a5588c6 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec_fs.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec_fs.c
@@ -1209,6 +1209,7 @@ macsec_fs_rx_add_rule(struct mlx5e_macsec_fs *macsec_fs,
 		rx_rule->rule[1] = rule;
 	}
 
+	kvfree(spec);
 	return macsec_rule;
 
 err:
-- 
2.38.1

[net 13/15] net/mlx5e: MACsec, remove replay window size limitation in offload path

[Saeed Mahameed]

From: Emeel Hakim <ehakim@nvidia.com>

Currently offload path limits replay window size to 32/64/128/256 bits,
such a limitation should not exist since software allows it.
Remove such limitation.

Fixes: eb43846b43c3 ("net/mlx5e: Support MACsec offload replay window")
Signed-off-by: Emeel Hakim <ehakim@nvidia.com>
Reviewed-by: Raed Salem <raeds@nvidia.com>
Signed-off-by: Saeed Mahameed <saeedm@nvidia.com>
---
 .../mellanox/mlx5/core/en_accel/macsec.c         | 16 ----------------
 include/linux/mlx5/mlx5_ifc.h                    |  7 -------
 2 files changed, 23 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c
index c19581f1f733..72f8be65fa90 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c
@@ -229,22 +229,6 @@ static int macsec_set_replay_protection(struct mlx5_macsec_obj_attrs *attrs, voi
 	if (!attrs->replay_protect)
 		return 0;
 
-	switch (attrs->replay_window) {
-	case 256:
-		window_sz = MLX5_MACSEC_ASO_REPLAY_WIN_256BIT;
-		break;
-	case 128:
-		window_sz = MLX5_MACSEC_ASO_REPLAY_WIN_128BIT;
-		break;
-	case 64:
-		window_sz = MLX5_MACSEC_ASO_REPLAY_WIN_64BIT;
-		break;
-	case 32:
-		window_sz = MLX5_MACSEC_ASO_REPLAY_WIN_32BIT;
-		break;
-	default:
-		return -EINVAL;
-	}
 	MLX5_SET(macsec_aso, aso_ctx, window_size, window_sz);
 	MLX5_SET(macsec_aso, aso_ctx, mode, MLX5_MACSEC_ASO_REPLAY_PROTECTION);
 
diff --git a/include/linux/mlx5/mlx5_ifc.h b/include/linux/mlx5/mlx5_ifc.h
index 5a4e914e2a6f..981fc7dfa408 100644
--- a/include/linux/mlx5/mlx5_ifc.h
+++ b/include/linux/mlx5/mlx5_ifc.h
@@ -11611,13 +11611,6 @@ enum {
 	MLX5_MACSEC_ASO_REPLAY_PROTECTION = 0x1,
 };
 
-enum {
-	MLX5_MACSEC_ASO_REPLAY_WIN_32BIT  = 0x0,
-	MLX5_MACSEC_ASO_REPLAY_WIN_64BIT  = 0x1,
-	MLX5_MACSEC_ASO_REPLAY_WIN_128BIT = 0x2,
-	MLX5_MACSEC_ASO_REPLAY_WIN_256BIT = 0x3,
-};
-
 #define MLX5_MACSEC_ASO_INC_SN  0x2
 #define MLX5_MACSEC_ASO_REG_C_4_5 0x2
 
-- 
2.38.1

[net 14/15] net/mlx5e: MACsec, fix Tx SA active field update

[Saeed Mahameed]

From: Raed Salem <raeds@nvidia.com>

Currently during update Tx security association (SA) flow, the Tx SA
active state is updated only if the Tx SA in question is the same SA
that the MACsec interface is using for Tx,in consequence when the
MACsec interface chose to work with this Tx SA later, where this SA
for example should have been updated to active state and it was not,
the relevant Tx SA HW context won't be installed, hence the MACSec
flow won't be offloaded.

Fix by update Tx SA active state as part of update flow regardless
whether the SA in question is the same Tx SA used by the MACsec
interface.

Fixes: 8ff0ac5be144 ("net/mlx5: Add MACsec offload Tx command support")
Signed-off-by: Raed Salem <raeds@nvidia.com>
Reviewed-by: Emeel Hakim <ehakim@nvidia.com>
Signed-off-by: Saeed Mahameed <saeedm@nvidia.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c
index 72f8be65fa90..137b34347de1 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c
@@ -602,6 +602,7 @@ static int mlx5e_macsec_upd_txsa(struct macsec_context *ctx)
 	if (tx_sa->active == ctx_tx_sa->active)
 		goto out;
 
+	tx_sa->active = ctx_tx_sa->active;
 	if (tx_sa->assoc_num != tx_sc->encoding_sa)
 		goto out;
 
@@ -617,8 +618,6 @@ static int mlx5e_macsec_upd_txsa(struct macsec_context *ctx)
 
 		mlx5e_macsec_cleanup_sa(macsec, tx_sa, true);
 	}
-
-	tx_sa->active = ctx_tx_sa->active;
 out:
 	mutex_unlock(&macsec->lock);
 
-- 
2.38.1

[net 15/15] net/mlx5e: MACsec, block offload requests with encrypt off

[Saeed Mahameed]

From: Emeel Hakim <ehakim@nvidia.com>

Currently offloading MACsec with authentication only (encrypt
property set to off) is not supported, block such requests
when adding/updating a macsec device.

Fixes: 8ff0ac5be144 ("net/mlx5: Add MACsec offload Tx command support")
Signed-off-by: Emeel Hakim <ehakim@nvidia.com>
Reviewed-by: Raed Salem <raeds@nvidia.com>
Signed-off-by: Saeed Mahameed <saeedm@nvidia.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c
index 137b34347de1..0d6dc394a12a 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c
@@ -458,6 +458,11 @@ static bool mlx5e_macsec_secy_features_validate(struct macsec_context *ctx)
 		return false;
 	}
 
+	if (!ctx->secy->tx_sc.encrypt) {
+		netdev_err(netdev, "MACsec offload: encrypt off isn't supported\n");
+		return false;
+	}
+
 	return true;
 }
 
-- 
2.38.1

Re: [net 07/15] net/mlx5e: Use kvfree() in mlx5e_accel_fs_tcp_create()

[Tariq Toukan]

On 11/24/2022 10:10 AM, Saeed Mahameed wrote:
> From: YueHaibing <yuehaibing@huawei.com>
> 
> 'accel_tcp' is allocated by kvzalloc(), which should freed by kvfree().
> 
> Fixes: f52f2faee581 ("net/mlx5e: Introduce flow steering API")
> Signed-off-by: YueHaibing <yuehaibing@huawei.com>
> Reviewed-by: Tariq Toukan <tariqt@nvidia.com>
> Signed-off-by: Saeed Mahameed <saeedm@nvidia.com>
> ---

Hi Saeed,
There was a v3 of this, that changes the alloc side instead.

Re: [net 07/15] net/mlx5e: Use kvfree() in mlx5e_accel_fs_tcp_create()

[Saeed Mahameed]

On 24 Nov 10:32, Tariq Toukan wrote:
>
>
>On 11/24/2022 10:10 AM, Saeed Mahameed wrote:
>>From: YueHaibing <yuehaibing@huawei.com>
>>
>>'accel_tcp' is allocated by kvzalloc(), which should freed by kvfree().
>>
>>Fixes: f52f2faee581 ("net/mlx5e: Introduce flow steering API")
>>Signed-off-by: YueHaibing <yuehaibing@huawei.com>
>>Reviewed-by: Tariq Toukan <tariqt@nvidia.com>
>>Signed-off-by: Saeed Mahameed <saeedm@nvidia.com>
>>---
>
>Hi Saeed,
>There was a v3 of this, that changes the alloc side instead.
>
Thanks Tariq, that patch was Marked for -next for some reason,
and it's in my net-next queue, i think it's ok if this went to net and the
other to net-next.

Re: [net 03/15] net/mlx5: E-switch, Fix duplicate lag creation

[Jacob Keller]

On 11/24/2022 12:10 AM, Saeed Mahameed wrote:
> From: Chris Mi <cmi@nvidia.com>
> 
> If creating bond first and then enabling sriov in switchdev mode,
> will hit the following syndrome:
> 
> mlx5_core 0000:08:00.0: mlx5_cmd_out_err:778:(pid 25543): CREATE_LAG(0x840) op_mod(0x0) failed, status bad parameter(0x3), syndrome (0x7d49cb), err(-22)
> 
> The reason is because the offending patch removes eswitch mode
> none. In vf lag, the checking of eswitch mode none is replaced
> by checking if sriov is enabled. But when driver enables sriov,
> it triggers the bond workqueue task first and then setting sriov
> number in pci_enable_sriov(). So the check fails.
> 
> Fix it by checking if sriov is enabled using eswitch internal
> counter that is set before triggering the bond workqueue task.
> 
> Fixes: f019679ea5f2 ("net/mlx5: E-switch, Remove dependency between sriov and eswitch mode")
> Signed-off-by: Chris Mi <cmi@nvidia.com>
> Reviewed-by: Roi Dayan <roid@nvidia.com>
> Reviewed-by: Mark Bloch <mbloch@nvidia.com>
> Reviewed-by: Vlad Buslov <vladbu@nvidia.com>
> Signed-off-by: Saeed Mahameed <saeedm@nvidia.com>
> ---
>   drivers/net/ethernet/mellanox/mlx5/core/eswitch.h | 8 ++++++++
>   drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c | 5 +++--
>   2 files changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
> index f68dc2d0dbe6..3029bc1c0dd0 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
> @@ -736,6 +736,14 @@ void mlx5_eswitch_offloads_destroy_single_fdb(struct mlx5_eswitch *master_esw,
>   					      struct mlx5_eswitch *slave_esw);
>   int mlx5_eswitch_reload_reps(struct mlx5_eswitch *esw);
>   
> +static inline int mlx5_eswitch_num_vfs(struct mlx5_eswitch *esw)
> +{
> +	if (mlx5_esw_allowed(esw))
> +		return esw->esw_funcs.num_vfs;
> +
> +	return 0;
> +}
> +
>   #else  /* CONFIG_MLX5_ESWITCH */
>   /* eswitch API stubs */
>   static inline int  mlx5_eswitch_init(struct mlx5_core_dev *dev) { return 0; }
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c b/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
> index be1307a63e6d..4070dc1d17cb 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
> @@ -701,8 +701,9 @@ static bool mlx5_lag_check_prereq(struct mlx5_lag *ldev)
>   
>   #ifdef CONFIG_MLX5_ESWITCH
>   	dev = ldev->pf[MLX5_LAG_P1].dev;
> -	if ((mlx5_sriov_is_enabled(dev)) && !is_mdev_switchdev_mode(dev))
> -		return false;
> +	for (i = 0; i  < ldev->ports; i++)
> +		if (mlx5_eswitch_num_vfs(dev->priv.eswitch) && !is_mdev_switchdev_mode(dev))
> +			return false;
>   

Am I missing something? whats with the for loop iterator here? i isn't 
used or passed into these functions?

Do you need to check multiple times or do these functions have some side 
effect? But looking at their implementation neither of them appear to 
have side effects?

What am I missing?

Shouldn't this just be:
>> -	if ((mlx5_sriov_is_enabled(dev)) && !is_mdev_switchdev_mode(dev) >> +	if (mlx5_eswitch_num_vfs(dev->priv.eswitch) && 
!is_mdev_switchdev_mode(dev))
>>  		return false;

Re: [net 05/15] net/mlx5e: Fix use-after-free when reverting termination table

[Jacob Keller]

On 11/24/2022 12:10 AM, Saeed Mahameed wrote:
> From: Roi Dayan <roid@nvidia.com>
> 
> When having multiple dests with termination tables and second one
> or afterwards fails the driver reverts usage of term tables but
> doesn't reset the assignment in attr->dests[num_vport_dests].termtbl
> which case a use-after-free when releasing the rule.
> Fix by resetting the assignment of termtbl to null.
> 

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

> Fixes: 10caabdaad5a ("net/mlx5e: Use termination table for VLAN push actions")
> Signed-off-by: Roi Dayan <roid@nvidia.com>
> Reviewed-by: Maor Dickman <maord@nvidia.com>
> Signed-off-by: Saeed Mahameed <saeedm@nvidia.com>
> ---
>   .../net/ethernet/mellanox/mlx5/core/eswitch_offloads_termtbl.c  | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads_termtbl.c b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads_termtbl.c
> index 108a3503f413..edd910258314 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads_termtbl.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads_termtbl.c
> @@ -312,6 +312,8 @@ mlx5_eswitch_add_termtbl_rule(struct mlx5_eswitch *esw,
>   	for (curr_dest = 0; curr_dest < num_vport_dests; curr_dest++) {
>   		struct mlx5_termtbl_handle *tt = attr->dests[curr_dest].termtbl;
>   
> +		attr->dests[curr_dest].termtbl = NULL;
> +
>   		/* search for the destination associated with the
>   		 * current term table
>   		 */

Re: [net 06/15] net/mlx5e: Fix a couple error codes

[Jacob Keller]

On 11/24/2022 12:10 AM, Saeed Mahameed wrote:
> From: Dan Carpenter <dan.carpenter@oracle.com>
> 
> If kvzalloc() fails then return -ENOMEM.  Don't return success.
> 

Makes sense.

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

> Fixes: 3b20949cb21b ("net/mlx5e: Add MACsec RX steering rules")
> Fixes: e467b283ffd5 ("net/mlx5e: Add MACsec TX steering rules")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Saeed Mahameed <saeedm@nvidia.com>
> ---
>   .../ethernet/mellanox/mlx5/core/en_accel/macsec_fs.c | 12 ++++++++----
>   1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec_fs.c b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec_fs.c
> index 1ac0cf04e811..96cec6d826c2 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec_fs.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec_fs.c
> @@ -250,7 +250,7 @@ static int macsec_fs_tx_create(struct mlx5e_macsec_fs *macsec_fs)
>   	struct mlx5_flow_handle *rule;
>   	struct mlx5_flow_spec *spec;
>   	u32 *flow_group_in;
> -	int err = 0;
> +	int err;
>   
>   	ns = mlx5_get_flow_namespace(macsec_fs->mdev, MLX5_FLOW_NAMESPACE_EGRESS_MACSEC);
>   	if (!ns)
> @@ -261,8 +261,10 @@ static int macsec_fs_tx_create(struct mlx5e_macsec_fs *macsec_fs)
>   		return -ENOMEM;
>   
>   	flow_group_in = kvzalloc(inlen, GFP_KERNEL);
> -	if (!flow_group_in)
> +	if (!flow_group_in) {
> +		err = -ENOMEM;
>   		goto out_spec;
> +	}
>   
>   	tx_tables = &tx_fs->tables;
>   	ft_crypto = &tx_tables->ft_crypto;
> @@ -898,7 +900,7 @@ static int macsec_fs_rx_create(struct mlx5e_macsec_fs *macsec_fs)
>   	struct mlx5_flow_handle *rule;
>   	struct mlx5_flow_spec *spec;
>   	u32 *flow_group_in;
> -	int err = 0;
> +	int err;
>   
>   	ns = mlx5_get_flow_namespace(macsec_fs->mdev, MLX5_FLOW_NAMESPACE_KERNEL_RX_MACSEC);
>   	if (!ns)
> @@ -909,8 +911,10 @@ static int macsec_fs_rx_create(struct mlx5e_macsec_fs *macsec_fs)
>   		return -ENOMEM;
>   
>   	flow_group_in = kvzalloc(inlen, GFP_KERNEL);
> -	if (!flow_group_in)
> +	if (!flow_group_in) {
> +		err = -ENOMEM;
>   		goto free_spec;
> +	}
>   
>   	rx_tables = &rx_fs->tables;
>   	ft_crypto = &rx_tables->ft_crypto;

Re: [net 07/15] net/mlx5e: Use kvfree() in mlx5e_accel_fs_tcp_create()

[Jacob Keller]

On 11/28/2022 11:55 AM, Saeed Mahameed wrote:
> On 24 Nov 10:32, Tariq Toukan wrote:
>>
>>
>> On 11/24/2022 10:10 AM, Saeed Mahameed wrote:
>>> From: YueHaibing <yuehaibing@huawei.com>
>>>
>>> 'accel_tcp' is allocated by kvzalloc(), which should freed by kvfree().
>>>
>>> Fixes: f52f2faee581 ("net/mlx5e: Introduce flow steering API")
>>> Signed-off-by: YueHaibing <yuehaibing@huawei.com>
>>> Reviewed-by: Tariq Toukan <tariqt@nvidia.com>
>>> Signed-off-by: Saeed Mahameed <saeedm@nvidia.com>
>>> ---
>>
>> Hi Saeed,
>> There was a v3 of this, that changes the alloc side instead.
>>
> Thanks Tariq, that patch was Marked for -next for some reason,
> and it's in my net-next queue, i think it's ok if this went to net and the
> other to net-next.

That route would make sense to me.

Thanks,
Jake

Re: [net 09/15] net/mlx5e: MACsec, fix memory leak when MACsec device is deleted

[Jacob Keller]

On 11/24/2022 12:10 AM, Saeed Mahameed wrote:
> From: Raed Salem <raeds@nvidia.com>
> 
> When the MACsec netdevice is deleted, all related Rx/Tx HW/SW
> states should be released/deallocated, however currently part
> of the Rx security channel association data is not cleaned
> properly, hence the memory leaks.
> 
> Fix by make sure all related Rx Sc resources are cleaned/freed,
> while at it improve code by grouping release SC context in a
> function so it can be used in both delete MACsec device and
> delete Rx SC operations.
> 
> Fixes: 5a39816a75e5 ("net/mlx5e: Add MACsec offload SecY support")
> Signed-off-by: Raed Salem <raeds@nvidia.com>
> Reviewed-by: Emeel Hakim <ehakim@nvidia.com>
> Signed-off-by: Saeed Mahameed <saeedm@nvidia.com>
> ---
>   .../mellanox/mlx5/core/en_accel/macsec.c      | 77 ++++++++-----------
>   1 file changed, 33 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c
> index 96fa553ef93a..b51de07d5bad 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c
> @@ -823,16 +823,43 @@ static int mlx5e_macsec_upd_rxsc(struct macsec_context *ctx)
>   	return err;
>   }
> 

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

> +static void macsec_del_rxsc_ctx(struct mlx5e_macsec *macsec, struct mlx5e_macsec_rx_sc *rx_sc)
> +{
> +	struct mlx5e_macsec_sa *rx_sa;
> +	int i;
> +
> +	for (i = 0; i < MACSEC_NUM_AN; ++i) {
> +		rx_sa = rx_sc->rx_sa[i];
> +		if (!rx_sa)
> +			continue;
> +
> +		mlx5e_macsec_cleanup_sa(macsec, rx_sa, false);
> +		mlx5_destroy_encryption_key(macsec->mdev, rx_sa->enc_key_id);
> +
> +		kfree(rx_sa);
> +		rx_sc->rx_sa[i] = NULL;
> +	}
> +
> +	/* At this point the relevant MACsec offload Rx rule already removed at
> +	 * mlx5e_macsec_cleanup_sa need to wait for datapath to finish current
> +	 * Rx related data propagating using xa_erase which uses rcu to sync,
> +	 * once fs_id is erased then this rx_sc is hidden from datapath.
> +	 */
> +	list_del_rcu(&rx_sc->rx_sc_list_element);
> +	xa_erase(&macsec->sc_xarray, rx_sc->sc_xarray_element->fs_id);
> +	metadata_dst_free(rx_sc->md_dst);
> +	kfree(rx_sc->sc_xarray_element);
> +	kfree_rcu(rx_sc);
> +}
> +
>   static int mlx5e_macsec_del_rxsc(struct macsec_context *ctx)
>   {
>   	struct mlx5e_priv *priv = netdev_priv(ctx->netdev);
>   	struct mlx5e_macsec_device *macsec_device;
>   	struct mlx5e_macsec_rx_sc *rx_sc;
> -	struct mlx5e_macsec_sa *rx_sa;
>   	struct mlx5e_macsec *macsec;
>   	struct list_head *list;
>   	int err = 0;
> -	int i;
>   
>   	mutex_lock(&priv->macsec->lock);
>   
> @@ -854,31 +881,7 @@ static int mlx5e_macsec_del_rxsc(struct macsec_context *ctx)
>   		goto out;
>   	}
>   
> -	for (i = 0; i < MACSEC_NUM_AN; ++i) {
> -		rx_sa = rx_sc->rx_sa[i];
> -		if (!rx_sa)
> -			continue;
> -
> -		mlx5e_macsec_cleanup_sa(macsec, rx_sa, false);
> -		mlx5_destroy_encryption_key(macsec->mdev, rx_sa->enc_key_id);
> -
> -		kfree(rx_sa);
> -		rx_sc->rx_sa[i] = NULL;
> -	}
> -
> -/*
> - * At this point the relevant MACsec offload Rx rule already removed at
> - * mlx5e_macsec_cleanup_sa need to wait for datapath to finish current
> - * Rx related data propagating using xa_erase which uses rcu to sync,
> - * once fs_id is erased then this rx_sc is hidden from datapath.
> - */
> -	list_del_rcu(&rx_sc->rx_sc_list_element);
> -	xa_erase(&macsec->sc_xarray, rx_sc->sc_xarray_element->fs_id);
> -	metadata_dst_free(rx_sc->md_dst);
> -	kfree(rx_sc->sc_xarray_element);
> -
> -	kfree_rcu(rx_sc);
> -
> +	macsec_del_rxsc_ctx(macsec, rx_sc);
>   out:
>   	mutex_unlock(&macsec->lock);
>   
> @@ -1239,7 +1242,6 @@ static int mlx5e_macsec_del_secy(struct macsec_context *ctx)
>   	struct mlx5e_priv *priv = netdev_priv(ctx->netdev);
>   	struct mlx5e_macsec_device *macsec_device;
>   	struct mlx5e_macsec_rx_sc *rx_sc, *tmp;
> -	struct mlx5e_macsec_sa *rx_sa;
>   	struct mlx5e_macsec_sa *tx_sa;
>   	struct mlx5e_macsec *macsec;
>   	struct list_head *list;
> @@ -1268,28 +1270,15 @@ static int mlx5e_macsec_del_secy(struct macsec_context *ctx)
>   	}
>   
>   	list = &macsec_device->macsec_rx_sc_list_head;
> -	list_for_each_entry_safe(rx_sc, tmp, list, rx_sc_list_element) {
> -		for (i = 0; i < MACSEC_NUM_AN; ++i) {
> -			rx_sa = rx_sc->rx_sa[i];
> -			if (!rx_sa)
> -				continue;
> -
> -			mlx5e_macsec_cleanup_sa(macsec, rx_sa, false);
> -			mlx5_destroy_encryption_key(macsec->mdev, rx_sa->enc_key_id);
> -			kfree(rx_sa);
> -			rx_sc->rx_sa[i] = NULL;
> -		}
> -
> -		list_del_rcu(&rx_sc->rx_sc_list_element);
> -
> -		kfree_rcu(rx_sc);
> -	}
> +	list_for_each_entry_safe(rx_sc, tmp, list, rx_sc_list_element)
> +		macsec_del_rxsc_ctx(macsec, rx_sc);
>   
>   	kfree(macsec_device->dev_addr);
>   	macsec_device->dev_addr = NULL;
>   
>   	list_del_rcu(&macsec_device->macsec_device_list_element);
>   	--macsec->num_of_devices;
> +	kfree(macsec_device);
>   
>   out:
>   	mutex_unlock(&macsec->lock);

Re: [net 10/15] net/mlx5e: MACsec, fix update Rx secure channel active field

[Jacob Keller]

On 11/24/2022 12:10 AM, Saeed Mahameed wrote:
> From: Raed Salem <raeds@nvidia.com>
> 
> The main functionality for this operation is to update the
> active state of the Rx security channel (SC) if the new
> active setting is different from the current active state
> of this Rx SC, however the relevant active state check is
> done post updating the current active state to match the
> new active state, effectively blocks any offload state
> update for the Rx SC in question.
> 
> Fix by delay the assignment to be post the relevant check.
> 

Makes sense.

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

> Fixes: aae3454e4d4c ("net/mlx5e: Add MACsec offload Rx command support")
> Signed-off-by: Raed Salem <raeds@nvidia.com>
> Reviewed-by: Emeel Hakim <ehakim@nvidia.com>
> Signed-off-by: Saeed Mahameed <saeedm@nvidia.com>
> ---
>   drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c
> index b51de07d5bad..9c891a877998 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c
> @@ -803,10 +803,10 @@ static int mlx5e_macsec_upd_rxsc(struct macsec_context *ctx)
>   		goto out;
>   	}
>   
> -	rx_sc->active = ctx_rx_sc->active;
>   	if (rx_sc->active == ctx_rx_sc->active)
>   		goto out;
>   
> +	rx_sc->active = ctx_rx_sc->active;
>   	for (i = 0; i < MACSEC_NUM_AN; ++i) {
>   		rx_sa = rx_sc->rx_sa[i];
>   		if (!rx_sa)

Re: [net 12/15] net/mlx5e: MACsec, fix add Rx security association (SA) rule memory leak

[Jacob Keller]

On 11/24/2022 12:10 AM, Saeed Mahameed wrote:
> From: Emeel Hakim <ehakim@nvidia.com>
> 
> Currently MACsec's add Rx SA flow steering (fs) rule routine
> uses a spec object which is dynamically allocated and do
> not free it upon leaving. The above led to a memory leak.
> 
> Fix by freeing dynamically allocated objects.
> 
> Fixes: 3b20949cb21b ("net/mlx5e: Add MACsec RX steering rules")
> Signed-off-by: Emeel Hakim <ehakim@nvidia.com>
> Reviewed-by: Raed Salem <raeds@nvidia.com>
> Signed-off-by: Saeed Mahameed <saeedm@nvidia.com>

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

> ---
>   drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec_fs.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec_fs.c b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec_fs.c
> index f87a1c4021bc..5b658a5588c6 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec_fs.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec_fs.c
> @@ -1209,6 +1209,7 @@ macsec_fs_rx_add_rule(struct mlx5e_macsec_fs *macsec_fs,
>   		rx_rule->rule[1] = rule;
>   	}
>   
> +	kvfree(spec);
>   	return macsec_rule;
>   
>   err:

Re: [net 13/15] net/mlx5e: MACsec, remove replay window size limitation in offload path

[Jacob Keller]

On 11/24/2022 12:10 AM, Saeed Mahameed wrote:
> From: Emeel Hakim <ehakim@nvidia.com>
> 
> Currently offload path limits replay window size to 32/64/128/256 bits,
> such a limitation should not exist since software allows it.
> Remove such limitation.
> 
> Fixes: eb43846b43c3 ("net/mlx5e: Support MACsec offload replay window")
> Signed-off-by: Emeel Hakim <ehakim@nvidia.com>
> Reviewed-by: Raed Salem <raeds@nvidia.com>
> Signed-off-by: Saeed Mahameed <saeedm@nvidia.com>
> ---
>   .../mellanox/mlx5/core/en_accel/macsec.c         | 16 ----------------
>   include/linux/mlx5/mlx5_ifc.h                    |  7 -------
>   2 files changed, 23 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c
> index c19581f1f733..72f8be65fa90 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c
> @@ -229,22 +229,6 @@ static int macsec_set_replay_protection(struct mlx5_macsec_obj_attrs *attrs, voi
>   	if (!attrs->replay_protect)
>   		return 0;
>   
> -	switch (attrs->replay_window) {
> -	case 256:
> -		window_sz = MLX5_MACSEC_ASO_REPLAY_WIN_256BIT;
> -		break;
> -	case 128:
> -		window_sz = MLX5_MACSEC_ASO_REPLAY_WIN_128BIT;
> -		break;
> -	case 64:
> -		window_sz = MLX5_MACSEC_ASO_REPLAY_WIN_64BIT;
> -		break;
> -	case 32:
> -		window_sz = MLX5_MACSEC_ASO_REPLAY_WIN_32BIT;
> -		break;
> -	default:
> -		return -EINVAL;
> -	}

What sets window_sz now? Looking at the current code wouldn't this leave 
window_sz uninitialized and this undefined behavior of MLX5_SET? Either 
you should just forward in attrs->replay_window and remove window_sz 
local or drop the MLX5_SET call for setting window size?

>   	MLX5_SET(macsec_aso, aso_ctx, window_size, window_sz);
>   	MLX5_SET(macsec_aso, aso_ctx, mode, MLX5_MACSEC_ASO_REPLAY_PROTECTION);
> > diff --git a/include/linux/mlx5/mlx5_ifc.h 
b/include/linux/mlx5/mlx5_ifc.h
> index 5a4e914e2a6f..981fc7dfa408 100644
> --- a/include/linux/mlx5/mlx5_ifc.h
> +++ b/include/linux/mlx5/mlx5_ifc.h
> @@ -11611,13 +11611,6 @@ enum {
>   	MLX5_MACSEC_ASO_REPLAY_PROTECTION = 0x1,
>   };
>   
> -enum {
> -	MLX5_MACSEC_ASO_REPLAY_WIN_32BIT  = 0x0,
> -	MLX5_MACSEC_ASO_REPLAY_WIN_64BIT  = 0x1,
> -	MLX5_MACSEC_ASO_REPLAY_WIN_128BIT = 0x2,
> -	MLX5_MACSEC_ASO_REPLAY_WIN_256BIT = 0x3,
> -};
> -
>   #define MLX5_MACSEC_ASO_INC_SN  0x2
>   #define MLX5_MACSEC_ASO_REG_C_4_5 0x2
>   

Re: [net 14/15] net/mlx5e: MACsec, fix Tx SA active field update

[Jacob Keller]

On 11/24/2022 12:10 AM, Saeed Mahameed wrote:
> From: Raed Salem <raeds@nvidia.com>
> 
> Currently during update Tx security association (SA) flow, the Tx SA
> active state is updated only if the Tx SA in question is the same SA
> that the MACsec interface is using for Tx,in consequence when the
> MACsec interface chose to work with this Tx SA later, where this SA
> for example should have been updated to active state and it was not,
> the relevant Tx SA HW context won't be installed, hence the MACSec
> flow won't be offloaded.
> 
> Fix by update Tx SA active state as part of update flow regardless
> whether the SA in question is the same Tx SA used by the MACsec
> interface.

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

> 
> Fixes: 8ff0ac5be144 ("net/mlx5: Add MACsec offload Tx command support")
> Signed-off-by: Raed Salem <raeds@nvidia.com>
> Reviewed-by: Emeel Hakim <ehakim@nvidia.com>
> Signed-off-by: Saeed Mahameed <saeedm@nvidia.com>
> ---
>   drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c
> index 72f8be65fa90..137b34347de1 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c
> @@ -602,6 +602,7 @@ static int mlx5e_macsec_upd_txsa(struct macsec_context *ctx)
>   	if (tx_sa->active == ctx_tx_sa->active)
>   		goto out;
>   
> +	tx_sa->active = ctx_tx_sa->active;
>   	if (tx_sa->assoc_num != tx_sc->encoding_sa)
>   		goto out;
>   
> @@ -617,8 +618,6 @@ static int mlx5e_macsec_upd_txsa(struct macsec_context *ctx)
>   
>   		mlx5e_macsec_cleanup_sa(macsec, tx_sa, true);
>   	}
> -
> -	tx_sa->active = ctx_tx_sa->active;
>   out:
>   	mutex_unlock(&macsec->lock);
>   

Re: [net 15/15] net/mlx5e: MACsec, block offload requests with encrypt off

[Jacob Keller]

On 11/24/2022 12:10 AM, Saeed Mahameed wrote:
> From: Emeel Hakim <ehakim@nvidia.com>
> 
> Currently offloading MACsec with authentication only (encrypt
> property set to off) is not supported, block such requests
> when adding/updating a macsec device.
> 

Makes sense.

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

> Fixes: 8ff0ac5be144 ("net/mlx5: Add MACsec offload Tx command support")
> Signed-off-by: Emeel Hakim <ehakim@nvidia.com>
> Reviewed-by: Raed Salem <raeds@nvidia.com>
> Signed-off-by: Saeed Mahameed <saeedm@nvidia.com>
> ---
>   drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c
> index 137b34347de1..0d6dc394a12a 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c
> @@ -458,6 +458,11 @@ static bool mlx5e_macsec_secy_features_validate(struct macsec_context *ctx)
>   		return false;
>   	}
>   
> +	if (!ctx->secy->tx_sc.encrypt) {
> +		netdev_err(netdev, "MACsec offload: encrypt off isn't supported\n");
> +		return false;
> +	}
> +
>   	return true;
>   }
>   

Re: [net 13/15] net/mlx5e: MACsec, remove replay window size limitation in offload path

[Jakub Kicinski]

On Mon, 28 Nov 2022 15:42:19 -0800 Jacob Keller wrote:
> > index c19581f1f733..72f8be65fa90 100644
> > --- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c
> > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c
> > @@ -229,22 +229,6 @@ static int macsec_set_replay_protection(struct mlx5_macsec_obj_attrs *attrs, voi
> >   	if (!attrs->replay_protect)
> >   		return 0;
> >   
> > -	switch (attrs->replay_window) {
> > -	case 256:
> > -		window_sz = MLX5_MACSEC_ASO_REPLAY_WIN_256BIT;
> > -		break;
> > -	case 128:
> > -		window_sz = MLX5_MACSEC_ASO_REPLAY_WIN_128BIT;
> > -		break;
> > -	case 64:
> > -		window_sz = MLX5_MACSEC_ASO_REPLAY_WIN_64BIT;
> > -		break;
> > -	case 32:
> > -		window_sz = MLX5_MACSEC_ASO_REPLAY_WIN_32BIT;
> > -		break;
> > -	default:
> > -		return -EINVAL;
> > -	}  
> 
> What sets window_sz now? Looking at the current code wouldn't this leave 
> window_sz uninitialized and this undefined behavior of MLX5_SET? Either 
> you should just forward in attrs->replay_window and remove window_sz 
> local or drop the MLX5_SET call for setting window size?

Damn it, this is a clang warning, I need to rescind the PR :/

Re: [net 13/15] net/mlx5e: MACsec, remove replay window size limitation in offload path

[Saeed Mahameed]

On 28 Nov 19:35, Jakub Kicinski wrote:
>On Mon, 28 Nov 2022 15:42:19 -0800 Jacob Keller wrote:
>> > index c19581f1f733..72f8be65fa90 100644
>> > --- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c
>> > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c
>> > @@ -229,22 +229,6 @@ static int macsec_set_replay_protection(struct mlx5_macsec_obj_attrs *attrs, voi
>> >   	if (!attrs->replay_protect)
>> >   		return 0;
>> >
>> > -	switch (attrs->replay_window) {
>> > -	case 256:
>> > -		window_sz = MLX5_MACSEC_ASO_REPLAY_WIN_256BIT;
>> > -		break;
>> > -	case 128:
>> > -		window_sz = MLX5_MACSEC_ASO_REPLAY_WIN_128BIT;
>> > -		break;
>> > -	case 64:
>> > -		window_sz = MLX5_MACSEC_ASO_REPLAY_WIN_64BIT;
>> > -		break;
>> > -	case 32:
>> > -		window_sz = MLX5_MACSEC_ASO_REPLAY_WIN_32BIT;
>> > -		break;
>> > -	default:
>> > -		return -EINVAL;
>> > -	}
>>
>> What sets window_sz now? Looking at the current code wouldn't this leave
>> window_sz uninitialized and this undefined behavior of MLX5_SET? Either
>> you should just forward in attrs->replay_window and remove window_sz
>> local or drop the MLX5_SET call for setting window size?
>
>Damn it, this is a clang warning, I need to rescind the PR :/

Make sense, Jacob found two real issues and this one is critical,
but I don't know how that works for PRs, let me know when you do it so I
will add his reviewed-by tags and address the two issues when is send v2.

another option is that i am currently working on my next PR for net, I can
address those two issues there, but we won't have Jacobs reviewed-by for
the work he already done on this series :/ .. 

Thanks.

Re: [net 03/15] net/mlx5: E-switch, Fix duplicate lag creation

[Saeed Mahameed]

On 28 Nov 15:23, Jacob Keller wrote:
>
>
>On 11/24/2022 12:10 AM, Saeed Mahameed wrote:
>>From: Chris Mi <cmi@nvidia.com>
>>
>>If creating bond first and then enabling sriov in switchdev mode,
>>will hit the following syndrome:
>>
>>mlx5_core 0000:08:00.0: mlx5_cmd_out_err:778:(pid 25543): CREATE_LAG(0x840) op_mod(0x0) failed, status bad parameter(0x3), syndrome (0x7d49cb), err(-22)
>>
>>The reason is because the offending patch removes eswitch mode
>>none. In vf lag, the checking of eswitch mode none is replaced
>>by checking if sriov is enabled. But when driver enables sriov,
>>it triggers the bond workqueue task first and then setting sriov
>>number in pci_enable_sriov(). So the check fails.
>>
>>Fix it by checking if sriov is enabled using eswitch internal
>>counter that is set before triggering the bond workqueue task.
>>
>>Fixes: f019679ea5f2 ("net/mlx5: E-switch, Remove dependency between sriov and eswitch mode")
>>Signed-off-by: Chris Mi <cmi@nvidia.com>
>>Reviewed-by: Roi Dayan <roid@nvidia.com>
>>Reviewed-by: Mark Bloch <mbloch@nvidia.com>
>>Reviewed-by: Vlad Buslov <vladbu@nvidia.com>
>>Signed-off-by: Saeed Mahameed <saeedm@nvidia.com>
>>---
>>  drivers/net/ethernet/mellanox/mlx5/core/eswitch.h | 8 ++++++++
>>  drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c | 5 +++--
>>  2 files changed, 11 insertions(+), 2 deletions(-)
>>

[...]

>>diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c b/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
>>index be1307a63e6d..4070dc1d17cb 100644
>>--- a/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
>>+++ b/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
>>@@ -701,8 +701,9 @@ static bool mlx5_lag_check_prereq(struct mlx5_lag *ldev)
>>  #ifdef CONFIG_MLX5_ESWITCH
>>  	dev = ldev->pf[MLX5_LAG_P1].dev;
>>-	if ((mlx5_sriov_is_enabled(dev)) && !is_mdev_switchdev_mode(dev))
>>-		return false;
>>+	for (i = 0; i  < ldev->ports; i++)
>>+		if (mlx5_eswitch_num_vfs(dev->priv.eswitch) && !is_mdev_switchdev_mode(dev))
>>+			return false;
>
>Am I missing something? whats with the for loop iterator here? i isn't 
>used or passed into these functions?
>
>Do you need to check multiple times or do these functions have some 
>side effect? But looking at their implementation neither of them 
>appear to have side effects?
>
>What am I missing?

Great catch! it's a copy/paste bug, here we need to grab each port's
eswitch on every iteration.

something like:
for (i = 0; i  < ldev->ports; i++) {
+     dev = ldev->pf[i].dev;
      if (mlx5_eswitch_num_vfs(dev->priv.eswitch) && !is_mdev_switchdev_mode(dev))
              return false;
}

Re: [net 13/15] net/mlx5e: MACsec, remove replay window size limitation in offload path

[Saeed Mahameed]

On 28 Nov 21:44, Saeed Mahameed wrote:
>On 28 Nov 19:35, Jakub Kicinski wrote:
>>Damn it, this is a clang warning, I need to rescind the PR :/
>
>Make sense, Jacob found two real issues and this one is critical,
>but I don't know how that works for PRs, let me know when you do it so I
>will add his reviewed-by tags and address the two issues when is send v2.
>

hmm, I thought you were planing to revert my PR :).. anyways, i am will
send the two fixes soon, so you could release your pr to linus,
sorry about the mess.

Re: [net 13/15] net/mlx5e: MACsec, remove replay window size limitation in offload path

[Jacob Keller]

On 11/29/2022 12:12 AM, Saeed Mahameed wrote:
> On 28 Nov 21:44, Saeed Mahameed wrote:
>> On 28 Nov 19:35, Jakub Kicinski wrote:
>>> Damn it, this is a clang warning, I need to rescind the PR :/
>>
>> Make sense, Jacob found two real issues and this one is critical,
>> but I don't know how that works for PRs, let me know when you do it so I
>> will add his reviewed-by tags and address the two issues when is send v2.
>>
> 
> hmm, I thought you were planing to revert my PR :).. anyways, i am will
> send the two fixes soon, so you could release your pr to linus,
> sorry about the mess.
> 

The fixes look fine to me! Glad things got caught and fixed before 
getting spread widely.

Thanks,
Jake