[PATCH net v4 0/5][pull request] ice: fix stats array overflow via proper realloc

[PATCH net v4 0/5][pull request] ice: fix stats array overflow via proper realloc

[Tony Nguyen]

Przemek Kitszel says:

Fix OOB access to the stats arrays.

The first three commits are simple refactors to make the rest smaller,
the fourth one untangles the logic/lifetime of the stats array entries,
then we have the final fix by Michal for VF OOB access to the stats array.

Michal's fix was made prior to the rest, I have only slightly changed it
to base it on top of the pre-work commits, that should resolve all the
corner/error cases reported by AI.

The series was made anew vs the [v2], but the goal and spirit is similar,
but I have started from beginning instead of playing whack-a-mole with
yet another corner cases. We are also back to [v0] by Michal, as it turned
out to be more elegant finish with my first four patches already intact.

[v0]
https://lore.kernel.org/netdev/20260520183501.3360810-3-anthony.l.nguyen@intel.com

[v2]
https://sashiko.dev/#/message/20260812204619.32253-3-przemyslaw.kitszel%40intel.com
---
v4:
Rebase and resend of:
https://lore.kernel.org/intel-wired-lan/20260911113848.44086-1-przemyslaw.kitszel@intel.com/

The following are changes since commit 1e24c4f2ee44be0eee94092b5d13cbdb4bdf0d60:
  selftests: tc-testing: add a lateral-drift hfsc classify-walk test
and are available in the git repository at:
  git://git.kernel.org/pub/scm/linux/kernel/git/tnguy/net-queue 100GbE

Michal Schmidt (1):
  ice: fix stats array overflow when VF requests more queues

Przemek Kitszel (4):
  ice: extract __ice_vsi_free_stats()
  ice: extract ice_vsi_new_stat_arrays()
  ice: extract ice_vsi_get_num_qs()
  ice: rebuild ring stats arrays instead of reallocating them in place

 drivers/net/ethernet/intel/ice/ice.h        |   8 +-
 drivers/net/ethernet/intel/ice/ice_lib.c    | 295 ++++++++++++--------
 drivers/net/ethernet/intel/ice/ice_lib.h    |   1 +
 drivers/net/ethernet/intel/ice/ice_vf_lib.c |   4 +
 4 files changed, 183 insertions(+), 125 deletions(-)

-- 
2.47.1

[PATCH net v4 1/5] ice: extract __ice_vsi_free_stats()

[Tony Nguyen]

From: Przemek Kitszel <przemyslaw.kitszel@intel.com>

Record the length of each ring stats array in struct ice_vsi_stats, so
that freeing the array entries no longer needs the owning VSI. Keep the
new fields in sync in both places that size the arrays.

With that, the body of ice_vsi_free_stats() becomes independent of the
VSI and can be split out as __ice_vsi_free_stats(). The @free_entries
parameter is always true here; a later commit adds a caller that frees
only the array container, after its entries have been handed over to a
freshly allocated stats structure.

No functional change intended. Just preparation to have easier time
reading subsequent commits.

Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/ice/ice.h     |  2 +
 drivers/net/ethernet/intel/ice/ice_lib.c | 52 ++++++++++++++----------
 2 files changed, 33 insertions(+), 21 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h
index db3c7015c56c..fadfe94bf1c8 100644
--- a/drivers/net/ethernet/intel/ice/ice.h
+++ b/drivers/net/ethernet/intel/ice/ice.h
@@ -328,6 +328,8 @@ enum ice_vsi_state {
 struct ice_vsi_stats {
 	struct ice_ring_stats **tx_ring_stats;  /* Tx ring stats array */
 	struct ice_ring_stats **rx_ring_stats;  /* Rx ring stats array */
+	u16 tx_ring_stats_len;  /* Length of the Tx ring stats array */
+	u16 rx_ring_stats_len;  /* Length of the Rx ring stats array */
 };
 
 /* struct that defines a VSI, associated with a dev */
diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c b/drivers/net/ethernet/intel/ice/ice_lib.c
index 9e08db376d3d..cc66ae6f2edb 100644
--- a/drivers/net/ethernet/intel/ice/ice_lib.c
+++ b/drivers/net/ethernet/intel/ice/ice_lib.c
@@ -330,6 +330,32 @@ static void ice_vsi_free_arrays(struct ice_vsi *vsi)
 	vsi->rxq_map = NULL;
 }
 
+/* free single stats memory */
+static void __ice_vsi_free_stats(struct ice_vsi_stats *vsi_stat, bool free_entries)
+{
+	if (!vsi_stat)
+		return;
+
+	if (free_entries) {
+		for (int i = 0; i < vsi_stat->tx_ring_stats_len; i++) {
+			if (vsi_stat->tx_ring_stats[i]) {
+				kfree_rcu(vsi_stat->tx_ring_stats[i], rcu);
+				WRITE_ONCE(vsi_stat->tx_ring_stats[i], NULL);
+			}
+		}
+		for (int i = 0; i < vsi_stat->rx_ring_stats_len; i++) {
+			if (vsi_stat->rx_ring_stats[i]) {
+				kfree_rcu(vsi_stat->rx_ring_stats[i], rcu);
+				WRITE_ONCE(vsi_stat->rx_ring_stats[i], NULL);
+			}
+		}
+	}
+
+	kfree(vsi_stat->tx_ring_stats);
+	kfree(vsi_stat->rx_ring_stats);
+	kfree(vsi_stat);
+}
+
 /**
  * ice_vsi_free_stats - Free the ring statistics structures
  * @vsi: VSI pointer
@@ -338,7 +364,6 @@ static void ice_vsi_free_stats(struct ice_vsi *vsi)
 {
 	struct ice_vsi_stats *vsi_stat;
 	struct ice_pf *pf = vsi->back;
-	int i;
 
 	if (vsi->type == ICE_VSI_CHNL)
 		return;
@@ -346,26 +371,7 @@ static void ice_vsi_free_stats(struct ice_vsi *vsi)
 		return;
 
 	vsi_stat = pf->vsi_stats[vsi->idx];
-	if (!vsi_stat)
-		return;
-
-	ice_for_each_alloc_txq(vsi, i) {
-		if (vsi_stat->tx_ring_stats[i]) {
-			kfree_rcu(vsi_stat->tx_ring_stats[i], rcu);
-			WRITE_ONCE(vsi_stat->tx_ring_stats[i], NULL);
-		}
-	}
-
-	ice_for_each_alloc_rxq(vsi, i) {
-		if (vsi_stat->rx_ring_stats[i]) {
-			kfree_rcu(vsi_stat->rx_ring_stats[i], rcu);
-			WRITE_ONCE(vsi_stat->rx_ring_stats[i], NULL);
-		}
-	}
-
-	kfree(vsi_stat->tx_ring_stats);
-	kfree(vsi_stat->rx_ring_stats);
-	kfree(vsi_stat);
+	__ice_vsi_free_stats(vsi_stat, true);
 	pf->vsi_stats[vsi->idx] = NULL;
 }
 
@@ -539,11 +545,13 @@ static int ice_vsi_alloc_stat_arrays(struct ice_vsi *vsi)
 		kzalloc_objs(*vsi_stat->tx_ring_stats, vsi->alloc_txq);
 	if (!vsi_stat->tx_ring_stats)
 		goto err_alloc_tx;
+	vsi_stat->tx_ring_stats_len = vsi->alloc_txq;
 
 	vsi_stat->rx_ring_stats =
 		kzalloc_objs(*vsi_stat->rx_ring_stats, vsi->alloc_rxq);
 	if (!vsi_stat->rx_ring_stats)
 		goto err_alloc_rx;
+	vsi_stat->rx_ring_stats_len = vsi->alloc_rxq;
 
 	pf->vsi_stats[vsi->idx] = vsi_stat;
 
@@ -3051,6 +3059,7 @@ ice_vsi_realloc_stat_arrays(struct ice_vsi *vsi)
 		vsi_stat->tx_ring_stats = tx_ring_stats;
 		return -ENOMEM;
 	}
+	vsi_stat->tx_ring_stats_len = req_txq;
 
 	if (req_rxq < prev_rxq) {
 		for (i = req_rxq; i < prev_rxq; i++) {
@@ -3070,6 +3079,7 @@ ice_vsi_realloc_stat_arrays(struct ice_vsi *vsi)
 		vsi_stat->rx_ring_stats = rx_ring_stats;
 		return -ENOMEM;
 	}
+	vsi_stat->rx_ring_stats_len = req_rxq;
 
 	return 0;
 }
-- 
2.47.1

Re: [PATCH net v4 1/5] ice: extract __ice_vsi_free_stats()

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit message states "No functional change intended", but
  the patch changes the authority for the stats-free loop bound from the…

Pre-existing issues:
- [High] Two sources of truth for the ring-stats array length: the patch
  records `vsi_stat->tx_ring_stats_len`/`rx_ring_stats_len` and makes…
- [High] NULL pointer dereference when rebuilding an ICE_VSI_CHNL VSI.

--

ice: extract __ice_vsi_free_stats()

This records the length of each ring stats array in struct ice_vsi_stats
and uses those lengths to free the array entries, so the freeing code no
longer needs the owning VSI. The body of ice_vsi_free_stats() is then
split out as __ice_vsi_free_stats() with a @free_entries parameter.

> diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h
> index db3c7015c56c4..fadfe94bf1c8c 100644
> --- a/drivers/net/ethernet/intel/ice/ice.h
> +++ b/drivers/net/ethernet/intel/ice/ice.h
> @@ -328,6 +328,8 @@ enum ice_vsi_state {
>  struct ice_vsi_stats {
>  	struct ice_ring_stats **tx_ring_stats;  /* Tx ring stats array */
>  	struct ice_ring_stats **rx_ring_stats;  /* Rx ring stats array */
> +	u16 tx_ring_stats_len;  /* Length of the Tx ring stats array */
> +	u16 rx_ring_stats_len;  /* Length of the Rx ring stats array */
>  };

