Re: [PATCH net-next V2 3/3] net/mlx5: E-Switch, fix deadlock between devlink lock and esw->wq

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Open-coded trylock retry loop used to bypass a lock ordering deadlock, violating networking subsystem guidelines.
- [High] Unsynchronized atomic notifier unregistration creates a race condition that defeats the stale work filter, risking execution of old work on torn-down state.
--

commit 1ce8a06ae391b110c8a8badeed88f41bec66031f
Author: Mark Bloch <mbloch@nvidia.com>

net/mlx5: E-Switch, fix deadlock between devlink lock and esw->wq

This commit aims to fix a deadlock during E-Switch cleanup by checking a
generation counter before taking the devlink lock in the workqueue handler,
and invalidating the workqueue around mode changes and teardown.

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
> index 1986d4d0e886..8ec52498be3f 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c

[ ... ]

> @@ -1785,6 +1793,7 @@ void mlx5_eswitch_disable_locked(struct mlx5_eswitch *esw)
>  
>  	mlx5_eq_notifier_unregister(esw->dev, &esw->nb);
>  	mlx5_eswitch_event_handler_unregister(esw);
> +	mlx5_eswitch_invalidate_wq(esw);

Since mlx5_eq_notifier_unregister() uses atomic_notifier_chain_unregister(),
which does not wait for in-flight callbacks to complete on other CPUs, is
there a race condition here?

If a concurrent hardware event callback is already executing when
mlx5_eswitch_invalidate_wq() increments the generation counter, could the
callback read the new generation value and assign it to the new work item?

If that happens, the stale work might bypass the filter in esw_wq_handler()
and execute against torn-down state.

[ ... ]

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
> index 23af5a12dc07..9e2ae217c224 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c

[ ... ]

> @@ -3694,21 +3694,38 @@ static void esw_wq_handler(struct work_struct *work)
>  	struct mlx5_host_work *host_work;
>  	struct mlx5_eswitch *esw;
>  	struct devlink *devlink;
> +	int work_gen;
>  
>  	host_work = container_of(work, struct mlx5_host_work, work);
>  	esw = host_work->esw;
> +	work_gen = host_work->work_gen;
>  	devlink = priv_to_devlink(esw->dev);
>  
> -	devl_lock(devlink);
> +	/* Do not block on devlink lock until stale work is filtered out.
> +	 * Teardown can invalidate the generation and then wait for this
> +	 * workqueue while holding devlink lock.
> +	 */
> +	for (;;) {
> +		if (work_gen != atomic_read(&esw->generation))
> +			goto free;
> +
> +		if (devl_trylock(devlink))
> +			break;
> +
> +		wait_event_timeout(esw->work_queue_wait,
> +				   work_gen != atomic_read(&esw->generation),
> +				   msecs_to_jiffies(60));
> +	}

Does this open-coded trylock retry loop mask a lock ordering issue?

The commit message mentions this is used to avoid an AB-BA deadlock when
mlx5_eswitch_cleanup() calls destroy_workqueue() while holding the devlink
lock. Using a trylock loop with a sleep fallback to bypass lock ordering
issues reimplements blocking without lockdep's deadlock detection, which
might indicate the teardown sequence itself needs adjustment.

>  
>  	/* Stale work from one or more mode changes ago. Bail out. */

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