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

[Simon Horman]

On Fri, Oct 30, 2026 at 10:08:31AM +0800, Xuanqiang Luo wrote:
> 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@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: Simon Horman <horms@xxxxxxxxxx>
> 
> 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.

Likewise, thanks.

Just to be clear, the patch I provided informally in my previous email
is compile tested only.