[Severity: High]
This isn't a bug introduced by this patch, but after this change there are
two notions of the array length, and only the free path consults the new
one.

ice_vsi_alloc_ring_stats() still walks the same arrays with the VSI queue
counts:

	tx_ring_stats = vsi_stats->tx_ring_stats;
	...
	ice_for_each_alloc_txq(vsi, i) {
		...
		ring_stats = tx_ring_stats[i];
		if (!ring_stats) {
			...
			WRITE_ONCE(tx_ring_stats[i], ring_stats);
		}

and the shrink loops in ice_vsi_realloc_stat_arrays() use
prev_txq = vsi->alloc_txq / prev_rxq = vsi->alloc_rxq rather than the
recorded lengths.

When vsi->alloc_txq or vsi->alloc_rxq is larger than the recorded array
length (the VF VIRTCHNL_OP_REQUEST_QUEUES case that the last patch of this
series documents with a KASAN splat), those sites read and WRITE_ONCE()
past the end of the kmalloc'ed pointer arrays, and the shrink loops can
kfree_rcu() a pointer read out of bounds.

Would it be worth mentioning in the changelog that the new fields are
authoritative only for the free path at this point in the series, and that
the allocation and shrink paths keep using vsi->alloc_txq/alloc_rxq until
the later patches? The end state of the series removes
ice_vsi_realloc_stat_arrays() in favour of ice_vsi_resize_stat_arrays()
plus ice_vsi_free_unused_stat_arrays() (which bounds itself with
tx_ring_stats_len/rx_ring_stats_len), and adds the missing resize call to
ice_vf_reconfig_vsi() before ice_vsi_decfg(), so the divergence does not
survive the series.

> diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c b/drivers/net/ethernet/intel/ice/ice_lib.c
> index 9e08db376d3d0..cc66ae6f2edbf 100644
> --- a/drivers/net/ethernet/intel/ice/ice_lib.c
> +++ b/drivers/net/ethernet/intel/ice/ice_lib.c
> @@ -330,6 +330,32 @@ static void ice_vsi_free_arrays(struct ice_vsi *vsi)
>  	vsi->rxq_map = NULL;
>  }
>  
> +/* free single stats memory */
> +static void __ice_vsi_free_stats(struct ice_vsi_stats *vsi_stat, bool free_entries)
> +{
> +	if (!vsi_stat)
> +		return;
> +
> +	if (free_entries) {
> +		for (int i = 0; i < vsi_stat->tx_ring_stats_len; i++) {
> +			if (vsi_stat->tx_ring_stats[i]) {
> +				kfree_rcu(vsi_stat->tx_ring_stats[i], rcu);
> +				WRITE_ONCE(vsi_stat->tx_ring_stats[i], NULL);
> +			}
> +		}
> +		for (int i = 0; i < vsi_stat->rx_ring_stats_len; i++) {
> +			if (vsi_stat->rx_ring_stats[i]) {
> +				kfree_rcu(vsi_stat->rx_ring_stats[i], rcu);
> +				WRITE_ONCE(vsi_stat->rx_ring_stats[i], NULL);
> +			}
> +		}
> +	}
> +
> +	kfree(vsi_stat->tx_ring_stats);
> +	kfree(vsi_stat->rx_ring_stats);
> +	kfree(vsi_stat);
> +}

[Severity: Low]
The changelog says "No functional change intended", but the loop bound in
the free path changes owner: it was ice_for_each_alloc_txq()/
ice_for_each_alloc_rxq() (vsi->alloc_txq / vsi->alloc_rxq) and is now the
newly recorded tx_ring_stats_len / rx_ring_stats_len. Are these always
equal at the point ice_vsi_free_stats() runs?

One case where they are not is a partial resize in
ice_vsi_realloc_stat_arrays(): the Tx array is committed and its length
recorded, and then the Rx krealloc_array() can still fail:

	vsi_stat->tx_ring_stats_len = req_txq;
	...
	vsi_stat->rx_ring_stats =
		krealloc_array(vsi_stat->rx_ring_stats, req_rxq, ...);
	if (!vsi_stat->rx_ring_stats) {
		vsi_stat->rx_ring_stats = rx_ring_stats;
		return -ENOMEM;
	}

ice_vsi_rebuild() then does "goto unlock" without undoing the Tx resize,
so vsi->alloc_txq keeps the old (possibly larger) value while the array is
only req_txq entries long. The old free loop walked
tx_ring_stats[0 .. vsi->alloc_txq - 1] over that shortened array; the new
loop stops at tx_ring_stats_len. The same applies to the VF case described
by the last patch of this series.

Would it make sense to note in the changelog that the free bound now
follows the real allocation instead of the VSI queue count, rather than
describing it as a no-op?

> @@ -338,7 +364,6 @@ static void ice_vsi_free_stats(struct ice_vsi *vsi)
>  {
>  	struct ice_vsi_stats *vsi_stat;
>  	struct ice_pf *pf = vsi->back;
> -	int i;
>  
>  	if (vsi->type == ICE_VSI_CHNL)
>  		return;
> @@ -346,26 +371,7 @@ static void ice_vsi_free_stats(struct ice_vsi *vsi)
>  		return;
>  
>  	vsi_stat = pf->vsi_stats[vsi->idx];
> -	if (!vsi_stat)
> -		return;
> -
> -	ice_for_each_alloc_txq(vsi, i) {
> -		if (vsi_stat->tx_ring_stats[i]) {
> -			kfree_rcu(vsi_stat->tx_ring_stats[i], rcu);
> -			WRITE_ONCE(vsi_stat->tx_ring_stats[i], NULL);
> -		}
> -	}
> -
> -	ice_for_each_alloc_rxq(vsi, i) {
> -		if (vsi_stat->rx_ring_stats[i]) {
> -			kfree_rcu(vsi_stat->rx_ring_stats[i], rcu);
> -			WRITE_ONCE(vsi_stat->rx_ring_stats[i], NULL);
> -		}
> -	}
> -
> -	kfree(vsi_stat->tx_ring_stats);
> -	kfree(vsi_stat->rx_ring_stats);
> -	kfree(vsi_stat);
> +	__ice_vsi_free_stats(vsi_stat, true);
>  	pf->vsi_stats[vsi->idx] = NULL;
>  }

[ ... ]

> @@ -3051,6 +3059,7 @@ ice_vsi_realloc_stat_arrays(struct ice_vsi *vsi)
>  		vsi_stat->tx_ring_stats = tx_ring_stats;
>  		return -ENOMEM;
>  	}
> +	vsi_stat->tx_ring_stats_len = req_txq;

[Severity: High]
This is a pre-existing issue, not something this patch introduces, but
since the new length bookkeeping lands in this function it may be worth
recording: can ice_vsi_realloc_stat_arrays() be entered with
pf->vsi_stats[vsi->idx] == NULL?

ice_vsi_alloc_stat_arrays() never publishes an entry for channel VSIs:

	if (vsi->type == ICE_VSI_CHNL)
		return 0;

while ice_vsi_rebuild() calls ice_vsi_realloc_stat_arrays()
unconditionally, and that function does:

	vsi_stat = pf->vsi_stats[vsi->idx];
	...
	tx_ring_stats = vsi_stat->tx_ring_stats;

The zero queue counts only make the shrink loop iterate zero times, they
do not stop the dereference. ice_rebuild_channels() explicitly selects
vsi->type == ICE_VSI_CHNL and calls ice_vsi_rebuild(vsi,
ICE_VSI_FLAG_INIT), so an ADQ setup followed by a reset appears to reach
it.

This one is addressed later in the series: the replacement
ice_vsi_resize_stat_arrays() starts with
"if (vsi->type == ICE_VSI_CHNL) return 0;".

