diff mbox series

[iwl-net,v2,2/2] ice: release the VF MSI-X window when ice_start_vfs() fails

Message ID 20260928065306.1514795-2-xiaolinkui@126.com
State Superseded
Headers show
Series [iwl-net,v2,1/2] ice: detach the VF representor when ice_start_vfs() fails | expand

Commit Message

Linkui Xiao Sept. 28, 2026, 6:53 a.m. UTC 
From: Linkui Xiao <xiaolinkui@kylinos.cn>

ice_init_vf_vsi_res() reserves the VF MSI-X window with
ice_virt_get_irqs(), which does bitmap_set() on the PF-wide
pf->virt_irq_tracker.bm, but nothing hands that window back when VF
creation fails. ice_virt_free_irqs() is not called anywhere on this
failure path: not from the release_vsi label of ice_init_vf_vsi_res(),
not from the ice_eswitch_attach_vf() failure branch, and not from the
teardown loop of ice_start_vfs(), which only undoes the queue mappings
and the VF VSI. The caller does not help either, because
err_unroll_vf_entries -> ice_free_vf_entries() -> ice_put_vf() ->
ice_sriov_free_vf() only does mutex_destroy() and kfree_rcu().

The tracker is allocated once per PF in ice_init_virt_irq_tracker() and
freed only in ice_deinit_virt_irq_tracker(), and ice_set_per_vf_res()
sizes the VFs from pf->virt_irq_tracker.num_entries rather than from the
number of free bits. So each failed "echo N > sriov_numvfs" leaks the
window reserved for every VF that got as far as ice_init_vf_vsi_res(),
and once the tracker is exhausted ice_virt_get_irqs() keeps returning
-ENOENT, which means SR-IOV cannot be enabled again without reloading
the driver. The hardware and the software bookkeeping also end up
disagreeing, because ice_dis_vf_mappings() clears
VPINT_ALLOC/VPINT_ALLOC_PCI and re-points GLINT_VECT2FUNC of exactly
those vectors back at the PF.

Return the window on the failure paths, in the order ice_free_vfs()
uses: ice_virt_free_irqs() after ice_eswitch_detach_vf() in the teardown
loop, and right after the VF VSI is released in the two branches that
fail before the loop can reach that VF.

The missing release is older than this change: the teardown loop has
never returned the window. It turns into an accumulating leak because
the tracker is now PF-wide and outlives a single SR-IOV enable.

Fixes: a203163274a4 ("ice: simplify VF MSI-X managing")
Cc: stable@vger.kernel.org
Signed-off-by: Linkui Xiao <xiaolinkui@kylinos.cn>
---
Changes in v2:
- New patch. Returns the VF MSI-X window that ice_init_vf_vsi_res() reserves
  when ice_start_vfs() fails. The missing release is older than the
  representor bug that patch 1/2 fixes, so it stays a separate patch.
  (Sashiko AI review)

 drivers/net/ethernet/intel/ice/ice_sriov.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

Comments

Tomasz Lichwala Sept. 28, 2026, 1:08 p.m. UTC | #1 
On 28.09.2026 08:53, Linkui Xiao wrote:

> diff --git a/drivers/net/ethernet/intel/ice/ice_sriov.c b/drivers/net/ethernet/intel/ice/ice_sriov.c
> index 95abc6704820..df84dbe12cba 100644
> --- a/drivers/net/ethernet/intel/ice/ice_sriov.c
> +++ b/drivers/net/ethernet/intel/ice/ice_sriov.c
> @@ -446,8 +446,10 @@ static int ice_init_vf_vsi_res(struct ice_vf *vf)
>  		return -ENOMEM;
>  
>  	vsi = ice_vf_vsi_setup(vf);
> -	if (!vsi)
> -		return -ENOMEM;
> +	if (!vsi) {
> +		err = -ENOMEM;
> +		goto free_irqs;
> +	}
>  
>  	err = ice_vf_init_host_cfg(vf, vsi);
>  	if (err)
> @@ -457,6 +459,9 @@ static int ice_init_vf_vsi_res(struct ice_vf *vf)
>  
>  release_vsi:
>  	ice_vf_vsi_release(vf);
> +free_irqs:
> +	ice_virt_free_irqs(pf, vf->first_vector_idx, vf->num_msix);
> +
>  	return err;
>  }
>  
> @@ -490,6 +495,8 @@ static int ice_start_vfs(struct ice_pf *pf)
>  				dev_err(ice_pf_to_dev(pf), "Failed to attach VF %d to eswitch, error %d",
>  					vf->vf_id, retval);
>  				ice_vf_vsi_release(vf);
> +				ice_virt_free_irqs(pf, vf->first_vector_idx,
> +						   vf->num_msix);

Nit: For consistency with the teardown: loop (which frees IRQs before releasing the VSI), consider swapping the order here too - currently this branch releases the VSI first, then frees IRQs, the opposite order. Not functionally significant, just readability.

>  				goto teardown;
>  			}
>  		}

