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

[PATCH v2] 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.

First, 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.

Second, the function calls ice_sriov_get_irqs(), which sets
vf->first_vector_idx. If this call returns a negative value, indicating an
error, the function returns an error without decrementing the reference
count of the vf pointer, resulting in another reference count leak. The
patch addresses this by adding a call to ice_put_vf(vf) before returning
an error when vf->first_vector_idx < 0. 

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")
Fixes: 4d38cb44bd32 ("ice: manage VFs MSI-X using resource tracking")
Cc: stable@vger.kernel.org
Signed-off-by: Gui-Dong Han <hanguidong02@outlook.com>
---
v2:
* In this patch v2, an additional resource leak was addressed when
vf->first_vector_idx < 0. The issue is now fixed by adding ice_put_vf(vf)
before returning an error.
  Thanks to Simon Horman for identifying this additional leak scenario.
---
 drivers/net/ethernet/intel/ice/ice_sriov.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_sriov.c b/drivers/net/ethernet/intel/ice/ice_sriov.c
index 55ef33208456..fbf18ac97875 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;
@@ -1142,8 +1144,10 @@ int ice_sriov_set_msix_vec_count(struct pci_dev *vf_dev, int msix_vec_count)
 	vf->num_msix = prev_msix;
 	vf->num_vf_qs = prev_queues;
 	vf->first_vector_idx = ice_sriov_get_irqs(pf, vf->num_msix);
