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

[PATCH net] 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 embedding a buffer for the driver-supplied ranges in struct
ethtool_fec_hist instead of using a pointer; this ensures there's an
ethtool core-owned consistent copy that can be used after the netdev ops
lock is dropped and later in fec_fill_reply(). While some drivers like
bnxt use a constant struct for their ranges and won't be affected by
this issue, others like mlx5 (and eventually ionic) will use a
dynamically constructed range struct and could potentially run into an
issue.

Since the kernel API changed here, change the in-tree drivers that
report FEC histogram stats to copy their ranges instead of just
supplying a pointer.

Fixes: cc2f08129925 ("ethtool: add FEC bins histogram report")
Signed-off-by: Eric Joyner <eric.joyner@amd.com>
---

This is a fix for the issue described by Jakub in:
https://lore.kernel.org/netdev/20260615182732.7d28e31a@kernel.org/

 drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c |  3 ++-
 drivers/net/ethernet/mellanox/mlx5/core/en.h      |  1 -
 drivers/net/ethernet/mellanox/mlx5/core/en_main.c |  7 -------
 .../net/ethernet/mellanox/mlx5/core/en_stats.c    | 15 ++++++---------
 drivers/net/netdevsim/ethtool.c                   |  3 ++-
 include/linux/ethtool.h                           |  2 +-
 net/ethtool/fec.c                                 |  3 ++-
 7 files changed, 13 insertions(+), 21 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
index 62bc9cae613c..2e0ec8543f7f 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
@@ -3291,7 +3291,8 @@ static void bnxt_hwrm_port_phy_fdrstat(struct bnxt *bp,
 	resp = hwrm_req_hold(bp, req);
 	rc = hwrm_req_send(bp, req);
 	if (!rc) {
-		hist->ranges = bnxt_fec_ranges;
+		memcpy(hist->ranges, bnxt_fec_ranges,
+		       ETHTOOL_FEC_HIST_MAX * sizeof(*hist->ranges));
 		for (i = 0; i <= 15; i++) {
 			__le64 sum = resp->accumulated_codewords_err_s[i];
 
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 aa8610cedaa8..8db235ac9ae4 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..5f45e3c77cf1 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,7 +1619,7 @@ 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);
+			fec_rs_histogram_fill_ranges(priv, mode, hist->ranges);
 		if (num_of_bins)
 			return fec_rs_histogram_fill_stats(priv, num_of_bins,
 							   hist);
diff --git a/drivers/net/netdevsim/ethtool.c b/drivers/net/netdevsim/ethtool.c
index 025ea79879f3..69849e4192d4 100644
--- a/drivers/net/netdevsim/ethtool.c
+++ b/drivers/net/netdevsim/ethtool.c
@@ -178,7 +178,8 @@ nsim_get_fec_stats(struct net_device *dev, struct ethtool_fec_stats *fec_stats,
 {
 	struct ethtool_fec_hist_value *values = hist->values;
 
-	hist->ranges = netdevsim_fec_ranges;
+	memcpy(hist->ranges, netdevsim_fec_ranges,
+	       ARRAY_SIZE(netdevsim_fec_ranges) * sizeof(*hist->ranges));
 
 	fec_stats->corrected_blocks.total = 123;
 	fec_stats->uncorrectable_blocks.total = 4;
diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 5d491a98265e..d3bf1b2ddecc 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -561,7 +561,7 @@ struct ethtool_fec_hist {
 		u64 sum;
 		u64 per_lane[ETHTOOL_MAX_LANES];
 	} values[ETHTOOL_FEC_HIST_MAX];
-	const struct ethtool_fec_hist_range *ranges;
+	struct ethtool_fec_hist_range ranges[ETHTOOL_FEC_HIST_MAX];
 };
 /**
  * struct ethtool_fec_stats - statistics for IEEE 802.3 FEC
diff --git a/net/ethtool/fec.c b/net/ethtool/fec.c
index e2d539271060..28373e16dd51 100644
--- a/net/ethtool/fec.c
+++ b/net/ethtool/fec.c
@@ -186,7 +186,8 @@ static int fec_put_hist(struct sk_buff *skb,
 	int i, j;
 	u64 sum;
 
-	if (!ranges)
+	if (values[0].sum == ETHTOOL_STAT_NOT_SET &&
+	    values[0].per_lane[0] == ETHTOOL_STAT_NOT_SET)
 		return 0;
 
 	for (i = 0; i < ETHTOOL_FEC_HIST_MAX; i++) {
-- 
2.17.1

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

[Vadim Fedorenko]

On 11/07/2026 00:00, 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 embedding a buffer for the driver-supplied ranges in struct
> ethtool_fec_hist instead of using a pointer; this ensures there's an
> ethtool core-owned consistent copy that can be used after the netdev ops
> lock is dropped and later in fec_fill_reply(). While some drivers like
> bnxt use a constant struct for their ranges and won't be affected by
> this issue, others like mlx5 (and eventually ionic) will use a
> dynamically constructed range struct and could potentially run into an
> issue.

I didn't like the idea of dynamic range, FEC is not changing while the
link is UP, I don't see a reason to dynamically reconstruct histogram
bins every single call. And the histogram itself is stable per HW per
FEC, can be constant pre-defined struct in a driver, like in bnxt.

But if dynamic allocation is the only option, then yes, we have to
change this ABI.

> Since the kernel API changed here, change the in-tree drivers that
> report FEC histogram stats to copy their ranges instead of just
> supplying a pointer.
> 
> Fixes: cc2f08129925 ("ethtool: add FEC bins histogram report")
> Signed-off-by: Eric Joyner <eric.joyner@amd.com>