Re: [PATCH net] net/mlx4: shut down catas recovery timer during teardown

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 (1) · Medium: 1 · Low: 0 (1)

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The commit message describes a race that the timer core
  already prevents, and the patch carries Fixes: and Cc: stable tags for…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but device removal
  can deadlock with catas recovery.
- [Low] This problem wasn't introduced by this patch, but on the first-
  ever registration mlx4_stop_catas_poll() can run timer_shutdown_sync()…

--

net/mlx4: shut down catas recovery timer during teardown

This commit changes mlx4_stop_catas_poll() to call timer_shutdown_sync()
instead of timer_delete_sync(), so that poll_catas() cannot re-arm the
catas timer during teardown. The timer is set up again by timer_setup() in
mlx4_start_catas_poll() before it is reused.

> The catas error timer can rearm itself while the device is being
> stopped. If poll_catas() is running when timer_delete_sync() drops the
> timer-base lock, it can call mod_timer() after the deletion. The delete
> can then return with the timer queued again, allowing a subsequent callback
> to access teardown-owned MMIO state after it has been unmapped.

[Severity: Medium]
Can this sequence happen with timer_delete_sync()?

While the callback is running, __try_to_del_timer_sync() does not detach
the timer and returns -1:

kernel/time/timer.c:__try_to_del_timer_sync() {
    ...
	if (base->running_timer != timer) {
		ret = detach_if_pending(timer, base, true);
		if (shutdown)
			timer->function = NULL;
	}
    ...
}

__timer_delete_sync() keeps retrying until that changes:

	do {
		ret = __try_to_del_timer_sync(timer, shutdown);
		if (unlikely(ret < 0)) {
			del_timer_wait_running(timer);
			cpu_relax();
		}
	} while (ret < 0);

So the delete cannot return while poll_catas() is still running. After the
callback finishes, the next retry calls detach_if_pending() under the base
lock. That removes the timer that poll_catas() re-queued with mod_timer().

__mod_timer() also does not move a running timer to another base:

	if (likely(base->running_timer != timer)) {

The comment there says this lets timer_delete_sync() detect that the
handler has not finished yet. The timer_delete_sync() kerneldoc only rules
out add_timer_on() from the handler, and poll_catas() re-arms only with
mod_timer().

So timer_shutdown_sync() does not seem to change runtime behaviour for
this timer. The only other place that arms it is mlx4_start_catas_poll(),
and that calls timer_setup() first.

Could the race description and the Fixes: and Cc: stable tags be dropped,
with the change described as hardening instead? Or is there a different
re-arm path that the commit message should name?

> diff --git a/drivers/net/ethernet/mellanox/mlx4/catas.c b/drivers/net/ethernet/mellanox/mlx4/catas.c
> index edcc6f6626180..62574b790fd59 100644
> --- a/drivers/net/ethernet/mellanox/mlx4/catas.c
> +++ b/drivers/net/ethernet/mellanox/mlx4/catas.c
> @@ -305,7 +305,7 @@ void mlx4_stop_catas_poll(struct mlx4_dev *dev)
>  {
>  	struct mlx4_priv *priv = mlx4_priv(dev);
>  
> -	timer_delete_sync(&priv->catas_err.timer);
> +	timer_shutdown_sync(&priv->catas_err.timer);

[Severity: Low]
This isn't a bug introduced by this patch, but can this run on a
catas_err.timer that was never initialized?

On the first registration, mlx4_register_device() sets
MLX4_INTERFACE_STATE_UP before calling rescan_drivers_locked(). If
add_drivers() fails, it unregisters before mlx4_start_catas_poll() has
called timer_setup():

drivers/net/ethernet/mellanox/mlx4/intf.c:mlx4_register_device() {
    ...
	if (ret) {
		mlx4_unregister_device(dev);
		return ret;
	}

	mlx4_start_catas_poll(dev);
    ...
}

mlx4_unregister_device() only checks STATE_UP before calling
mlx4_stop_catas_poll(). That then calls timer_shutdown_sync() on a timer
that devlink_alloc() zeroed but timer_setup() never initialized.

Production kernels see no effect. With CONFIG_LOCKDEP, the zeroed
lockdep_map triggers a "trying to register non-static key" splat and
lockdep is turned off. With CONFIG_DEBUG_OBJECTS_TIMERS, debug_assert_init()
emits an ODEBUG warning. timer_delete_sync() behaved the same way before
this patch.

[Severity: High]
This is a pre-existing issue, but can device removal deadlock against
catas_work in this function?

mlx4_remove_one() takes devl_lock(devlink), sets
MLX4_INTERFACE_STATE_DELETION, and calls mlx4_unload_one() while still
holding the lock. That path reaches the flush here:

mlx4_remove_one()
  devl_lock(devlink)
  mlx4_unload_one()
    mlx4_unregister_device()
      mlx4_stop_catas_poll()
        flush_workqueue(dev->persist->catas_wq)

If the interface is already down, mlx4_catas_end() drains the same
workqueue through destroy_workqueue().

If poll_catas() has already queued catas_work, the worker first runs the
device reset. That can take a long time; the VF reset loop alone can run
for up to MLX4_COMM_TIME (10000 ms). Only then does the worker take the
devlink lock, before it checks DELETION:

drivers/net/ethernet/mellanox/mlx4/catas.c:mlx4_handle_error_state() {
	mlx4_enter_error_state(persist);
	devlink = priv_to_devlink(mlx4_priv(dev));
	devl_lock(devlink);
	mutex_lock(&persist->interface_state_mutex);
	if (persist->interface_state & MLX4_INTERFACE_STATE_UP &&
	    !(persist->interface_state & MLX4_INTERFACE_STATE_DELETION)) {
    ...
}

The remover waits for the work to finish, and the work waits for the
devlink lock that the remover holds. Neither wait has a timeout.

timer_shutdown_sync() only prevents future timer runs, so work that is
already queued is not covered. Could catas_wq be drained without holding
the devlink lock? Alternatively, mlx4_handle_error_state() could check
DELETION before calling devl_lock().

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261004121259.1245171-1-runyu.xiao%40seu.edu.cn