| Message ID | 14ebc311-6fd6-4b0b-b314-8347c4efd9fc@stanley.mountain (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [next] ice: Fix signedness bug in ice_init_interrupt_scheme() | expand |
From: Dan Carpenter <dan.carpenter@linaro.org> Date: Wed, 12 Feb 2025 18:27:09 +0300 > [PATCH next] ice: Fix signedness bug in ice_init_interrupt_scheme() I believe it should be "PATCH net" with > If pci_alloc_irq_vectors() can't allocate the minimum number of vectors > then it returns -ENOSPC so there is no need to check for that in the > caller. In fact, because pf->msix.min is an unsigned int, it means that > any negative error codes are type promoted to high positive values and > treated as success. So here the "return -ENOMEM;" is unreachable code. > Check for negatives instead. > > Fixes: 79d97b8cf9a8 ("ice: remove splitting MSI-X between features") a 'Stable:' tag here. > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> > --- > drivers/net/ethernet/intel/ice/ice_irq.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/intel/ice/ice_irq.c b/drivers/net/ethernet/intel/ice/ice_irq.c > index cbae3d81f0f1..b1fdad154203 100644 > --- a/drivers/net/ethernet/intel/ice/ice_irq.c > +++ b/drivers/net/ethernet/intel/ice/ice_irq.c > @@ -149,7 +149,7 @@ int ice_init_interrupt_scheme(struct ice_pf *pf) > > vectors = pci_alloc_irq_vectors(pf->pdev, pf->msix.min, vectors, > PCI_IRQ_MSIX); > - if (vectors < pf->msix.min) > + if (vectors < 0) > return -ENOMEM; This pattern most likely repeats in other Intel drivers >_< BTW it's a bit weird that we return -ENOMEM here, although we have a precise errno in case of error. Shouldn't we do `return vectors` here? (but this is more of an improvement, not a fix, so out of this patch's scope) > > ice_init_irq_tracker(pf, pf->msix.max, vectors); Thanks, Olek
On Wed, 12 Feb 2025 17:46:54 +0100 Alexander Lobakin wrote: > > [PATCH next] ice: Fix signedness bug in ice_init_interrupt_scheme() > > I believe it should be "PATCH net" with > > > If pci_alloc_irq_vectors() can't allocate the minimum number of vectors > > then it returns -ENOSPC so there is no need to check for that in the > > caller. In fact, because pf->msix.min is an unsigned int, it means that > > any negative error codes are type promoted to high positive values and > > treated as success. So here the "return -ENOMEM;" is unreachable code. > > Check for negatives instead. > > > > Fixes: 79d97b8cf9a8 ("ice: remove splitting MSI-X between features") > > a 'Stable:' tag here. Bug only exists in net-next if it comes from commit under Fixes. So I think the patch is good as is.
On Wed, Feb 12, 2025 at 05:59:01PM -0800, Jakub Kicinski wrote: > On Wed, 12 Feb 2025 17:46:54 +0100 Alexander Lobakin wrote: > > > [PATCH next] ice: Fix signedness bug in ice_init_interrupt_scheme() > > > > I believe it should be "PATCH net" with > > > > > If pci_alloc_irq_vectors() can't allocate the minimum number of vectors > > > then it returns -ENOSPC so there is no need to check for that in the > > > caller. In fact, because pf->msix.min is an unsigned int, it means that > > > any negative error codes are type promoted to high positive values and > > > treated as success. So here the "return -ENOMEM;" is unreachable code. > > > Check for negatives instead. > > > > > > Fixes: 79d97b8cf9a8 ("ice: remove splitting MSI-X between features") > > > > a 'Stable:' tag here. > > Bug only exists in net-next if it comes from commit under Fixes. > So I think the patch is good as is. I want to resen this. My scripts should have put a net-next in the subject and I think that changing: - return -ENOMEM; + return vectors; actually does fall within the scope of the patch so I want to change that as well. There is no point in really breaking that into a separate patch from a practical perspective. regards, dan carpenter
On Thu, Feb 13, 2025 at 08:26:09AM +0300, Dan Carpenter wrote: > On Wed, Feb 12, 2025 at 05:59:01PM -0800, Jakub Kicinski wrote: > > On Wed, 12 Feb 2025 17:46:54 +0100 Alexander Lobakin wrote: > > > > [PATCH next] ice: Fix signedness bug in ice_init_interrupt_scheme() > > > > > > I believe it should be "PATCH net" with > > > > > > > If pci_alloc_irq_vectors() can't allocate the minimum number of vectors > > > > then it returns -ENOSPC so there is no need to check for that in the > > > > caller. In fact, because pf->msix.min is an unsigned int, it means that > > > > any negative error codes are type promoted to high positive values and > > > > treated as success. So here the "return -ENOMEM;" is unreachable code. > > > > Check for negatives instead. > > > > > > > > Fixes: 79d97b8cf9a8 ("ice: remove splitting MSI-X between features") > > > > > > a 'Stable:' tag here. > > > > Bug only exists in net-next if it comes from commit under Fixes. > > So I think the patch is good as is. > > I want to resen this. My scripts should have put a net-next in the > subject and I think that changing: > > - return -ENOMEM; > + return vectors; > > actually does fall within the scope of the patch so I want to change > that as well. There is no point in really breaking that into a separate > patch from a practical perspective. Thanks for fixing, I blindly followed scheme from idpf (there is the same issue). However in ice it was done correctly before my patch. Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com> > > regards, > dan carpenter
diff --git a/drivers/net/ethernet/intel/ice/ice_irq.c b/drivers/net/ethernet/intel/ice/ice_irq.c index cbae3d81f0f1..b1fdad154203 100644 --- a/drivers/net/ethernet/intel/ice/ice_irq.c +++ b/drivers/net/ethernet/intel/ice/ice_irq.c @@ -149,7 +149,7 @@ int ice_init_interrupt_scheme(struct ice_pf *pf) vectors = pci_alloc_irq_vectors(pf->pdev, pf->msix.min, vectors, PCI_IRQ_MSIX); - if (vectors < pf->msix.min) + if (vectors < 0) return -ENOMEM; ice_init_irq_tracker(pf, pf->msix.max, vectors);
If pci_alloc_irq_vectors() can't allocate the minimum number of vectors then it returns -ENOSPC so there is no need to check for that in the caller. In fact, because pf->msix.min is an unsigned int, it means that any negative error codes are type promoted to high positive values and treated as success. So here the "return -ENOMEM;" is unreachable code. Check for negatives instead. Fixes: 79d97b8cf9a8 ("ice: remove splitting MSI-X between features") Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> --- drivers/net/ethernet/intel/ice/ice_irq.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)