Reviewed-by: Tomasz Lichwala <tomasz.lichwala@linux.intel.com>
Loktionov, Aleksandr Sept. 28, 2026, 3:16 p.m. UTC | #2 
> -----Original Message-----
> From: Linkui Xiao <xiaolinkui@126.com>
> Sent: Monday, September 28, 2026 8:53 AM
> To: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Kitszel,
> Przemyslaw <przemyslaw.kitszel@intel.com>; andrew+netdev@lunn.ch;
> davem@davemloft.net; edumazet@google.com; kuba@kernel.org;
> pabeni@redhat.com
> Cc: intel-wired-lan@lists.osuosl.org; netdev@vger.kernel.org; linux-
> kernel@vger.kernel.org; Linkui Xiao <xiaolinkui@kylinos.cn>;
> stable@vger.kernel.org
> Subject: [PATCH iwl-net v2 2/2] ice: release the VF MSI-X window when
> ice_start_vfs() fails
> 
> From: Linkui Xiao <xiaolinkui@kylinos.cn>
> 
> ice_init_vf_vsi_res() reserves the VF MSI-X window with
> ice_virt_get_irqs(), which does bitmap_set() on the PF-wide
> pf->virt_irq_tracker.bm, but nothing hands that window back when VF
> creation fails. ice_virt_free_irqs() is not called anywhere on this
> failure path: not from the release_vsi label of ice_init_vf_vsi_res(),
> not from the ice_eswitch_attach_vf() failure branch, and not from the
> teardown loop of ice_start_vfs(), which only undoes the queue mappings
> and the VF VSI. The caller does not help either, because
> err_unroll_vf_entries -> ice_free_vf_entries() -> ice_put_vf() ->
> ice_sriov_free_vf() only does mutex_destroy() and kfree_rcu().
> 
> The tracker is allocated once per PF in ice_init_virt_irq_tracker()
> and freed only in ice_deinit_virt_irq_tracker(), and
> ice_set_per_vf_res() sizes the VFs from pf-
> >virt_irq_tracker.num_entries rather than from the number of free
> bits. So each failed "echo N > sriov_numvfs" leaks the window reserved
> for every VF that got as far as ice_init_vf_vsi_res(), and once the
> tracker is exhausted ice_virt_get_irqs() keeps returning -ENOENT,
> which means SR-IOV cannot be enabled again without reloading the
> driver. The hardware and the software bookkeeping also end up
> disagreeing, because ice_dis_vf_mappings() clears
> VPINT_ALLOC/VPINT_ALLOC_PCI and re-points GLINT_VECT2FUNC of exactly
> those vectors back at the PF.
> 
> Return the window on the failure paths, in the order ice_free_vfs()
> uses: ice_virt_free_irqs() after ice_eswitch_detach_vf() in the
> teardown loop, and right after the VF VSI is released in the two
> branches that fail before the loop can reach that VF.
> 
> The missing release is older than this change: the teardown loop has
> never returned the window. It turns into an accumulating leak because
> the tracker is now PF-wide and outlives a single SR-IOV enable.
> 
> Fixes: a203163274a4 ("ice: simplify VF MSI-X managing")
> Cc: stable@vger.kernel.org
> Signed-off-by: Linkui Xiao <xiaolinkui@kylinos.cn>
> ---
> Changes in v2:
> - New patch. Returns the VF MSI-X window that ice_init_vf_vsi_res()
> reserves
>   when ice_start_vfs() fails. The missing release is older than the
>   representor bug that patch 1/2 fixes, so it stays a separate patch.
>   (Sashiko AI review)
> 
>  drivers/net/ethernet/intel/ice/ice_sriov.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_sriov.c
> b/drivers/net/ethernet/intel/ice/ice_sriov.c
> index 95abc6704820..df84dbe12cba 100644
> --- a/drivers/net/ethernet/intel/ice/ice_sriov.c
> +++ b/drivers/net/ethernet/intel/ice/ice_sriov.c
> @@ -446,8 +446,10 @@ static int ice_init_vf_vsi_res(struct ice_vf *vf)
>  		return -ENOMEM;
> 
>  	vsi = ice_vf_vsi_setup(vf);
> -	if (!vsi)
> -		return -ENOMEM;
> +	if (!vsi) {
> +		err = -ENOMEM;
> +		goto free_irqs;
> +	}
> 
>  	err = ice_vf_init_host_cfg(vf, vsi);
>  	if (err)
> @@ -457,6 +459,9 @@ static int ice_init_vf_vsi_res(struct ice_vf *vf)
> 
>  release_vsi:
>  	ice_vf_vsi_release(vf);
> +free_irqs:
> +	ice_virt_free_irqs(pf, vf->first_vector_idx, vf->num_msix);
> +
>  	return err;
>  }
> 
> @@ -490,6 +495,8 @@ static int ice_start_vfs(struct ice_pf *pf)
>  				dev_err(ice_pf_to_dev(pf), "Failed to
> attach VF %d to eswitch, error %d",
>  					vf->vf_id, retval);
>  				ice_vf_vsi_release(vf);
> +				ice_virt_free_irqs(pf, vf-
> >first_vector_idx,
> +						   vf->num_msix);
>  				goto teardown;
>  			}
>  		}
> @@ -511,6 +518,7 @@ static int ice_start_vfs(struct ice_pf *pf)
>  		mutex_lock(&vf->cfg_lock);
> 
>  		ice_eswitch_detach_vf(pf, vf);
> +		ice_virt_free_irqs(pf, vf->first_vector_idx, vf-
> >num_msix);
>  		ice_dis_vf_mappings(vf);
>  		ice_vf_vsi_release(vf);
>  		mutex_unlock(&vf->cfg_lock);
> --
> 2.25.1

Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Linkui Xiao Sept. 29, 2026, 12:55 a.m. UTC | #3 
On 2026/9/28 21:08, Tomasz Lichwala wrote:
> 
> 
> On 28.09.2026 08:53, Linkui Xiao wrote:
> 
>> diff --git a/drivers/net/ethernet/intel/ice/ice_sriov.c b/drivers/net/ethernet/intel/ice/ice_sriov.c
>> index 95abc6704820..df84dbe12cba 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_sriov.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_sriov.c
>> @@ -446,8 +446,10 @@ static int ice_init_vf_vsi_res(struct ice_vf *vf)
>>   		return -ENOMEM;
>>   
>>   	vsi = ice_vf_vsi_setup(vf);
>> -	if (!vsi)
>> -		return -ENOMEM;
>> +	if (!vsi) {
>> +		err = -ENOMEM;
>> +		goto free_irqs;
>> +	}
>>   
>>   	err = ice_vf_init_host_cfg(vf, vsi);
>>   	if (err)
>> @@ -457,6 +459,9 @@ static int ice_init_vf_vsi_res(struct ice_vf *vf)
>>   
>>   release_vsi:
>>   	ice_vf_vsi_release(vf);
>> +free_irqs:
>> +	ice_virt_free_irqs(pf, vf->first_vector_idx, vf->num_msix);
>> +
>>   	return err;
>>   }
>>   
>> @@ -490,6 +495,8 @@ static int ice_start_vfs(struct ice_pf *pf)
>>   				dev_err(ice_pf_to_dev(pf), "Failed to attach VF %d to eswitch, error %d",
>>   					vf->vf_id, retval);
>>   				ice_vf_vsi_release(vf);
>> +				ice_virt_free_irqs(pf, vf->first_vector_idx,
>> +						   vf->num_msix);
> 
> Nit: For consistency with the teardown: loop (which frees IRQs before releasing the VSI), consider swapping the order here too - currently this branch releases the VSI first, then frees IRQs, the opposite order. Not functionally significant, just readability.

