Re: [PATCH net 1/3] i40e: keep q_vectors array in sync with channel count changes

[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.
---
i40e: keep q_vectors array in sync with channel count changes

This patch makes the main VSI's num_q_vectors track the effective
requested channel count and rebuilds q_vectors during VSI reinit so the
vector topology is refreshed together with the ring arrays. It also
switches to netif_napi_add_config() so NAPI identity survives q_vector
reallocation across channel-count changes.

Link: https://lore.kernel.org/intel-wired-lan/20260316133100.6054a11f@kernel.org/

> diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
> index 6d4f9218dc68..c3fbe14ff2d3 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_main.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c

[ ... ]

> @@ -14264,12 +14264,26 @@ static struct i40e_vsi *i40e_vsi_reinit_setup(struct i40e_vsi *vsi)
>  
>  	pf = vsi->back;
>  
> +	if (test_bit(I40E_FLAG_MSIX_ENA, pf->flags)) {
> +		i40e_put_lump(pf->irq_pile, vsi->base_vector, vsi->idx);
> +		vsi->base_vector = 0;
> +	}
> +
>  	i40e_put_lump(pf->qp_pile, vsi->base_queue, vsi->idx);
> +	i40e_vsi_free_q_vectors(vsi);
>  	i40e_vsi_clear_rings(vsi);
> +	i40e_vsi_free_arrays(vsi);
>  
> -	i40e_vsi_free_arrays(vsi, false);
>  	i40e_set_num_rings_in_vsi(vsi);
> -	ret = i40e_vsi_alloc_arrays(vsi, false);
> +	ret = i40e_vsi_alloc_arrays(vsi);
> +	if (ret)
> +		goto err_vsi;
> +
> +	/* Rebuild q_vectors during VSI reinit because the effective channel
> +	 * count may change num_q_vectors. Keep vector topology aligned with the
> +	 * queue configuration after ethtool's .set_channels() callback.
> +	 */
> +	ret = i40e_vsi_setup_vectors(vsi);
>  	if (ret)
>  		goto err_vsi;

[High]
Can the err_vsi path here cause a use-after-free of vsi via netdev_priv?

Reinit is invoked from i40e_rebuild() -> i40e_setup_pf_switch(pf, true,
...) on an already-registered netdev (vsi->netdev_registered == true),
and struct i40e_netdev_priv keeps a pointer at the vsi. The err_vsi
label only calls i40e_vsi_clear() which kfrees the vsi without
unregistering vsi->netdev, destroying the devlink port, or deleting
the FW VSI element:

drivers/net/ethernet/intel/i40e/i40e_main.c:i40e_vsi_clear() {
    ...
    i40e_put_lump(pf->qp_pile, vsi->base_queue, vsi->idx);
    i40e_put_lump(pf->irq_pile, vsi->base_vector, vsi->idx);

    bitmap_free(vsi->af_xdp_zc_qps);
    i40e_vsi_free_arrays(vsi);
    i40e_clear_rss_config_user(vsi);
    ...
    kfree(vsi);
    ...
}

If i40e_vsi_alloc_arrays() or the newly added i40e_vsi_setup_vectors()
fails (e.g. kzalloc of q_vectors, kzalloc of an individual q_vector,
or i40e_get_lump(irq_pile)), would a subsequent ndo_* callback on the
still-registered netdev dereference the freed vsi through
netdev_priv()?

Two new failure points the patch routes into err_vsi were not reachable
before: the unconditional q_vectors kzalloc in i40e_vsi_alloc_arrays()
and the entire i40e_vsi_setup_vectors() call.

> @@ -14281,7 +14295,7 @@ static struct i40e_vsi *i40e_vsi_reinit_setup(struct i40e_vsi *vsi)
>  		dev_info(&pf->pdev->dev,
>  			 "failed to get tracking for %d queues for VSI %d err %d\n",
>  			 alloc_queue_pairs, vsi->seid, ret);
> -		goto err_vsi;
> +		goto err_lump;
>  	}
>  	vsi->base_queue = ret;
>  
> @@ -14305,7 +14319,6 @@ static struct i40e_vsi *i40e_vsi_reinit_setup(struct i40e_vsi *vsi)
>  	return vsi;
>  
>  err_rings:
> -	i40e_vsi_free_q_vectors(vsi);
>  	if (vsi->netdev_registered) {
>  		vsi->netdev_registered = false;
>  		unregister_netdev(vsi->netdev);
> @@ -14315,6 +14328,8 @@ static struct i40e_vsi *i40e_vsi_reinit_setup(struct i40e_vsi *vsi)
>  	if (vsi->type == I40E_VSI_MAIN)
>  		i40e_devlink_destroy_port(pf);
>  	i40e_aq_delete_element(&pf->hw, vsi->seid, NULL);
> +err_lump:
> +	i40e_vsi_free_q_vectors(vsi);
>  err_vsi:
>  	i40e_vsi_clear(vsi);
>  	return NULL;

Should the new err_lump label, and the existing err_vsi exits from the
two allocation steps above, instead unwind through the err_rings block
(unregister_netdev / free_netdev / i40e_devlink_destroy_port /
i40e_aq_delete_element) the way i40e_vsi_setup()'s err_msix path does?

The pre-patch code had the same defective err_vsi target for the
qp_pile and arrays paths, but the patch adds two new failure points
(the unconditional q_vectors kzalloc and the new
i40e_vsi_setup_vectors() call) that route into it during reset
rebuild, where vsi->netdev is already registered.