-	if (vf->first_vector_idx < 0)
+	if (vf->first_vector_idx < 0) {
+		ice_put_vf(vf);
 		return -EINVAL;
+	}
 
 	if (needs_rebuild) {
 		ice_vf_reconfig_vsi(vf);
-- 
2.25.1

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

[Simon Horman]

On Tue, Sep 03, 2024 at 11:59:43AM +0000, Gui-Dong Han wrote:
> This patch addresses an issue with improper reference count handling in the
> ice_sriov_set_msix_vec_count() function.
> 
> First, 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.
> 
> Second, the function calls ice_sriov_get_irqs(), which sets
> vf->first_vector_idx. If this call returns a negative value, indicating an
> error, the function returns an error without decrementing the reference
> count of the vf pointer, resulting in another reference count leak. The
> patch addresses this by adding a call to ice_put_vf(vf) before returning
> an error when vf->first_vector_idx < 0. 
> 
> 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")
> Fixes: 4d38cb44bd32 ("ice: manage VFs MSI-X using resource tracking")
> Cc: stable@vger.kernel.org
> Signed-off-by: Gui-Dong Han <hanguidong02@outlook.com>
> ---
> v2:
> * In this patch v2, an additional resource leak was addressed when
> vf->first_vector_idx < 0. The issue is now fixed by adding ice_put_vf(vf)
> before returning an error.
>   Thanks to Simon Horman for identifying this additional leak scenario.

Thanks for the update,

I agree with the analysis and that the two instances of
this problem were introduced by each of the cited commits.

Reviewed-by: Simon Horman <horms@kernel.org>

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

[Markus Elfring]

…
> +++ 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;
> @@ -1142,8 +1144,10 @@ int ice_sriov_set_msix_vec_count(struct pci_dev *vf_dev, int msix_vec_count)
>  	vf->num_msix = prev_msix;
>  	vf->num_vf_qs = prev_queues;
>  	vf->first_vector_idx = ice_sriov_get_irqs(pf, vf->num_msix);
> -	if (vf->first_vector_idx < 0)
> +	if (vf->first_vector_idx < 0) {
> +		ice_put_vf(vf);
>  		return -EINVAL;
> +	}
>
>  	if (needs_rebuild) {
>  		ice_vf_reconfig_vsi(vf);

Would you like to collaborate with any goto chains according to
the desired completion of exception handling?

Regards,
Markus

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

[Simon Horman]

On Sat, Sep 07, 2024 at 02:40:10PM +0200, Markus Elfring wrote:
> …
> > +++ 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;
> > @@ -1142,8 +1144,10 @@ int ice_sriov_set_msix_vec_count(struct pci_dev *vf_dev, int msix_vec_count)
> >  	vf->num_msix = prev_msix;
> >  	vf->num_vf_qs = prev_queues;
> >  	vf->first_vector_idx = ice_sriov_get_irqs(pf, vf->num_msix);
> > -	if (vf->first_vector_idx < 0)
> > +	if (vf->first_vector_idx < 0) {
> > +		ice_put_vf(vf);
> >  		return -EINVAL;
> > +	}
> >
> >  	if (needs_rebuild) {
> >  		ice_vf_reconfig_vsi(vf);
> 
> Would you like to collaborate with any goto chains according to
> the desired completion of exception handling?

Yes, I agree that might be nice. But the changes made by this patch are
consistent with the exiting error handling in this function. And as a fix,
this minimal approach seems appropriate to me. IOW, I believe clean-up
should be separated from fixes in this case.

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

[Greg KH]

On Sat, Sep 07, 2024 at 02:40:10PM +0200, Markus Elfring wrote:
> …
> > +++ 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;
> > @@ -1142,8 +1144,10 @@ int ice_sriov_set_msix_vec_count(struct pci_dev *vf_dev, int msix_vec_count)
> >  	vf->num_msix = prev_msix;
> >  	vf->num_vf_qs = prev_queues;
> >  	vf->first_vector_idx = ice_sriov_get_irqs(pf, vf->num_msix);
> > -	if (vf->first_vector_idx < 0)
> > +	if (vf->first_vector_idx < 0) {
> > +		ice_put_vf(vf);
> >  		return -EINVAL;
> > +	}
> >
> >  	if (needs_rebuild) {
> >  		ice_vf_reconfig_vsi(vf);
> 
> Would you like to collaborate with any goto chains according to
> the desired completion of exception handling?
> 
> Regards,
> Markus
> 


Hi,

This is the semi-friendly patch-bot of Greg Kroah-Hartman.

Markus, you seem to have sent a nonsensical or otherwise pointless
review comment to a patch submission on a Linux kernel developer mailing
list.  I strongly suggest that you not do this anymore.  Please do not
bother developers who are actively working to produce patches and
features with comments that, in the end, are a waste of time.

Patch submitter, please ignore Markus's suggestion; you do not need to
follow it at all.  The person/bot/AI that sent it is being ignored by
almost all Linux kernel maintainers for having a persistent pattern of
behavior of producing distracting and pointless commentary, and
inability to adapt to feedback.  Please feel free to also ignore emails
from them.

thanks,

greg k-h's patch email bot

RE: [Intel-wired-lan] [PATCH v2] ice: Fix improper handling of refcount in ice_sriov_set_msix_vec_count()

[Romanowski, Rafal]

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of Greg
> KH
> Sent: Saturday, September 7, 2024 4:13 PM
> To: Markus Elfring <Markus.Elfring@web.de>
> Cc: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; netdev@vger.kernel.org;
> Eric Dumazet <edumazet@google.com>; stable@vger.kernel.org; LKML <linux-
> kernel@vger.kernel.org>; Gui-Dong Han <hanguidong02@outlook.com>; Jia-Ju
> Bai <baijiaju1990@gmail.com>; intel-wired-lan@lists.osuosl.org; Simon Horman
> <horms@kernel.org>; Kitszel, Przemyslaw <przemyslaw.kitszel@intel.com>;
> Jakub Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; David S.
> Miller <davem@davemloft.net>
> Subject: Re: [Intel-wired-lan] [PATCH v2] ice: Fix improper handling of refcount in
> ice_sriov_set_msix_vec_count()
> 
> On Sat, Sep 07, 2024 at 02:40:10PM +0200, Markus Elfring wrote:
> > …
> > > +++ 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;
> > > @@ -1142,8 +1144,10 @@ int ice_sriov_set_msix_vec_count(struct pci_dev
> *vf_dev, int msix_vec_count)
> > >  	vf->num_msix = prev_msix;
> > >  	vf->num_vf_qs = prev_queues;
> > >  	vf->first_vector_idx = ice_sriov_get_irqs(pf, vf->num_msix);
> > > -	if (vf->first_vector_idx < 0)
> > > +	if (vf->first_vector_idx < 0) {
> > > +		ice_put_vf(vf);
> > >  		return -EINVAL;
> > > +	}
> > >
> > >  	if (needs_rebuild) {
> > >  		ice_vf_reconfig_vsi(vf);
> >
> > Would you like to collaborate with any goto chains according to the
> > desired completion of exception handling?
> >
> > Regards,
> > Markus
> >
> 
> 
> Hi,
> 
> This is the semi-friendly patch-bot of Greg Kroah-Hartman.
> 
> Markus, you seem to have sent a nonsensical or otherwise pointless review
> comment to a patch submission on a Linux kernel developer mailing list.  I
> strongly suggest that you not do this anymore.  Please do not bother developers
> who are actively working to produce patches and features with comments that, in
> the end, are a waste of time.
> 
> Patch submitter, please ignore Markus's suggestion; you do not need to follow it
> at all.  The person/bot/AI that sent it is being ignored by almost all Linux kernel
> maintainers for having a persistent pattern of behavior of producing distracting
> and pointless commentary, and inability to adapt to feedback.  Please feel free to
> also ignore emails from them.
> 
> thanks,
> 
> greg k-h's patch email bot


Tested-by: Rafal Romanowski <rafal.romanowski@intel.com>