* [Intel-wired-lan] [PATCH next] ice: Fix signedness bug in ice_init_interrupt_scheme()
@ 2025-02-12 15:27 ` Dan Carpenter
0 siblings, 0 replies; 10+ messages in thread
From: Dan Carpenter @ 2025-02-12 15:27 UTC (permalink / raw)
To: Michal Swiatkowski
Cc: Tony Nguyen, Przemek Kitszel, Andrew Lunn, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Jacob Keller,
Wojciech Drewek, intel-wired-lan, netdev, linux-kernel,
kernel-janitors
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(-)
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);
--
2.47.2
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH next] ice: Fix signedness bug in ice_init_interrupt_scheme()
@ 2025-02-12 15:27 ` Dan Carpenter
0 siblings, 0 replies; 10+ messages in thread
From: Dan Carpenter @ 2025-02-12 15:27 UTC (permalink / raw)
To: Michal Swiatkowski
Cc: Tony Nguyen, Przemek Kitszel, Andrew Lunn, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Jacob Keller,
Wojciech Drewek, intel-wired-lan, netdev, linux-kernel,
kernel-janitors
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(-)
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);
--
2.47.2
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [Intel-wired-lan] [PATCH next] ice: Fix signedness bug in ice_init_interrupt_scheme()
2025-02-12 15:27 ` Dan Carpenter
@ 2025-02-12 16:46 ` Alexander Lobakin
-1 siblings, 0 replies; 10+ messages in thread
From: Alexander Lobakin @ 2025-02-12 16:46 UTC (permalink / raw)
To: Dan Carpenter
Cc: Michal Swiatkowski, Tony Nguyen, Przemek Kitszel, Andrew Lunn,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Jacob Keller, Wojciech Drewek, intel-wired-lan, netdev,
linux-kernel, kernel-janitors
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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH next] ice: Fix signedness bug in ice_init_interrupt_scheme()
@ 2025-02-12 16:46 ` Alexander Lobakin
0 siblings, 0 replies; 10+ messages in thread
From: Alexander Lobakin @ 2025-02-12 16:46 UTC (permalink / raw)
To: Dan Carpenter
Cc: Michal Swiatkowski, Tony Nguyen, Przemek Kitszel, Andrew Lunn,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Jacob Keller, Wojciech Drewek, intel-wired-lan, netdev,
linux-kernel, kernel-janitors
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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [Intel-wired-lan] [PATCH next] ice: Fix signedness bug in ice_init_interrupt_scheme()
2025-02-12 16:46 ` Alexander Lobakin
@ 2025-02-13 1:59 ` Jakub Kicinski
-1 siblings, 0 replies; 10+ messages in thread
From: Jakub Kicinski @ 2025-02-13 1:59 UTC (permalink / raw)
To: Alexander Lobakin
Cc: Dan Carpenter, Michal Swiatkowski, Tony Nguyen, Przemek Kitszel,
Andrew Lunn, David S. Miller, Eric Dumazet, Paolo Abeni,
Jacob Keller, Wojciech Drewek, intel-wired-lan, netdev,
linux-kernel, kernel-janitors
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.
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH next] ice: Fix signedness bug in ice_init_interrupt_scheme()
@ 2025-02-13 1:59 ` Jakub Kicinski
0 siblings, 0 replies; 10+ messages in thread
From: Jakub Kicinski @ 2025-02-13 1:59 UTC (permalink / raw)
To: Alexander Lobakin
Cc: Dan Carpenter, Michal Swiatkowski, Tony Nguyen, Przemek Kitszel,
Andrew Lunn, David S. Miller, Eric Dumazet, Paolo Abeni,
Jacob Keller, Wojciech Drewek, intel-wired-lan, netdev,
linux-kernel, kernel-janitors
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.
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [Intel-wired-lan] [PATCH next] ice: Fix signedness bug in ice_init_interrupt_scheme()
2025-02-13 1:59 ` Jakub Kicinski
@ 2025-02-13 5:26 ` Dan Carpenter
-1 siblings, 0 replies; 10+ messages in thread
From: Dan Carpenter @ 2025-02-13 5:26 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Alexander Lobakin, Michal Swiatkowski, Tony Nguyen,
Przemek Kitszel, Andrew Lunn, David S. Miller, Eric Dumazet,
Paolo Abeni, Jacob Keller, Wojciech Drewek, intel-wired-lan,
netdev, linux-kernel, kernel-janitors
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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH next] ice: Fix signedness bug in ice_init_interrupt_scheme()
@ 2025-02-13 5:26 ` Dan Carpenter
0 siblings, 0 replies; 10+ messages in thread
From: Dan Carpenter @ 2025-02-13 5:26 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Alexander Lobakin, Michal Swiatkowski, Tony Nguyen,
Przemek Kitszel, Andrew Lunn, David S. Miller, Eric Dumazet,
Paolo Abeni, Jacob Keller, Wojciech Drewek, intel-wired-lan,
netdev, linux-kernel, kernel-janitors
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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [Intel-wired-lan] [PATCH next] ice: Fix signedness bug in ice_init_interrupt_scheme()
2025-02-13 5:26 ` Dan Carpenter
@ 2025-02-13 5:43 ` Michal Swiatkowski
-1 siblings, 0 replies; 10+ messages in thread
From: Michal Swiatkowski @ 2025-02-13 5:43 UTC (permalink / raw)
To: Dan Carpenter
Cc: Jakub Kicinski, Alexander Lobakin, Michal Swiatkowski,
Tony Nguyen, Przemek Kitszel, Andrew Lunn, David S. Miller,
Eric Dumazet, Paolo Abeni, Jacob Keller, Wojciech Drewek,
intel-wired-lan, netdev, linux-kernel, kernel-janitors
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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH next] ice: Fix signedness bug in ice_init_interrupt_scheme()
@ 2025-02-13 5:43 ` Michal Swiatkowski
0 siblings, 0 replies; 10+ messages in thread
From: Michal Swiatkowski @ 2025-02-13 5:43 UTC (permalink / raw)
To: Dan Carpenter
Cc: Jakub Kicinski, Alexander Lobakin, Michal Swiatkowski,
Tony Nguyen, Przemek Kitszel, Andrew Lunn, David S. Miller,
Eric Dumazet, Paolo Abeni, Jacob Keller, Wojciech Drewek,
intel-wired-lan, netdev, linux-kernel, kernel-janitors
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
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2025-02-13 5:46 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-02-12 15:27 [Intel-wired-lan] [PATCH next] ice: Fix signedness bug in ice_init_interrupt_scheme() Dan Carpenter
2025-02-12 15:27 ` Dan Carpenter
2025-02-12 16:46 ` [Intel-wired-lan] " Alexander Lobakin
2025-02-12 16:46 ` Alexander Lobakin
2025-02-13 1:59 ` [Intel-wired-lan] " Jakub Kicinski
2025-02-13 1:59 ` Jakub Kicinski
2025-02-13 5:26 ` [Intel-wired-lan] " Dan Carpenter
2025-02-13 5:26 ` Dan Carpenter
2025-02-13 5:43 ` [Intel-wired-lan] " Michal Swiatkowski
2025-02-13 5:43 ` Michal Swiatkowski
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.