>  
>  	if (req_rxq < prev_rxq) {
>  		for (i = req_rxq; i < prev_rxq; i++) {
> @@ -3070,6 +3079,7 @@ ice_vsi_realloc_stat_arrays(struct ice_vsi *vsi)
>  		vsi_stat->rx_ring_stats = rx_ring_stats;
>  		return -ENOMEM;
>  	}
> +	vsi_stat->rx_ring_stats_len = req_rxq;
>  
>  	return 0;
>  }

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260921182106.1015019-1-anthony.l.nguyen%40intel.com

[PATCH net v4 2/5] ice: extract ice_vsi_new_stat_arrays()

[Tony Nguyen]

From: Przemek Kitszel <przemyslaw.kitszel@intel.com>

Split the allocation of a struct ice_vsi_stats and its two ring stats
arrays out of ice_vsi_alloc_stat_arrays(), which is left with just the
lookup and the store into pf->vsi_stats[].

The sole caller keeps passing the very same queue counts as before,
vsi->alloc_txq and vsi->alloc_rxq. A later commit adds a second caller
in the rebuild path, which cannot use vsi->alloc_* because it has to
size the arrays before the VSI is reconfigured.

Note that the error path no longer clears pf->vsi_stats[vsi->idx]; the
early return above guarantees it is already NULL.

No functional change intended.

Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_lib.c | 47 +++++++++++++-----------
 1 file changed, 25 insertions(+), 22 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c b/drivers/net/ethernet/intel/ice/ice_lib.c
index cc66ae6f2edb..456b524c0c5f 100644
--- a/drivers/net/ethernet/intel/ice/ice_lib.c
+++ b/drivers/net/ethernet/intel/ice/ice_lib.c
@@ -519,6 +519,30 @@ static irqreturn_t ice_msix_clean_rings(int __always_unused irq, void *data)
 	return IRQ_HANDLED;
 }
 
+static struct ice_vsi_stats *ice_vsi_new_stat_arrays(int txq, int rxq)
+{
+	struct ice_ring_stats **tx_ring_stats;
+	struct ice_ring_stats **rx_ring_stats;
+	struct ice_vsi_stats *vsi_stat;
+
+	vsi_stat = kzalloc_obj(*vsi_stat);
+	tx_ring_stats = kzalloc_objs(*tx_ring_stats, txq);
+	rx_ring_stats = kzalloc_objs(*rx_ring_stats, rxq);
+	if (!vsi_stat || !tx_ring_stats || !rx_ring_stats) {
+		kfree(vsi_stat);
+		kfree(tx_ring_stats);
+		kfree(rx_ring_stats);
+		return NULL;
+	}
+
+	vsi_stat->tx_ring_stats = tx_ring_stats;
+	vsi_stat->rx_ring_stats = rx_ring_stats;
+	vsi_stat->tx_ring_stats_len = txq;
+	vsi_stat->rx_ring_stats_len = rxq;
+
+	return vsi_stat;
+}
+
 /**
  * ice_vsi_alloc_stat_arrays - Allocate statistics arrays
  * @vsi: VSI pointer
@@ -537,33 +561,12 @@ static int ice_vsi_alloc_stat_arrays(struct ice_vsi *vsi)
 	/* realloc will happen in rebuild path */
 		return 0;
 
-	vsi_stat = kzalloc_obj(*vsi_stat);
+	vsi_stat = ice_vsi_new_stat_arrays(vsi->alloc_txq, vsi->alloc_rxq);
 	if (!vsi_stat)
 		return -ENOMEM;
 
-	vsi_stat->tx_ring_stats =
-		kzalloc_objs(*vsi_stat->tx_ring_stats, vsi->alloc_txq);
-	if (!vsi_stat->tx_ring_stats)
-		goto err_alloc_tx;
-	vsi_stat->tx_ring_stats_len = vsi->alloc_txq;
-
-	vsi_stat->rx_ring_stats =
-		kzalloc_objs(*vsi_stat->rx_ring_stats, vsi->alloc_rxq);
-	if (!vsi_stat->rx_ring_stats)
-		goto err_alloc_rx;
-	vsi_stat->rx_ring_stats_len = vsi->alloc_rxq;
-
 	pf->vsi_stats[vsi->idx] = vsi_stat;
-
 	return 0;
-
-err_alloc_rx:
-	kfree(vsi_stat->rx_ring_stats);
-err_alloc_tx:
-	kfree(vsi_stat->tx_ring_stats);
-	kfree(vsi_stat);
-	pf->vsi_stats[vsi->idx] = NULL;
-	return -ENOMEM;
 }
 
 /**
-- 
2.47.1

[PATCH net v4 3/5] ice: extract ice_vsi_get_num_qs()

[Tony Nguyen]

From: Przemek Kitszel <przemyslaw.kitszel@intel.com>

ice_vsi_set_num_qs() mixed two things: deciding how many queues a VSI
gets, and applying all the side effects of that decision. Split the
decision out into ice_vsi_get_num_qs(), returning both counts at once.

To make that possible, group alloc_txq and alloc_rxq in struct ice_vsi
with struct_group_tagged(), which gives the return type without adding
any new storage. Field order changes slightly, num_txq now follows
alloc_rxq, but nothing depends on it.

ice_vsi_get_num_qs() takes @held_txq and @held_rxq from the start, even
though the only caller so far passes zero for both. They let a caller
ask what the counts would be once the queues the VSI currently owns are
back in the PF pool. A later commit adds the caller that needs this: the
rebuild path has to size its ring stats arrays before ice_vsi_decfg()
releases the queues, yet the arrays must match what ice_vsi_set_num_qs()
computes afterwards.

No functional change intended.

Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/ice/ice.h     |  6 +-
 drivers/net/ethernet/intel/ice/ice_lib.c | 86 ++++++++++++++----------
 2 files changed, 55 insertions(+), 37 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h
index fadfe94bf1c8..f1ba86d066be 100644
--- a/drivers/net/ethernet/intel/ice/ice.h
+++ b/drivers/net/ethernet/intel/ice/ice.h
@@ -403,9 +403,11 @@ struct ice_vsi {
 	u8 rx_mapping_mode;		 /* ICE_MAP_MODE_[CONTIG|SCATTER] */
 	u16 *txq_map;			 /* index in pf->avail_txqs */
 	u16 *rxq_map;			 /* index in pf->avail_rxqs */
-	u16 alloc_txq;			 /* Allocated Tx queues */
+	struct_group_tagged(ice_vsi_alloc_queues_params, alloc_txq_rxq,
+		u16 alloc_txq;		 /* Allocated Tx queues */
+		u16 alloc_rxq;		 /* Allocated Rx queues */
+	);
 	u16 num_txq;			 /* Used Tx queues */
-	u16 alloc_rxq;			 /* Allocated Rx queues */
 	u16 num_rxq;			 /* Used Rx queues */
 	u16 req_txq;			 /* User requested Tx queues */
 	u16 req_rxq;			 /* User requested Rx queues */
diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c b/drivers/net/ethernet/intel/ice/ice_lib.c
index 456b524c0c5f..c6023c11eed3 100644
--- a/drivers/net/ethernet/intel/ice/ice_lib.c
+++ b/drivers/net/ethernet/intel/ice/ice_lib.c
@@ -153,16 +153,56 @@ static void ice_vsi_set_num_desc(struct ice_vsi *vsi)
 	}
 }
 
-static u16 ice_get_rxq_count(struct ice_pf *pf)
+static u16 ice_get_rxq_count(struct ice_pf *pf, u16 held)
 {
-	return min(ice_get_avail_rxq_count(pf),
-		   netif_get_num_default_rss_queues());
+	return min_t(u16, ice_get_avail_rxq_count(pf) + held,
+		     netif_get_num_default_rss_queues());
 }
 
