[PATCH net v2] ethtool: Embed FEC hist ranges as buffer in struct

[PATCH net v2] ethtool: Embed FEC hist ranges as buffer in struct

[Eric Joyner]

When a driver's .get_fec_stats() handler is called and the driver
supports FEC histogram stats, the driver supplies the histogram bin
ranges via a pointer.  This pointer is assigned while under the netdev
ops lock in fec_prepare_data(), but the actual data is only read after
the lock is released; so this allows the driver to change the ranges
(e.g. from another .get_fec_stats() call) while the current call chain
is reading them in fec_fill_reply().

Fix this by adding an ethtool core-owned buffer, ranges_buf, to struct
ethtool_fec_hist. Drivers whose ranges are built dynamically (currently
just mlx5) fill ranges_buf and then point the existing ranges pointer at
it, giving ethtool a consistent copy that stays valid after the netdev
ops lock is dropped and later in fec_fill_reply(). Drivers whose ranges
are compile-time constants (bnxt, netdevsim) are unaffected by the
potential race and keep setting the existing ranges pointer to their
constant array, without making copies.

Fixes: cc2f08129925 ("ethtool: add FEC bins histogram report")
Assisted-by: Claude:claude-sonnet-5
Signed-off-by: Eric Joyner <eric.joyner@amd.com>
---
v2: Take Vadim Fedorenko's suggestion
(https://lore.kernel.org/netdev/f136bfb9-1756-4668-acc3-68962cdc6069@linux.dev/)
and keep the ranges pointer for drivers that use constant ranges arrays;
only use the new memory for dynamically constructed ranges.
This makes this patch mostly an mlx5 change.

 drivers/net/ethernet/mellanox/mlx5/core/en.h  |  1 -
 .../net/ethernet/mellanox/mlx5/core/en_main.c |  7 -------
 .../ethernet/mellanox/mlx5/core/en_stats.c    | 19 +++++++++----------
 include/linux/ethtool.h                       |  1 +
 4 files changed, 10 insertions(+), 18 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en.h b/drivers/net/ethernet/mellanox/mlx5/core/en.h
index d507289096c2..6867a5aed42c 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en.h
@@ -984,7 +984,6 @@ struct mlx5e_priv {
 	struct mlx5e_mqprio_rl    *mqprio_rl;
 	struct dentry             *dfs_root;
 	struct mlx5_devcom_comp_dev *devcom;
-	struct ethtool_fec_hist_range *fec_ranges;
 };
 
 static inline u16 mlx5e_stats_nch_read(const struct mlx5e_priv *priv)
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
index c1acb9012d3f..7d47a1da8b6b 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
@@ -6415,14 +6415,8 @@ int mlx5e_priv_init(struct mlx5e_priv *priv,
 	if (!priv->channel_stats)
 		goto err_free_tx_rates;
 
-	priv->fec_ranges = kzalloc_objs(*priv->fec_ranges, ETHTOOL_FEC_HIST_MAX);
-	if (!priv->fec_ranges)
-		goto err_free_channel_stats;
-
 	return 0;
 
-err_free_channel_stats:
-	kfree(priv->channel_stats);
 err_free_tx_rates:
 	kfree(priv->tx_rates);
 err_free_txq2sq_stats:
@@ -6447,7 +6441,6 @@ void mlx5e_priv_cleanup(struct mlx5e_priv *priv)
 	if (!priv->mdev)
 		return;
 
-	kfree(priv->fec_ranges);
 	for (i = 0; i < priv->stats_nch; i++)
 		kvfree(priv->channel_stats[i]);
 	kfree(priv->channel_stats);
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c b/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c
index de38b60806c2..e7e6db7f6bf1 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c
@@ -1550,7 +1550,7 @@ static bool fec_rs_validate_hist_type(int mode, int hist_type)
 
 static u8
 fec_rs_histogram_fill_ranges(struct mlx5e_priv *priv, int mode,
-			     const struct ethtool_fec_hist_range **ranges)
+			     struct ethtool_fec_hist_range *ranges)
 {
 	struct mlx5_core_dev *mdev = priv->mdev;
 	u32 out[MLX5_ST_SZ_DW(pphcr_reg)] = {0};
@@ -1558,8 +1558,6 @@ fec_rs_histogram_fill_ranges(struct mlx5e_priv *priv, int mode,
 	int sz = MLX5_ST_SZ_BYTES(pphcr_reg);
 	u8 hist_type, num_of_bins;
 
-	memset(priv->fec_ranges, 0,
-	       ETHTOOL_FEC_HIST_MAX * sizeof(*priv->fec_ranges));
 	MLX5_SET(pphcr_reg, in, local_port, 1);
 	if (mlx5_core_access_reg(mdev, in, sz, out, sz, MLX5_REG_PPHCR, 0, 0))
 		return 0;
@@ -1575,12 +1573,11 @@ fec_rs_histogram_fill_ranges(struct mlx5e_priv *priv, int mode,
 	for (int i = 0; i < num_of_bins; i++) {
 		void *bin_range = MLX5_ADDR_OF(pphcr_reg, out, bin_range[i]);
 
-		priv->fec_ranges[i].high = MLX5_GET(bin_range_layout, bin_range,
-						    high_val);
-		priv->fec_ranges[i].low = MLX5_GET(bin_range_layout, bin_range,
-						   low_val);
+		ranges[i].high = MLX5_GET(bin_range_layout, bin_range,
+					  high_val);
+		ranges[i].low = MLX5_GET(bin_range_layout, bin_range,
+					 low_val);
 	}
-	*ranges = priv->fec_ranges;
 
 	return num_of_bins;
 }
@@ -1622,10 +1619,12 @@ static void fec_set_histograms_stats(struct mlx5e_priv *priv, int mode,
 	case MLX5E_FEC_LLRS_272_257_1:
 	case MLX5E_FEC_RS_544_514_INTERLEAVED_QUAD:
 		num_of_bins =
-			fec_rs_histogram_fill_ranges(priv, mode, &hist->ranges);
-		if (num_of_bins)
+			fec_rs_histogram_fill_ranges(priv, mode, hist->ranges_buf);
+		if (num_of_bins) {
+			hist->ranges = hist->ranges_buf;
 			return fec_rs_histogram_fill_stats(priv, num_of_bins,
 							   hist);
+		}
 		break;
 	default:
 		return;
diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 5d491a98265e..12683b5d125e 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -562,6 +562,7 @@ struct ethtool_fec_hist {
 		u64 per_lane[ETHTOOL_MAX_LANES];
 	} values[ETHTOOL_FEC_HIST_MAX];
 	const struct ethtool_fec_hist_range *ranges;
+	struct ethtool_fec_hist_range ranges_buf[ETHTOOL_FEC_HIST_MAX];
 };
 /**
  * struct ethtool_fec_stats - statistics for IEEE 802.3 FEC

base-commit: fca68249b6f5e84859b200b54cb5e0aef98f2b3a
-- 
2.17.1

Re: [PATCH net v2] ethtool: Embed FEC hist ranges as buffer in struct

[Vadim Fedorenko]

On 23/07/2026 05:13, Eric Joyner wrote:
> When a driver's .get_fec_stats() handler is called and the driver
> supports FEC histogram stats, the driver supplies the histogram bin
> ranges via a pointer.  This pointer is assigned while under the netdev
> ops lock in fec_prepare_data(), but the actual data is only read after
> the lock is released; so this allows the driver to change the ranges
> (e.g. from another .get_fec_stats() call) while the current call chain
> is reading them in fec_fill_reply().
> 
> Fix this by adding an ethtool core-owned buffer, ranges_buf, to struct
> ethtool_fec_hist. Drivers whose ranges are built dynamically (currently
> just mlx5) fill ranges_buf and then point the existing ranges pointer at
> it, giving ethtool a consistent copy that stays valid after the netdev
> ops lock is dropped and later in fec_fill_reply(). Drivers whose ranges
> are compile-time constants (bnxt, netdevsim) are unaffected by the
> potential race and keep setting the existing ranges pointer to their
> constant array, without making copies.
> 
> Fixes: cc2f08129925 ("ethtool: add FEC bins histogram report")
> Assisted-by: Claude:claude-sonnet-5
> Signed-off-by: Eric Joyner <eric.joyner@amd.com>
> ---
> v2: Take Vadim Fedorenko's suggestion
> (https://lore.kernel.org/netdev/f136bfb9-1756-4668-acc3-68962cdc6069@linux.dev/)
> and keep the ranges pointer for drivers that use constant ranges arrays;
> only use the new memory for dynamically constructed ranges.
> This makes this patch mostly an mlx5 change.

LGTM, but we definitely need a stamp from mlx folks.

Reviewed-by: Vadim Fedorenko <vadim.fedorenko@linux.dev>

Re: [PATCH net v2] ethtool: Embed FEC hist ranges as buffer in struct

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Wed, 22 Jul 2026 21:13:42 -0700 you wrote:
> When a driver's .get_fec_stats() handler is called and the driver
> supports FEC histogram stats, the driver supplies the histogram bin
> ranges via a pointer.  This pointer is assigned while under the netdev
> ops lock in fec_prepare_data(), but the actual data is only read after
> the lock is released; so this allows the driver to change the ranges
> (e.g. from another .get_fec_stats() call) while the current call chain
> is reading them in fec_fill_reply().
> 
> [...]

Here is the summary with links:
  - [net,v2] ethtool: Embed FEC hist ranges as buffer in struct
    https://git.kernel.org/netdev/net/c/97ac08560d23

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html