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

[netdev-bot+sashiko@kernel.org]

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