Re: [PATCH net-next V2 02/13] net/mlx5: Detach auxiliary devices on attach failure during load

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit message says the fix "doesn't deserve a Fixes tag"
  because the bug was found by code inspection and never reproduced.

--

net/mlx5: Detach auxiliary devices on attach failure during load

This patch adds a mlx5_detach_device() call to the err_attach path of
mlx5_load_one_devl_locked(). The goal is to detach the auxiliary devices
that attached successfully before mlx5_attach_device() hit a failure.

> This issue was found during code inspection and was not reproduced.
> Hence it doesn't deserve a Fixes tag.

[Severity: Low]
Is this a valid reason to leave out the Fixes tag? The Fixes convention
points at the commit that introduced the bug. It doesn't exempt bugs that
were found by inspection and never reproduced.

mlx5_attach_device() in dev.c stops at the first failing entry. It does
not unwind the entries it has already added or resumed:

drivers/net/ethernet/mellanox/mlx5/core/dev.c:mlx5_attach_device() {
    ...
		if (ret) {
			mlx5_core_warn(dev, "Device[%d] (%s) failed to load\n",
				       i, mlx5_adev_devices[i].suffix);

			break;
		}
    ...
}

Before this patch, err_attach in mlx5_load_one_devl_locked() went
directly to clear_bit() and mlx5_unload(). mlx5_unload() destroys the
EQ table, the IRQ table, the flow steering core, the clock and the
bfreg. mlx5_function_teardown() then tears down the HCA.

The aux drivers at lower indices were still bound at that point. One
example is a resumed mlx5e netdev whose CQs still reference the freed
EQs. Later, TX timeout recovery or the remove path
(mlx5_unregister_device() -> aux remove -> CQ destroy) could touch
those freed resources.

This can be triggered by any aux probe or resume failure, such as
-ENOMEM, during devlink reload, firmware reset, health recovery
(recovery=true) or PCI resume.

According to git blame, the err_attach label without a detach came from:

Fixes: 6dea2f7eff96 ("net/mlx5: Separate probe vs. reload flows")

Could the tag be added, and could the commit message say what happens
without the fix (resources torn down while aux drivers are still
attached)? Without the tag, stable maintainers have no signal to pick
this up.

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/main.c b/drivers/net/ethernet/mellanox/mlx5/core/main.c
> index 1a45474ce03db..7e762f72b19ef 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/main.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/main.c
> @@ -1547,6 +1547,7 @@ int mlx5_load_one_devl_locked(struct mlx5_core_dev *dev, bool recovery)
>  	return 0;
>  
>  err_attach:
> +	mlx5_detach_device(dev, true);
>  	clear_bit(MLX5_INTERFACE_STATE_UP, &dev->intf_state);
>  	mlx5_unload(dev);
>  err_load:

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261110183826.2927274-1-tariqt%40nvidia.com