Re: [PATCH net-next 05/15] net/mlx5: LAG, decouple iteration end-of-walk from MLX5_MAX_PORTS

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] net/mlx5: LAG, wrong loop sentinel check in mlx5_handle_changeupper_event()
--

commit 2ad8acabb3b31aebe50252b21fd494729c3bdc6a
Author: Shay Drory <shayd@nvidia.com>

net/mlx5: LAG, decouple iteration end-of-walk from MLX5_MAX_PORTS

This commit introduces ldev->max_funcs to replace MLX5_MAX_PORTS as the
end-of-walk sentinel for LAG xarray iterators and updates callers and
xa_alloc() bounds accordingly.

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c b/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
> index b2ac5c671ae60..73bce918cddba 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
> @@ -395,6 +395,7 @@ static struct mlx5_lag *mlx5_lag_dev_alloc(struct mlx5_core_dev *dev)
>  		mlx5_core_err(dev, "Failed to init multipath lag err=%d\n",
>  			      err);
>  
> +	ldev->max_funcs = ldev->ports;
>  	return ldev;
>  
>  err_wq:

[ ... ]

> @@ -2095,7 +2096,7 @@ static int mlx5_handle_changeupper_event(struct mlx5_lag *ldev,
>  				break;
>  			}
>  		}
> -		if (i < MLX5_MAX_PORTS) {
> +		if (i < ldev->ports) {
>  			if (netif_is_bond_master(upper)) {
>  				slave = bond_slave_get_rcu(ndev_tmp);
>  				if (slave)

[Severity: Medium]
Should the check in mlx5_handle_changeupper_event() compare i against
ldev->max_funcs instead of ldev->ports?

In mlx5_handle_changeupper_event(), the loop uses mlx5_ldev_for_each():

	for_each_netdev_in_bond_rcu(upper, ndev_tmp) {
		mlx5_ldev_for_each(i, 0, ldev) {
			pf = mlx5_lag_pf(ldev, i);
			if (pf->netdev == ndev_tmp) {
				idx++;
				break;
			}
		}
		if (i < ldev->ports) {
			...
			bond_status |= (1 << idx);
		}

mlx5_ldev_for_each() calls mlx5_get_next_lag_func(), which now returns
ldev->max_funcs when the walk reaches the end without a match.

If ldev->max_funcs has grown past ldev->ports and a port function resides at
an xarray index i where ldev->ports <= i < ldev->max_funcs, a matching
ndev_tmp will increment idx and break out of the loop, but (i < ldev->ports)
will be false, skipping the has_inactive and bond_status updates for that
slave.

Additionally, in mlx5_lag_dev_alloc(), ldev->ports is initialized and
register_netdevice_notifier_net() registers mlx5_lag_netdev_event() before
mlx5_lag_mp_init() and before ldev->max_funcs = ldev->ports. If a
NETDEV_CHANGEUPPER event runs during that window while ldev->max_funcs is 0
and ldev->ports >= 2, mlx5_ldev_for_each() finishes immediately with i == 0
and idx == -1, causing (i < ldev->ports) to evaluate to true and execute
bond_status |= (1 << -1).

Would it make sense to compare i < ldev->max_funcs in
mlx5_handle_changeupper_event() and initialize ldev->max_funcs alongside
ldev->ports before registering the netdevice notifier in
mlx5_lag_dev_alloc()?

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