[PATCH net V2 0/4] net/mlx5e: Fix offload lifetime and exclusion bugs

[PATCH net V2 0/4] net/mlx5e: Fix offload lifetime 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.
- Serialize TC/IPsec exclusion counters with a dedicated mutex.

Regards,
Tariq

V2:
- Use mlx5_esw_lock() in mlx5_eswitch_block_mode() (Sashiko)
- Clarified the fix split across first two patches (Sashiko)
- More careful dereferencing priv->mdev in mlx5e_xfrm_add_state() (Sashiko)
- Avoided spurious error message during IPsec SW fallback (Sashiko)
- Harmonized check_users arg for policy & state add (Sashiko)
- Clarified the scope of the "tc: tie..." fix (Sashiko)
- Moved macsec patches into a separate series.

V1:
https://lore.kernel.org/netdev/20260917175433.4090878-1-tariqt@nvidia.com/

Cosmin Ratiu (4):
  net/mlx5e: ipsec: Block eswitch mode changes during policy creation
  net/mlx5e: ipsec: Block eswitch mode changes before accessing
    priv->ipsec
  net/mlx5e: Serialize TC and IPsec offload exclusion counters
  net/mlx5e: tc: Tie esw & accel blocking refs to the flow's lifetime

 .../mellanox/mlx5/core/en_accel/ipsec.c       | 65 +++++++++++++-----
 .../mellanox/mlx5/core/en_accel/ipsec_fs.c    | 47 ++++---------
 .../net/ethernet/mellanox/mlx5/core/en_tc.c   | 66 ++++++++++++-------
 .../net/ethernet/mellanox/mlx5/core/eswitch.c | 11 ++--
 .../net/ethernet/mellanox/mlx5/core/eswitch.h | 10 ++-
 .../mellanox/mlx5/core/eswitch_offloads.c     |  7 +-
 .../net/ethernet/mellanox/mlx5/core/main.c    |  3 +
 include/linux/mlx5/driver.h                   |  7 +-
 8 files changed, 128 insertions(+), 88 deletions(-)


base-commit: 99b43ede9e355ba35244cc9470bf1819774ce39d
-- 
2.44.0

[PATCH net V2 1/4] 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().

mlx5e_xfrm_add_state() has the same race, and will be fixed in the next
patch.

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    |  6 ++---
 .../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, 37 insertions(+), 20 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 8ffa8068e90a..cd7ff3613524 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;
 
