Re: [PATCH net v1] net/mlx5e: kTLS, Fix missing error unwind when enabling RX offload

[Simon Horman <horms@kernel.org>]

On Mon, Oct 26, 2026 at 06:53:46PM +0900, Xuanqiang Luo wrote:
> From: Xuanqiang Luo <luoxuanqiang@kylinos.cn>
> 
> Since commit abed42f9cd80 ("net/mlx5e: Conditionally create async
> ICOSQ"), the async ICOSQ is only created when an XDP program is attached
> or priv->ktls_rx_was_enabled is set. When TLS RX offload is enabled for
> the first time, mlx5e_ktls_set_feature_rx() sets the flag and reopens
> the channels so that they get the queue, but it ignores the return value
> of mlx5e_safe_reopen_channels().
> 
> If the reopen fails, mlx5e_safe_switch_params() keeps the old channels,
> which have no async ICOSQ, while NETIF_F_HW_TLS_RX is still reported as
> enabled. The next offloaded RX connection then dereferences the NULL
> c->async_icosq in post_rx_param_wqes(). Enabling the feature again does
> not help, as the flag is already set and the reopen is skipped.
> 
> Propagate the error, clear the flag and destroy the accel TCP flow
> tables again, so that the feature stays disabled and the next attempt
> retries the reopen.
> 
> Fixes: abed42f9cd80 ("net/mlx5e: Conditionally create async ICOSQ")
> Signed-off-by: Xuanqiang Luo <luoxuanqiang@kylinos.cn>
> ---
> Found by code inspection while backporting commit abed42f9cd80 to a
> downstream kernel. I do not have a crypto-enabled ConnectX NIC, so the
> failure has not been reproduced and the fix is only compile-tested.
> 
> The crash itself is the one described in the changelog of abed42f9cd80
> ("async ICOSQ allocation is not triggered, causing a NULL pointer
> crash"). This patch only covers the case where the reopen that should
> create the queue fails. ktls.c is also the only caller that ignores the
> return value of mlx5e_safe_reopen_channels(); the callers in health.c
> and reporter_tx.c check it.
> 
>  drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)

I agree that a minimal approach to addressing this problem is appropriate
for net.

Reviewed-by: Simon Horman <horms@kernel.org>

However, I do think the logic of this function is becoming somewhat
convoluted and would benefit from:

1. Handling error paths in conditionals while leaving
   the main thread of execution unconditional.

2. Use goto for unwind.

This is approach is complicated by the function needing to
handle the enable condition. But at any rate, maybe a follow-up patch do do
something like this would be worthwhile?

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls.c b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls.c
index 3cc1213c2f68..0f5dcd1b822e 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls.c
@@ -137,19 +137,22 @@ int mlx5e_ktls_set_feature_rx(struct net_device *netdev, bool enable)
 	mutex_lock(&priv->state_lock);
 	if (enable) {
 		err = mlx5e_accel_fs_tcp_create(priv->fs);
-		if (!err && !priv->ktls_rx_was_enabled) {
-			priv->ktls_rx_was_enabled = true;
-			err = mlx5e_safe_reopen_channels(priv);
-			if (err) {
-				priv->ktls_rx_was_enabled = false;
-				mlx5e_accel_fs_tcp_destroy(priv->fs);
-			}
+		if (err || priv->ktls_rx_was_enabled)
+			goto out_unlock;
+
+		priv->ktls_rx_was_enabled = true;
+		err = mlx5e_safe_reopen_channels(priv);
+		if (err) {
+			priv->ktls_rx_was_enabled = false;
+			enable = false;
 		}
-	} else {
-		mlx5e_accel_fs_tcp_destroy(priv->fs);
 	}
-	mutex_unlock(&priv->state_lock);
 
+	if (!enable)
+		mlx5e_accel_fs_tcp_destroy(priv->fs);
+
+out_unlock:
+	mutex_unlock(&priv->state_lock);
 	return err;
 }
 

...