Re: [Intel-wired-lan] [PATCH net v2 2/2] ice: fix stats array overflow via proper realloc

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
    ice: fix stats array overflow via proper realloc

This patch removes ice_vsi_alloc_stat_arrays() and makes
ice_vsi_realloc_stat_arrays() handle the initial (no vsi_stat) case, tracking
the array sizes in the new tx_ring_stats_len / rx_ring_stats_len fields.  The
single remaining call site is in ice_vsi_cfg_def(), and the call in
ice_vsi_rebuild() is dropped.

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 e48ee5940f17..420e72df7f5e 100644
> --- a/drivers/net/ethernet/intel/ice/ice_lib.c
> +++ b/drivers/net/ethernet/intel/ice/ice_lib.c

[ ... ]

> @@ -2379,7 +2353,7 @@ static int ice_vsi_cfg_def(struct ice_vsi *vsi)
>  		return ret;
>  
>  	/* allocate memory for Tx/Rx ring stat pointers */
> -	ret = ice_vsi_alloc_stat_arrays(vsi);
> +	ret = ice_vsi_realloc_stat_arrays(vsi);
>  	if (ret)
>  		goto unroll_vsi_alloc;
>  
> @@ -3097,10 +3071,6 @@ int ice_vsi_rebuild(struct ice_vsi *vsi, u32 vsi_flags)
>  
>  	mutex_lock(&vsi->xdp_state_lock);
>  
> -	ret = ice_vsi_realloc_stat_arrays(vsi);
> -	if (ret)
> -		goto unlock;
> -
>  	ice_vsi_decfg(vsi);
>  	ret = ice_vsi_cfg_def(vsi);
>  	if (ret)

[Severity: Low]
Does moving the ice_vsi_realloc_stat_arrays() call change what happens when
the stat array allocation fails during a rebuild?

Before this patch, ice_vsi_rebuild() did the realloc before
ice_vsi_decfg(), so a krealloc_array() failure returned -ENOMEM with the VSI
still fully configured:

	ret = ice_vsi_realloc_stat_arrays(vsi);
	if (ret)
		goto unlock;

	ice_vsi_decfg(vsi);

Now the only call site is inside ice_vsi_cfg_def(), which runs after
ice_vsi_decfg(), so an -ENOMEM there unwinds via unroll_vsi_alloc and is
propagated out of ice_vsi_rebuild() with the queues, vectors and rings
already released.

For the ethtool -L path that leaves ice_vsi_recfg_qs() in ice_main.c with
only this recovery:

	rebuild_err:
		dev_err(ice_pf_to_dev(pf), "Error during VSI rebuild: %d. Unload and reload the driver.\n",
			err);

The same outcome was already reachable pre-patch through the other
allocations in ice_vsi_cfg_def() (ice_vsi_alloc_def(),
ice_vsi_alloc_q_vectors(), ice_vsi_alloc_rings(),
ice_vsi_alloc_ring_stats()), so this adds one more allocation to an
already-destructive region rather than a new failure mode.

The commit message describes the move only as:

    Also drop one callsite to avoid some duplication.

Could it also mention that the previously non-destructive failure path for
this allocation is gone?
-- 
pw-bot: cr