@@ -2581,7 +2581,7 @@ static int mlx5e_ipsec_block_tc_offload(struct mlx5_core_dev *mdev)
 	int err = 0;
 
 	if (esw) {
-		err = mlx5_esw_lock(esw);
+		err = mlx5_esw_lock(esw, false);
 		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..69d922712048 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
@@ -3009,9 +3009,9 @@ void mlx5_esw_put(struct mlx5_core_dev *mdev)
  * 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.
+ * * -EINVAL - lock is already held.
  */
 int mlx5_esw_try_lock(struct mlx5_eswitch *esw)
 {
@@ -3027,11 +3027,12 @@ int mlx5_esw_try_lock(struct mlx5_eswitch *esw)
 	return esw->mode;
 }
 
-int mlx5_esw_lock(struct mlx5_eswitch *esw)
+int mlx5_esw_lock(struct mlx5_eswitch *esw, bool check_users)
 {
 	down_write(&esw->mode_lock);
 
-	if (esw->eswitch_operation_in_progress) {
+	if (esw->eswitch_operation_in_progress ||
+	    (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..1200018ced7a 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
@@ -947,7 +947,7 @@ 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_lock(struct mlx5_eswitch *esw);
+int mlx5_esw_lock(struct mlx5_eswitch *esw, bool check_users);
 void mlx5_esw_unlock(struct mlx5_eswitch *esw);
 
 void esw_vport_change_handle_locked(struct mlx5_vport *vport);
@@ -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..95e856f8d834 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,9 +4351,8 @@ 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);
-	if (err < 0)
+	err = mlx5_esw_lock(esw, check_users);
+	if (err)
 		return err;
 
 	esw->offloads.num_block_mode++;
-- 
2.44.0

[PATCH net V2 2/4] 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 saving the IPsec context and validating the
state, and release it on all error paths. Retain an early availability
check to preserve software fallback when IPsec is unavailable, and check
the context again after taking the mode block. Keep the atomic
acquire-placeholder path exempt, since it creates no hardware state and
cannot take sleeping locks.

As in policy creation, do not check eswitch users when taking this
temporary mode block. This allows states to reuse existing IPsec tables
when TC rules exist on VF representors, instead of rejecting them
unconditionally. New RX/TX tables still check eswitch users, and the
TC/IPsec exclusion counters still reject conflicting packet offloads.

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       | 50 +++++++++++++------
 1 file changed, 34 insertions(+), 16 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..cf721ef83d59 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c
@@ -771,28 +771,44 @@ 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 mlx5_core_dev *mdev;
 	struct mlx5e_ipsec *ipsec;
 	struct mlx5e_priv *priv;
 	gfp_t gfp;
 	int err;
 
 	priv = netdev_priv(dev);
-	if (!priv->ipsec)
+	mdev = priv->mdev;
+	if (!mdev || !priv->ipsec)
 		return -EOPNOTSUPP;
 
+	if (!is_acq) {
+		err = mlx5_eswitch_block_mode(mdev, false);
+		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 +822,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 +829,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;
 		}
 	}
 
@@ -876,7 +888,7 @@ static int mlx5e_xfrm_add_state(struct net_device *dev,
 	if (allow_tunnel_mode)
 		mlx5_eswitch_unblock_encap(priv->mdev);
 
-	mlx5_eswitch_unblock_mode(priv->mdev);
+	mlx5_eswitch_unblock_mode(mdev);
 
 	return 0;
 
@@ -893,13 +905,14 @@ 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);
 	NL_SET_ERR_MSG_WEAK_MOD(extack, "Device failed to offload this state");
+unblock_mode:
+	if (!is_acq)
+		mlx5_eswitch_unblock_mode(mdev);
 	return err;
 }
 
@@ -1262,12 +1275,17 @@ static int mlx5e_xfrm_add_policy(struct xfrm_policy *x,
 {
 	struct net_device *netdev = x->xdo.dev;
 	struct mlx5e_ipsec_pol_entry *pol_entry;
+	struct mlx5_core_dev *mdev;
 	struct mlx5e_priv *priv;
 	int err;
 
 	priv = netdev_priv(netdev);
+	mdev = priv->mdev;
+	if (!mdev)
+		return -EOPNOTSUPP;
+
 	/* Block esw mode changes until the policy holds its own block. */
-	err = mlx5_eswitch_block_mode(priv->mdev, false);
+	err = mlx5_eswitch_block_mode(mdev, false);
 	if (err) {
 		NL_SET_ERR_MSG_MOD(extack, "Eswitch busy, can't add policy");
 		return err;
@@ -1303,7 +1321,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);
+	mlx5_eswitch_unblock_mode(mdev);
 	return 0;
 
 err_fs:
@@ -1312,7 +1330,7 @@ static int mlx5e_xfrm_add_policy(struct xfrm_policy *x,
 	kfree(pol_entry);
 	NL_SET_ERR_MSG_MOD(extack, "Device failed to offload this policy");
 unblock_mode:
-	mlx5_eswitch_unblock_mode(priv->mdev);
+	mlx5_eswitch_unblock_mode(mdev);
 	return err;
 }
 
-- 
2.44.0

[PATCH net V2 3/4] 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.

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/main.c    |  3 ++
 include/linux/mlx5/driver.h                   |  7 ++-
 4 files changed, 32 insertions(+), 39 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 cd7ff3613524..1a4145d039ea 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, false);
-		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 b290beb4369a..3d2850e2d76e 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
@@ -4833,16 +4833,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)
@@ -4850,7 +4853,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/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 V2 4/4] 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.

Bulk cleanup still bypasses flow reference counting and can free flows
that remain referenced by asynchronous workers. This pre-existing
lifetime issue requires changes to worker quiescing and resource
teardown ordering and is outside the scope of this patch.

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 3d2850e2d76e..89463d18880c 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,
@@ -4871,12 +4894,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) {
@@ -4920,9 +4937,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;
 }
