Skip to contentSkip to conversation

net/mlx5: fix state corruption in dev start error path

PATCH In review Maayan Kashani 2 participants 2 messages Nov 13, 2025 – Nov 17, 2025 Network drivers

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 net/mlx5: fix state corruption in dev start error path

When mlx5_dev_start() fails partway through initialization, the error cleanup code unconditionally calls cleanup functions for all steps, including those that were never successfully initialized. This causes state corruption leading to incorrect behavior on subsequent start attempts.

The issue manifests as:
1. First start attempt fails with -ENOMEM (expected)
2. Second start attempt returns -EINVAL instead of -ENOMEM
3. With flow isolated mode, second attempt incorrectly succeeds, leading to segfault in rte_eth_rx_burst()

Root cause: The single error label cleanup path calls functions like mlx5_traffic_disable() and mlx5_flow_stop_default() even when their corresponding initialization functions (mlx5_traffic_enable() and mlx5_flow_start_default()) were never called due to earlier failure.

For example, when mlx5_rxq_start() fails:
- mlx5_traffic_enable() at line 1403 never executes
- mlx5_flow_start_default() at line 1420 never executes
- But cleanup unconditionally calls:
* mlx5_traffic_disable() - destroys control flows list
* mlx5_flow_stop_default() - corrupts flow metadata state

This corrupts the device state, causing subsequent start attempts to fail with different errors or, in isolated mode, to incorrectly succeed with an improperly initialized device.

Fix by replacing the single error label with cascading error labels (Linux kernel style). Each label cleans up only its corresponding step, then falls through to clean up earlier steps.
This ensures only successfully initialized steps are cleaned up, maintaining device state consistency across failed start attempts.

Bugzilla ID: 1419

Fixes 8db7e3b69822 ("net/mlx5: change operations for non-cached flows")
---
 drivers/net/mlx5/mlx5_trigger.c | 66 +++++++++++++++++++++++----------
 1 file changed, 46 insertions(+), 20 deletions(-)
drivers/net/mlx5/mlx5_trigger.c +46 -20
index 916ac03c164..afe3cb32a89 100644
@@ -1226,6 +1226,11 @@ static void mlx5_dev_free_consec_tx_mem(struct rte_eth_dev *dev, bool on_stop)
 	}
 }
 
+#define SAVE_RTE_ERRNO_AND_STOP(ret, dev) do {	\
+	ret = rte_errno;			\
+	(dev)->data->dev_started = 0;		\
+} while (0)
+
 /**
  * DPDK callback to start the device.
  *
@@ -1316,25 +1321,30 @@ mlx5_dev_start(struct rte_eth_dev *dev)
 	if (ret) {
 		DRV_LOG(ERR, "port %u Tx packet pacing init failed: %s",
 			dev->data->port_id, strerror(rte_errno));
+		SAVE_RTE_ERRNO_AND_STOP(ret, dev);
 		goto error;
 	}
 	if (mlx5_devx_obj_ops_en(priv->sh) &&
 	    priv->obj_ops.lb_dummy_queue_create) {
 		ret = priv->obj_ops.lb_dummy_queue_create(dev);
-		if (ret)
-			goto error;
+		if (ret) {
+			SAVE_RTE_ERRNO_AND_STOP(ret, dev);
+			goto txpp_stop;
+		}
 	}
 	ret = mlx5_dev_allocate_consec_tx_mem(dev);
 	if (ret) {
 		DRV_LOG(ERR, "port %u Tx queues memory allocation failed: %s",
 			dev->data->port_id, strerror(rte_errno));
-		goto error;
+		SAVE_RTE_ERRNO_AND_STOP(ret, dev);
+		goto lb_dummy_queue_release;
 	}
 	ret = mlx5_txq_start(dev);
 	if (ret) {
 		DRV_LOG(ERR, "port %u Tx queue allocation failed: %s",
 			dev->data->port_id, strerror(rte_errno));
-		goto error;
+		SAVE_RTE_ERRNO_AND_STOP(ret, dev);
+		goto free_consec_tx_mem;
 	}
 	if (priv->config.std_delay_drop || priv->config.hp_delay_drop) {
 		if (!priv->sh->dev_cap.vf && !priv->sh->dev_cap.sf &&
@@ -1358,7 +1368,8 @@ mlx5_dev_start(struct rte_eth_dev *dev)
 	if (ret) {
 		DRV_LOG(ERR, "port %u Rx queue allocation failed: %s",
 			dev->data->port_id, strerror(rte_errno));
-		goto error;
+		SAVE_RTE_ERRNO_AND_STOP(ret, dev);
+		goto txq_stop;
 	}
 	/*
 	 * Such step will be skipped if there is no hairpin TX queue configured
@@ -1368,7 +1379,8 @@ mlx5_dev_start(struct rte_eth_dev *dev)
 	if (ret) {
 		DRV_LOG(ERR, "port %u hairpin auto binding failed: %s",
 			dev->data->port_id, strerror(rte_errno));
-		goto error;
+		SAVE_RTE_ERRNO_AND_STOP(ret, dev);
+		goto rxq_stop;
 	}
 	/* Set started flag here for the following steps like control flow. */
 	dev->data->dev_started = 1;
