Re: [PATCH net V2 3/4] {RDMA,net}/mlx5: notify IB uplink on LAG aggregate speed change

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] RDMA/mlx5: use-after-free of ib_device from lag_event() on teardown
- [Medium] net/mlx5: LAG speed change notified before updating eswitch vport speeds
--

commit 5dd9a6a9f1014a459afa674b27b22373d933dc51
Author: Or Har-Toov <ohartoov@nvidia.com>

{RDMA,net}/mlx5: notify IB uplink on LAG aggregate speed change

This commit adds MLX5_DRIVER_EVENT_LAG_SPEED_CHANGE to notify the mlx5 IB
driver when the cached LAG aggregate speed changes, dispatching
IB_EVENT_DEVICE_SPEED_CHANGE to RDMA consumers.

> diff --git a/drivers/infiniband/hw/mlx5/main.c b/drivers/infiniband/hw/mlx5/main.c
> index 6594d38cf1993..cfa7c7ba2b737 100644
> --- a/drivers/infiniband/hw/mlx5/main.c
> +++ b/drivers/infiniband/hw/mlx5/main.c
> @@ -3690,6 +3690,16 @@ static int lag_event(struct notifier_block *nb, unsigned long event, void *data)
>  			rdma_roce_rescan_port(ibdev, portnum + 1);
>  		}
>  		break;
> +	case MLX5_DRIVER_EVENT_LAG_SPEED_CHANGE: {
> +		struct ib_event speed_event = {};
> +
> +		if (!dev->ib_active)
> +			break;
> +		speed_event.device = ibdev;
> +		speed_event.event = IB_EVENT_DEVICE_SPEED_CHANGE;
> +		ib_dispatch_event(&speed_event);
> +		break;
> +	}

[Severity: High]
Can this race with __mlx5_ib_remove() and cause a use-after-free of
struct ib_device on ib_wq?

In drivers/infiniband/core/cache.c, ib_dispatch_event() does not deliver
the event synchronously inside lag_event(). Instead, it queues a work item
with an unreferenced pointer to ibdev onto ib_wq:

drivers/infiniband/core/cache.c:ib_dispatch_event() {
	...
	INIT_WORK(&work->work, ib_generic_event_task);

	work->event = *event;
	...
	queue_work(ib_wq, &work->work);
}

The RDMA core drains ib_wq during ib_unregister_device() via
__ib_unregister_device() -> ib_cache_cleanup_one() ->
flush_workqueue(ib_wq), which runs in MLX5_IB_STAGE_IB_REG cleanup.

However, in drivers/infiniband/hw/mlx5/main.c, dev->lag_events is
unregistered in mlx5_eth_lag_cleanup() under MLX5_IB_STAGE_ROCE, which is
an earlier stage than MLX5_IB_STAGE_IB_REG and therefore runs after
MLX5_IB_STAGE_IB_REG during the reverse-order teardown loop in
__mlx5_ib_remove():

drivers/infiniband/hw/mlx5/main.c:__mlx5_ib_remove() {
	dev->ib_active = false;

	/* Number of stages to cleanup */
	while (stage) {
		stage--;
		if (profile->stage[stage].cleanup)
			profile->stage[stage].cleanup(dev);
	}

	kfree(dev->port);
	ib_dealloc_device(&dev->ib_dev);
}

If lag_event() on ldev->wq checks dev->ib_active just before
__mlx5_ib_remove() sets dev->ib_active = false, __mlx5_ib_remove() can
complete MLX5_IB_STAGE_IB_REG cleanup (including flush_workqueue(ib_wq))
before lag_event() calls ib_dispatch_event(&speed_event).