-static u16 ice_get_txq_count(struct ice_pf *pf)
+static u16 ice_get_txq_count(struct ice_pf *pf, u16 held)
 {
-	return min(ice_get_avail_txq_count(pf),
-		   netif_get_num_default_rss_queues());
+	return min_t(u16, ice_get_avail_txq_count(pf) + held,
+		     netif_get_num_default_rss_queues());
+}
+
+/* @held_txq, @held_rxq: queues the VSI still owns but is about to return to the
+ * PF pool, so that the result matches what it will be once they are back there.
+ */
+static struct ice_vsi_alloc_queues_params
+ice_vsi_get_num_qs(struct ice_vsi *vsi, u16 held_txq, u16 held_rxq)
+{
+	struct ice_vsi_alloc_queues_params qs = {};
+	struct ice_pf *pf = vsi->back;
+
+	switch (vsi->type) {
+	case ICE_VSI_PF:
+		qs.alloc_txq = vsi->req_txq ?: ice_get_txq_count(pf, held_txq);
+
+		/* only 1 Rx queue unless RSS is enabled */
+		if (!test_bit(ICE_FLAG_RSS_ENA, pf->flags))
+			qs.alloc_rxq = 1;
+		else
+			qs.alloc_rxq = vsi->req_rxq ?:
+				       ice_get_rxq_count(pf, held_rxq);
+		break;
+	case ICE_VSI_SF:
+	case ICE_VSI_CTRL:
+	case ICE_VSI_LB:
+		qs.alloc_txq = 1;
+		qs.alloc_rxq = 1;
+		break;
+	case ICE_VSI_VF:
+		qs.alloc_txq = vsi->vf->num_req_qs ?: vsi->vf->num_vf_qs;
+		qs.alloc_rxq = qs.alloc_txq;
+		break;
+	case ICE_VSI_CHNL:
+		break;
+	default:
+		dev_warn(ice_pf_to_dev(pf), "Unknown VSI type %d\n", vsi->type);
+		return vsi->alloc_txq_rxq;
+	}
+
+	return qs;
 }
 
 /**
@@ -180,44 +220,27 @@ static void ice_vsi_set_num_qs(struct ice_vsi *vsi)
 	if (WARN_ON(vsi_type == ICE_VSI_VF && !vf))
 		return;
 
+	vsi->alloc_txq_rxq = ice_vsi_get_num_qs(vsi, 0, 0);
+
 	switch (vsi_type) {
 	case ICE_VSI_PF:
-		if (vsi->req_txq) {
-			vsi->alloc_txq = vsi->req_txq;
+		if (vsi->req_txq)
 			vsi->num_txq = vsi->req_txq;
-		} else {
-			vsi->alloc_txq = ice_get_txq_count(pf);
-		}
+		if (vsi->req_rxq && test_bit(ICE_FLAG_RSS_ENA, pf->flags))
+			vsi->num_rxq = vsi->req_rxq;
 
 		pf->num_lan_tx = vsi->alloc_txq;
-
-		/* only 1 Rx queue unless RSS is enabled */
-		if (!test_bit(ICE_FLAG_RSS_ENA, pf->flags)) {
-			vsi->alloc_rxq = 1;
-		} else {
-			if (vsi->req_rxq) {
-				vsi->alloc_rxq = vsi->req_rxq;
-				vsi->num_rxq = vsi->req_rxq;
-			} else {
-				vsi->alloc_rxq = ice_get_rxq_count(pf);
-			}
-		}
-
 		pf->num_lan_rx = vsi->alloc_rxq;
 
 		vsi->num_q_vectors = max(vsi->alloc_rxq, vsi->alloc_txq);
 		break;
 	case ICE_VSI_SF:
-		vsi->alloc_txq = 1;
-		vsi->alloc_rxq = 1;
 		vsi->num_q_vectors = 1;
 		vsi->irq_dyn_alloc = true;
 		break;
 	case ICE_VSI_VF:
 		if (vf->num_req_qs)
 			vf->num_vf_qs = vf->num_req_qs;
-		vsi->alloc_txq = vf->num_vf_qs;
-		vsi->alloc_rxq = vf->num_vf_qs;
 		/* pf->vfs.num_msix_per includes (VF miscellaneous vector +
 		 * data queue interrupts). Since vsi->num_q_vectors is number
 		 * of queues vectors, subtract 1 (ICE_NONQ_VECS_VF) from the
@@ -226,22 +249,15 @@ static void ice_vsi_set_num_qs(struct ice_vsi *vsi)
 		vsi->num_q_vectors = vf->num_msix - ICE_NONQ_VECS_VF;
 		break;
 	case ICE_VSI_CTRL:
-		vsi->alloc_txq = 1;
-		vsi->alloc_rxq = 1;
 		vsi->num_q_vectors = 1;
 		break;
 	case ICE_VSI_CHNL:
-		vsi->alloc_txq = 0;
-		vsi->alloc_rxq = 0;
 		break;
 	case ICE_VSI_LB:
-		vsi->alloc_txq = 1;
-		vsi->alloc_rxq = 1;
 		/* A dummy q_vector, no actual IRQ. */
 		vsi->num_q_vectors = 1;
 		break;
 	default:
-		dev_warn(ice_pf_to_dev(pf), "Unknown VSI type %d\n", vsi_type);
 		break;
 	}
 
-- 
2.47.1

[PATCH net v4 4/5] ice: rebuild ring stats arrays instead of reallocating them in place

[Tony Nguyen]

From: Przemek Kitszel <przemyslaw.kitszel@intel.com>

ice_vsi_realloc_stat_arrays() resized the ring stats arrays in place
with krealloc_array(), sizing them from vsi->req_txq/req_rxq. That is
not what ice_vsi_set_num_qs() computes later in ice_vsi_cfg_def(), so
after a rebuild the arrays could end up shorter than vsi->alloc_txq /
vsi->alloc_rxq, and ice_vsi_alloc_ring_stats() then walked past their
end. Requesting fewer queues than the PF pool can hand out was enough
to trigger it.

Replace it with ice_vsi_resize_stat_arrays(), which allocates a fresh
struct ice_vsi_stats instead, sized with ice_vsi_get_num_qs() and the
queues ice_vsi_decfg() is about to return to the PF pool, which is
exactly what ice_vsi_set_num_qs() will compute once they are back
there. Copy the surviving entry pointers over and install the new
structure, all before ice_vsi_decfg() runs. The entries that did not
fit are freed together with the old container, via
__ice_vsi_free_stats() with @free_entries set to false.

Doing the allocation up front also means the failure path is a plain
unlock and return, with the VSI still fully configured, rather than a
half-torn-down VSI to unwind.

While at it, skip the stats handling for ICE_VSI_CHNL, which has no
entry in pf->vsi_stats[] to begin with.

Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_lib.c | 118 +++++++++++++----------
 1 file changed, 69 insertions(+), 49 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c b/drivers/net/ethernet/intel/ice/ice_lib.c
index c6023c11eed3..c6166ff44fc9 100644
--- a/drivers/net/ethernet/intel/ice/ice_lib.c
+++ b/drivers/net/ethernet/intel/ice/ice_lib.c
@@ -2956,6 +2956,51 @@ ice_vsi_rebuild_get_coalesce(struct ice_vsi *vsi,
 	return vsi->num_q_vectors;
 }
 
+static void ice_vsi_free_unused_stat_arrays(struct ice_vsi_stats *vsi_stat,
+					    struct ice_vsi_stats *new_vsi_stat)
+{
+	int new_txq = new_vsi_stat->tx_ring_stats_len;
+	int new_rxq = new_vsi_stat->rx_ring_stats_len;
+	int prev_txq = vsi_stat->tx_ring_stats_len;
+	int prev_rxq = vsi_stat->rx_ring_stats_len;
+
+	for (int i = new_txq; i < prev_txq; i++) {
+		if (vsi_stat->tx_ring_stats[i]) {
+			kfree_rcu(vsi_stat->tx_ring_stats[i], rcu);
+			WRITE_ONCE(vsi_stat->tx_ring_stats[i], NULL);
+		}
+	}
+	for (int i = new_rxq; i < prev_rxq; i++) {
+		if (vsi_stat->rx_ring_stats[i]) {
+			kfree_rcu(vsi_stat->rx_ring_stats[i], rcu);
+			WRITE_ONCE(vsi_stat->rx_ring_stats[i], NULL);
+		}
+	}
+}
+
+static void ice_vsi_set_stat_arrays(struct ice_vsi *vsi,
+				    struct ice_vsi_stats *new_vsi_stat)
+{
+	u16 new_txq, new_rxq, prev_txq, prev_rxq;
+	struct ice_vsi_stats *vsi_stat;
+	struct ice_pf *pf = vsi->back;
+
+	new_txq = new_vsi_stat->tx_ring_stats_len;
+	new_rxq = new_vsi_stat->rx_ring_stats_len;
+	vsi_stat = pf->vsi_stats[vsi->idx];
+	pf->vsi_stats[vsi->idx] = new_vsi_stat;
+	if (!vsi_stat)
+		return; /* don't copy if there is no source */
+
+	prev_txq = vsi_stat->tx_ring_stats_len;
+	prev_rxq = vsi_stat->rx_ring_stats_len;
+
+	memcpy(new_vsi_stat->tx_ring_stats, vsi_stat->tx_ring_stats,
+	       sizeof(*vsi_stat->tx_ring_stats) * min(prev_txq, new_txq));
+	memcpy(new_vsi_stat->rx_ring_stats, vsi_stat->rx_ring_stats,
+	       sizeof(*vsi_stat->rx_ring_stats) * min(prev_rxq, new_rxq));
+}
+
 /**
  * ice_vsi_rebuild_set_coalesce - set coalesce from earlier saved arrays
  * @vsi: VSI connected with q_vectors
@@ -3042,63 +3087,38 @@ ice_vsi_rebuild_set_coalesce(struct ice_vsi *vsi,
 }
 
 /**
- * ice_vsi_realloc_stat_arrays - Frees unused stat structures or alloc new ones
- * @vsi: VSI pointer
+ * ice_vsi_resize_stat_arrays - resize ring stats arrays for new queue count
+ * @vsi: VSI to swap the ring stats arrays of
+ *
+ * Call while @vsi still owns its queues and before ice_vsi_decfg() returns them
+ * to the PF pool, so that the new size is what ice_vsi_set_num_qs() will compute
+ * afterwards. Surviving entries are carried over, the rest is freed.
+ *
+ * Return: 0 on success and negative value on failure
  */
