ping
On Tue, Feb 09 2016, Rasmus Villemoes <[email protected]> wrote:
> We need to use post-decrement to ensure that irq_dispose_mapping is
> also called on priv->rxq[0]->irq_no; moreover, if one of the above for
> loops failed already at i==0 (so we reach one of these labels with
> that value of i), we'll enter an essentially infinite loop of
> out-of-bounds accesses.
>
> Signed-off-by: Rasmus Villemoes <[email protected]>
> ---
> drivers/net/ethernet/samsung/sxgbe/sxgbe_platform.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/samsung/sxgbe/sxgbe_platform.c b/drivers/net/ethernet/samsung/sxgbe/sxgbe_platform.c
> index b02eed12bfc5..73427e29df2a 100644
> --- a/drivers/net/ethernet/samsung/sxgbe/sxgbe_platform.c
> +++ b/drivers/net/ethernet/samsung/sxgbe/sxgbe_platform.c
> @@ -155,11 +155,11 @@ static int sxgbe_platform_probe(struct platform_device *pdev)
> return 0;
>
> err_rx_irq_unmap:
> - while (--i)
> + while (i--)
> irq_dispose_mapping(priv->rxq[i]->irq_no);
> i = SXGBE_TX_QUEUES;
> err_tx_irq_unmap:
> - while (--i)
> + while (i--)
> irq_dispose_mapping(priv->txq[i]->irq_no);
> irq_dispose_mapping(priv->irq);
> err_drv_remove:
ping^2
On Tue, Mar 08 2016, Rasmus Villemoes <[email protected]> wrote:
> ping
>
> On Tue, Feb 09 2016, Rasmus Villemoes <[email protected]> wrote:
>
>> We need to use post-decrement to ensure that irq_dispose_mapping is
>> also called on priv->rxq[0]->irq_no; moreover, if one of the above for
>> loops failed already at i==0 (so we reach one of these labels with
>> that value of i), we'll enter an essentially infinite loop of
>> out-of-bounds accesses.
>>
>> Signed-off-by: Rasmus Villemoes <[email protected]>
>> ---
>> drivers/net/ethernet/samsung/sxgbe/sxgbe_platform.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/samsung/sxgbe/sxgbe_platform.c b/drivers/net/ethernet/samsung/sxgbe/sxgbe_platform.c
>> index b02eed12bfc5..73427e29df2a 100644
>> --- a/drivers/net/ethernet/samsung/sxgbe/sxgbe_platform.c
>> +++ b/drivers/net/ethernet/samsung/sxgbe/sxgbe_platform.c
>> @@ -155,11 +155,11 @@ static int sxgbe_platform_probe(struct platform_device *pdev)
>> return 0;
>>
>> err_rx_irq_unmap:
>> - while (--i)
>> + while (i--)
>> irq_dispose_mapping(priv->rxq[i]->irq_no);
>> i = SXGBE_TX_QUEUES;
>> err_tx_irq_unmap:
>> - while (--i)
>> + while (i--)
>> irq_dispose_mapping(priv->txq[i]->irq_no);
>> irq_dispose_mapping(priv->irq);
>> err_drv_remove:
We need to use post-decrement to ensure that irq_dispose_mapping is
also called on priv->rxq[0]->irq_no; moreover, if one of the above for
loops failed already at i==0 (so we reach one of these labels with
that value of i), we'll enter an essentially infinite loop of
out-of-bounds accesses.
Signed-off-by: Rasmus Villemoes <[email protected]>
---
David, can you take this directly? Of the three samsung people listed
by get_maintainer.pl, one address bounces and another informed me
privately that he's not actually a maintainer of this anymore.
drivers/net/ethernet/samsung/sxgbe/sxgbe_platform.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/samsung/sxgbe/sxgbe_platform.c b/drivers/net/ethernet/samsung/sxgbe/sxgbe_platform.c
index b02eed12bfc5..73427e29df2a 100644
--- a/drivers/net/ethernet/samsung/sxgbe/sxgbe_platform.c
+++ b/drivers/net/ethernet/samsung/sxgbe/sxgbe_platform.c
@@ -155,11 +155,11 @@ static int sxgbe_platform_probe(struct platform_device *pdev)
return 0;
err_rx_irq_unmap:
- while (--i)
+ while (i--)
irq_dispose_mapping(priv->rxq[i]->irq_no);
i = SXGBE_TX_QUEUES;
err_tx_irq_unmap:
- while (--i)
+ while (i--)
irq_dispose_mapping(priv->txq[i]->irq_no);
irq_dispose_mapping(priv->irq);
err_drv_remove:
--
2.1.4
Rasmus Villemoes <[email protected]> :
> We need to use post-decrement to ensure that irq_dispose_mapping is
> also called on priv->rxq[0]->irq_no; moreover, if one of the above for
> loops failed already at i==0 (so we reach one of these labels with
> that value of i), we'll enter an essentially infinite loop of
> out-of-bounds accesses.
>
> Signed-off-by: Rasmus Villemoes <[email protected]>
(ok, i is signed)
Reviewed-by: Francois Romieu <[email protected]>
Someone messed with my review on 2014/03/25 and got it wrong. :o/
Two years after the initial submission, there is zero change regarding use
of sxgbe_core_ops for extension or manageability. The extra indirection is
ripe for removal during next net-next.
--
Ueimor
On Sun, Mar 27 2016, Francois Romieu <[email protected]> wrote:
> Rasmus Villemoes <[email protected]> :
>> We need to use post-decrement to ensure that irq_dispose_mapping is
>> also called on priv->rxq[0]->irq_no; moreover, if one of the above for
>> loops failed already at i==0 (so we reach one of these labels with
>> that value of i), we'll enter an essentially infinite loop of
>> out-of-bounds accesses.
>>
>> Signed-off-by: Rasmus Villemoes <[email protected]>
>
> (ok, i is signed)
>
> Reviewed-by: Francois Romieu <[email protected]>
>
Thanks for reviewing, but just FTR I want to point out that it doesn't
matter whether i is signed or not in
while (i--)
However, when i is signed, there's another slightly less popular variant
which is equivalent:
while (--i >= 0)
(a precondition for their equivalence is that i has a non-negative value
before reaching the while statement).
Neither are equivalent to the almost-always broken
while (--i)
Rasmus
From: Francois Romieu <[email protected]>
Date: Sun, 27 Mar 2016 10:22:54 +0200
> Two years after the initial submission, there is zero change regarding use
> of sxgbe_core_ops for extension or manageability. The extra indirection is
> ripe for removal during next net-next.
I completely agree, that stuff has to go.
From: Rasmus Villemoes <[email protected]>
Date: Sat, 26 Mar 2016 22:24:09 +0100
> We need to use post-decrement to ensure that irq_dispose_mapping is
> also called on priv->rxq[0]->irq_no; moreover, if one of the above for
> loops failed already at i==0 (so we reach one of these labels with
> that value of i), we'll enter an essentially infinite loop of
> out-of-bounds accesses.
>
> Signed-off-by: Rasmus Villemoes <[email protected]>
Applied, thanks.
> David, can you take this directly? Of the three samsung people listed
> by get_maintainer.pl, one address bounces and another informed me
> privately that he's not actually a maintainer of this anymore.
That's awesome, another pump and dump driver submission.