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

[netdev-bot+sashiko@kernel.org]

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