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

[Tony Nguyen <anthony.l.nguyen@intel.com>]

On 6/3/2026 6:27 PM, Jakub Kicinski wrote:
> 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.

Hi Jakub,

This does seem valid, but as mentioned by Sashiko the pre-patch code has 
the same target/issue. There's a recent submission [1], with changes 
requested, that should cover this. Did you want to take this now or wait 
and have it sent with this other one?

Thanks,
Tony

[1] 
https://lore.kernel.org/intel-wired-lan/eb74ae1c-3027-42f5-ad5b-a6f2c2cd6a98@onemain.com/