[PATCH net v5 6/6] ice: size ring stats arrays from the final queue count

[Przemek Kitszel <przemyslaw.kitszel@intel.com>]

The ring stats arrays were sized in one place and indexed in another,
with nothing reconciling the two. ice_vsi_alloc_stat_arrays() gave up as
soon as a container existed:

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

while ice_vsi_alloc_ring_stats() walks the arrays with
ice_for_each_alloc_txq() / ice_for_each_alloc_rxq(), that is, up to
vsi->alloc_txq / vsi->alloc_rxq. Whenever the arrays are shorter than
the count ice_vsi_set_num_qs() ends up computing, that loop reads and
stores past the end of the kmalloc'ed pointer arrays.

Deferring to "the rebuild path" does not hold. It cannot get the size
right, and some paths never call it at all.

ice_devlink_reinit_down() is one of the latter. It calls ice_vsi_decfg()
directly, which does not free pf->vsi_stats[], and nothing on the way
down releases the main VSI's entry either: ice_dealloc_vsis() runs only
from ice_init()/ice_deinit(), and the single entry that ice_unload()
does drop is the control VSI's, via ice_deinit_fdir(). The main VSI's
arrays therefore survive the reload, while ice_devlink_reinit_up() runs
ice_vsi_cfg() and recomputes the count from
netif_get_num_default_rss_queues(), which follows cpu_online_mask.
Sizing the arrays with most CPUs offline and then onlining them is
enough, no VF involved:

  for c in /sys/devices/system/cpu/cpu[0-9]*/online; do echo 0 > $c; done
  echo 0000:18:00.0 > /sys/bus/pci/drivers/ice/unbind
  echo 0000:18:00.0 > /sys/bus/pci/drivers/ice/bind
  for c in /sys/devices/system/cpu/cpu[0-9]*/online; do echo 1 > $c; done
  devlink dev reload pci/0000:18:00.0 action driver_reinit

The unbind/bind is what makes the arrays actually be allocated at the
small size; a reload alone only reuses them.

The read then runs off the end of tx_ring_stats[] and picks up whatever
follows it in kmalloc-32. Here that was a string, 0x74756f5f6c697475
spelling "util_out" in memory order. It is stored into ring->ring_stats
and faults on the first dereference, in ice_setup_tx_ring(), call trace:

 RIP: 0010:ice_setup_tx_ring+0x8d/0xe0 [ice]
  ice_vsi_setup_tx_rings+0x2a/0x80 [ice]
  ice_vsi_open+0x28/0x170 [ice]
  ice_open_internal+0xc9/0x160 [ice]
  __dev_open+0x138/0x2b0
  __dev_change_flags+0x1d5/0x250
  netif_change_flags+0x21/0x60
  do_setlink.constprop.0+0x336/0xcf0
  rtnl_newlink+0x4a4/0x9b0

Even where the rebuild path does run, it sizes the arrays before
ice_vsi_decfg() returns the queues to the PF pool, and for an
ICE_VSI_PF VSI with no explicit request it derives the count from that
shared pool. Nothing serializes it against the recount in
ice_vsi_set_num_qs(), so a concurrent release, say
"echo 0 > sriov_numvfs", leaves the final count larger than the arrays
that were already installed.

ice_vsi_alloc_stat_arrays() runs from ice_vsi_cfg_def(), right after
ice_vsi_alloc_def() called ice_vsi_set_num_qs(), so it is the first
place where the queue count is final. Make it the only authority: keep
the existing arrays when they are already long enough, and otherwise
grow them with ice_vsi_install_stat_arrays(), which carries the
surviving entries over. The check only forces a grow: while both arrays
are long enough they are kept, an oversized one being merely wasteful.
Once either is too short the whole container is replaced, and then the
other one follows the final count too, shrinking if that is what it
takes.

Michal Schmidt hit the same corruption through the VF path, where a
guest raises its queue count with VIRTCHNL_OP_REQUEST_QUEUES and the
following VF reset reconfigures the VSI with the larger count while the
old, shorter arrays are still installed:

 BUG: KASAN: slab-out-of-bounds in ice_vsi_alloc_ring_stats+0x385/0x4a0 [ice]
 Workqueue: ice ice_service_task [ice]
 Call Trace:
  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

 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)

Fixes: 288ecf491b16 ("ice: Accumulate ring statistics over reset")
Reported-by: Michal Schmidt <mschmidt@redhat.com>
Closes: https://redhat.atlassian.net/browse/RHEL-164321
Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
---
v5:
 - new patch, replaces the VF-only fix with one that covers every path
   reaching ice_vsi_alloc_ring_stats() (Clashiko)
 - the reproducers both postdate the cited tag: devlink reinit arrived
   with 31c8db2c4fa7 ("ice: implement devlink reinit action"), the VF
   path with 2a2cb4c6c181 ("ice: replace ice_vf_recreate_vsi() with
   ice_vf_reconfig_vsi()"). The sizing itself went wrong in the cited
   commit, hence the older tag (Clashiko)
 - drop the follow-up VF resize patch, it could abort a VFLR after the
   hardware had already reset the VF (Clashiko)
---
 drivers/net/ethernet/intel/ice/ice_lib.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c b/drivers/net/ethernet/intel/ice/ice_lib.c
index 448d3c7780ad..c393d913d037 100644
--- a/drivers/net/ethernet/intel/ice/ice_lib.c
+++ b/drivers/net/ethernet/intel/ice/ice_lib.c
@@ -634,27 +634,29 @@ static int ice_vsi_install_stat_arrays(struct ice_vsi *vsi, u16 txq, u16 rxq)
 /**
  * ice_vsi_alloc_stat_arrays - Allocate statistics arrays
  * @vsi: VSI pointer
+ *
+ * Runs after ice_vsi_set_num_qs(), so this is the first point where the queue
+ * count is final. Grow the arrays if an earlier sizing guessed too low.
+ *
+ * Return: 0 on success, negative error code otherwise.
  */
 static int ice_vsi_alloc_stat_arrays(struct ice_vsi *vsi)
 {
-	struct ice_vsi_stats *vsi_stat;
+	struct ice_vsi_stats *old_stat;
 	struct ice_pf *pf = vsi->back;
 
 	if (vsi->type == ICE_VSI_CHNL)
 		return 0;
 	if (!pf->vsi_stats)
 		return -ENOENT;
 
-	if (pf->vsi_stats[vsi->idx])
-	/* realloc will happen in rebuild path */
+	old_stat = pf->vsi_stats[vsi->idx];
+	if (old_stat && old_stat->tx_ring_stats_len >= vsi->alloc_txq &&
+	    old_stat->rx_ring_stats_len >= vsi->alloc_rxq)
 		return 0;
 
-	vsi_stat = ice_vsi_new_stat_arrays(vsi->alloc_txq, vsi->alloc_rxq);
-	if (!vsi_stat)
-		return -ENOMEM;
-
-	pf->vsi_stats[vsi->idx] = vsi_stat;
-	return 0;
+	return ice_vsi_install_stat_arrays(vsi, vsi->alloc_txq,
+					   vsi->alloc_rxq);
 }
 
 /**
-- 
2.51.1