@@ -4963,8 +4977,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 V2 0/4] net/mlx5e: Fix offload lifetime and exclusion bugs

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

 - What hardware the change was tested on. For driver fixes please
   mention the device (and if relevant firmware version) used for
   testing, or say that the change was not tested on real hardware.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

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

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email says "Auto-emailing seemed not to have triggered."
---
net/mlx5e: ipsec: Block eswitch mode changes during policy creation

This commit takes a temporary eswitch mode block at the start of
mlx5e_xfrm_add_policy() and releases it once the flow tables hold their
own reference. It also adds a check_users argument to
mlx5_eswitch_block_mode() and mlx5_esw_lock(), and switches
mlx5_eswitch_block_mode() from mlx5_esw_try_lock() to mlx5_esw_lock().

> 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

[ ... ]

> @@ -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);

[Severity: High]

Can priv->mdev be NULL at this point? mlx5_eswitch_block_mode() reads
dev->priv.eswitch right away.

mlx5e_priv_cleanup() does memset(priv, 0, sizeof(*priv)), but the netdev
stays registered and keeps its xfrmdev_ops. One case is the
switchdev->legacy profile swap in mlx5e_vport_uplink_rep_unload(). The
state becomes permanent if a profile change and its rollback both fail.

xfrm_dev_policy_add() only checks that xdo_dev_policy_add exists, so this
path looks reachable:

xfrm_dev_policy_add()
  mlx5e_xfrm_add_policy()
    mlx5_eswitch_block_mode(NULL, false)
      esw = dev->priv.eswitch;

Before this patch, the !priv->ipsec check ran first and returned
-EOPNOTSUPP.

The next patch in the series, "net/mlx5e: ipsec: Block eswitch mode
changes before accessing priv->ipsec", adds a !mdev check before this
call. Without that patch, this commit oopses with CONFIG_MLX5_ESWITCH=y.

[Severity: High]

Can this blocking down_write() deadlock against driver unload or devlink
reload? The xfrm core calls this callback with a netdev reference held.
xfrm_dev_policy_add() takes it with dev_get_by_index(), and the call runs
under xfrm_cfg_mutex.

On the unload side, mlx5_eswitch_safe_aux_devs_remove() holds
esw->mode_lock for read across the uplink netdev unregister:

CPU1
_mlx5e_remove()
  mlx5_eswitch_safe_aux_devs_remove()
    mlx5_esw_hold()                  /* mode_lock held for read */
    mlx5_core_reps_aux_devs_remove()
      ...
      mlx5e_vport_uplink_rep_unload()
        unregister_netdev()
          netdev_wait_allrefs_any()  /* waits for CPU0's netdev ref */

CPU0
xfrm_dev_policy_add()
  dev_get_by_index()
  mlx5e_xfrm_add_policy()
    mlx5_eswitch_block_mode()
      mlx5_esw_lock()
        down_write(&esw->mode_lock)  /* waits for CPU1's read hold */

CPU0's policy isn't inserted yet, so the unregister flush never reaches
it. Each task then waits on the other forever. xfrm_cfg_mutex stays held,
so XFRM configuration in the netns would hang as well.

Before this patch, mlx5_eswitch_block_mode() used down_write_trylock()
and failed at once. mlx5e_xfrm_add_state(), rx_get() and tx_get() get the
same trylock-to-blocking change. The mlx5_eswitch_unblock_mode() on the
success path here is another down_write().