Thanks for the review, Tomasz.

Agreed this is only a readability nit. The two calls are independent and
the current order is functionally safe, so I'll keep v2 as is.

No v3 planned for this change.

Thanks,
Linkui
> 
>>   				goto teardown;
>>   			}
>>   		}
> 
> Reviewed-by: Tomasz Lichwala <tomasz.lichwala@linux.intel.com>
>
diff mbox series

Patch

diff --git a/drivers/net/ethernet/intel/ice/ice_sriov.c b/drivers/net/ethernet/intel/ice/ice_sriov.c
index 95abc6704820..df84dbe12cba 100644
--- a/drivers/net/ethernet/intel/ice/ice_sriov.c
+++ b/drivers/net/ethernet/intel/ice/ice_sriov.c
@@ -446,8 +446,10 @@  static int ice_init_vf_vsi_res(struct ice_vf *vf)
 		return -ENOMEM;
 
 	vsi = ice_vf_vsi_setup(vf);
-	if (!vsi)
-		return -ENOMEM;
+	if (!vsi) {
+		err = -ENOMEM;
+		goto free_irqs;
+	}
 
 	err = ice_vf_init_host_cfg(vf, vsi);
 	if (err)
@@ -457,6 +459,9 @@  static int ice_init_vf_vsi_res(struct ice_vf *vf)
 
 release_vsi:
 	ice_vf_vsi_release(vf);
+free_irqs:
+	ice_virt_free_irqs(pf, vf->first_vector_idx, vf->num_msix);
+
 	return err;
 }
 
@@ -490,6 +495,8 @@  static int ice_start_vfs(struct ice_pf *pf)
 				dev_err(ice_pf_to_dev(pf), "Failed to attach VF %d to eswitch, error %d",
 					vf->vf_id, retval);
 				ice_vf_vsi_release(vf);
+				ice_virt_free_irqs(pf, vf->first_vector_idx,
+						   vf->num_msix);
 				goto teardown;
 			}
 		}
@@ -511,6 +518,7 @@  static int ice_start_vfs(struct ice_pf *pf)
 		mutex_lock(&vf->cfg_lock);
 
 		ice_eswitch_detach_vf(pf, vf);
+		ice_virt_free_irqs(pf, vf->first_vector_idx, vf->num_msix);
 		ice_dis_vf_mappings(vf);
 		ice_vf_vsi_release(vf);
 		mutex_unlock(&vf->cfg_lock);