-static int
-ice_vsi_realloc_stat_arrays(struct ice_vsi *vsi)
+static int ice_vsi_resize_stat_arrays(struct ice_vsi *vsi)
 {
-	u16 req_txq = vsi->req_txq ? vsi->req_txq : vsi->alloc_txq;
-	u16 req_rxq = vsi->req_rxq ? vsi->req_rxq : vsi->alloc_rxq;
-	struct ice_ring_stats **tx_ring_stats;
-	struct ice_ring_stats **rx_ring_stats;
-	struct ice_vsi_stats *vsi_stat;
+	struct ice_vsi_alloc_queues_params qs;
+	struct ice_vsi_stats *old_stat;
+	struct ice_vsi_stats *new_stat;
 	struct ice_pf *pf = vsi->back;
-	u16 prev_txq = vsi->alloc_txq;
-	u16 prev_rxq = vsi->alloc_rxq;
-	int i;
 
-	vsi_stat = pf->vsi_stats[vsi->idx];
+	if (vsi->type == ICE_VSI_CHNL)
+		return 0;
 
-	if (req_txq < prev_txq) {
-		for (i = req_txq; i < prev_txq; i++) {
-			if (vsi_stat->tx_ring_stats[i]) {
-				kfree_rcu(vsi_stat->tx_ring_stats[i], rcu);
-				WRITE_ONCE(vsi_stat->tx_ring_stats[i], NULL);
-			}
-		}
-	}
+	qs = ice_vsi_get_num_qs(vsi, vsi->alloc_txq + vsi->num_xdp_txq,
+				vsi->alloc_rxq);
 
-	tx_ring_stats = vsi_stat->tx_ring_stats;
-	vsi_stat->tx_ring_stats =
-		krealloc_array(vsi_stat->tx_ring_stats, req_txq,
-			       sizeof(*vsi_stat->tx_ring_stats),
-			       GFP_KERNEL | __GFP_ZERO);
-	if (!vsi_stat->tx_ring_stats) {
-		vsi_stat->tx_ring_stats = tx_ring_stats;
+	new_stat = ice_vsi_new_stat_arrays(qs.alloc_txq, qs.alloc_rxq);
+	if (!new_stat)
 		return -ENOMEM;
-	}
-	vsi_stat->tx_ring_stats_len = req_txq;
-
-	if (req_rxq < prev_rxq) {
-		for (i = req_rxq; i < prev_rxq; i++) {
-			if (vsi_stat->rx_ring_stats[i]) {
-				kfree_rcu(vsi_stat->rx_ring_stats[i], rcu);
-				WRITE_ONCE(vsi_stat->rx_ring_stats[i], NULL);
-			}
-		}
-	}
 
-	rx_ring_stats = vsi_stat->rx_ring_stats;
-	vsi_stat->rx_ring_stats =
-		krealloc_array(vsi_stat->rx_ring_stats, req_rxq,
-			       sizeof(*vsi_stat->rx_ring_stats),
-			       GFP_KERNEL | __GFP_ZERO);
-	if (!vsi_stat->rx_ring_stats) {
-		vsi_stat->rx_ring_stats = rx_ring_stats;
-		return -ENOMEM;
+	old_stat = pf->vsi_stats[vsi->idx];
+	ice_vsi_set_stat_arrays(vsi, new_stat);
+	if (old_stat) {
+		ice_vsi_free_unused_stat_arrays(old_stat, new_stat);
+		__ice_vsi_free_stats(old_stat, false);
 	}
-	vsi_stat->rx_ring_stats_len = req_rxq;
 
 	return 0;
 }
@@ -3130,7 +3150,7 @@ int ice_vsi_rebuild(struct ice_vsi *vsi, u32 vsi_flags)
 
 	mutex_lock(&vsi->xdp_state_lock);
 
-	ret = ice_vsi_realloc_stat_arrays(vsi);
+	ret = ice_vsi_resize_stat_arrays(vsi);
 	if (ret)
 		goto unlock;
 
-- 
2.47.1

Re: [PATCH net v4 4/5] ice: rebuild ring stats arrays instead of reallocating them in place

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 4 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 4 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] Missing Fixes:/stable tag on a patch that fixes a heap out-of-
  bounds write.
- [Medium] A NULL-pointer-dereference fix is buried in a "while at it"
  sentence with no Fixes: tag.
- [Medium] The commit message's reproducer and its stated failure-path
  benefit are both inaccurate.
- [Medium] ice_vsi_resize_stat_arrays()
  (drivers/net/ethernet/intel/ice/ice_lib.c:3099) sizes and installs the…

--

ice: rebuild ring stats arrays instead of reallocating them in place

This replaces the in-place krealloc_array() resize in
ice_vsi_realloc_stat_arrays() with ice_vsi_resize_stat_arrays(), which
allocates a fresh struct ice_vsi_stats sized from ice_vsi_get_num_qs(),
copies the surviving entry pointers over and installs it before
ice_vsi_decfg() runs.  It also skips the stats handling entirely for
ICE_VSI_CHNL.

A few questions about the changelog and the new sizing contract below.

[Severity: Medium]

Should this patch carry a Fixes: tag?

The commit message describes a real out-of-bounds access:

>     after a rebuild the arrays could end up shorter than vsi->alloc_txq /
>     vsi->alloc_rxq, and ice_vsi_alloc_ring_stats() then walked past their
>     end.

ice_vsi_alloc_ring_stats() iterates with ice_for_each_alloc_txq() /
ice_for_each_alloc_rxq() and never consults tx_ring_stats_len /
rx_ring_stats_len:

