Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.
Critical: 0 · High: 0 · Medium: 1 · 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).
- [Medium] In ice_vsi_realloc_stat_arrays()
(drivers/net/ethernet/intel/ice/ice_lib.c), the new guard skips only…
--
ice: skip stats handling for channel VSIs on rebuild
This patch makes ice_vsi_realloc_stat_arrays() return early for
ICE_VSI_CHNL VSIs. Those VSIs never get an entry in pf->vsi_stats[], so
this avoids a NULL dereference when a PF reset rebuilds channel VSIs
while an mqprio hardware offload is active.
> diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c
> b/drivers/net/ethernet/intel/ice/ice_lib.c
> index 9e08db376d3d0..31af378aa0e7a 100644
> --- a/drivers/net/ethernet/intel/ice/ice_lib.c
> +++ b/drivers/net/ethernet/intel/ice/ice_lib.c
> @@ -3031,6 +3031,10 @@ ice_vsi_realloc_stat_arrays(struct ice_vsi *vsi)
> u16 prev_rxq = vsi->alloc_rxq;
> int i;
>
> + /* channel VSIs have no entry in pf->vsi_stats[] */
> + if (vsi->type == ICE_VSI_CHNL)
> + return 0;
> +
> vsi_stat = pf->vsi_stats[vsi->idx];
>
> if (req_txq < prev_txq) {
[Severity: Medium]
This guard only checks the VSI type. Can pf->vsi_stats[vsi->idx] also be
NULL here for a PF, VF or other non-channel VSI?
If an earlier ice_vsi_rebuild() failed inside ice_vsi_cfg_def(), the
unroll path frees the entry:
ice_vsi_cfg_def() {
...
unroll_vsi_alloc_stat:
ice_vsi_free_stats(vsi);
...
}
and ice_vsi_free_stats() ends with:
pf->vsi_stats[vsi->idx] = NULL;
ice_vsi_alloc_ring_stats() does the same on its err_out path.
ice_vsi_rebuild() then returns through goto unlock without restoring the
entry, and the VSI stays in pf->vsi[].
Some callers then call ice_vsi_rebuild() on the same VSI again. One
example is the fallback in ice_setup_tc_mqprio_qdisc():
ret = ice_vsi_rebuild(vsi, ICE_VSI_FLAG_NO_INIT);
if (ret) {
...
if (ice_vsi_rebuild(vsi, ICE_VSI_FLAG_NO_INIT)) {
The same thing can happen through the unroll path in
ice_sriov_set_msix_vec_count(), when userspace retries ethtool -L or -G
through ice_vsi_recfg_qs(), or on a later PF or VF reset.
On that second call vsi_stat is NULL and the type check does not catch
it. If req_txq < prev_txq, the oops happens at
vsi_stat->tx_ring_stats[i]. Otherwise it happens at:
tx_ring_stats = vsi_stat->tx_ring_stats;
ice_vsi_free_stats() and ice_qp_reset_stats() already handle a NULL
entry. Would a !vsi_stat check here cover both the channel VSI case and
this one?
The new comment, "channel VSIs have no entry in pf->vsi_stats[]", also
implies that only channel VSIs can be missing an entry.
A later patch in this series, "ice: rebuild ring stats arrays instead of
reallocating them in place", appears to fix this. It replaces the
function with ice_vsi_resize_stat_arrays() -> ice_vsi_install_stat_arrays(),
and ice_vsi_set_stat_arrays() handles a NULL entry with:
if (!vsi_stat)
return;
Should this patch check for a NULL vsi_stat as well, so that it is a
complete fix on its own?
--
Sashiko AI review ·
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260925132636.123300-1-przemyslaw.kitszel%40intel.com
