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

[Tariq Toukan <tariqt@nvidia.com>]

On 01/11/2026 12:41, Tariq Toukan wrote:
> 
> 
> 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@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: Tariq Toukan <tariqt@nvidia.com>
> 
> 
>> 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?
>>
> 
> 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.

Seems that we need a resend here.
Let me know if you want me to do that.