Re: [PATCH net-next 2/2] net/mlx5e: Batch ICOSQ trigger RCU synchronization

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] net/mlx5e: Lock bypass in mlx5e_trigger_napi_icosq due to premature flag clear
- [High] net/mlx5e: race condition in ICOSQ NAPI trigger due to unordered clear_bit
--

commit dcc196ca7d5ea35aa83767536fe42c5734f5a395
Author: Carolina Jubran <cjubran@nvidia.com>

net/mlx5e: Batch ICOSQ trigger RCU synchronization

This commit splits mlx5e_trigger_napi_icosq() to mark ICOSQs as requiring
locking, waits for a single RCU grace period, and then triggers all of them
during channel activation. It also retains the existing synchronization
semantics for standalone callers.

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> index fe4e5d0b2e9a9..58e4874b664f1 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> @@ -2837,14 +2837,11 @@ static int mlx5e_channel_stats_alloc(struct mlx5e_priv *priv, int ix, int cpu)
[ ... ]
> -void mlx5e_trigger_napi_icosq(struct mlx5e_channel *c)
> +static void mlx5e_trigger_napi_icosq_post_sync(struct mlx5e_channel *c)
>  {
>  	struct mlx5e_icosq *sq = &c->icosq;
>  	bool locked;
>  
> -	set_bit(MLX5E_SQ_STATE_LOCK_NEEDED, &sq->state);
> -	synchronize_net();
> -
>  	locked = mlx5e_icosq_sync_lock(sq);
>  	mlx5e_trigger_irq(sq);
>  	mlx5e_icosq_sync_unlock(sq, locked);
> @@ -2852,6 +2849,13 @@ void mlx5e_trigger_napi_icosq(struct mlx5e_channel *c)
>  	clear_bit(MLX5E_SQ_STATE_LOCK_NEEDED, &sq->state);

[Severity: High]
Can the unconditional clearing of this bit break the lock bypass mechanism
for concurrent callers that have already waited for the RCU grace period?

If concurrent devlink health recoveries for TX and RX reporters run on the
same channel (e.g., mlx5e_tx_reporter_err_cqe_recover() and
mlx5e_rx_reporter_err_rq_cqe_recover()), both will call
mlx5e_trigger_napi_icosq() and set the MLX5E_SQ_STATE_LOCK_NEEDED bit
before calling synchronize_net().

If the first thread to wake up executes mlx5e_trigger_napi_icosq_post_sync(),
acquires the lock, and unconditionally clears the bit here, would the second
thread then bypass the lock?

When the second thread executes mlx5e_trigger_napi_icosq_post_sync(), it
calls mlx5e_icosq_sync_lock(). Since the bit was cleared by the first
thread, mlx5e_icosq_sync_lock() would see the bit cleared:

drivers/net/ethernet/mellanox/mlx5/core/en.h:mlx5e_icosq_sync_lock() {
    if (likely(!test_bit(MLX5E_SQ_STATE_LOCK_NEEDED, &sq->state)))
        return false;
    ...
}

Will this cause the second thread to call mlx5e_trigger_irq() without the
lock, racing with NAPI softirq operations and leading to queue corruption?

[Severity: High]
Does this clear_bit() execution lack a memory barrier, allowing it to be
reordered before the doorbell write in mlx5e_trigger_irq()?

spin_unlock_bh() in mlx5e_icosq_sync_unlock() provides RELEASE semantics,
which prevents preceding memory operations from being reordered after the
release. However, it allows subsequent operations, like this clear_bit(),
to be reordered before the release.

If the CPU reorders the clear_bit() operation before the I/O doorbell write
in mlx5e_trigger_irq(), and is delayed, could a concurrent NAPI poll write
new WQEs and its own doorbell, followed by this CPU writing its older
doorbell and corrupting the queue?

>  }
>  
> +void mlx5e_trigger_napi_icosq(struct mlx5e_channel *c)
> +{
> +	set_bit(MLX5E_SQ_STATE_LOCK_NEEDED, &c->icosq.state);
> +	synchronize_net();
> +	mlx5e_trigger_napi_icosq_post_sync(c);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261011133212.807979-1-tariqt@nvidia.com?part=2