At baseline, mlx5e_ipsec_block_tc_offload()->mlx5_esw_lock() already
took the lock in a similar blocking way. The later patch "net/mlx5e:
Serialize TC and IPsec offload exclusion counters" removes that
acquisition, which leaves these block_mode/unblock_mode calls as the ones
that can trigger the hang. The lock is now also taken for policies that
fail validation and for devices without IPsec.

> +	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;
>  	}

[ ... ]

> 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 8ffa8068e90ac..cd7ff36135244 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;

[Severity: High]

This is a pre-existing issue and was not introduced by this patch. This
patch adds more blocking down_write() sites on mode_lock, though. Can
the release side of this reference self-deadlock during driver unload or
devlink reload in switchdev mode?

mlx5_eswitch_safe_aux_devs_remove() holds esw->mode_lock for read while
it unregisters the uplink netdev. NETDEV_UNREGISTER then flushes the
offloaded policies in the same task:

mlx5_eswitch_safe_aux_devs_remove()
  mlx5_esw_hold()            /* down_read_trylock(&esw->mode_lock) */
  mlx5_core_reps_aux_devs_remove()
    ...
    mlx5e_vport_uplink_rep_unload()
      unregister_netdev()
        xfrm_dev_unregister()
          xfrm_dev_policy_flush()
            xfrm_policy_kill()
              xfrm_dev_policy_delete()
                mlx5e_xfrm_del_policy()
                  mlx5e_accel_ipsec_fs_del_pol()
                    rx_ft_put_policy()
                      rx_put()
                        mlx5_eswitch_unblock_mode()
                          down_write(&esw->mode_lock)

tx_put() has the same pattern. An rwsem read hold can't be upgraded to a
write hold.

Wouldn't the task hang whenever an offloaded IPsec policy exists on the
uplink at unload time? The read hold comes from commit 2ae8c7edea87f5,
and this path is still there at the end of the series.

[ ... ]

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
> index eb74b62601682..95e856f8d834d 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,9 +4351,8 @@ 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);
> -	if (err < 0)
> +	err = mlx5_esw_lock(esw, check_users);
> +	if (err)
>  		return err;

[Severity: Medium]

This replaces mlx5_esw_try_lock() with mlx5_esw_lock(). The old function
used down_write_trylock() and returned -EINVAL when mode_lock was held.
The new one does an unconditional down_write().

All existing callers now sleep waiting for mode_lock where they used to
fail right away. That covers mlx5e_xfrm_add_state(), rx_get() and
tx_get(). rx_get() and tx_get() run under rx->ft.mutex and tx->ft.mutex.

The commit message only says:

    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().

Could the commit message also mention the switch from trylock to a
blocking acquisition? It adds a new lock dependency, as described in the
deadlock question in mlx5e_xfrm_add_policy() above.

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

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net/mlx5e: ipsec: Block eswitch mode changes before accessing priv->ipsec

This patch moves the eswitch mode block in mlx5e_xfrm_add_state() so it is
taken before priv->ipsec is read and before the state is validated. A
concurrent profile change can then no longer free the IPsec context while
it is in use. The acquire placeholder path is still not blocked, and
mlx5e_xfrm_add_policy() gains a local mdev with a NULL check.

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

[ ... ]

> Fixes: 22239eb258bc ("net/mlx5e: Prevent tunnel reformat when tunnel mode not allowed")

[Severity: Medium]
Is this the right Fixes: tag for the race being closed?

Before 22239eb258bc, mlx5e_xfrm_add_state() had no eswitch mode block at
all. It already read priv->ipsec, stored it in sa_entry, and used it for
validation, SA hardware context creation and
mlx5e_accel_ipsec_fs_add_rule(). 22239eb258bc added a block after that
read. It did not move an existing block later.

Before 22239eb258bc, the only mode block on this path was the late one in
rx_get()/tx_get() in ipsec_fs.c:

    err = mlx5_eswitch_block_mode(mdev, true);