@@ -1376,7 +1388,8 @@ mlx5_dev_start(struct rte_eth_dev *dev)
 	if (ret) {
 		DRV_LOG(ERR, "port %u Rx interrupt vector creation failed",
 			dev->data->port_id);
-		goto error;
+		SAVE_RTE_ERRNO_AND_STOP(ret, dev);
+		goto rxq_stop;
 	}
 	mlx5_os_stats_init(dev);
 	/*
@@ -1388,7 +1401,8 @@ mlx5_dev_start(struct rte_eth_dev *dev)
 		DRV_LOG(ERR,
 			"port %u failed to attach indirect actions: %s",
 			dev->data->port_id, rte_strerror(rte_errno));
-		goto error;
+		SAVE_RTE_ERRNO_AND_STOP(ret, dev);
+		goto rx_intr_vec_disable;
 	}
 #ifdef HAVE_MLX5_HWS_SUPPORT
 	if (priv->sh->config.dv_flow_en == 2) {
@@ -1396,7 +1410,8 @@ mlx5_dev_start(struct rte_eth_dev *dev)
 		if (ret) {
 			DRV_LOG(ERR, "port %u failed to update HWS tables",
 				dev->data->port_id);
-			goto error;
+			SAVE_RTE_ERRNO_AND_STOP(ret, dev);
+			goto action_handle_detach;
 		}
 	}
 #endif
@@ -1404,7 +1419,8 @@ mlx5_dev_start(struct rte_eth_dev *dev)
 	if (ret) {
 		DRV_LOG(ERR, "port %u failed to set defaults flows",
 			dev->data->port_id);
-		goto error;
+		SAVE_RTE_ERRNO_AND_STOP(ret, dev);
+		goto action_handle_detach;
 	}
 	/* Set dynamic fields and flags into Rx queues. */
 	mlx5_flow_rxq_dynf_set(dev);
@@ -1421,12 +1437,14 @@ mlx5_dev_start(struct rte_eth_dev *dev)
 	if (ret) {
 		DRV_LOG(DEBUG, "port %u failed to start default actions: %s",
 			dev->data->port_id, strerror(rte_errno));
-		goto error;
+		SAVE_RTE_ERRNO_AND_STOP(ret, dev);
+		goto traffic_disable;
 	}
 	if (mlx5_dev_ctx_shared_mempool_subscribe(dev) != 0) {
 		DRV_LOG(ERR, "port %u failed to subscribe for mempool life cycle: %s",
 			dev->data->port_id, rte_strerror(rte_errno));
-		goto error;
+		SAVE_RTE_ERRNO_AND_STOP(ret, dev);
+		goto stop_default;
 	}
 	if (mlx5_flow_is_steering_disabled())
 		mlx5_flow_rxq_mark_flag_set(dev);
@@ -1455,19 +1473,27 @@ mlx5_dev_start(struct rte_eth_dev *dev)
 		priv->sh->port[priv->dev_port - 1].devx_ih_port_id =
 					(uint32_t)dev->data->port_id;
 	return 0;
-error:
-	ret = rte_errno; /* Save rte_errno before cleanup. */
-	/* Rollback. */
-	dev->data->dev_started = 0;
+stop_default:
 	mlx5_flow_stop_default(dev);
+traffic_disable:
 	mlx5_traffic_disable(dev);
-	mlx5_txq_stop(dev);
+action_handle_detach:
+	mlx5_action_handle_detach(dev);
+rx_intr_vec_disable:
+	mlx5_rx_intr_vec_disable(dev);
+rxq_stop:
 	mlx5_rxq_stop(dev);
+txq_stop:
+	mlx5_txq_stop(dev);
+free_consec_tx_mem:
+	mlx5_dev_free_consec_tx_mem(dev, false);
+lb_dummy_queue_release:
 	if (priv->obj_ops.lb_dummy_queue_release)
 		priv->obj_ops.lb_dummy_queue_release(dev);
-	mlx5_dev_free_consec_tx_mem(dev, false);
-	mlx5_txpp_stop(dev); /* Stop last. */
-	rte_errno = ret; /* Restore rte_errno. */
+txpp_stop:
+	mlx5_txpp_stop(dev);
+error:
+	rte_errno = ret;
 	return -rte_errno;
 }
 
Signature · 1 line
2.21.0
#1 lore raw

Hi,

Quoted 34 lines · Maayan Kashani

When mlx5_dev_start() fails partway through initialization, the error cleanup code unconditionally calls cleanup functions for all steps, including those that were never successfully initialized. This causes state corruption leading to incorrect behavior on subsequent start attempts.

The issue manifests as:
1. First start attempt fails with -ENOMEM (expected)
2. Second start attempt returns -EINVAL instead of -ENOMEM
3. With flow isolated mode, second attempt incorrectly succeeds, leading to segfault in rte_eth_rx_burst()

Root cause: The single error label cleanup path calls functions like mlx5_traffic_disable() and mlx5_flow_stop_default() even when their corresponding initialization functions (mlx5_traffic_enable() and mlx5_flow_start_default()) were never called due to earlier failure.

For example, when mlx5_rxq_start() fails:
- mlx5_traffic_enable() at line 1403 never executes
- mlx5_flow_start_default() at line 1420 never executes
- But cleanup unconditionally calls:
* mlx5_traffic_disable() - destroys control flows list
* mlx5_flow_stop_default() - corrupts flow metadata state

This corrupts the device state, causing subsequent start attempts to fail with different errors or, in isolated mode, to incorrectly succeed with an improperly initialized device.

Fix by replacing the single error label with cascading error labels (Linux kernel style). Each label cleans up only its corresponding step, then falls through to clean up earlier steps.
This ensures only successfully initialized steps are cleaned up, maintaining device state consistency across failed start attempts.

Quoted 7 lines

Bugzilla ID: 1419

Fixes 8db7e3b69822 ("net/mlx5: change operations for non-cached flows")

---

Patch applied to next-net-mlx,

Kindest regards
Raslan Darawsheh