drivers/net/ethernet/intel/ice/ice_lib.c:ice_vsi_alloc_ring_stats() {
	ice_for_each_alloc_txq(vsi, i) {
		...
		ring_stats = tx_ring_stats[i];
		if (!ring_stats) {
			ring_stats = kzalloc_obj(*ring_stats);
			...
			WRITE_ONCE(tx_ring_stats[i], ring_stats);
}

so a short array means out-of-bounds reads and out-of-bounds pointer
stores into the kmalloc'ed pointer arrays.

One reachable divergence is the VF case: the old resizer used vsi->req_txq,
which ice_sriov_set_msix_vec_count() sets, while ice_vsi_get_num_qs()
returns vf->num_req_qs ?: vf->num_vf_qs and ignores vsi->req_txq
altogether.  If the guest then raises vf->num_req_qs via
VIRTCHNL_OP_REQUEST_QUEUES, a PF reset (ice_vf_rebuild_vsi() ->
ice_vsi_rebuild()) shrinks the arrays while alloc_txq / alloc_rxq grow.

The next patch in this series ("ice: fix stats array overflow when VF
requests more queues") documents the same corruption class with a KASAN
slab-out-of-bounds splat and does carry a Fixes: tag.  Without a tag here,
and with this change sitting on top of three preceding refactors in the
same series (__ice_vsi_free_stats(), ice_vsi_new_stat_arrays(),
ice_vsi_get_num_qs()), how is a stable maintainer expected to identify the
affected kernels?  If the fix is deliberately not backportable, could the
changelog say so?

[Severity: Medium]

Two statements in the changelog do not seem to match the code.

First:

>     Requesting fewer queues than the PF pool can hand out was enough
>     to trigger it.

When a request is present, the removed resizer used vsi->req_txq /
vsi->req_rxq, and ice_vsi_get_num_qs() uses exactly the same values for
ICE_VSI_PF:

drivers/net/ethernet/intel/ice/ice_lib.c:ice_vsi_get_num_qs() {
	case ICE_VSI_PF:
		qs.alloc_txq = vsi->req_txq ?: ice_get_txq_count(pf, held_txq);
		if (!test_bit(ICE_FLAG_RSS_ENA, pf->flags))
			qs.alloc_rxq = 1;
		else
			qs.alloc_rxq = vsi->req_rxq ?:
				       ice_get_rxq_count(pf, held_rxq);
}

and ice_vsi_recfg_qs() stores the nonzero request before rebuilding:

drivers/net/ethernet/intel/ice/ice_main.c:ice_vsi_recfg_qs() {
	if (new_tx)
		vsi->req_txq = (u16)new_tx;
	if (new_rx)
		vsi->req_rxq = (u16)new_rx;
}

so the two sizes are identical on that path, and with RSS disabled the
recomputed alloc_rxq is 1, i.e. smaller rather than larger.  Could the
changelog use a trigger that the code can actually exhibit, such as the VF
case where vsi->req_txq disagrees with vf->num_req_qs ?: vf->num_vf_qs, or
a PF VSI with no request where the pool grew since the last
configuration?

Second:

>     Doing the allocation up front also means the failure path is a plain
>     unlock and return, with the VSI still fully configured, rather than a
>     half-torn-down VSI to unwind.

The old ice_vsi_realloc_stat_arrays() was already called before
ice_vsi_decfg() in ice_vsi_rebuild(), and its -ENOMEM already went straight
to the unlock label with the VSI fully configured.  Isn't the actual new
benefit that a failure no longer leaves a half-updated ice_vsi_stats, where
the Tx array was already reallocated and surplus Tx entries freed before
the Rx allocation failed?

> diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c b/drivers/net/ethernet/intel/ice/ice_lib.c
> index c6023c11eed3c..c6166ff44fc91 100644
> --- a/drivers/net/ethernet/intel/ice/ice_lib.c
> +++ b/drivers/net/ethernet/intel/ice/ice_lib.c

[ ... ]

> @@ -3042,63 +3087,38 @@ ice_vsi_rebuild_set_coalesce(struct ice_vsi *vsi,
>  }
>  
>  /**
> - * ice_vsi_realloc_stat_arrays - Frees unused stat structures or alloc new ones
> - * @vsi: VSI pointer
> + * ice_vsi_resize_stat_arrays - resize ring stats arrays for new queue count
> + * @vsi: VSI to swap the ring stats arrays of
> + *
> + * Call while @vsi still owns its queues and before ice_vsi_decfg() returns them
> + * to the PF pool, so that the new size is what ice_vsi_set_num_qs() will compute
> + * afterwards. Surviving entries are carried over, the rest is freed.
> + *
> + * Return: 0 on success and negative value on failure
>   */
> -static int
> -ice_vsi_realloc_stat_arrays(struct ice_vsi *vsi)
> +static int ice_vsi_resize_stat_arrays(struct ice_vsi *vsi)
>  {
> -	u16 req_txq = vsi->req_txq ? vsi->req_txq : vsi->alloc_txq;
> -	u16 req_rxq = vsi->req_rxq ? vsi->req_rxq : vsi->alloc_rxq;
> -	struct ice_ring_stats **tx_ring_stats;
> -	struct ice_ring_stats **rx_ring_stats;
> -	struct ice_vsi_stats *vsi_stat;
> +	struct ice_vsi_alloc_queues_params qs;
> +	struct ice_vsi_stats *old_stat;
> +	struct ice_vsi_stats *new_stat;
>  	struct ice_pf *pf = vsi->back;
> -	u16 prev_txq = vsi->alloc_txq;
> -	u16 prev_rxq = vsi->alloc_rxq;
> -	int i;
>  
> -	vsi_stat = pf->vsi_stats[vsi->idx];
> +	if (vsi->type == ICE_VSI_CHNL)
> +		return 0;

[Severity: Medium]

Is this new guard really just a "while at it" cleanup?

>     While at it, skip the stats handling for ICE_VSI_CHNL, which has no
>     entry in pf->vsi_stats[] to begin with.

Before this patch the function did:

	vsi_stat = pf->vsi_stats[vsi->idx];
	...
	tx_ring_stats = vsi_stat->tx_ring_stats;

with no NULL check.  For a channel VSI pf->vsi_stats[vsi->idx] is always
NULL, since ice_vsi_alloc_stat_arrays() returns early:

drivers/net/ethernet/intel/ice/ice_lib.c:ice_vsi_alloc_stat_arrays() {
	if (vsi->type == ICE_VSI_CHNL)
		return 0;
}

and the preceding if (req_txq < prev_txq) loop is skipped because a CHNL
VSI has alloc_txq == alloc_rxq == 0, so the dereference happens
unconditionally.

The path looks live: ice_rebuild_channels() calls ice_vsi_rebuild() for
every vsi->type == ICE_VSI_CHNL, and it is reached from ice_rebuild() for
reset_type == ICE_RESET_PFR with ICE_FLAG_TC_MQPRIO set:

drivers/net/ethernet/intel/ice/ice_main.c:ice_rebuild() {
	if (reset_type == ICE_RESET_PFR) {
		err = ice_rebuild_channels(pf);
}

So doesn't a PF reset with ADQ/mqprio configured oops on the pre-patch
code?  If so, should this part be split out as its own patch with a Fixes:
tag, or at least described as a NULL-pointer dereference fix in the
changelog?

>  
> -	if (req_txq < prev_txq) {
> -		for (i = req_txq; i < prev_txq; i++) {
> -			if (vsi_stat->tx_ring_stats[i]) {
> -				kfree_rcu(vsi_stat->tx_ring_stats[i], rcu);
> -				WRITE_ONCE(vsi_stat->tx_ring_stats[i], NULL);
> -			}
> -		}
> -	}
> +	qs = ice_vsi_get_num_qs(vsi, vsi->alloc_txq + vsi->num_xdp_txq,
> +				vsi->alloc_rxq);
>  
> -	tx_ring_stats = vsi_stat->tx_ring_stats;
> -	vsi_stat->tx_ring_stats =
> -		krealloc_array(vsi_stat->tx_ring_stats, req_txq,
> -			       sizeof(*vsi_stat->tx_ring_stats),
> -			       GFP_KERNEL | __GFP_ZERO);
> -	if (!vsi_stat->tx_ring_stats) {
> -		vsi_stat->tx_ring_stats = tx_ring_stats;
> +	new_stat = ice_vsi_new_stat_arrays(qs.alloc_txq, qs.alloc_rxq);
> +	if (!new_stat)
>  		return -ENOMEM;

[Severity: Medium]

Can the size computed here still end up smaller than what
ice_vsi_set_num_qs() computes afterwards?

For an ICE_VSI_PF VSI with req_txq / req_rxq == 0, ice_vsi_get_num_qs()
derives the counts from the shared PF pool via ice_get_txq_count() /
ice_get_rxq_count() -> ice_get_avail_q_count().  ice_vsi_rebuild() then
runs ice_vsi_decfg() and ice_vsi_cfg_def() -> ice_vsi_set_num_qs(), which
re-reads the same pool independently:

drivers/net/ethernet/intel/ice/ice_lib.c:ice_vsi_set_num_qs() {
	vsi->alloc_txq_rxq = ice_vsi_get_num_qs(vsi, 0, 0);
}

Nothing appears to serialize the two evaluations.  ice_get_avail_q_count()
holds pf->avail_q_mutex only for the duration of the count:

drivers/net/ethernet/intel/ice/ice_main.c:ice_get_avail_q_count() {
	mutex_lock(lock);
	for_each_clear_bit(bit, pf_qmap, size)
		count++;
	mutex_unlock(lock);
}

and ice_vsi_put_qs() takes it only while releasing.  vsi->xdp_state_lock is
per-VSI, and the reset-driven rebuild (ice_reset_subtask() ->
ice_rebuild() -> ice_vsi_rebuild_by_type()) holds no rtnl.

If another context frees queues inside that window, for example
echo 0 > sriov_numvfs -> ice_sriov_configure() -> ice_free_vfs() ->
ice_vsi_release() -> ice_vsi_put_qs(), the recomputed alloc_txq /
alloc_rxq become larger than the already-installed tx_ring_stats_len /
rx_ring_stats_len.  Nothing reconciles them afterwards, because
ice_vsi_alloc_stat_arrays() returns 0 early once pf->vsi_stats[idx] is
populated:

drivers/net/ethernet/intel/ice/ice_lib.c:ice_vsi_alloc_stat_arrays() {
	if (pf->vsi_stats[vsi->idx])
	/* realloc will happen in rebuild path */
		return 0;
}

and ice_vsi_alloc_ring_stats() indexes by alloc_txq / alloc_rxq without
looking at the recorded lengths, so it would again store pointers past the
end of the arrays.

Would it be more robust to bound the loops in ice_vsi_alloc_ring_stats() by
tx_ring_stats_len / rx_ring_stats_len, which struct ice_vsi_stats now
carries, or to size the arrays after the final queue count is established,
rather than keeping the sizing expression duplicated in two places with no
cross-check?

> -	}
> -	vsi_stat->tx_ring_stats_len = req_txq;
> -
> -	if (req_rxq < prev_rxq) {
> -		for (i = req_rxq; i < prev_rxq; i++) {
> -			if (vsi_stat->rx_ring_stats[i]) {
> -				kfree_rcu(vsi_stat->rx_ring_stats[i], rcu);
> -				WRITE_ONCE(vsi_stat->rx_ring_stats[i], NULL);
> -			}
> -		}
> -	}
>  
> -	rx_ring_stats = vsi_stat->rx_ring_stats;
> -	vsi_stat->rx_ring_stats =
> -		krealloc_array(vsi_stat->rx_ring_stats, req_rxq,
> -			       sizeof(*vsi_stat->rx_ring_stats),
> -			       GFP_KERNEL | __GFP_ZERO);
> -	if (!vsi_stat->rx_ring_stats) {
> -		vsi_stat->rx_ring_stats = rx_ring_stats;
> -		return -ENOMEM;
> +	old_stat = pf->vsi_stats[vsi->idx];
> +	ice_vsi_set_stat_arrays(vsi, new_stat);
> +	if (old_stat) {
> +		ice_vsi_free_unused_stat_arrays(old_stat, new_stat);
> +		__ice_vsi_free_stats(old_stat, false);
>  	}
> -	vsi_stat->rx_ring_stats_len = req_rxq;
>  
>  	return 0;
>  }

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260921182106.1015019-1-anthony.l.nguyen%40intel.com

[PATCH net v4 5/5] ice: fix stats array overflow when VF requests more queues

[Tony Nguyen]

From: Michal Schmidt <mschmidt@redhat.com>

When a VF increases its queue count via VIRTCHNL_OP_REQUEST_QUEUES,
ice_vc_request_qs_msg() sets vf->num_req_qs and triggers a VF reset.
The reset calls ice_vf_reconfig_vsi(), which does ice_vsi_decfg()
followed by ice_vsi_cfg(). ice_vsi_decfg() does not free the per-ring
stats arrays. Inside ice_vsi_cfg_def(), ice_vsi_set_num_qs() updates
alloc_txq/alloc_rxq to the new larger value, but
ice_vsi_alloc_stat_arrays() returns early because the stats already
exist. ice_vsi_alloc_ring_stats() then iterates using the new larger
alloc_txq and writes beyond the bounds of the old, smaller
tx_ring_stats/rx_ring_stats pointer arrays, corrupting adjacent SLUB
metadata.

KASAN detects the bug:
 ==================================================================
 BUG: KASAN: slab-out-of-bounds in ice_vsi_alloc_ring_stats+0x385/0x4a0 [ice]
 Read of size 8 at addr ffff88810affea60 by task kworker/u131:7/221

 CPU: 24 UID: 0 PID: 221 Comm: kworker/u131:7 Not tainted 7.1.0-rc1+ #1 PREEMPT(lazy)
 ...
 Workqueue: ice ice_service_task [ice]
 Call Trace:
  <TASK>
  ...
  kasan_report+0xd7/0x120
  ice_vsi_alloc_ring_stats+0x385/0x4a0 [ice]
  ice_vsi_cfg_def+0x12e2/0x2060 [ice]
  ice_vsi_cfg+0xb5/0x3c0 [ice]
  ice_reset_vf+0x858/0xf80 [ice]
  ice_vc_request_qs_msg+0x1da/0x290 [ice]
  ice_vc_process_vf_msg+0xb15/0x1430 [ice]
  __ice_clean_ctrlq+0x70d/0x9d0 [ice]
  ice_service_task+0x840/0xf20 [ice]
  process_one_work+0x690/0xff0
  worker_thread+0x4d9/0xd20
  kthread+0x322/0x410
  ret_from_fork+0x332/0x660
  ret_from_fork_asm+0x1a/0x30
  </TASK>

 Allocated by task 2439:
  kasan_save_stack+0x1c/0x40
  kasan_save_track+0x10/0x30
  __kasan_kmalloc+0x96/0xb0
  __kmalloc_noprof+0x1d8/0x580
  ice_vsi_cfg_def+0x115c/0x2060 [ice]
  ice_vsi_cfg+0xb5/0x3c0 [ice]
  ice_vsi_setup+0x180/0x320 [ice]
  ice_start_vfs+0x1f3/0x590 [ice]
  ice_ena_vfs+0x66d/0x798 [ice]
  ice_sriov_configure.cold+0xe4/0x121 [ice]
  sriov_numvfs_store+0x279/0x480
  kernfs_fop_write_iter+0x331/0x4f0
  vfs_write+0x4c4/0xe40
  ksys_write+0x10c/0x240
  do_syscall_64+0xd9/0x650
  entry_SYSCALL_64_after_hwframe+0x76/0x7e

 The buggy address belongs to the object at ffff88810affea40
                which belongs to the cache kmalloc-32 of size 32
 The buggy address is located 0 bytes to the right of
                allocated 32-byte region [ffff88810affea40, ffff88810affea60)
 ...
 ==================================================================

ice_vsi_rebuild() handles this correctly by calling
ice_vsi_resize_stat_arrays() before reconfiguration, but
ice_vf_reconfig_vsi() was missing this call.

Fix by calling ice_vsi_resize_stat_arrays() in ice_vf_reconfig_vsi()
before ice_vsi_decfg(), mirroring the ice_vsi_rebuild() pattern. The
helper sizes the new arrays with ice_vsi_get_num_qs(), which for a VF
reads vf->num_req_qs directly, so there is nothing else to set up for
it.

See the linked RHEL Jira item for a reproducer.

Fixes: 2a2cb4c6c181 ("ice: replace ice_vf_recreate_vsi() with ice_vf_reconfig_vsi()")
Closes: https://redhat.atlassian.net/browse/RHEL-164321
Assisted-by: Claude:claude-opus-4-6 semcode
Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Reviewed-by: Simon Horman <horms@kernel.org>
Co-developed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_lib.c    | 2 +-
 drivers/net/ethernet/intel/ice/ice_lib.h    | 1 +
 drivers/net/ethernet/intel/ice/ice_vf_lib.c | 4 ++++
 3 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c b/drivers/net/ethernet/intel/ice/ice_lib.c
index c6166ff44fc9..8d462570d152 100644
--- a/drivers/net/ethernet/intel/ice/ice_lib.c
+++ b/drivers/net/ethernet/intel/ice/ice_lib.c
@@ -3096,7 +3096,7 @@ ice_vsi_rebuild_set_coalesce(struct ice_vsi *vsi,
  *
  * Return: 0 on success and negative value on failure
  */
-static int ice_vsi_resize_stat_arrays(struct ice_vsi *vsi)
+int ice_vsi_resize_stat_arrays(struct ice_vsi *vsi)
 {
 	struct ice_vsi_alloc_queues_params qs;
 	struct ice_vsi_stats *old_stat;
diff --git a/drivers/net/ethernet/intel/ice/ice_lib.h b/drivers/net/ethernet/intel/ice/ice_lib.h
index 49454d98dcfe..5236088c320b 100644
--- a/drivers/net/ethernet/intel/ice/ice_lib.h
+++ b/drivers/net/ethernet/intel/ice/ice_lib.h
@@ -67,6 +67,7 @@ void ice_vsi_decfg(struct ice_vsi *vsi);
 void ice_dis_vsi(struct ice_vsi *vsi, bool locked);
 
 int ice_vsi_rebuild(struct ice_vsi *vsi, u32 vsi_flags);
+int ice_vsi_resize_stat_arrays(struct ice_vsi *vsi);
 int ice_vsi_cfg(struct ice_vsi *vsi);
 struct ice_vsi *ice_vsi_alloc(struct ice_pf *pf);
 void ice_vsi_free(struct ice_vsi *vsi);
diff --git a/drivers/net/ethernet/intel/ice/ice_vf_lib.c b/drivers/net/ethernet/intel/ice/ice_vf_lib.c
index a54cb2b8d3c7..c56a35b8dd48 100644
--- a/drivers/net/ethernet/intel/ice/ice_vf_lib.c
+++ b/drivers/net/ethernet/intel/ice/ice_vf_lib.c
@@ -268,6 +268,10 @@ static int ice_vf_reconfig_vsi(struct ice_vf *vf)
 
 	vsi->flags = ICE_VSI_FLAG_NO_INIT;
 
+	err = ice_vsi_resize_stat_arrays(vsi);
+	if (err)
+		return err;
+
 	ice_vsi_decfg(vsi);
 	ice_fltr_remove_all(vsi);
 
-- 
2.47.1

Re: [PATCH net v4 5/5] ice: fix stats array overflow when VF requests more queues

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 1 (2) · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] Incomplete fix: the identical stats-array out-of-bounds write
  remains reachable through the PF devlink `driver_reinit` reload path.
- [Medium] The patch introduces a new abort point in the middle of an
  already-started hardware VF reset.

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but the reader
  the patch now depends on for sizing (`ice_vsi_get_num_qs()`) disagrees…
- [Medium] This problem wasn't introduced by this patch, but the patch
  adds a new concrete failure source to it: `ice_vc_request_qs_msg()`…

--

ice: fix stats array overflow when VF requests more queues

The patch exports ice_vsi_resize_stat_arrays() and calls it from
ice_vf_reconfig_vsi() before ice_vsi_decfg(), so the per-ring stats
pointer arrays are grown before alloc_txq/alloc_rxq are bumped to the
larger VF-requested queue count.  This mirrors what ice_vsi_rebuild()
already does.

Closes: https://redhat.atlassian.net/browse/RHEL-164321

> diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c b/drivers/net/ethernet/intel/ice/ice_lib.c
> index c6166ff44fc91..8d462570d1529 100644
> --- a/drivers/net/ethernet/intel/ice/ice_lib.c
> +++ b/drivers/net/ethernet/intel/ice/ice_lib.c
> @@ -3096,7 +3096,7 @@ ice_vsi_rebuild_set_coalesce(struct ice_vsi *vsi,
>   *
>   * Return: 0 on success and negative value on failure
>   */
> -static int ice_vsi_resize_stat_arrays(struct ice_vsi *vsi)
> +int ice_vsi_resize_stat_arrays(struct ice_vsi *vsi)
>  {
>  	struct ice_vsi_alloc_queues_params qs;
>  	struct ice_vsi_stats *old_stat;

[Severity: High]
Since the helper is now exported, would it make sense to also cover the
PF devlink reload path, or to put the capacity check inside
ice_vsi_alloc_stat_arrays() instead?  The same overflow looks reachable
without any VF involved.

ice_devlink_reinit_down() tears the main VSI down with:

	rtnl_lock();
	ice_vsi_decfg(ice_get_main_vsi(pf));
	rtnl_unlock();
	ice_deinit_pf(pf);

ice_vsi_decfg() only calls ice_vsi_free_arrays(); it does not free
pf->vsi_stats[vsi->idx], and neither ice_unload(), ice_deinit_pf() nor
ice_deinit_dev() touch pf->vsi_stats (ice_dealloc_vsis() is only reached
from ice_init()/ice_deinit(), i.e. probe/remove).

ice_devlink_reinit_up() then does:

	vsi->flags = ICE_VSI_FLAG_INIT;

	rtnl_lock();
	err = ice_vsi_cfg(vsi);
	rtnl_unlock();

with no ice_vsi_resize_stat_arrays() call, so ice_vsi_set_num_qs() picks a
fresh alloc_txq/alloc_rxq from min(ice_get_avail_txq_count(pf),
netif_get_num_default_rss_queues()), ice_vsi_alloc_stat_arrays() takes its
early return:

	if (pf->vsi_stats[vsi->idx])
	/* realloc will happen in rebuild path */
		return 0;

and ice_vsi_alloc_ring_stats() indexes the retained, smaller arrays:

	ice_for_each_alloc_txq(vsi, i) {
		...
		ring_stats = tx_ring_stats[i];

Can this overflow tx_ring_stats[]/rx_ring_stats[] the same way as the VF
case?  netif_get_num_default_rss_queues() is derived from cpu_online_mask
and ice_deinit_pf()/ice_init_pf() re-allocate avail_txqs/avail_rxqs as
all-free, so sizing the arrays while some CPUs are offline, onlining more
CPUs, then running "devlink dev reload ... action driver_reinit" would grow
the count across the reload.

> diff --git a/drivers/net/ethernet/intel/ice/ice_vf_lib.c b/drivers/net/ethernet/intel/ice/ice_vf_lib.c
> index a54cb2b8d3c76..c56a35b8dd48f 100644
> --- a/drivers/net/ethernet/intel/ice/ice_vf_lib.c
> +++ b/drivers/net/ethernet/intel/ice/ice_vf_lib.c
> @@ -268,6 +268,10 @@ static int ice_vf_reconfig_vsi(struct ice_vf *vf)
>  
>  	vsi->flags = ICE_VSI_FLAG_NO_INIT;
>  
> +	err = ice_vsi_resize_stat_arrays(vsi);
> +	if (err)
> +		return err;
> +

[Severity: Medium]
Is this a safe place to abort?  By the time ice_reset_vf() reaches
ice_vf_reconfig_vsi(), the hardware reset has already been started:
ICE_VF_STATE_DIS is set, ice_trigger_vf_reset() wrote VFGEN_RSTAT =
VIRTCHNL_VFR_INPROGRESS and toggled VPGEN_VFRTRIG.VFSWR, ice_dis_vf_qs()
disabled the VF's queues and ice_reset_interrupts() dropped its interrupt
mappings.

If ice_vsi_resize_stat_arrays() returns -ENOMEM here, the function returns
before ice_vsi_decfg()/ice_fltr_remove_all()/ice_vsi_cfg(), and
ice_reset_vf() takes:

	if (ice_vf_reconfig_vsi(vf)) {
		dev_err(dev, "Failed to release and setup the VF%u's VSI\n",
			vf->vf_id);
		err = -EFAULT;
		goto out_unlock;
	}

	ice_vf_post_vsi_rebuild(vf);

so ice_vf_post_vsi_rebuild() is skipped.  That is the only caller of
ice_sriov_post_vsi_rebuild():

	static void ice_sriov_post_vsi_rebuild(struct ice_vf *vf)
	{
		ice_ena_vf_mappings(vf);
		wr32(&vf->pf->hw, VFGEN_RSTAT(vf->vf_id), VIRTCHNL_VFR_VFACTIVE);
	}

and the only path that reaches ice_vf_set_initialized(), which is the one
place ICE_VF_STATE_DIS gets cleared:

	clear_bit(ICE_VF_STATE_DIS, vf->vf_states);
	set_bit(ICE_VF_STATE_INIT, vf->vf_states);

Does that leave the VF wedged?  Afterwards VFGEN_RSTAT still reports reset
in progress with no mappings programmed, the VSI keeps its pre-reset
LAN/RDMA scheduler config because decfg was skipped,
ice_vc_process_vf_msg() rejects every virtchnl message with -EPERM while
ICE_VF_STATE_DIS is set, and a subsequent ice_reset_vf() short-circuits on
ice_is_vf_disabled() and returns 0.

Would doing the allocation before the reset is triggered, or treating a
resize failure as non-fatal (ice_vsi_alloc_ring_stats() already tolerates
pre-existing entries), keep the hardware sequence complete?

[Severity: Medium]
This isn't a bug introduced by this patch, but the new -ENOMEM path feeds
an existing unchecked return.  ice_vc_request_qs_msg() in virt/queues.c
discards the ice_reset_vf() result:

		vf->num_req_qs = req_queues;
		ice_reset_vf(vf, ICE_VF_RESET_NOTIFY);
		dev_info(dev, "VF %d granted request of %u queues.\n",
			 vf->vf_id, req_queues);
		return 0;

When ice_vsi_resize_stat_arrays() fails, ice_reset_vf() converts that to
-EFAULT and goes to out_unlock, skipping ice_vf_post_vsi_rebuild() and
ice_reset_vf_mbx_cnt(), yet the VF is still told the request was granted.
Should the return value be checked now that there is a new failure source?

[Severity: Medium]
This is a pre-existing issue and not something this patch introduces, but
the sizing helper the patch now relies on disagrees with one of its
writers.  ice_sriov_set_msix_vec_count() requests a new VF queue count via:

	vsi->req_txq = queues;
	vsi->req_rxq = queues;

	if (ice_vsi_rebuild(vsi, ICE_VSI_FLAG_NO_INIT)) {

and restores prev_queues the same way on the unroll path.  However
req_txq/req_rxq are only read under case ICE_VSI_PF in
ice_vsi_get_num_qs() and ice_vsi_set_num_qs(); the VF branch uses:

	case ICE_VSI_VF:
		qs.alloc_txq = vsi->vf->num_req_qs ?: vsi->vf->num_vf_qs;
		qs.alloc_rxq = qs.alloc_txq;

vf->num_req_qs has a single write site, in ice_vc_request_qs_msg(), and is
never cleared, so once a VF has issued VIRTCHNL_OP_REQUEST_QUEUES a later
sriov_vf_msix_count write changes vf->num_msix (and num_q_vectors) while
the queue count stays pinned at the stale value, and the "Changing VF %d
resources to %d vectors and %d queues" message reports the overwritten
count.

Note this does not cause the overflow the patch fixes, since
ice_vsi_resize_stat_arrays() and ice_vsi_set_num_qs() evaluate the same
expression.  Should those four req_txq/req_rxq assignments be dropped or
should the VF branch honor them?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260921182106.1015019-1-anthony.l.nguyen%40intel.com

Re: [PATCH net v4 5/5] ice: fix stats array overflow when VF requests more queues

[Przemek Kitszel]

On 9/24/26 2:23 PM, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 1 · Medium: 1 (2) · Low: 0
> 
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
> 
> - [High] Incomplete fix: the identical stats-array out-of-bounds write
>    remains reachable through the PF devlink `driver_reinit` reload path.

Confirmed. The issue is pre-existing though (reproduces w/o the series).
This series was targeted at fixing issue for VF, and it does.

I will send another commit to fix PF case - normally I would argue to
leave it for followup, but will check if a simple fix is enough
(compare to this commit - a oneliner that changed into 5 patches and few
months...)

> - [Medium] The patch introduces a new abort point in the middle of an
>    already-started hardware VF reset.

this warrants a respin, I will have a look at pre-existing issues, but
will rather try to avoid touching more than needed
pw-bot: cr