When MLX5_IB_STAGE_ROCE cleanup then calls mlx5_eth_lag_cleanup() ->
mlx5e_lag_event_unregister() -> blocking_notifier_chain_unregister(), it
only waits for lag_event() to finish queuing the work item, not for ib_wq
to execute it. __mlx5_ib_remove() then calls ib_dealloc_device(&dev->ib_dev)
while ib_generic_event_task() is still pending on ib_wq, so
ib_generic_event_task() -> ib_dispatch_event_clients() in
drivers/infiniband/core/device.c can dereference event->device after it has
been freed.

>  	default:
>  		return NOTIFY_DONE;
>  	}
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c b/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
> index 05e6d0aa31fcf..afe0b9909f2dc 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
> @@ -1540,8 +1540,24 @@ static int mlx5_lag_get_devices_oper_speed(struct mlx5_lag *ldev,
>  	return 0;
>  }
>  
> +void mlx5_lag_notify_speed_change(struct mlx5_lag *ldev)
> +{
> +	struct lag_func *pf;
> +	int idx;
> +
> +	idx = mlx5_lag_get_dev_index_by_seq(ldev, MLX5_LAG_P1);
> +	if (idx < 0)
> +		return;
> +	pf = mlx5_lag_pf(ldev, idx);
> +	if (!pf)
> +		return;
> +	blocking_notifier_call_chain(&pf->dev->priv.lag_nh,
> +				     MLX5_DRIVER_EVENT_LAG_SPEED_CHANGE, NULL);
> +}
> +
>  int mlx5_lag_update_agg_speed(struct mlx5_lag *ldev)
>  {
> +	u32 old_speed;
>  	u32 speed;
>  	int err;
>  
> @@ -1553,7 +1569,11 @@ int mlx5_lag_update_agg_speed(struct mlx5_lag *ldev)
>  		return err;
>  	}
>  
> +	old_speed = READ_ONCE(ldev->agg_speed_mbps);
>  	WRITE_ONCE(ldev->agg_speed_mbps, speed);
> +	if (speed != old_speed)
> +		mlx5_lag_notify_speed_change(ldev);
> +
>  	return 0;
>  }

[Severity: Medium]
In switchdev mode, does notifying here fire IB_EVENT_DEVICE_SPEED_CHANGE
before the eswitch vport speeds have been updated in firmware?

In drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c,
mlx5_lag_set_vports_agg_speed() calls mlx5_lag_update_agg_speed() before
iterating over the PFs to update the vport speeds via
mlx5_lag_modify_device_vports_speed():

drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c:mlx5_lag_set_vports_agg_speed() {
	if (mlx5_lag_update_agg_speed(ldev))
		return;
	...
	mlx5_ldev_for_each(pf_idx, 0, ldev) {
		...
		mlx5_lag_modify_device_vports_speed(mdev, speed);
	}
}

On the switchdev representor ib_device, mlx5_ib_query_port_speed_rep() in
drivers/infiniband/hw/mlx5/main.c only reads the cached bond speed for the
uplink representor port, while non-uplink VF/SF representor ports on the
same ib_device query MLX5_VPORT_STATE_OP_MOD_ESW_VPORT from firmware:

drivers/infiniband/hw/mlx5/main.c:mlx5_ib_query_port_speed_rep() {
	if (rep->vport == MLX5_VPORT_UPLINK) {
		if (mlx5_lag_is_sriov(mdev))
			return mlx5_ib_query_port_speed_from_bond(dev,
								  port_num,
								  speed);

		return mlx5_ib_query_port_speed_from_port(dev, port_num,
							  speed);
	}

	op_mod = MLX5_VPORT_STATE_OP_MOD_ESW_VPORT;
	return mlx5_ib_query_port_speed_from_vport(dev->mdev, op_mod,
						   rep->vport, true, speed, dev,
						   port_num);
}

If a consumer of the switchdev representor ib_device queries port speeds
upon receiving IB_EVENT_DEVICE_SPEED_CHANGE, could non-uplink representor
ports still return the old max_tx_speed from firmware because
mlx5_lag_modify_device_vports_speed() has not finished yet?

[ ... ]

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