[PATCH net 0/7] net/mlx5e: Fix offload lifetime, cleanup and exclusion bugs

[PATCH net 0/7] net/mlx5e: Fix offload lifetime, cleanup and exclusion bugs

[Tariq Toukan]

Hi,

This series by Cosmin fixes several mlx5e offload issues:

- Block eswitch mode changes before accessing the IPsec context during
  policy and SA creation.
- Tie TC eswitch and IPsec-blocking references to flow destruction,
  covering bulk cleanup and flows with outstanding references.
- Track MACsec hardware-object ownership explicitly and delete remaining
  SecYs during driver cleanup.
- Serialize TC/IPsec exclusion counters with a dedicated mutex.
- Prevent SHAMPO from merging PSP packets.

Regards,
Tariq

Cosmin Ratiu (7):
  net/mlx5e: ipsec: Block eswitch mode changes during policy creation
  net/mlx5e: ipsec: Block eswitch mode changes before accessing
    priv->ipsec
  net/mlx5e: tc: Tie esw & accel blocking refs to the flow's lifetime
  net/mlx5e: macsec: Track hardware object ownership for SA teardown
  net/mlx5e: macsec: Delete remaining SecYs during cleanup
  net/mlx5e: Serialize TC and IPsec offload exclusion counters
  net/mlx5e: shampo: Do not merge PSP packets

 .../mellanox/mlx5/core/en_accel/ipsec.c       | 55 +++++++++++-----
 .../mellanox/mlx5/core/en_accel/ipsec_fs.c    | 47 ++++---------
 .../mellanox/mlx5/core/en_accel/macsec.c      | 60 +++++++++++------
 .../net/ethernet/mellanox/mlx5/core/en_rx.c   | 15 ++++-
 .../net/ethernet/mellanox/mlx5/core/en_tc.c   | 66 ++++++++++++-------
 .../net/ethernet/mellanox/mlx5/core/eswitch.c | 23 ++-----
 .../net/ethernet/mellanox/mlx5/core/eswitch.h | 11 ++--
 .../mellanox/mlx5/core/eswitch_offloads.c     |  7 +-
 .../net/ethernet/mellanox/mlx5/core/main.c    |  3 +
 include/linux/mlx5/driver.h                   |  7 +-
 10 files changed, 172 insertions(+), 122 deletions(-)


base-commit: c9151088f1674fd29ff26a20f5fc687acf53a2f0
-- 
2.44.0

[PATCH net 1/7] net/mlx5e: ipsec: Block eswitch mode changes during policy creation

[Tariq Toukan]

From: Cosmin Ratiu <cratiu@nvidia.com>

Eswitch mode changes can tear down the IPsec context while policy
creation is accessing it. The mode-blocking reference acquired when
creating a flow table comes too late: the table lookup already accesses
the IPsec context before taking that reference.

Block mode changes before checking the IPsec context and validating the
policy. Release the temporary reference after successful setup, when the
flow table holds its own reference, or after unwinding on failure.

Unfortunately, simply using mlx5_eswitch_block_mode() for this would
introduce a regression where:
1. An offloaded inbound IPsec policy is added on the uplink.
2. A TC flower rule is added on a VF representor.
3. Another uplink IPsec policy using the same RX tables is added.

Before this change, the 3rd rule would reuse an existing RX/TX table
from 1 and would avoid an mlx5_eswitch_block_mode() check in
rx_get()/tx_get(). After this change, the temporary mode block added
would reject the 3rd rule because esw->user_count > 0.

To avoid that, I was forced to separate the mode blocking from the TC
check by introducing a check_users argument to
mlx5_eswitch_block_mode().

Fixes: a5b8ca9471d3 ("net/mlx5e: Add XFRM policy offload logic")
Signed-off-by: Cosmin Ratiu <cratiu@nvidia.com>
Reviewed-by: Dragos Tatulea <dtatulea@nvidia.com>
Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
---
 .../mellanox/mlx5/core/en_accel/ipsec.c       | 23 +++++++++++++++----
 .../mellanox/mlx5/core/en_accel/ipsec_fs.c    |  4 ++--
 .../net/ethernet/mellanox/mlx5/core/eswitch.c | 11 +++++----
 .../net/ethernet/mellanox/mlx5/core/eswitch.h | 10 +++++---
 .../mellanox/mlx5/core/eswitch_offloads.c     |  7 +++---
 5 files changed, 36 insertions(+), 19 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 db260e3d1412..841ecdc2c4d9 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c
@@ -806,7 +806,7 @@ static int mlx5e_xfrm_add_state(struct net_device *dev,
 		goto err_xfrm;
 	}
 
-	err = mlx5_eswitch_block_mode(priv->mdev);
+	err = mlx5_eswitch_block_mode(priv->mdev, true);
 	if (err)
 		goto unblock_ipsec;
 
