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

[Xuanqiang Luo <xuanqiang.luo@linux.dev>]

On 2026/10/29 at 21:40 Simon Horman wrote:
> 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>

Thanks for the review!

> 
> 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;
>   }
>   
Thanks for the suggestion. Agreed, the nesting is getting hard to
follow. I'll send a cleanup along these lines to net-next once this
fix has made it there.

Thanks,
Xuanqiang