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

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

> [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?

I see no point documenting such minor details in commit message.

As a whole, this patch trades a real bug - memory overwrite -  for
a slightly different order of operations and slightly different
failure point when there is no memory

 > --
 > pw-bot: cr

I would still like to apply as-is, it would be better to spent
"refactor time" on different things in this driver.