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

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?

[ ... ]

[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