The previous patch in this series ("net/mlx5e: ipsec: Block eswitch mode
changes during policy creation") says that late block comes too late,
because the table lookup already touches the IPsec context. That patch
tags a5b8ca9471d3 and says "mlx5e_xfrm_add_state() has the same race".

With the current tag, could a stable tree that has the packet offload
state path but not 22239eb258bc take the add_policy half of the fix and
never get this add_state half? Should this patch carry the same tag as
the policy patch?

> 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..cf721ef83d597 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c
> @@ -771,28 +771,44 @@ static int mlx5e_xfrm_add_state(struct net_device *dev,
>  				struct xfrm_state *x,
>  				struct netlink_ext_ack *extack)
>  {

[ ... ]

>  	priv = netdev_priv(dev);
> -	if (!priv->ipsec)
> +	mdev = priv->mdev;
> +	if (!mdev || !priv->ipsec)
>  		return -EOPNOTSUPP;
>  
> +	if (!is_acq) {
> +		err = mlx5_eswitch_block_mode(mdev, false);
> +		if (err)
> +			return err;
> +	}

[Severity: Low]
Should these loads of priv->mdev and priv->ipsec use READ_ONCE()?

They happen without a lock, before the mode block is held, so they can
race with a concurrent profile change:

mlx5e_netdev_change_profile()
  mlx5e_priv_cleanup()
    memset(priv, 0, sizeof(*priv));

The !mdev check only covers that window if priv->mdev is loaded exactly
once. With a plain load, the compiler may reload priv->mdev for the
mlx5_eswitch_block_mode() argument. The reloaded value could be NULL, and
mlx5_eswitch_block_mode() would then dereference dev->priv.eswitch. KCSAN
would also report these as data races.

The same question applies to the new mdev = priv->mdev read in
mlx5e_xfrm_add_policy().

[ ... ]

> @@ -1262,12 +1275,17 @@ static int mlx5e_xfrm_add_policy(struct xfrm_policy *x,
>  {
>  	struct net_device *netdev = x->xdo.dev;
>  	struct mlx5e_ipsec_pol_entry *pol_entry;
> +	struct mlx5_core_dev *mdev;
>  	struct mlx5e_priv *priv;
>  	int err;
>  
>  	priv = netdev_priv(netdev);
> +	mdev = priv->mdev;
> +	if (!mdev)
> +		return -EOPNOTSUPP;
> +
>  	/* Block esw mode changes until the policy holds its own block. */
> -	err = mlx5_eswitch_block_mode(priv->mdev, false);
> +	err = mlx5_eswitch_block_mode(mdev, false);

[Severity: Medium]
The commit message only covers add_state and does not mention this new
NULL check in mlx5e_xfrm_add_policy(). Should the check go into the
previous patch instead?

The previous patch ("net/mlx5e: ipsec: Block eswitch mode changes during
policy creation") moved mlx5_eswitch_block_mode(priv->mdev, false) ahead
of the !priv->ipsec check. mlx5_eswitch_block_mode() dereferences
dev->priv.eswitch without checking dev for NULL.

priv->mdev can be NULL while the netdev is still registered. If a profile
change fails and its rollback also fails, mlx5e_priv_cleanup() zeroes
priv:

mlx5e_priv_cleanup() {
    ...
	/* bail if change profile failed and also rollback failed */
	if (!priv->mdev)
		return;
    ...
	memset(priv, 0, sizeof(*priv));
}

netdev->xfrmdev_ops is not cleared, so a CAP_NET_ADMIN user who adds an
offloaded policy still reaches:

xfrm_dev_policy_add()
  mlx5e_xfrm_add_policy()
    mlx5_eswitch_block_mode(NULL, false)

At baseline, the !priv->ipsec check returned -EOPNOTSUPP in that state.
With only the previous patch applied, this path oopses.

The previous patch carries Fixes: a5b8ca9471d3 and this one carries
Fixes: 22239eb258bc. Could the previous patch be backported without this
guard? As posted, the series is also not bisect-safe at the previous
patch.

[ ... ]