From: Gal Pressman <gal@nvidia.com>
Add validation to reject rates exceeding 255 Gbps that would overflow
the 8 bits max bandwidth field.
Fixes: d8880795dabf ("net/mlx5e: Implement DCBNL IEEE max rate")
Signed-off-by: Gal Pressman <gal@nvidia.com>
Reviewed-by: Nimrod Oren <noren@nvidia.com>
Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
---
drivers/net/ethernet/mellanox/mlx5/core/en_dcbnl.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_dcbnl.c b/drivers/net/ethernet/mellanox/mlx5/core/en_dcbnl.c
index 345614471052..d88a48210fdc 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_dcbnl.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_dcbnl.c
@@ -596,11 +596,13 @@ static int mlx5e_dcbnl_ieee_setmaxrate(struct net_device *netdev,
u8 max_bw_value[IEEE_8021QAZ_MAX_TCS];
u8 max_bw_unit[IEEE_8021QAZ_MAX_TCS];
__u64 upper_limit_mbps;
+ __u64 upper_limit_gbps;
int i;
memset(max_bw_value, 0, sizeof(max_bw_value));
memset(max_bw_unit, 0, sizeof(max_bw_unit));
upper_limit_mbps = 255 * MLX5E_100MB;
+ upper_limit_gbps = 255 * MLX5E_1GB;
for (i = 0; i <= mlx5_max_tc(mdev); i++) {
if (!maxrate->tc_maxrate[i]) {
@@ -612,10 +614,16 @@ static int mlx5e_dcbnl_ieee_setmaxrate(struct net_device *netdev,
MLX5E_100MB);
max_bw_value[i] = max_bw_value[i] ? max_bw_value[i] : 1;
max_bw_unit[i] = MLX5_100_MBPS_UNIT;
- } else {
+ } else if (max_bw_value[i] <= upper_limit_gbps) {
max_bw_value[i] = div_u64(maxrate->tc_maxrate[i],
MLX5E_1GB);
max_bw_unit[i] = MLX5_GBPS_UNIT;
+ } else {
+ netdev_err(netdev,
+ "tc_%d maxrate %llu Kbps exceeds limit %llu\n",
+ i, maxrate->tc_maxrate[i],
+ upper_limit_gbps);
+ return -EINVAL;
}
}
--
2.31.1On Sun, Nov 9, 2025 at 11:37:52AM +0200, Gal Pressman wrote:
> Add validation to reject rates exceeding 255 Gbps that would overflow
> the 8 bits max bandwidth field.
Hi Gal, Tariq, Paolo,
While reviewing this commit (43b27d1bd88a) for backporting, I believe
I've found a logic error in the validation condition.
The issue is on line 617:
} else if (max_bw_value[i] <= upper_limit_gbps) {
max_bw_value[i] = div_u64(maxrate->tc_maxrate[i], MLX5E_1GB);
max_bw_unit[i] = MLX5_GBPS_UNIT;
Here, max_bw_value[i] is used in the condition before it's assigned in
this branch. This appears to be a copy-paste error from the previous
commit a7bf4d5063c7 ("net/mlx5e: Fix maxrate wraparound in threshold
between units").
The condition should check the input value maxrate->tc_maxrate[i], not
the output variable max_bw_value[i]:
} else if (maxrate->tc_maxrate[i] <= upper_limit_gbps) {
This matches the pattern used in the first branch:
if (maxrate->tc_maxrate[i] <= upper_limit_mbps) {
max_bw_value[i] = div_u64(maxrate->tc_maxrate[i], MLX5E_100MB);
...
}
Impact:
-------
The current code will compare an uninitialized (or stale from previous
iteration) max_bw_value[i] against upper_limit_gbps, rather than
comparing the actual requested rate. This means:
1. For rates between 25.5 Gbps and 255 Gbps:
- If max_bw_value[i] happens to be 0 (from memset), the condition
(0 <= 255000000000) is true, incorrectly allowing the GBPS path
- The rate gets converted to Gbps units, which may be correct by
accident
2. The validation in the else clause that rejects rates > 255 Gbps may
never trigger correctly if max_bw_value[i] from a previous iteration
is small enough
3. For i > 0, max_bw_value[i] contains the computed value from the
previous TC, leading to incorrect branching logic
This makes the overflow validation unreliable and could allow rates that
should be rejected, or reject rates that should be accepted.
Suggested fix:
--------------
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_dcbnl.c b/drivers/net/ethernet/mellanox/mlx5/core/en_dcbnl.c
index d88a48210fdc..XXXXXXXX 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_dcbnl.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_dcbnl.c
@@ -614,7 +614,7 @@ static int mlx5e_dcbnl_ieee_setmaxrate(struct net_device *netdev,
MLX5E_100MB);
max_bw_value[i] = max_bw_value[i] ? max_bw_value[i] : 1;
max_bw_unit[i] = MLX5_100_MBPS_UNIT;
- } else if (max_bw_value[i] <= upper_limit_gbps) {
+ } else if (maxrate->tc_maxrate[i] <= upper_limit_gbps) {
max_bw_value[i] = div_u64(maxrate->tc_maxrate[i],
MLX5E_1GB);
max_bw_unit[i] = MLX5_GBPS_UNIT;
Let me know if you'd like me to send a formal patch for this.
Thanks,
--
Danielle Costantino
Meta Platforms, Inc.
Flagged by Claude Code with https://github.com/masoncl/review-prompts/
On 20/11/2025 23:42, Danielle Costantino wrote:
> On Sun, Nov 9, 2025 at 11:37:52AM +0200, Gal Pressman wrote:
>> Add validation to reject rates exceeding 255 Gbps that would overflow
>> the 8 bits max bandwidth field.
>
> Hi Gal, Tariq, Paolo,
>
> While reviewing this commit (43b27d1bd88a) for backporting, I believe
> I've found a logic error in the validation condition.
>
> The issue is on line 617:
>
> } else if (max_bw_value[i] <= upper_limit_gbps) {
> max_bw_value[i] = div_u64(maxrate->tc_maxrate[i], MLX5E_1GB);
> max_bw_unit[i] = MLX5_GBPS_UNIT;
>
> Suggested fix:
> --------------
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_dcbnl.c b/drivers/net/ethernet/mellanox/mlx5/core/en_dcbnl.c
> index d88a48210fdc..XXXXXXXX 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_dcbnl.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_dcbnl.c
> @@ -614,7 +614,7 @@ static int mlx5e_dcbnl_ieee_setmaxrate(struct net_device *netdev,
> MLX5E_100MB);
> max_bw_value[i] = max_bw_value[i] ? max_bw_value[i] : 1;
> max_bw_unit[i] = MLX5_100_MBPS_UNIT;
> - } else if (max_bw_value[i] <= upper_limit_gbps) {
> + } else if (maxrate->tc_maxrate[i] <= upper_limit_gbps) {
> max_bw_value[i] = div_u64(maxrate->tc_maxrate[i],
> MLX5E_1GB);
> max_bw_unit[i] = MLX5_GBPS_UNIT;
>
> Let me know if you'd like me to send a formal patch for this.
Hi Danielle,
Your fix is correct, please submit a patch.
Thanks for catching this!
© 2016 - 2026 Red Hat, Inc.