| Message ID | SY8P300MB0460D0263B2105307C444520C0932@SY8P300MB0460.AUSP300.PROD.OUTLOOK.COM (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v2] ice: Fix improper handling of refcount in ice_sriov_set_msix_vec_count() | expand |
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>
… > +++ 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
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.
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
> -----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>
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);
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(-)