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

[Tariq Toukan]

On 29/10/2026 15:40, Simon Horman wrote:
> On Mon, Oct 26, 2026 at 06:53:46PM +0900, Xuanqiang Luo wrote:
> > From: Xuanqiang Luo <luoxuanqiang@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
> > ---
> > 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: Tariq Toukan <tariqt@xxxxxxxxxx>


> Reviewed-by: Simon Horman <horms@xxxxxxxxxx>
>
> 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?

Agree.

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

Makes sense.