@@ -1266,18 +1266,28 @@ static int mlx5e_xfrm_add_policy(struct xfrm_policy *x,
 	int err;
 
 	priv = netdev_priv(netdev);
+	/* Block esw mode changes until the policy holds its own block. */
+	err = mlx5_eswitch_block_mode(priv->mdev, false);
+	if (err) {
+		NL_SET_ERR_MSG_MOD(extack, "Eswitch busy, can't add policy");
+		return err;
+	}
+
 	if (!priv->ipsec) {
 		NL_SET_ERR_MSG_MOD(extack, "Device doesn't support IPsec packet offload");
-		return -EOPNOTSUPP;
+		err = -EOPNOTSUPP;
+		goto unblock_mode;
 	}
 
 	err = mlx5e_xfrm_validate_policy(priv->mdev, x, extack);
 	if (err)
-		return err;
+		goto unblock_mode;
 
 	pol_entry = kzalloc_obj(*pol_entry);
-	if (!pol_entry)
-		return -ENOMEM;
+	if (!pol_entry) {
+		err = -ENOMEM;
+		goto unblock_mode;
+	}
 
 	pol_entry->x = x;
 	pol_entry->ipsec = priv->ipsec;
@@ -1293,6 +1303,7 @@ static int mlx5e_xfrm_add_policy(struct xfrm_policy *x,
 		goto err_fs;
 
 	x->xdo.offload_handle = (unsigned long)pol_entry;
+	mlx5_eswitch_unblock_mode(priv->mdev);
 	return 0;
 
 err_fs:
@@ -1300,6 +1311,8 @@ static int mlx5e_xfrm_add_policy(struct xfrm_policy *x,
 ipsec_busy:
 	kfree(pol_entry);
 	NL_SET_ERR_MSG_MOD(extack, "Device failed to offload this policy");
+unblock_mode:
+	mlx5_eswitch_unblock_mode(priv->mdev);
 	return err;
 }
 
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 329608c59313..5c72656c623f 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
@@ -1127,7 +1127,7 @@ static int rx_get(struct mlx5_core_dev *mdev, struct mlx5e_ipsec *ipsec,
 	if (rx->ft.refcnt)
 		goto skip;
 
-	err = mlx5_eswitch_block_mode(mdev);
+	err = mlx5_eswitch_block_mode(mdev, true);
 	if (err)
 		return err;
 
@@ -1416,7 +1416,7 @@ static int tx_get(struct mlx5_core_dev *mdev, struct mlx5e_ipsec *ipsec,
 	if (tx->ft.refcnt)
 		goto skip;
 
-	err = mlx5_eswitch_block_mode(mdev);
+	err = mlx5_eswitch_block_mode(mdev, true);
 	if (err)
 		return err;
 
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
index fc197d1dc9df..0b48cc7a6734 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
@@ -3005,21 +3005,22 @@ void mlx5_esw_put(struct mlx5_core_dev *mdev)
 /**
  * mlx5_esw_try_lock() - Take a write lock on esw mode lock.
  * @esw: eswitch device.
+ * @check_users: reject the lock if eswitch users exist.
  *
  * Should be called by esw mode change routine.
  *
  * Return:
- * * 0       - esw mode if successfully locked and refcount is 0.
- * * -EBUSY  - refcount is not 0.
- * * -EINVAL - In the middle of switching mode or lock is already held.
+ * * >= 0    - esw mode if successfully locked.
+ * * -EBUSY  - mode change in progress or users exist with check_users set.
+ * * -EINVAL - lock is already held.
  */
-int mlx5_esw_try_lock(struct mlx5_eswitch *esw)
+int mlx5_esw_try_lock(struct mlx5_eswitch *esw, bool check_users)
 {
 	if (down_write_trylock(&esw->mode_lock) == 0)
 		return -EINVAL;
 
 	if (esw->eswitch_operation_in_progress ||
-	    atomic64_read(&esw->user_count) > 0) {
+	    (check_users && atomic64_read(&esw->user_count) > 0)) {
 		up_write(&esw->mode_lock);
 		return -EBUSY;
 	}
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
index 8b1f93b13ea9..4a9a1656f6db 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
@@ -946,7 +946,7 @@ bool mlx5_esw_hold(struct mlx5_core_dev *dev);
 void mlx5_esw_release(struct mlx5_core_dev *dev);
 void mlx5_esw_get(struct mlx5_core_dev *dev);
 void mlx5_esw_put(struct mlx5_core_dev *dev);
-int mlx5_esw_try_lock(struct mlx5_eswitch *esw);
+int mlx5_esw_try_lock(struct mlx5_eswitch *esw, bool check_users);
 int mlx5_esw_lock(struct mlx5_eswitch *esw);
 void mlx5_esw_unlock(struct mlx5_eswitch *esw);
 
@@ -970,7 +970,7 @@ bool mlx5_eswitch_is_peer(struct mlx5_eswitch *esw,
 bool mlx5_eswitch_block_encap(struct mlx5_core_dev *dev, bool from_fdb);
 void mlx5_eswitch_unblock_encap(struct mlx5_core_dev *dev);
 
-int mlx5_eswitch_block_mode(struct mlx5_core_dev *dev);
+int mlx5_eswitch_block_mode(struct mlx5_core_dev *dev, bool check_users);
 void mlx5_eswitch_unblock_mode(struct mlx5_core_dev *dev);
 
 static inline int mlx5_eswitch_num_vfs(struct mlx5_eswitch *esw)
@@ -1081,7 +1081,11 @@ static inline void mlx5_eswitch_unblock_encap(struct mlx5_core_dev *dev)
 {
 }
 
-static inline int mlx5_eswitch_block_mode(struct mlx5_core_dev *dev) { return 0; }
+static inline int mlx5_eswitch_block_mode(struct mlx5_core_dev *dev,
+					  bool check_users)
+{
+	return 0;
+}
 static inline void mlx5_eswitch_unblock_mode(struct mlx5_core_dev *dev) {}
 static inline bool mlx5_eswitch_block_ipsec(struct mlx5_core_dev *dev)
 {
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
index eb74b6260168..996490e11625 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
@@ -4343,7 +4343,7 @@ static int esw_inline_mode_to_devlink(u8 mlx5_mode, u8 *mode)
 	return 0;
 }
 
-int mlx5_eswitch_block_mode(struct mlx5_core_dev *dev)
+int mlx5_eswitch_block_mode(struct mlx5_core_dev *dev, bool check_users)
 {
 	struct mlx5_eswitch *esw = dev->priv.eswitch;
 	int err;
@@ -4351,8 +4351,7 @@ int mlx5_eswitch_block_mode(struct mlx5_core_dev *dev)
 	if (!mlx5_esw_allowed(esw))
 		return 0;
 
-	/* Take TC into account */
-	err = mlx5_esw_try_lock(esw);
+	err = mlx5_esw_try_lock(esw, check_users);
 	if (err < 0)
 		return err;
 
@@ -4485,7 +4484,7 @@ int mlx5_devlink_eswitch_mode_set(struct devlink *devlink, u16 mode,
 		return 0;
 
 	mlx5_lag_disable_change(esw->dev);
-	err = mlx5_esw_try_lock(esw);
+	err = mlx5_esw_try_lock(esw, true);
 	if (err < 0) {
 		NL_SET_ERR_MSG_MOD(extack, "Can't change mode, E-Switch is busy");
 		goto enable_lag;
-- 
2.44.0

Re: [PATCH net 1/7] net/mlx5e: ipsec: Block eswitch mode changes during policy creation

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0 (1)

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] `mlx5e_xfrm_add_policy()`
  (drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c:1268-1274)…

Pre-existing issues:
- [Low] Incomplete-fix concern: at this commit `mlx5e_xfrm_add_state()`
  (drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c:781-813)…

--

net/mlx5e: ipsec: Block eswitch mode changes during policy creation

This moves the eswitch mode block in mlx5e_xfrm_add_policy() to the top of
the function, before priv->ipsec is checked and the policy is validated, and
releases it once the flow table holds its own block or after unwinding.
A check_users argument is added to mlx5_eswitch_block_mode() /
mlx5_esw_try_lock() so the new temporary block does not reject policies while
TC users are present.

> 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 db260e3d1412f..841ecdc2c4d9e 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c
> @@ -806,7 +806,7 @@ static int mlx5e_xfrm_add_state(struct net_device *dev,
>  		goto err_xfrm;
>  	}
>  
> -	err = mlx5_eswitch_block_mode(priv->mdev);
> +	err = mlx5_eswitch_block_mode(priv->mdev, true);
>  	if (err)
>  		goto unblock_ipsec;

[Severity: Low]
This isn't a bug introduced by this patch, but as of this commit
mlx5e_xfrm_add_state() still has the ordering the changelog describes as
unsafe: priv->ipsec is tested, cached into ipsec and sa_entry->ipsec, and
dereferenced through mlx5e_ipsec_fs_tunnel_allowed(sa_entry) before the
mode block above is taken.

	priv = netdev_priv(dev);
	if (!priv->ipsec)
		return -EOPNOTSUPP;

	ipsec = priv->ipsec;
	...
	err = mlx5_eswitch_block_mode(priv->mdev, true);

The immediately following patch in the series, "net/mlx5e: ipsec: Block
eswitch mode changes before accessing priv->ipsec", moves the block to the
top of mlx5e_xfrm_add_state() and adds the matching unblock_mode: unwind, so
the ordering is consistent again at the end of the series. Would it be worth
mentioning in this changelog that the SA path is handled in the next patch,
so this intermediate state is not read as an oversight?

> @@ -1266,18 +1266,28 @@ static int mlx5e_xfrm_add_policy(struct xfrm_policy *x,
>  	int err;
>  
>  	priv = netdev_priv(netdev);
> +	/* Block esw mode changes until the policy holds its own block. */
> +	err = mlx5_eswitch_block_mode(priv->mdev, false);
> +	if (err) {
> +		NL_SET_ERR_MSG_MOD(extack, "Eswitch busy, can't add policy");
> +		return err;
> +	}
> +

[Severity: Medium]
Can this make policy adds fail spuriously with -EINVAL?

The new block is taken on every policy add, and mlx5_esw_try_lock() bails out
before the check_users test:

drivers/net/ethernet/mellanox/mlx5/core/eswitch.c:mlx5_esw_try_lock() {
	if (down_write_trylock(&esw->mode_lock) == 0)
		return -EINVAL;
	...
}

So passing check_users=false does not help when the mode_lock is simply held
by someone else at that moment. A concrete reader is the TC path, which holds
the read side across the whole flow creation:

drivers/net/ethernet/mellanox/mlx5/core/en_tc.c:mlx5e_configure_flower() {
	if (!mlx5_esw_hold(priv->mdev))
		return -EBUSY;
	...
	err = mlx5e_tc_add_flow(priv, f, flags, dev, &flow);
	...
	mlx5_esw_release(priv->mdev);
}

mlx5e_ipsec_block_tc_offload(), mlx5_eswitch_unblock_mode() and
mlx5_devlink_switchdev_active_mode_change() take the write side with a
blocking down_write().

The errno is passed straight to userspace:

net/xfrm/xfrm_device.c:xfrm_dev_policy_add() {
	err = dev->xfrmdev_ops->xdo_dev_policy_add(xp, extack);
	if (err) {
		...
		return err;
	}
}

Before this change, a policy add that reused already-created tables never ran
a trylock at all:

drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec_fs.c:rx_get() {
	if (rx->ft.refcnt)
		goto skip;

	err = mlx5_eswitch_block_mode(mdev, true);
	...
}

which is the same "TC rule on a VF representor" case the changelog wants to
keep working. Would an ip xfrm policy add now fail immediately, with no wait
and no retry, whenever it overlaps a tc filter add on a representor?

Also, the extack text says "Eswitch busy" while the returned errno can be
-EINVAL, which the kernel-doc updated by this patch defines as "lock is
already held" rather than a busy/retriable condition. Should the transient
trylock collision be reported as -EBUSY so userspace can tell it apart from a
permanent validation rejection?

>  	if (!priv->ipsec) {
>  		NL_SET_ERR_MSG_MOD(extack, "Device doesn't support IPsec packet offload");
> -		return -EOPNOTSUPP;
> +		err = -EOPNOTSUPP;
> +		goto unblock_mode;
>  	}
>  
>  	err = mlx5e_xfrm_validate_policy(priv->mdev, x, extack);
>  	if (err)
> -		return err;
> +		goto unblock_mode;
>  
>  	pol_entry = kzalloc_obj(*pol_entry);
> -	if (!pol_entry)
> -		return -ENOMEM;
> +	if (!pol_entry) {
> +		err = -ENOMEM;
> +		goto unblock_mode;
> +	}
>  
>  	pol_entry->x = x;
>  	pol_entry->ipsec = priv->ipsec;
> @@ -1293,6 +1303,7 @@ static int mlx5e_xfrm_add_policy(struct xfrm_policy *x,
>  		goto err_fs;
>  
>  	x->xdo.offload_handle = (unsigned long)pol_entry;
> +	mlx5_eswitch_unblock_mode(priv->mdev);
>  	return 0;
>  
> err_fs:
> @@ -1300,6 +1311,8 @@ static int mlx5e_xfrm_add_policy(struct xfrm_policy *x,
> ipsec_busy:
>  	kfree(pol_entry);
>  	NL_SET_ERR_MSG_MOD(extack, "Device failed to offload this policy");
> +unblock_mode:
> +	mlx5_eswitch_unblock_mode(priv->mdev);
>  	return err;
>  }

[ ... ]

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
> index fc197d1dc9dfe..0b48cc7a67343 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
> @@ -3005,21 +3005,22 @@ void mlx5_esw_put(struct mlx5_core_dev *mdev)
>  /**
>   * mlx5_esw_try_lock() - Take a write lock on esw mode lock.
>   * @esw: eswitch device.
> + * @check_users: reject the lock if eswitch users exist.
>   *
>   * Should be called by esw mode change routine.
>   *
>   * Return:
> - * * 0       - esw mode if successfully locked and refcount is 0.
> - * * -EBUSY  - refcount is not 0.
> - * * -EINVAL - In the middle of switching mode or lock is already held.
> + * * >= 0    - esw mode if successfully locked.
> + * * -EBUSY  - mode change in progress or users exist with check_users set.
> + * * -EINVAL - lock is already held.
>   */
> -int mlx5_esw_try_lock(struct mlx5_eswitch *esw)
> +int mlx5_esw_try_lock(struct mlx5_eswitch *esw, bool check_users)
>  {
>  	if (down_write_trylock(&esw->mode_lock) == 0)
>  		return -EINVAL;
>  
>  	if (esw->eswitch_operation_in_progress ||
> -	    atomic64_read(&esw->user_count) > 0) {
> +	    (check_users && atomic64_read(&esw->user_count) > 0)) {
>  		up_write(&esw->mode_lock);
>  		return -EBUSY;
>  	}

[ ... ]

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
> index eb74b62601682..996490e116253 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
> @@ -4351,8 +4351,7 @@ int mlx5_eswitch_block_mode(struct mlx5_core_dev *dev)
>  	if (!mlx5_esw_allowed(esw))
>  		return 0;
>  
> -	/* Take TC into account */
> -	err = mlx5_esw_try_lock(esw);
> +	err = mlx5_esw_try_lock(esw, check_users);
>  	if (err < 0)
>  		return err;

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260917175433.4090878-1-tariqt%40nvidia.com

[PATCH net 2/7] net/mlx5e: ipsec: Block eswitch mode changes before accessing priv->ipsec

[Tariq Toukan]

From: Cosmin Ratiu <cratiu@nvidia.com>

mlx5e_xfrm_add_state() reads priv->ipsec and validates mode-dependent
capabilities before blocking eswitch mode changes. A concurrent profile
change can free the saved IPsec context and cause use-after-free.

Move the mode block before the first context access and release it on
all error paths. Keep the atomic acquire-placeholder path exempt, since
it creates no hardware state and cannot take sleeping locks.

Fixes: 22239eb258bc ("net/mlx5e: Prevent tunnel reformat when tunnel mode not allowed")
Signed-off-by: Cosmin Ratiu <cratiu@nvidia.com>
Reviewed-by: Dragos Tatulea <dtatulea@nvidia.com>
Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
---
 .../mellanox/mlx5/core/en_accel/ipsec.c       | 34 ++++++++++++-------
 1 file changed, 21 insertions(+), 13 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 841ecdc2c4d9..1488faece80e 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c
@@ -771,6 +771,7 @@ static int mlx5e_xfrm_add_state(struct net_device *dev,
 				struct xfrm_state *x,
 				struct netlink_ext_ack *extack)
 {
+	bool is_acq = x->xso.flags & XFRM_DEV_OFFLOAD_FLAG_ACQ;
 	struct mlx5e_ipsec_sa_entry *sa_entry = NULL;
 	bool allow_tunnel_mode = false;
 	struct mlx5e_ipsec *ipsec;
@@ -779,20 +780,30 @@ static int mlx5e_xfrm_add_state(struct net_device *dev,
 	int err;
 
 	priv = netdev_priv(dev);
-	if (!priv->ipsec)
-		return -EOPNOTSUPP;
+	if (!is_acq) {
+		err = mlx5_eswitch_block_mode(priv->mdev, true);
+		if (err)
+			return err;
+	}
 
 	ipsec = priv->ipsec;
-	gfp = (x->xso.flags & XFRM_DEV_OFFLOAD_FLAG_ACQ) ? GFP_ATOMIC : GFP_KERNEL;
+	if (!ipsec) {
+		err = -EOPNOTSUPP;
+		goto unblock_mode;
+	}
+
+	gfp = is_acq ? GFP_ATOMIC : GFP_KERNEL;
 	sa_entry = kzalloc_obj(*sa_entry, gfp);
-	if (!sa_entry)
-		return -ENOMEM;
+	if (!sa_entry) {
+		err = -ENOMEM;
+		goto unblock_mode;
+	}
 
 	sa_entry->x = x;
 	sa_entry->dev = dev;
 	sa_entry->ipsec = ipsec;
 	/* Check if this SA is originated from acquire flow temporary SA */
-	if (x->xso.flags & XFRM_DEV_OFFLOAD_FLAG_ACQ) {
+	if (is_acq) {
 		x->xso.offload_handle = (unsigned long)sa_entry;
 		return 0;
 	}
@@ -806,10 +817,6 @@ static int mlx5e_xfrm_add_state(struct net_device *dev,
 		goto err_xfrm;
 	}
 
-	err = mlx5_eswitch_block_mode(priv->mdev, true);
-	if (err)
-		goto unblock_ipsec;
-
 	if (x->props.mode == XFRM_MODE_TUNNEL &&
 	    x->xso.type == XFRM_DEV_OFFLOAD_PACKET) {
 		allow_tunnel_mode = mlx5e_ipsec_fs_tunnel_allowed(sa_entry);
@@ -817,7 +824,7 @@ static int mlx5e_xfrm_add_state(struct net_device *dev,
 			NL_SET_ERR_MSG_MOD(extack,
 					   "Packet offload tunnel mode is disabled due to encap settings");
 			err = -EINVAL;
-			goto unblock_mode;
+			goto unblock_ipsec;
 		}
 	}
 
@@ -893,12 +900,13 @@ static int mlx5e_xfrm_add_state(struct net_device *dev,
 unblock_encap:
 	if (allow_tunnel_mode)
 		mlx5_eswitch_unblock_encap(priv->mdev);
-unblock_mode:
-	mlx5_eswitch_unblock_mode(priv->mdev);
 unblock_ipsec:
 	mlx5_eswitch_unblock_ipsec(priv->mdev);
 err_xfrm:
 	kfree(sa_entry);
+unblock_mode:
+	if (!is_acq)
+		mlx5_eswitch_unblock_mode(priv->mdev);
 	NL_SET_ERR_MSG_WEAK_MOD(extack, "Device failed to offload this state");
 	return err;
 }
-- 
2.44.0

Re: [PATCH net 2/7] net/mlx5e: ipsec: Block eswitch mode changes before accessing priv->ipsec

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 1 (2) · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] By moving mlx5_eswitch_block_mode(priv->mdev, true) to the top
  of mlx5e_xfrm_add_state(), the first profile-owned field touched is…
- [Medium] mlx5e_xfrm_add_state() now calls
  mlx5_eswitch_block_mode(priv->mdev, true) *before* checking whether…
- [Low] mlx5e_xfrm_add_state() now emits the generic extack message
  "mlx5_core: Device failed to offload this state" on paths that report…

Pre-existing issues:
- [Medium] The two sibling xfrmdev_ops add callbacks take the same
  temporary eswitch-mode block for the same stated purpose (protect…
- [Medium] The CONFIG_MLX5_ESWITCH=n stub of mlx5_eswitch_block_ipsec()
  (eswitch.h:1090-1093) returns false, i.e. "blocked/busy", whereas the…

--

net/mlx5e: ipsec: Block eswitch mode changes before accessing priv->ipsec

This moves the mlx5_eswitch_block_mode() call in mlx5e_xfrm_add_state()
ahead of the first priv->ipsec access so a concurrent profile change
cannot free the context under it, and releases the block on all error
paths.  The acquire-placeholder path is kept exempt from the block.

A few questions about the reordering below.

> 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 841ecdc2c4d9e..1488faece80eb 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c
> @@ -771,6 +771,7 @@ static int mlx5e_xfrm_add_state(struct net_device *dev,
>  				struct xfrm_state *x,
>  				struct netlink_ext_ack *extack)
>  {
> +	bool is_acq = x->xso.flags & XFRM_DEV_OFFLOAD_FLAG_ACQ;
>  	struct mlx5e_ipsec_sa_entry *sa_entry = NULL;
>  	bool allow_tunnel_mode = false;
>  	struct mlx5e_ipsec *ipsec;
> @@ -779,20 +780,30 @@ static int mlx5e_xfrm_add_state(struct net_device *dev,
>  	int err;
>  
>  	priv = netdev_priv(dev);
> -	if (!priv->ipsec)
> -		return -EOPNOTSUPP;
> +	if (!is_acq) {
> +		err = mlx5_eswitch_block_mode(priv->mdev, true);
> +		if (err)
> +			return err;
> +	}

[Severity: High]
Can this dereference a NULL priv->mdev?  After this change the first
profile-owned field touched is priv->mdev rather than priv->ipsec, and
mlx5e_priv_cleanup() ends with:

drivers/net/ethernet/mellanox/mlx5/core/en_main.c:mlx5e_priv_cleanup() {
	...
	memset(priv, 0, sizeof(*priv));
}

so priv->mdev can be observed as NULL while the netdev is still
registered.  Besides the transient window between mlx5e_priv_cleanup()
and mlx5e_netdev_init_profile() during a profile change, it also stays
NULL indefinitely when both the new attach and the rollback fail:

drivers/net/ethernet/mellanox/mlx5/core/en_main.c:mlx5e_netdev_change_profile() {
	...
rollback:
	if (!orig_profile) {
		netdev_warn(netdev, "no original profile to rollback to\n");
		priv->profile = NULL;
		return err;
	}

	rollback_err = mlx5e_netdev_attach_profile(netdev, mdev, orig_profile, orig_ppriv);
	if (rollback_err) {
		netdev_err(netdev, "failed to rollback to orig profile, %d\n", rollback_err);
		priv->profile = NULL;
	}
}

mlx5e_priv_cleanup() itself documents that state with an early
"if (!priv->mdev) return;".  mlx5_eswitch_block_mode() then dereferences
its argument right away:

drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c:mlx5_eswitch_block_mode() {
	struct mlx5_eswitch *esw = dev->priv.eswitch;
	...
}

The previous first access, "if (!priv->ipsec) return -EOPNOTSUPP;",
handled a zeroed priv gracefully.  Would a priv->mdev check (or keeping
the priv->ipsec test first) be worth adding here?

[Severity: Medium]
Does this change the errno userspace sees for crypto-mode offload?
mlx5_eswitch_block_mode() forwards mlx5_esw_try_lock()'s returns:

drivers/net/ethernet/mellanox/mlx5/core/eswitch.c:mlx5_esw_try_lock() {
	if (down_write_trylock(&esw->mode_lock) == 0)
		return -EINVAL;

	if (esw->eswitch_operation_in_progress ||
	    (check_users && atomic64_read(&esw->user_count) > 0)) {
		up_write(&esw->mode_lock);
		return -EBUSY;
	}
}

Those now preempt the -EOPNOTSUPP that this netdev used to return
whenever priv->ipsec was NULL, and only -EOPNOTSUPP gets the software
fallback in the core:

net/xfrm/xfrm_device.c:xfrm_dev_state_add() {
		if ((err != -EOPNOTSUPP && !is_packet_offload) || is_packet_offload) {
			NL_SET_ERR_MSG_WEAK(extack, "Device failed to offload this state");
			return err;
		}
}

priv->ipsec is NULL when mlx5e_ipsec_init() failed (it logs "IPSec
initialization failed" and leaves the pointer unset while
mlx5e_ipsec_build_netdev() has already installed xfrmdev_ops), or while
the profile is detached.  So "ip xfrm state add ... offload dev X" in
crypto mode on such a netdev now fails hard with -EBUSY/-EINVAL instead
of falling back to software.  Should the priv->ipsec test stay ahead of
the mode block, or the eswitch error be translated for that case?

[Severity: Medium]
This isn't a bug introduced by this patch (the baseline already called
mlx5_eswitch_block_mode(priv->mdev, true) for every non-acquire SA add,
this only moves it earlier), but the two sibling add callbacks now sit
side by side with opposite check_users arguments for the same stated
purpose.  mlx5e_xfrm_add_policy() uses:

	/* Block esw mode changes until the policy holds its own block. */
	err = mlx5_eswitch_block_mode(priv->mdev, false);

while mlx5e_xfrm_add_state() passes true, which makes
mlx5_esw_try_lock() return -EBUSY whenever esw->user_count > 0, i.e.
whenever any tc flower rule exists on a VF representor - the case the
preceding commit added check_users to avoid.  Was check_users=true
intentional for the state path?

>  
>  	ipsec = priv->ipsec;
> -	gfp = (x->xso.flags & XFRM_DEV_OFFLOAD_FLAG_ACQ) ? GFP_ATOMIC : GFP_KERNEL;
> +	if (!ipsec) {
> +		err = -EOPNOTSUPP;
> +		goto unblock_mode;
> +	}
> +
> +	gfp = is_acq ? GFP_ATOMIC : GFP_KERNEL;
>  	sa_entry = kzalloc_obj(*sa_entry, gfp);
> -	if (!sa_entry)
> -		return -ENOMEM;
> +	if (!sa_entry) {
> +		err = -ENOMEM;
> +		goto unblock_mode;
> +	}
>  
>  	sa_entry->x = x;
>  	sa_entry->dev = dev;
>  	sa_entry->ipsec = ipsec;
>  	/* Check if this SA is originated from acquire flow temporary SA */
> -	if (x->xso.flags & XFRM_DEV_OFFLOAD_FLAG_ACQ) {
> +	if (is_acq) {
>  		x->xso.offload_handle = (unsigned long)sa_entry;
>  		return 0;
>  	}
> @@ -806,10 +817,6 @@ static int mlx5e_xfrm_add_state(struct net_device *dev,
>  		goto err_xfrm;
>  	}
>  
> -	err = mlx5_eswitch_block_mode(priv->mdev, true);
> -	if (err)
> -		goto unblock_ipsec;
> -

[Severity: Medium]
This is a pre-existing issue and not introduced here (git blame points
at 8efd7b17a3b032 from 2023), but while the goto labels on this error
path are being reshuffled it may be worth noting: the
CONFIG_MLX5_ESWITCH=n stub has the opposite polarity from the real
function.

drivers/net/ethernet/mellanox/mlx5/core/eswitch.h:
static inline bool mlx5_eswitch_block_ipsec(struct mlx5_core_dev *dev)
{
	return false;
}

drivers/net/ethernet/mellanox/mlx5/core/eswitch.c:mlx5_eswitch_block_ipsec() {
	if (!mlx5_esw_allowed(esw))
		return true;
	...
}

The caller just above this hunk maps false to -EBUSY:

	if (!mlx5_eswitch_block_ipsec(priv->mdev)) {
		err = -EBUSY;
		goto err_xfrm;
	}

so with CONFIG_MLX5_EN_IPSEC=y and CONFIG_MLX5_ESWITCH=n, does every SA
and policy offload fail with -EBUSY?  The sibling stubs follow the
"allow" convention (mlx5_eswitch_block_encap() returns true,
mlx5_eswitch_block_mode() returns 0).

>  	if (x->props.mode == XFRM_MODE_TUNNEL &&
>  	    x->xso.type == XFRM_DEV_OFFLOAD_PACKET) {
>  		allow_tunnel_mode = mlx5e_ipsec_fs_tunnel_allowed(sa_entry);
> @@ -817,7 +824,7 @@ static int mlx5e_xfrm_add_state(struct net_device *dev,
>  			NL_SET_ERR_MSG_MOD(extack,
>  					   "Packet offload tunnel mode is disabled due to encap settings");
>  			err = -EINVAL;
> -			goto unblock_mode;
> +			goto unblock_ipsec;
>  		}
>  	}
>  
> @@ -893,12 +900,13 @@ static int mlx5e_xfrm_add_state(struct net_device *dev,
>  unblock_encap:
>  	if (allow_tunnel_mode)
>  		mlx5_eswitch_unblock_encap(priv->mdev);
> -unblock_mode:
> -	mlx5_eswitch_unblock_mode(priv->mdev);
>  unblock_ipsec:
>  	mlx5_eswitch_unblock_ipsec(priv->mdev);
>  err_xfrm:
>  	kfree(sa_entry);
> +unblock_mode:
> +	if (!is_acq)
> +		mlx5_eswitch_unblock_mode(priv->mdev);
>  	NL_SET_ERR_MSG_WEAK_MOD(extack, "Device failed to offload this state");
>  	return err;
>  }

[Severity: Low]
Placing unblock_mode above the NL_SET_ERR_MSG_WEAK_MOD() means the
-EOPNOTSUPP (priv->ipsec == NULL) and -ENOMEM paths now attach
"mlx5_core: Device failed to offload this state" to the extack, where
they previously returned silently.

For crypto-mode requests the core swallows -EOPNOTSUPP and returns 0:

net/xfrm/xfrm_device.c:xfrm_dev_state_add() {
		if ((err != -EOPNOTSUPP && !is_packet_offload) || is_packet_offload) {
			NL_SET_ERR_MSG_WEAK(extack, "Device failed to offload this state");
			return err;
		}
	}

	return 0;
}

but the message is still delivered:

net/netlink/af_netlink.c:netlink_ack_tlv_len() {
	if (extack->_msg)
		tlvlen += nla_total_size(strlen(extack->_msg) + 1);
	...
	/* Following attributes are only reported as error (not warning) */
	if (!err)
		return tlvlen;
}

So does a successful "ip xfrm state add ... offload dev X" now print
"Warning: mlx5_core: Device failed to offload this state"?  The sibling
mlx5e_xfrm_add_policy() keeps its generic message above the label:

	NL_SET_ERR_MSG_MOD(extack, "Device failed to offload this policy");
unblock_mode:
	mlx5_eswitch_unblock_mode(priv->mdev);

Would the same layout work here?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260917175433.4090878-1-tariqt%40nvidia.com

[PATCH net 3/7] net/mlx5e: tc: Tie esw & accel blocking refs to the flow's lifetime

[Tariq Toukan]

From: Cosmin Ratiu <cratiu@nvidia.com>

TC flow creation acquires an esw user ref and, where required, an
IPsec-blocking reference. mlx5e_delete_flower() releases these, but
bulk cleanup (mlx5e_tc_nic_cleanup -> _mlx5e_tc_del_flow) destroys the
remaining flows without releasing either.

When bulk cleanup runs during suspend, the core device survives with
stale counters, which can prevent subsequent eswitch mode changes and
IPsec offload.

For the same reason, two more bugs are that the refs are dropped in
mlx5e_delete_flower(), before the flow is actually freed, leaving a
window of time where:
- a racing esw mode change could pull the rug from underneath the
  existing flow, leading to use after free.
- new IPsec objects might be installed, violating the restriction of
  mutual exclusion between TC and IPsec.

To fix these issues, this patch moves the reference acquisitions in
mlx5e_alloc_flow(), before the HW objects are actually allocated, and
moves the reference dropping to mlx5e_tc_del_flow(), after the HW
objects are deallocated.

Fixes: 7dc84de98bab ("net/mlx5: E-Switch, Protect changing mode while adding rules")
Fixes: c8e350e62fc5 ("net/mlx5e: Make TC and IPsec offloads mutually exclusive on a netdev")
Signed-off-by: Cosmin Ratiu <cratiu@nvidia.com>
Reviewed-by: Dragos Tatulea <dtatulea@nvidia.com>
Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
---
 .../net/ethernet/mellanox/mlx5/core/en_tc.c   | 48 ++++++++++++-------
 1 file changed, 30 insertions(+), 18 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
index b290beb4369a..44fc421e7b8c 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
@@ -603,6 +603,10 @@ struct mlx5e_hairpin_entry {
 
 static void mlx5e_tc_del_flow(struct mlx5e_priv *priv,
 			      struct mlx5e_tc_flow *flow);
+static int mlx5e_tc_block_ipsec_offload(struct net_device *filter,
+					struct mlx5e_priv *priv);
+static void mlx5e_tc_unblock_ipsec_offload(struct net_device *filter,
+					   struct mlx5e_priv *priv);
 
 struct mlx5e_tc_flow *mlx5e_flow_get(struct mlx5e_tc_flow *flow)
 {
@@ -2158,13 +2162,16 @@ static void mlx5e_tc_del_fdb_peers_flow(struct mlx5e_tc_flow *flow)
 static void mlx5e_tc_del_flow(struct mlx5e_priv *priv,
 			      struct mlx5e_tc_flow *flow)
 {
+	struct net_device *filter_dev = flow->attr->parse_attr->filter_dev;
+	bool peer = flow_flag_test(flow, PEER);
+
 	if (mlx5e_is_eswitch_flow(flow)) {
 		struct mlx5_devcom_comp_dev *devcom = flow->priv->mdev->priv.eswitch->devcom;
 
-		if (flow_flag_test(flow, PEER) ||
+		if (peer ||
 		    !mlx5_devcom_for_each_peer_begin(devcom)) {
 			mlx5e_tc_del_fdb_flow(priv, flow);
-			return;
+			goto out;
 		}
 
 		mlx5e_tc_del_fdb_peers_flow(flow);
@@ -2173,6 +2180,11 @@ static void mlx5e_tc_del_flow(struct mlx5e_priv *priv,
 	} else {
 		mlx5e_tc_del_nic_flow(priv, flow);
 	}
+out:
+	if (!peer) {
+		mlx5e_tc_unblock_ipsec_offload(filter_dev, flow->priv);
+		mlx5_esw_put(flow->priv->mdev);
+	}
 }
 
 static bool flow_requires_tunnel_mapping(u32 chain, struct flow_cls_offload *f)
@@ -4463,6 +4475,7 @@ mlx5_free_flow_attr_actions(struct mlx5e_tc_flow *flow, struct mlx5_flow_attr *a
 static int
 mlx5e_alloc_flow(struct mlx5e_priv *priv, int attr_size,
 		 struct flow_cls_offload *f, unsigned long flow_flags,
+		 struct net_device *filter_dev,
 		 struct mlx5e_tc_flow_parse_attr **__parse_attr,
 		 struct mlx5e_tc_flow **__flow)
 {
@@ -4497,11 +4510,23 @@ mlx5e_alloc_flow(struct mlx5e_priv *priv, int attr_size,
 	init_completion(&flow->init_done);
 	init_completion(&flow->del_hw_done);
 
+	parse_attr->filter_dev = filter_dev;
+	attr->parse_attr = parse_attr;
+	/* Non-peer flows own the reservations until final destruction. */
+	if (!flow_flag_test(flow, PEER)) {
+		err = mlx5e_tc_block_ipsec_offload(filter_dev, priv);
+		if (err)
+			goto err_free_attr;
+		mlx5_esw_get(priv->mdev);
+	}
+
 	*__flow = flow;
 	*__parse_attr = parse_attr;
 
 	return 0;
 
+err_free_attr:
+	kfree(attr);
 err_free:
 	kfree(flow);
 	kvfree(parse_attr);
@@ -4558,11 +4583,10 @@ __mlx5e_add_fdb_flow(struct mlx5e_priv *priv,
 	flow_flags |= BIT(MLX5E_TC_FLOW_FLAG_ESWITCH);
 	attr_size  = sizeof(struct mlx5_esw_flow_attr);
 	err = mlx5e_alloc_flow(priv, attr_size, f, flow_flags,
-			       &parse_attr, &flow);
+			       filter_dev, &parse_attr, &flow);
 	if (err)
 		goto out;
 
-	parse_attr->filter_dev = filter_dev;
 	mlx5e_flow_esw_attr_init(flow->attr,
 				 priv, parse_attr,
 				 f, in_rep, in_mdev);
@@ -4712,7 +4736,7 @@ mlx5e_add_fdb_flow(struct mlx5e_priv *priv,
 	mlx5e_tc_del_fdb_peers_flow(flow);
 	mlx5_devcom_for_each_peer_end(devcom);
 clean_flow:
-	mlx5e_tc_del_fdb_flow(priv, flow);
+	mlx5e_flow_put(priv, flow);
 	return err;
 }
 
@@ -4739,11 +4763,10 @@ mlx5e_add_nic_flow(struct mlx5e_priv *priv,
 	flow_flags |= BIT(MLX5E_TC_FLOW_FLAG_NIC);
 	attr_size  = sizeof(struct mlx5_nic_flow_attr);
 	err = mlx5e_alloc_flow(priv, attr_size, f, flow_flags,
-			       &parse_attr, &flow);
+			       filter_dev, &parse_attr, &flow);
 	if (err)
 		goto out;
 
-	parse_attr->filter_dev = filter_dev;
 	mlx5e_flow_attr_init(flow->attr, parse_attr, f);
 
 	err = parse_cls_flower(flow->priv, flow, &parse_attr->spec,
@@ -4865,12 +4888,6 @@ int mlx5e_configure_flower(struct net_device *dev, struct mlx5e_priv *priv,
 	if (!mlx5_esw_hold(priv->mdev))
 		return -EBUSY;
 
-	err = mlx5e_tc_block_ipsec_offload(dev, priv);
-	if (err)
-		goto esw_release;
-
-	mlx5_esw_get(priv->mdev);
-
 	rcu_read_lock();
 	flow = rhashtable_lookup(tc_ht, &f->cookie, tc_ht_params);
 	if (flow) {
@@ -4914,9 +4931,6 @@ int mlx5e_configure_flower(struct net_device *dev, struct mlx5e_priv *priv,
 err_free:
 	mlx5e_flow_put(priv, flow);
 out:
-	mlx5e_tc_unblock_ipsec_offload(dev, priv);
-	mlx5_esw_put(priv->mdev);
-esw_release:
 	mlx5_esw_release(priv->mdev);
 	return err;
 }
@@ -4957,8 +4971,6 @@ int mlx5e_delete_flower(struct net_device *dev, struct mlx5e_priv *priv,
 	trace_mlx5e_delete_flower(f);
 	mlx5e_flow_put(priv, flow);
 
-	mlx5e_tc_unblock_ipsec_offload(dev, priv);
-	mlx5_esw_put(priv->mdev);
 	return 0;
 
 errout:
-- 
2.44.0

Re: [PATCH net 3/7] net/mlx5e: tc: Tie esw & accel blocking refs to the flow's lifetime

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 0 (1) · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] `_mlx5e_tc_del_flow()` (en_tc.c:5396-5403, the
  `rhashtable_free_and_destroy()` callback used by…

Pre-existing issues:
- [Medium] The TC/IPsec mutual-exclusion counters `mdev->num_block_tc` /
  `mdev->num_block_ipsec` are plain `u64` fields…

--

net/mlx5e: tc: Tie esw & accel blocking refs to the flow's lifetime

This moves the eswitch user reference and the IPsec-blocking reference
acquisition from mlx5e_configure_flower() into mlx5e_alloc_flow(), and moves
the matching release from mlx5e_delete_flower() into mlx5e_tc_del_flow() so
bulk flow destruction also releases them. A couple of questions below about
the new release site.

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> index b290beb4369ad..44fc421e7b8c1 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c

[ ... ]

> @@ -2158,13 +2162,16 @@ static void mlx5e_tc_del_fdb_peers_flow(struct mlx5e_tc_flow *flow)
>  static void mlx5e_tc_del_flow(struct mlx5e_priv *priv,
>  			      struct mlx5e_tc_flow *flow)
>  {
> +	struct net_device *filter_dev = flow->attr->parse_attr->filter_dev;
> +	bool peer = flow_flag_test(flow, PEER);
> +
>  	if (mlx5e_is_eswitch_flow(flow)) {
>  		struct mlx5_devcom_comp_dev *devcom = flow->priv->mdev->priv.eswitch->devcom;
>  
> -		if (flow_flag_test(flow, PEER) ||
> +		if (peer ||
>  		    !mlx5_devcom_for_each_peer_begin(devcom)) {
>  			mlx5e_tc_del_fdb_flow(priv, flow);
> -			return;
> +			goto out;
>  		}
>  
>  		mlx5e_tc_del_fdb_peers_flow(flow);
> @@ -2173,6 +2180,11 @@ static void mlx5e_tc_del_fdb_peers_flow(struct mlx5e_tc_flow *flow)
>  	} else {
>  		mlx5e_tc_del_nic_flow(priv, flow);
>  	}
> +out:
> +	if (!peer) {
> +		mlx5e_tc_unblock_ipsec_offload(filter_dev, flow->priv);
> +		mlx5_esw_put(flow->priv->mdev);
> +	}
>  }

[Severity: High]
Can this new release pair run twice for the same flow?

The bulk cleanup callback used by mlx5e_tc_nic_cleanup() and
mlx5e_tc_ht_cleanup() ignores flow->refcnt entirely:

drivers/net/ethernet/mellanox/mlx5/core/en_tc.c:_mlx5e_tc_del_flow() {
	struct mlx5e_tc_flow *flow = ptr;
	struct mlx5e_priv *priv = flow->priv;

	mlx5e_tc_del_flow(priv, flow);
	kfree(flow);
}

If another context is holding a reference taken with mlx5e_flow_get() when
rhashtable_free_and_destroy() runs this callback, its later
mlx5e_flow_put() calls mlx5e_tc_del_flow() a second time on the already
freed flow, so mlx5e_tc_unblock_ipsec_offload() and mlx5_esw_put() execute
twice for one acquisition.

Does that underflow mdev->num_block_ipsec (leaving IPsec packet offload
rejected with -EBUSY until reload), and does the extra mlx5_esw_put()
consume an esw->user_count belonging to a different, still-live flow,
re-opening the mode-change-versus-live-rules window that 7dc84de98bab
closes?

The window looks structurally open during teardown. mlx5e_cleanup_rep_tx()
destroys the flow hash before the reference-holding contexts are quiesced:

drivers/net/ethernet/mellanox/mlx5/core/en_rep.c:mlx5e_cleanup_rep_tx() {
	mlx5e_tc_ht_cleanup(&rpriv->tc_ht);

	if (rpriv->rep->vport == MLX5_VPORT_UPLINK)
		mlx5e_cleanup_uplink_rep_tx(rpriv);

	mlx5e_rep_neigh_cleanup(rpriv);
}

The netevent notifier unregister, flush_workqueue(priv->wq) and
cancel_delayed_work_sync(&rpriv->neigh_update.neigh_stats_work) all happen
inside mlx5e_rep_neigh_cleanup(), i.e. after the hash is gone, and the FIB
notifier is unregistered even later via mlx5e_cleanup_uplink_rep_tx() ->
mlx5e_tc_tun_cleanup(). Both of those workers drop their flow references
outside any lock that the cleanup path takes:

drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_encap.c:mlx5e_tc_fib_event_work() {
	mutex_unlock(&esw->offloads.encap_tbl_lock);
	rtnl_unlock();

	mlx5e_put_flow_list(priv, &flow_list);
}

mlx5e_tc_update_neigh_used_value() does the same after unlocking
encap_tbl_lock, and mlx5e_rep_neigh_stats_work() is queued on fc_stats->wq
rather than priv->wq, so the earlier flush_workqueue(priv->wq) in
mlx5e_detach_netdev() does not cover it.

Would it be safer for _mlx5e_tc_del_flow() to go through mlx5e_flow_put()
(or for the reservations to be tied to the tc_ht-owned reference), and for
the neigh/FIB work to be quiesced before the flow table is destroyed?

[ ... ]

> @@ -4497,11 +4510,23 @@ mlx5e_alloc_flow(struct mlx5e_priv *priv, int attr_size,
>  	init_completion(&flow->init_done);
>  	init_completion(&flow->del_hw_done);
>  
> +	parse_attr->filter_dev = filter_dev;
> +	attr->parse_attr = parse_attr;
> +	/* Non-peer flows own the reservations until final destruction. */
> +	if (!flow_flag_test(flow, PEER)) {
> +		err = mlx5e_tc_block_ipsec_offload(filter_dev, priv);
> +		if (err)
> +			goto err_free_attr;
> +		mlx5_esw_get(priv->mdev);
> +	}
> +

[Severity: Medium]
This isn't a bug introduced by this patch, but it does widen the set of
unsynchronized contexts, so it seems worth asking: what serializes the
TC/IPsec exclusion counters now?

At this commit num_block_tc and num_block_ipsec are plain u64 fields in
struct mlx5_core_dev, and mlx5e_tc_block_ipsec_offload() does a
check-then-increment with no lock of its own:

	if (mdev->num_block_tc)
		return -EBUSY;

	mdev->num_block_ipsec++;

On this path the only thing held is the eswitch mode_lock read side taken by
mlx5_esw_hold() in mlx5e_configure_flower(), which does not exclude a
concurrent TC add on another netdev of the same mdev. The decrement side is
now reached from mlx5e_tc_del_flow() with no eswitch lock at all, including
from bulk cleanup and from mlx5e_tc_fib_event_work() /
mlx5e_tc_update_neigh_used_value() after they drop rtnl and encap_tbl_lock.

The peer side in mlx5e_ipsec_block_tc_offload() reads num_block_ipsec and
increments num_block_tc under mlx5_esw_lock(), and only when an eswitch
exists.

Can a lost update here leave num_block_ipsec permanently non-zero (IPsec
offload stuck at -EBUSY) or prematurely zero (IPsec offload accepted while
TC rules exist), i.e. break the invariant of c8e350e62fc5 in either
direction? On 32-bit the u64 read-modify-write can also tear.

A later patch in the series, "net/mlx5e: Serialize TC and IPsec offload
exclusion counters", moves both counters into mdev->offload_block and wraps
every check, increment and decrement in a dedicated mutex. Would it make
sense to order that patch before this one, since this one adds new
unlocked decrement contexts?

>  	*__flow = flow;
>  	*__parse_attr = parse_attr;
>  
>  	return 0;
>  
> +err_free_attr:
> +	kfree(attr);
>  err_free:
>  	kfree(flow);
>  	kvfree(parse_attr);

[ ... ]

> @@ -4865,12 +4888,6 @@ int mlx5e_configure_flower(struct net_device *dev, struct mlx5e_priv *priv,
>  	if (!mlx5_esw_hold(priv->mdev))
>  		return -EBUSY;
>  
> -	err = mlx5e_tc_block_ipsec_offload(dev, priv);
> -	if (err)
> -		goto esw_release;
> -
> -	mlx5_esw_get(priv->mdev);
> -
>  	rcu_read_lock();
>  	flow = rhashtable_lookup(tc_ht, &f->cookie, tc_ht_params);
>  	if (flow) {

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260917175433.4090878-1-tariqt%40nvidia.com

[PATCH net 4/7] net/mlx5e: macsec: Track hardware object ownership for SA teardown

[Tariq Toukan]

From: Cosmin Ratiu <cratiu@nvidia.com>

A driver MACsec SA can exist without a hardware MACsec object. Adding
an inactive RX SA or a TX SA while the SecY is non-operational creates
the encryption key but skips MACsec object creation. RX SA deactivation
also destroys the hardware object while retaining the driver SA and
its object ID.

TX SA deletion and the SA teardown performed by RX SC and SecY deletion
unconditionally destroy the hardware object. This can issue a destroy
command with an unallocated zero ID or an ID that has already been
destroyed and potentially reused. Failed RX SA activation can leave
the same stale-ID state after steering creation unwinds.

Track hardware object creation separately from SA activity. Set the
ownership flag after successful creation, clear it on destruction and
creation unwind, and check it before destroying the object during SA
cleanup. This preserves destruction of objects retained by inactive
TX SAs while avoiding destruction of unowned objects.

Fixes: 8ff0ac5be144 ("net/mlx5: Add MACsec offload Tx command support")
Fixes: aae3454e4d4c ("net/mlx5e: Add MACsec offload Rx command support")
Signed-off-by: Cosmin Ratiu <cratiu@nvidia.com>
Reviewed-by: Dragos Tatulea <dtatulea@nvidia.com>
Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c | 7 ++++++-
 1 file changed, 6 insertions(+), 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 daff53ba7d09..20eec345b36c 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c
@@ -58,6 +58,7 @@ struct mlx5e_macsec_async_work {
 
 struct mlx5e_macsec_sa {
 	bool active;
+	bool macsec_obj_created;
 	u8  assoc_num;
 	u32 macsec_obj_id;
 	u32 enc_key_id;
@@ -331,7 +332,10 @@ static void mlx5e_macsec_cleanup_sa(struct mlx5e_macsec *macsec,
 				    struct net_device *netdev, u32 fs_id)
 {
 	mlx5e_macsec_cleanup_sa_fs(macsec, sa, is_tx, netdev, fs_id);
-	mlx5e_macsec_destroy_object(macsec->mdev, sa->macsec_obj_id);
+	if (sa->macsec_obj_created) {
+		mlx5e_macsec_destroy_object(macsec->mdev, sa->macsec_obj_id);
+		sa->macsec_obj_created = false;
+	}
 }
 
 static int mlx5e_macsec_init_sa_fs(struct macsec_context *ctx,
@@ -396,6 +400,7 @@ static int mlx5e_macsec_init_sa(struct macsec_context *ctx,
 		if (err)
 			goto destroy_macsec_object;
 	}
+	sa->macsec_obj_created = true;
 
 	return 0;
 
-- 
2.44.0

Re: [PATCH net 4/7] net/mlx5e: macsec: Track hardware object ownership for SA teardown

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] `mlx5e_macsec_destroy_object()`
  (drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c:303-312)…

Pre-existing issues:
- [High] TX steering rules are still programmed with an unallocated
  MACsec object ID.
- [Medium] `mlx5e_macsec_upd_txsa()` commits `tx_sa->active =
  ctx_tx_sa->active;`…

--

net/mlx5e: macsec: Track hardware object ownership for SA teardown

This adds a macsec_obj_created flag to struct mlx5e_macsec_sa, sets it after
a successful hardware MACsec object creation, and checks it in
mlx5e_macsec_cleanup_sa() before issuing the destroy command. The intent is
to avoid destroying an object the driver never allocated, or one that was
already destroyed while the driver SA kept its object ID.

> 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 daff53ba7d09f..20eec345b36cd 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c
> @@ -58,6 +58,7 @@ struct mlx5e_macsec_async_work {
>  
>  struct mlx5e_macsec_sa {
>  	bool active;
> +	bool macsec_obj_created;
>  	u8  assoc_num;
>  	u32 macsec_obj_id;
>  	u32 enc_key_id;
> @@ -331,7 +332,10 @@ static void mlx5e_macsec_cleanup_sa(struct mlx5e_macsec *macsec,
>  				    struct net_device *netdev, u32 fs_id)
>  {
>  	mlx5e_macsec_cleanup_sa_fs(macsec, sa, is_tx, netdev, fs_id);
> -	mlx5e_macsec_destroy_object(macsec->mdev, sa->macsec_obj_id);
> +	if (sa->macsec_obj_created) {
> +		mlx5e_macsec_destroy_object(macsec->mdev, sa->macsec_obj_id);
> +		sa->macsec_obj_created = false;
> +	}
>  }

[Severity: High]

This isn't a bug introduced by this patch, but the new flag establishes that
macsec_obj_id is only meaningful while macsec_obj_created is set, and the
paths that consume the ID do not honour that.  Should the rule creation path
check it too?

mlx5e_macsec_init_sa_fs() copies the ID with no check:

	rule_attrs.macsec_obj_id = sa->macsec_obj_id;
	rule_attrs.sci = sa->sci;
	rule_attrs.assoc_num = sa->assoc_num;

and macsec_fs_tx_setup_fte() in lib/macsec_fs.c hands that value to the
device as the crypto object of the encrypt rule:

	flow_act->crypto.obj_id = macsec_obj_id;

A TX SA can reach that point with macsec_obj_id == 0 and
macsec_obj_created == false, because mlx5e_macsec_add_txsa() keeps the SA
after creating only the key:

	macsec_device->tx_sa[assoc_num] = tx_sa;
	if (!secy->operational)
		goto out;

Nothing creates the object afterwards: mlx5e_macsec_upd_txsa() and
mlx5e_macsec_upd_secy() only call mlx5e_macsec_init_sa_fs(), never
mlx5e_macsec_init_sa().

Can the following plain configuration sequence reach that state, with no
failure injection?

  ip macsec add <dev> tx sa 0 on key ...      /* object created */
  ip macsec set <dev> tx sa 0 off             /* macsec_upd_txsa() in
                                                 drivers/net/macsec.c sets
                                                 secy->operational = false */
  ip macsec add <dev> tx sa 1 on key ...      /* assoc_num != encoding_sa, so
                                                 operational stays false and
                                                 mlx5e_macsec_add_txsa()
                                                 takes the early exit */
  ip link set <dev> type macsec encoding-sa 1 /* macsec_changelink_common()
                                                 sets secy->operational and
                                                 macsec_changelink() calls
                                                 mdo_upd_secy */

mlx5e_macsec_upd_secy() then deletes all TX rules in its first loop and, in
the second loop, matches SA 1:

		if (tx_sa->assoc_num == tx_sc->encoding_sa && tx_sa->active) {
			err = mlx5e_macsec_init_sa_fs(ctx, tx_sa, tx_sc->encrypt, true, NULL);
			if (err)
				goto out;
		}

so the encrypt rule is installed against object ID 0, which the driver never
allocated.  If firmware rejects it, the error surfaces as -ENOMEM from
mlx5e_macsec_init_sa_fs() after the previous TX rules were already removed;
if it accepts it, the SecY is operational with an encrypt rule bound to an
object the driver does not own.  Would guarding init_sa_fs() on
macsec_obj_created (or creating the object when the SecY becomes operational)
be the matching change?

The ASO event lookups have the same predicate mismatch.
get_macsec_tx_sa_from_obj_id() and get_macsec_rx_sa_from_obj_id() select SAs
by activity, not by ownership:

			macsec_sa = iter->tx_sa[i];
			if (!macsec_sa || !macsec_sa->active)
				continue;
			if (macsec_sa->macsec_obj_id == obj_id)
				return macsec_sa;

An active SA that carries an ID it does not own can be picked up for
macsec_epn_update()/mlx5e_macsec_modify_obj(), while the inactive TX SA whose
object this patch deliberately keeps alive is skipped and its EPN event is
dropped as not found.  Should these lookups test macsec_obj_created instead
of active?

[Severity: Low]

Separately, mlx5e_macsec_destroy_object() discards the command status:

	mlx5_cmd_exec(mdev, in, sizeof(in), out, sizeof(out));

and the hunk above clears sa->macsec_obj_created regardless of the result.
Before this change, a failed destroy would be retried by the next
unconditional cleanup (RX SA deactivation followed by RX SC or SecY
deletion); with the flag cleared unconditionally that retry no longer
happens, so a firmware destroy failure would leave the object allocated for
good.  The destroy_macsec_object unwind in mlx5e_macsec_init_sa() has the
same property.  Ignoring the status of a destroy command in teardown is the
existing mlx5 convention (see mlx5_destroy_ipsec_obj() and the void
mlx5_destroy_encryption_key()), so this is only about the retry that the new
flag removes.

> @@ -396,6 +400,7 @@ static int mlx5e_macsec_init_sa(struct macsec_context *ctx,
>  		if (err)
>  			goto destroy_macsec_object;
>  	}
> +	sa->macsec_obj_created = true;
>  
>  	return 0;
>  

[Severity: Medium]

This is a pre-existing issue and not something the patch changes, but while
looking at the SA activity bookkeeping next door: mlx5e_macsec_upd_txsa()
commits the new activity value before validating the deactivation
precondition, and the error exit does not undo it:

	tx_sa->active = ctx_tx_sa->active;
	if (tx_sa->assoc_num != tx_sc->encoding_sa)
		goto out;

	if (ctx_tx_sa->active) {
		err = mlx5e_macsec_init_sa_fs(ctx, tx_sa, tx_sc->encrypt, true, NULL);
		if (err)
			goto out;
	} else {
		if (!tx_sa->macsec_rule) {
			err = -EINVAL;
			goto out;
		}

The core rolls its own state back on offload error in macsec_upd_txsa():

	tx_sa->active = was_active;
	secy->operational = was_operational;

So if an earlier activation failed in mlx5e_macsec_init_sa_fs(), the driver
SA is left active with macsec_rule == NULL while the core believes it is
inactive.  A later "tx sa N on" then hits the tx_sa->active ==
ctx_tx_sa->active early return and reports success without installing a rule,
and a later "off" returns -EINVAL with the driver flag already flipped to
inactive.  Should tx_sa->active only be updated once the rule work has
succeeded?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260917175433.4090878-1-tariqt%40nvidia.com

[PATCH net 5/7] net/mlx5e: macsec: Delete remaining SecYs during cleanup

[Tariq Toukan]

From: Cosmin Ratiu <cratiu@nvidia.com>

Suspend invokes MACsec cleanup without unregistering the netdev, so
offloaded SecYs may still exist. Cleanup frees their owning context
without deleting them, leaking driver state, encryption keys, MACsec
objects and steering resources.

This change deletes all remaining SecYs, which releases all HW objects
and avoids leaks.

But this only fixes resource teardown. On resume, the macsec driver may
still potentially have a lot of state it considers offloaded, but the
resumed device will have a clean slate. Fixing that is outside the scope
of this patch.

Fixes: 3fd3fb6b6b88 ("net/mlx5e: Move MACsec initialization from profile init stage to profile enable stage")
Signed-off-by: Cosmin Ratiu <cratiu@nvidia.com>
Reviewed-by: Dragos Tatulea <dtatulea@nvidia.com>
Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
---
 .../mellanox/mlx5/core/en_accel/macsec.c      | 53 ++++++++++++-------
 1 file changed, 35 insertions(+), 18 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 20eec345b36c..84ce388d3846 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c
@@ -1238,33 +1238,21 @@ static int mlx5e_macsec_upd_secy(struct macsec_context *ctx)
 	return err;
 }
 
-static int mlx5e_macsec_del_secy(struct macsec_context *ctx)
+static void macsec_del_secy(struct mlx5e_macsec *macsec,
+			    struct mlx5e_macsec_device *macsec_device)
 {
-	struct mlx5e_priv *priv = macsec_netdev_priv(ctx->netdev);
-	struct mlx5e_macsec_device *macsec_device;
+	struct net_device *netdev = (struct net_device *)macsec_device->netdev;
 	struct mlx5e_macsec_rx_sc *rx_sc, *tmp;
 	struct mlx5e_macsec_sa *tx_sa;
-	struct mlx5e_macsec *macsec;
 	struct list_head *list;
-	int err = 0;
 	int i;
 
-	mutex_lock(&priv->macsec->lock);
-	macsec = priv->macsec;
-	macsec_device = mlx5e_macsec_get_macsec_device_context(macsec, ctx);
-	if (!macsec_device) {
-		netdev_err(ctx->netdev, "MACsec offload: Failed to find device context\n");
-		err = -EINVAL;
-
-		goto out;
-	}
-
 	for (i = 0; i < MACSEC_NUM_AN; ++i) {
 		tx_sa = macsec_device->tx_sa[i];
 		if (!tx_sa)
 			continue;
 
-		mlx5e_macsec_cleanup_sa(macsec, tx_sa, true, ctx->secy->netdev, 0);
+		mlx5e_macsec_cleanup_sa(macsec, tx_sa, true, netdev, 0);
 		mlx5_destroy_encryption_key(macsec->mdev, tx_sa->enc_key_id);
 		kfree(tx_sa);
 		macsec_device->tx_sa[i] = NULL;
@@ -1272,7 +1260,7 @@ 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)
-		macsec_del_rxsc_ctx(macsec, rx_sc, ctx->secy->netdev);
+		macsec_del_rxsc_ctx(macsec, rx_sc, netdev);
 
 	kfree(macsec_device->dev_addr);
 	macsec_device->dev_addr = NULL;
@@ -1280,7 +1268,24 @@ static int mlx5e_macsec_del_secy(struct macsec_context *ctx)
 	list_del_rcu(&macsec_device->macsec_device_list_element);
 	--macsec->num_of_devices;
 	kfree(macsec_device);
+}
+
+static int mlx5e_macsec_del_secy(struct macsec_context *ctx)
+{
+	struct mlx5e_priv *priv = macsec_netdev_priv(ctx->netdev);
+	struct mlx5e_macsec *macsec = priv->macsec;
+	struct mlx5e_macsec_device *macsec_device;
+	int err = 0;
 
+	mutex_lock(&macsec->lock);
+	macsec_device = mlx5e_macsec_get_macsec_device_context(macsec, ctx);
+	if (!macsec_device) {
+		netdev_err(ctx->netdev, "MACsec offload: Failed to find device context\n");
+		err = -EINVAL;
+		goto out;
+	}
+
+	macsec_del_secy(macsec, macsec_device);
 out:
 	mutex_unlock(&macsec->lock);
 
@@ -1796,6 +1801,7 @@ int mlx5e_macsec_init(struct mlx5e_priv *priv)
 
 void mlx5e_macsec_cleanup(struct mlx5e_priv *priv)
 {
+	struct mlx5e_macsec_device *macsec_device, *tmp;
 	struct mlx5e_macsec *macsec = priv->macsec;
 	struct mlx5_core_dev *mdev = priv->mdev;
 
@@ -1803,9 +1809,20 @@ void mlx5e_macsec_cleanup(struct mlx5e_priv *priv)
 		return;
 
 	mlx5_notifier_unregister(mdev, &macsec->nb);
-	mlx5_macsec_fs_cleanup(mdev->macsec_fs);
 	destroy_workqueue(macsec->wq);
+
+	mutex_lock(&macsec->lock);
+	list_for_each_entry_safe(macsec_device, tmp,
+				 &macsec->macsec_device_list_head,
+				 macsec_device_list_element)
+		macsec_del_secy(macsec, macsec_device);
+	mutex_unlock(&macsec->lock);
+
+	xa_destroy(&macsec->sc_xarray);
+	mlx5_macsec_fs_cleanup(mdev->macsec_fs);
+	mdev->macsec_fs = NULL;
 	mlx5e_macsec_aso_cleanup(&macsec->aso, mdev);
 	mutex_destroy(&macsec->lock);
 	kfree(macsec);
+	priv->macsec = NULL;
 }
-- 
2.44.0

Re: [PATCH net 5/7] net/mlx5e: macsec: Delete remaining SecYs during cleanup

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 2 (1) · Medium: 0 (1) · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] mlx5e_macsec_cleanup() frees the MACsec context and sets
  `priv->macsec = NULL` (macsec.c:1826-1827) without ever withdrawing…
- [High] mlx5e_macsec_cleanup() takes macsec->lock only for the new SecY-
  deletion sweep (macsec.c:1814-1819), then drops it and destroys/frees…

Pre-existing issues:
- [High] macsec_del_rxsc_ctx() (macsec.c:820-849), which this patch newly
  invokes from the bulk cleanup path, releases the RX-SC's xarray…
- [Medium] Suspend-time cleanup now deletes every programmed SecY, SA,
  encryption key, MACsec HW object and steering rule…

--

net/mlx5e: macsec: Delete remaining SecYs during cleanup

The patch factors the SecY teardown out of mlx5e_macsec_del_secy() into a
new macsec_del_secy() helper, and calls it for every remaining entry of
macsec->macsec_device_list_head from mlx5e_macsec_cleanup().  It also
adds xa_destroy(), clears mdev->macsec_fs and clears priv->macsec at the
end of cleanup.

A few questions below about the teardown ordering and about the callbacks
that remain registered after cleanup has run.

> 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 20eec345b36cd..84ce388d3846e 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c

[ ... ]

> @@ -1272,7 +1260,7 @@ 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)
> -		macsec_del_rxsc_ctx(macsec, rx_sc, ctx->secy->netdev);
> +		macsec_del_rxsc_ctx(macsec, rx_sc, netdev);

[Severity: High]

This isn't a bug introduced by this patch, but since the cleanup sweep is
now a second caller of macsec_del_rxsc_ctx() it seems worth asking about
the lifetime of rx_sc->sc_xarray_element there:

drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c:macsec_del_rxsc_ctx() {
	...
	list_del_rcu(&rx_sc->rx_sc_list_element);
	xa_erase(&macsec->sc_xarray, rx_sc->sc_xarray_element->fs_id);
	dst_release(&rx_sc->md_dst->dst);
	kfree(rx_sc->sc_xarray_element);
	kfree_rcu_mightsleep(rx_sc);
}

The RX datapath loads that element under RCU:

drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c:mlx5e_macsec_offload_handle_rx_skb() {
	...
	rcu_read_lock();
	sc_xarray_element = xa_load(&macsec->sc_xarray, fs_id);
	rx_sc = sc_xarray_element ? sc_xarray_element->rx_sc : NULL;
	if (rx_sc && rx_sc->md_dst) {
	...
}

Only rx_sc is freed via kfree_rcu_mightsleep(); sc_xarray_element is freed
with plain kfree() right after xa_erase().  Can a reader that already
obtained sc_xarray_element from xa_load() then dereference freed memory for
sc_xarray_element->rx_sc, and go on to dereference the resulting garbage
pointer at rx_sc->md_dst?

The comment just above those lines says:

	/* 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.
	 */

xa_erase() stores NULL into the slot but does not wait for a grace period,
so is the stated justification for the immediate kfree() of
sc_xarray_element accurate?

>  
>  	kfree(macsec_device->dev_addr);
>  	macsec_device->dev_addr = NULL;
> @@ -1280,7 +1268,24 @@ static int mlx5e_macsec_del_secy(struct macsec_context *ctx)
>  	list_del_rcu(&macsec_device->macsec_device_list_element);
>  	--macsec->num_of_devices;
>  	kfree(macsec_device);
> +}
> +
> +static int mlx5e_macsec_del_secy(struct macsec_context *ctx)
> +{
> +	struct mlx5e_priv *priv = macsec_netdev_priv(ctx->netdev);
> +	struct mlx5e_macsec *macsec = priv->macsec;
> +	struct mlx5e_macsec_device *macsec_device;
> +	int err = 0;
>  
> +	mutex_lock(&macsec->lock);
> +	macsec_device = mlx5e_macsec_get_macsec_device_context(macsec, ctx);
> +	if (!macsec_device) {
> +		netdev_err(ctx->netdev, "MACsec offload: Failed to find device context\n");
> +		err = -EINVAL;
> +		goto out;
> +	}
> +
> +	macsec_del_secy(macsec, macsec_device);
>  out:
>  	mutex_unlock(&macsec->lock);
>  
> @@ -1796,6 +1801,7 @@ int mlx5e_macsec_init(struct mlx5e_priv *priv)
>  
>  void mlx5e_macsec_cleanup(struct mlx5e_priv *priv)
>  {
> +	struct mlx5e_macsec_device *macsec_device, *tmp;
>  	struct mlx5e_macsec *macsec = priv->macsec;
>  	struct mlx5_core_dev *mdev = priv->mdev;
>  
> @@ -1803,9 +1809,20 @@ void mlx5e_macsec_cleanup(struct mlx5e_priv *priv)
>  		return;
>  
>  	mlx5_notifier_unregister(mdev, &macsec->nb);
> -	mlx5_macsec_fs_cleanup(mdev->macsec_fs);
>  	destroy_workqueue(macsec->wq);
> +
> +	mutex_lock(&macsec->lock);
> +	list_for_each_entry_safe(macsec_device, tmp,
> +				 &macsec->macsec_device_list_head,
> +				 macsec_device_list_element)
> +		macsec_del_secy(macsec, macsec_device);
> +	mutex_unlock(&macsec->lock);

[Severity: Medium]

This is a pre-existing restoration gap rather than something this patch
introduces, and the commit message already says fixing it is out of scope,
but it may be worth spelling out what the observable result is.

After this sweep plus mlx5_macsec_fs_cleanup(), every SecY, SA, encryption
key, MACsec object and steering rule is gone, while the MACsec upper
netdevices stay registered with their SecYs still marked offloaded.
mlx5e_macsec_init() on resume only allocates a fresh context, an empty
sc_xarray, a new ASO and a new flow-steering instance, with no replay.

So on resume, does offloaded TX end up dropping frames here?

drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c:mlx5e_macsec_handle_tx_skb() {
	...
	fs_id = mlx5_macsec_fs_get_fs_id_from_hashtable(macsec->mdev->macsec_fs,
							&md_dst->u.macsec_info.sci);
	if (!fs_id)
		goto err_out;
	...
}

and do later control operations on those retained SecYs fail with -EINVAL
from the device-context lookup in mlx5e_macsec_del_secy(), which also makes
macsec_update_offload() abort the transition?

> +
> +	xa_destroy(&macsec->sc_xarray);
> +	mlx5_macsec_fs_cleanup(mdev->macsec_fs);
> +	mdev->macsec_fs = NULL;
>  	mlx5e_macsec_aso_cleanup(&macsec->aso, mdev);
>  	mutex_destroy(&macsec->lock);
>  	kfree(macsec);

[Severity: High]

macsec->lock is released before mutex_destroy() and kfree(macsec), and
priv->macsec is cleared only after the free.  Every offload callback reads
priv->macsec locklessly and then blocks on that same mutex, for example
mlx5e_macsec_add_secy() and the rewritten mlx5e_macsec_del_secy():

	struct mlx5e_macsec *macsec = priv->macsec;
	...
	mutex_lock(&macsec->lock);

mlx5e_macsec_cleanup() runs from mlx5e_nic_disable() after rtnl_unlock(),
while the MACsec core dispatches the mdo_* ops under rtnl (macsec_newlink()
-> mdo_add_secy, macsec_upd_offload() -> macsec_update_offload()), so rtnl
does not serialize the two paths and macsec->lock is the only mutual
exclusion:

CPU0 (netlink, rtnl held)
mlx5e_macsec_add_secy()
	macsec = priv->macsec;		/* still non-NULL */
	mutex_lock(&macsec->lock);	/* blocks */

CPU1 (devlink reload / health recovery / suspend, no rtnl)
mlx5e_macsec_cleanup()
	mutex_unlock(&macsec->lock);
	...
	mutex_destroy(&macsec->lock);
	kfree(macsec);
	priv->macsec = NULL;

Can CPU0 then acquire a mutex inside freed memory, list_add a new
macsec_device into the freed macsec_device_list_head, and mutex_unlock()
freed memory?  A SecY added in that window would also be leaked, which is
the case this patch is trying to close.  Would clearing the gating state
(and withdrawing the offload registration) before dropping the lock and
freeing be a better ordering?

> +	priv->macsec = NULL;
>  }

[Severity: High]

Clearing priv->macsec here leaves the offload registration in place.
netdev->macsec_ops and NETIF_F_HW_MACSEC are set once in
mlx5e_macsec_build_netdev(), called from mlx5e_build_nic_netdev() at netdev
creation, and nothing in mlx5e_macsec_cleanup() reverses that.  The core
only checks those two fields:

drivers/net/macsec.c:macsec_check_offload() {
	...
		return macsec->real_dev->features & NETIF_F_HW_MACSEC &&
		       macsec->real_dev->macsec_ops;
	...
}

There is no netif_device_present() test there, and none of the mlx5e
callbacks NULL-check priv->macsec; they all start with
mutex_lock(&priv->macsec->lock), and the rewritten mlx5e_macsec_del_secy()
does:

	struct mlx5e_macsec *macsec = priv->macsec;
	...
	mutex_lock(&macsec->lock);

Since mlx5e_macsec_cleanup() is called from mlx5e_nic_disable() after
rtnl_unlock() and with the netdev still registered (only
netif_device_detach()'ed), can "ip link del macsec0" reach
macsec_common_dellink() -> mdo_del_secy -> mlx5e_macsec_del_secy() with
priv->macsec == NULL and oops on the mutex_lock()?

The same applies indefinitely if mlx5e_macsec_init() fails on resume, since
mlx5e_nic_enable() only logs the error:

drivers/net/ethernet/mellanox/mlx5/core/en_main.c:mlx5e_nic_enable() {
	...
	err = mlx5e_macsec_init(priv);
	if (err)
		mlx5_core_err(mdev, "MACsec initialization failed, %d\n", err);
	...
}

so priv->macsec stays NULL while the ops remain advertised.  The TX helpers
reached from en_accel.h have the same exposure, for instance
mlx5e_macsec_handle_tx_skb() dereferencing macsec->mdev->macsec_fs with no
NULL check on macsec.  Should cleanup also drop macsec_ops and
NETIF_F_HW_MACSEC, or should the callbacks check priv->macsec?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260917175433.4090878-1-tariqt%40nvidia.com

[PATCH net 6/7] net/mlx5e: Serialize TC and IPsec offload exclusion counters

[Tariq Toukan]

From: Cosmin Ratiu <cratiu@nvidia.com>

The counters enforcing TC and IPsec packet offload mutual exclusion are
not consistently serialized. The IPsec add path conditionally takes the
eswitch write lock, but neither release path takes it. The TC add path
only holds the eswitch read lock, and devices without an eswitch cannot
rely on that lock at all.

Concurrent read-modify-write operations on the same counter can lose an
update. A stale nonzero count can keep rejecting offload requests after
the last user has gone, while an undercount can allow conflicting
offloads to coexist.

Move the counters into mdev->offload_block and protect all checks,
increments and decrements with a dedicated mutex. Keep the
opposing-counter check and reservation in the same critical section,
independent of eswitch availability. Initialize the lock for the core
device lifetime and add warnings for unbalanced releases. Remove the
now-unused mlx5_esw_lock() helper.

Fixes: c8e350e62fc5 ("net/mlx5e: Make TC and IPsec offloads mutually exclusive on a netdev")
Signed-off-by: Cosmin Ratiu <cratiu@nvidia.com>
Reviewed-by: Dragos Tatulea <dtatulea@nvidia.com>
Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
---
 .../mellanox/mlx5/core/en_accel/ipsec_fs.c    | 43 ++++++-------------
 .../net/ethernet/mellanox/mlx5/core/en_tc.c   | 18 +++++---
 .../net/ethernet/mellanox/mlx5/core/eswitch.c | 12 ------
 .../net/ethernet/mellanox/mlx5/core/eswitch.h |  1 -
 .../net/ethernet/mellanox/mlx5/core/main.c    |  3 ++
 include/linux/mlx5/driver.h                   |  7 ++-
 6 files changed, 32 insertions(+), 52 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 5c72656c623f..2b856d0db917 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
@@ -2574,45 +2574,26 @@ void mlx5e_accel_ipsec_fs_read_stats(struct mlx5e_priv *priv, void *ipsec_stats)
 	}
 }
 
-#ifdef CONFIG_MLX5_ESWITCH
 static int mlx5e_ipsec_block_tc_offload(struct mlx5_core_dev *mdev)
 {
-	struct mlx5_eswitch *esw = mdev->priv.eswitch;
-	int err = 0;
-
-	if (esw) {
-		err = mlx5_esw_lock(esw);
-		if (err)
-			return err;
-	}
-
-	if (mdev->num_block_ipsec) {
-		err = -EBUSY;
-		goto unlock;
-	}
+	int ret = 0;
 
-	mdev->num_block_tc++;
-
-unlock:
-	if (esw)
-		mlx5_esw_unlock(esw);
-
-	return err;
-}
-#else
-static int mlx5e_ipsec_block_tc_offload(struct mlx5_core_dev *mdev)
-{
-	if (mdev->num_block_ipsec)
-		return -EBUSY;
+	mutex_lock(&mdev->offload_block.lock);
+	if (mdev->offload_block.num_block_ipsec)
+		ret = -EBUSY;
+	else
+		mdev->offload_block.num_block_tc++;
+	mutex_unlock(&mdev->offload_block.lock);
 
-	mdev->num_block_tc++;
-	return 0;
+	return ret;
 }
-#endif
 
 static void mlx5e_ipsec_unblock_tc_offload(struct mlx5_core_dev *mdev)
 {
-	mdev->num_block_tc--;
+	mutex_lock(&mdev->offload_block.lock);
+	if (!WARN_ON_ONCE(!mdev->offload_block.num_block_tc))
+		mdev->offload_block.num_block_tc--;
+	mutex_unlock(&mdev->offload_block.lock);
 }
 
 int mlx5e_accel_ipsec_fs_add_rule(struct mlx5e_ipsec_sa_entry *sa_entry)
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
index 44fc421e7b8c..89463d18880c 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
@@ -4856,16 +4856,19 @@ static bool is_tc_ipsec_order_check_needed(struct net_device *filter, struct mlx
 static int mlx5e_tc_block_ipsec_offload(struct net_device *filter, struct mlx5e_priv *priv)
 {
 	struct mlx5_core_dev *mdev = priv->mdev;
+	int ret = 0;
 
 	if (!is_tc_ipsec_order_check_needed(filter, priv))
 		return 0;
 
-	if (mdev->num_block_tc)
-		return -EBUSY;
-
-	mdev->num_block_ipsec++;
+	mutex_lock(&mdev->offload_block.lock);
+	if (mdev->offload_block.num_block_tc)
+		ret = -EBUSY;
+	else
+		mdev->offload_block.num_block_ipsec++;
+	mutex_unlock(&mdev->offload_block.lock);
 
-	return 0;
+	return ret;
 }
 
 static void mlx5e_tc_unblock_ipsec_offload(struct net_device *filter, struct mlx5e_priv *priv)
@@ -4873,7 +4876,10 @@ static void mlx5e_tc_unblock_ipsec_offload(struct net_device *filter, struct mlx
 	if (!is_tc_ipsec_order_check_needed(filter, priv))
 		return;
 
-	priv->mdev->num_block_ipsec--;
+	mutex_lock(&priv->mdev->offload_block.lock);
+	if (!WARN_ON_ONCE(!priv->mdev->offload_block.num_block_ipsec))
+		priv->mdev->offload_block.num_block_ipsec--;
+	mutex_unlock(&priv->mdev->offload_block.lock);
 }
 
 int mlx5e_configure_flower(struct net_device *dev, struct mlx5e_priv *priv,
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
index 0b48cc7a6734..989ca26e8851 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
@@ -3028,18 +3028,6 @@ int mlx5_esw_try_lock(struct mlx5_eswitch *esw, bool check_users)
 	return esw->mode;
 }
 
-int mlx5_esw_lock(struct mlx5_eswitch *esw)
-{
-	down_write(&esw->mode_lock);
-
-	if (esw->eswitch_operation_in_progress) {
-		up_write(&esw->mode_lock);
-		return -EBUSY;
-	}
-
-	return 0;
-}
-
 /**
  * mlx5_esw_unlock() - Release write lock on esw mode lock
  * @esw: eswitch device.
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
index 4a9a1656f6db..d52146cff496 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
@@ -947,7 +947,6 @@ void mlx5_esw_release(struct mlx5_core_dev *dev);
 void mlx5_esw_get(struct mlx5_core_dev *dev);
 void mlx5_esw_put(struct mlx5_core_dev *dev);
 int mlx5_esw_try_lock(struct mlx5_eswitch *esw, bool check_users);
-int mlx5_esw_lock(struct mlx5_eswitch *esw);
 void mlx5_esw_unlock(struct mlx5_eswitch *esw);
 
 void esw_vport_change_handle_locked(struct mlx5_vport *vport);
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/main.c b/drivers/net/ethernet/mellanox/mlx5/core/main.c
index 5f28d906c35b..46b34c80c458 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/main.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/main.c
@@ -1810,6 +1810,7 @@ int mlx5_mdev_init(struct mlx5_core_dev *dev, int profile_idx)
 	lockdep_register_key(&dev->lock_key);
 	mutex_init(&dev->intf_state_mutex);
 	lockdep_set_class(&dev->intf_state_mutex, &dev->lock_key);
+	mutex_init(&dev->offload_block.lock);
 	mutex_init(&dev->mlx5e_res.uplink_netdev_lock);
 	mutex_init(&dev->wc_state_lock);
 
@@ -1901,6 +1902,7 @@ int mlx5_mdev_init(struct mlx5_core_dev *dev, int profile_idx)
 	mutex_destroy(&priv->alloc_mutex);
 	mutex_destroy(&priv->bfregs.wc_head.lock);
 	mutex_destroy(&priv->bfregs.reg_head.lock);
+	mutex_destroy(&dev->offload_block.lock);
 	mutex_destroy(&dev->intf_state_mutex);
 	lockdep_unregister_key(&dev->lock_key);
 	return err;
@@ -1928,6 +1930,7 @@ void mlx5_mdev_uninit(struct mlx5_core_dev *dev)
 	mutex_destroy(&priv->bfregs.reg_head.lock);
 	mutex_destroy(&dev->wc_state_lock);
 	mutex_destroy(&dev->mlx5e_res.uplink_netdev_lock);
+	mutex_destroy(&dev->offload_block.lock);
 	mutex_destroy(&dev->intf_state_mutex);
 	lockdep_unregister_key(&dev->lock_key);
 }
diff --git a/include/linux/mlx5/driver.h b/include/linux/mlx5/driver.h
index 83d0a83bbfbc..4e207bf49c31 100644
--- a/include/linux/mlx5/driver.h
+++ b/include/linux/mlx5/driver.h
@@ -788,8 +788,11 @@ struct mlx5_core_dev {
 	u32                      vsc_addr;
 	struct mlx5_hv_vhca	*hv_vhca;
 	struct mlx5_hwmon	*hwmon;
-	u64			num_block_tc;
-	u64			num_block_ipsec;
+	struct {
+		struct mutex lock;
+		u64 num_block_tc;
+		u64 num_block_ipsec;
+	} offload_block;
 #ifdef CONFIG_MLX5_MACSEC
 	struct mlx5_macsec_fs *macsec_fs;
 	/* MACsec notifier chain to sync MACsec core and IB database */
-- 
2.44.0

[PATCH net 7/7] net/mlx5e: shampo: Do not merge PSP packets

[Tariq Toukan]

From: Cosmin Ratiu <cratiu@nvidia.com>

PSP packets are UDP-encapsulated, and enabling HW GRO (SHAMPO in mlx5)
makes it possible to merge those packets in a silly way:
IP|UDP1|PSP1|payload1|UDP2|PSP2|payload2|...

That happens because the HW GRO machinery doesn't know about PSP and
considers it part of the UDP payload.

This patch enforces no match in SW.

Fixes: 29d7f433fcec ("net/mlx5e: Add Rx data path offload")
Signed-off-by: Cosmin Ratiu <cratiu@nvidia.com>
Reviewed-by: Dragos Tatulea <dtatulea@nvidia.com>
Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/en_rx.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
index 7bd0606a5253..5e07cc8586f6 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
@@ -2224,6 +2224,16 @@ static bool mlx5e_hw_gro_skb_has_enough_space(struct sk_buff *skb,
 		return page_size * nr_frags + data_bcnt <= GRO_LEGACY_MAX_SIZE;
 }
 
+static bool mlx5e_hw_gro_psp_match(struct sk_buff *skb, struct mlx5_cqe64 *cqe)
+{
+#ifdef CONFIG_MLX5_EN_PSP
+	/* PSP packets cannot be merged. */
+	return !mlx5e_psp_is_rx_flow(cqe);
+#else
+	return true;
+#endif
+}
+
 static void mlx5e_handle_rx_cqe_mpwrq_shampo(struct mlx5e_rq *rq, struct mlx5_cqe64 *cqe)
 {
 	u16 data_bcnt		= mpwrq_get_cqe_byte_cnt(cqe) - cqe->shampo.header_size;
@@ -2266,8 +2276,9 @@ static void mlx5e_handle_rx_cqe_mpwrq_shampo(struct mlx5e_rq *rq, struct mlx5_cq
 	}
 
 	if (*skb &&
-	    !(match && mlx5e_hw_gro_skb_has_enough_space(*skb, data_bcnt,
-							 page_size))) {
+	    !(match &&
+	      mlx5e_hw_gro_skb_has_enough_space(*skb, data_bcnt, page_size) &&
+	      mlx5e_hw_gro_psp_match(*skb, cqe))) {
 		match = false;
 		mlx5e_shampo_flush_skb(rq, cqe, match);
 	}
-- 
2.44.0

Re: [PATCH net 7/7] net/mlx5e: shampo: Do not merge PSP packets

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] net/mlx5e: redundant #ifdef for PSP in mlx5e_hw_gro_psp_match()

--

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
> @@ -2224,6 +2224,16 @@ static bool mlx5e_hw_gro_skb_has_enough_space(struct sk_buff *skb,
>  		return page_size * nr_frags + data_bcnt <= GRO_LEGACY_MAX_SIZE;
>  }
>  
> +static bool mlx5e_hw_gro_psp_match(struct sk_buff *skb, struct mlx5_cqe64 *cqe)
> +{
> +#ifdef CONFIG_MLX5_EN_PSP
> +	/* PSP packets cannot be merged. */
> +	return !mlx5e_psp_is_rx_flow(cqe);
> +#else
> +	return true;
> +#endif
> +}
> +
>  static void mlx5e_handle_rx_cqe_mpwrq_shampo(struct mlx5e_rq *rq, struct mlx5_cqe64 *cqe)
>  {
>  	u16 data_bcnt		= mpwrq_get_cqe_byte_cnt(cqe) - cqe->shampo.header_size;

[Severity: Low]
The #ifdef CONFIG_MLX5_EN_PSP in mlx5e_hw_gro_psp_match() is
redundant. en_accel/psp_rxtx.h already provides a stub
mlx5e_psp_is_rx_flow() that returns false when CONFIG_MLX5_EN_PSP is
not set, so !mlx5e_psp_is_rx_flow(cqe) already evaluates to true in
that configuration, which is exactly what the #else branch returns.

Please drop the conditional and just return
!mlx5e_psp_is_rx_flow(cqe) unconditionally. That keeps both
configurations compiling the same code path, avoids the risk of the
two branches drifting apart when the PSP helper changes, and matches
the rest of the driver, which relies on the header stubs rather than
sprinkling #ifdefs through en_rx.c. It also means the stub keeps
getting type-checked when PSP is disabled.

While you are there: the skb argument of mlx5e_hw_gro_psp_match() is
never used. Is it there for a follow-up that will compare the
in-progress skb against the new CQE, or can it be dropped?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260917175433.4090878-1-tariqt%40nvidia.com