[PATCH] ice: Fix improper handling of refcount in ice_sriov_set_msix_vec_count()

[PATCH] ice: Fix improper handling of refcount in ice_sriov_set_msix_vec_count()

[Gui-Dong Han]

This patch addresses an issue with improper reference count handling in the
ice_sriov_set_msix_vec_count() function. Specifically, the function calls
ice_get_vf_by_id(), which increments the reference count of the vf pointer.
If the subsequent call to ice_get_vf_vsi() fails, the function currently
returns an error without decrementing the reference count of the vf
pointer, leading to a reference count leak.

The correct behavior, as implemented in this patch, is to decrement the
reference count using ice_put_vf(vf) before returning an error when vsi
is NULL.
 
This bug was identified by an experimental static analysis tool developed
by our team. The tool specializes in analyzing reference count operations
and identifying potential mismanagement of reference counts. In this case,
the tool flagged the missing decrement operation as a potential issue,
leading to this patch.

Fixes: 4035c72dc1ba ("ice: reconfig host after changing MSI-X on VF")
Cc: stable@vger.kernel.org
Signed-off-by: Gui-Dong Han <hanguidong02@outlook.com>
---
 drivers/net/ethernet/intel/ice/ice_sriov.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_sriov.c b/drivers/net/ethernet/intel/ice/ice_sriov.c
index 55ef33208456..eb5030aba9a5 100644
--- a/drivers/net/ethernet/intel/ice/ice_sriov.c
+++ b/drivers/net/ethernet/intel/ice/ice_sriov.c
@@ -1096,8 +1096,10 @@ int ice_sriov_set_msix_vec_count(struct pci_dev *vf_dev, int msix_vec_count)
 		return -ENOENT;
 
 	vsi = ice_get_vf_vsi(vf);
-	if (!vsi)
+	if (!vsi) {
+		ice_put_vf(vf);
 		return -ENOENT;
+	}
 
 	prev_msix = vf->num_msix;
 	prev_queues = vf->num_vf_qs;
-- 
2.25.1

Re: [PATCH] ice: Fix improper handling of refcount in ice_sriov_set_msix_vec_count()

[Simon Horman]

On Fri, Aug 09, 2024 at 12:59:11PM +0800, Gui-Dong Han wrote:
> This patch addresses an issue with improper reference count handling in the
> ice_sriov_set_msix_vec_count() function. Specifically, the function calls
> ice_get_vf_by_id(), which increments the reference count of the vf pointer.
> If the subsequent call to ice_get_vf_vsi() fails, the function currently
> returns an error without decrementing the reference count of the vf
> pointer, leading to a reference count leak.
> 
> The correct behavior, as implemented in this patch, is to decrement the
> reference count using ice_put_vf(vf) before returning an error when vsi
> is NULL.
>  
> This bug was identified by an experimental static analysis tool developed
> by our team. The tool specializes in analyzing reference count operations
> and identifying potential mismanagement of reference counts. In this case,
> the tool flagged the missing decrement operation as a potential issue,
> leading to this patch.
> 
> Fixes: 4035c72dc1ba ("ice: reconfig host after changing MSI-X on VF")
> Cc: stable@vger.kernel.org
> Signed-off-by: Gui-Dong Han <hanguidong02@outlook.com>

Thanks Gui-Dong Han,

I agree with your analysis.
However, I wonder if the same resource leak can occur
in the unroll label, if the if clause results in a return.

It is around line 1444 without your patch applied.

	vf->first_vector_idx = ice_sriov_get_irqs(pf, vf->num_msix);
	if (vf->first_vector_idx < 0)
		return -EINVAL;

> ---
>  drivers/net/ethernet/intel/ice/ice_sriov.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_sriov.c b/drivers/net/ethernet/intel/ice/ice_sriov.c
> index 55ef33208456..eb5030aba9a5 100644
> --- a/drivers/net/ethernet/intel/ice/ice_sriov.c
> +++ b/drivers/net/ethernet/intel/ice/ice_sriov.c
> @@ -1096,8 +1096,10 @@ int ice_sriov_set_msix_vec_count(struct pci_dev *vf_dev, int msix_vec_count)
>  		return -ENOENT;
>  
>  	vsi = ice_get_vf_vsi(vf);
> -	if (!vsi)
> +	if (!vsi) {
> +		ice_put_vf(vf);
>  		return -ENOENT;
> +	}
>  
>  	prev_msix = vf->num_msix;
>  	prev_queues = vf->num_vf_qs;
> -- 
> 2.25.1
> 
>