[PATCH 0/5] pre-decrement in error paths considered harmful

[PATCH 0/5] pre-decrement in error paths considered harmful

[Rasmus Villemoes]

There are a few instances of

  for (i = 0; i < FOO; ++i) {
    ret = do_stuff(i)
    if (ret)
      goto err;
  }
  ...
  err:
  while (--i)
    undo_stuff(i);

At best, this fails to undo_stuff for i==0, but if i==0 was the case
that failed, we'll end up with an "infinite" loop in the error path
doing nasty stuff.

These were found with a simple coccinelle script

@@
expression i;
identifier l;
statement S;
@@
* l:
* while (--i)
    S

(and there were no false positives).

There's no dependencies between the patches; I just wanted to include
a common cover letter with a little background info.

Rasmus Villemoes (5):
  drm/gma500: fix error path in gma_intel_setup_gmbus()
  drm/i915: fix error path in intel_setup_gmbus()
  net/mlx4: fix some error handling in mlx4_multi_func_init()
  net: sxgbe: fix error paths in sxgbe_platform_probe()
  mm/backing-dev.c: fix error path in wb_init()

 drivers/gpu/drm/gma500/intel_gmbus.c                | 2 +-
 drivers/gpu/drm/i915/intel_i2c.c                    | 2 +-
 drivers/net/ethernet/mellanox/mlx4/cmd.c            | 4 ++--
 drivers/net/ethernet/samsung/sxgbe/sxgbe_platform.c | 4 ++--
 mm/backing-dev.c                                    | 2 +-
 5 files changed, 7 insertions(+), 7 deletions(-)

-- 
2.1.4

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 3/5] net/mlx4: fix some error handling in mlx4_multi_func_init()

[Rasmus Villemoes]

The while loop after err_slaves should use post-decrement; otherwise
we'll fail to do the kfrees for i==0, and will run into out-of-bounds
accesses if the setup above failed already at i==0.

The predecrement in the --port is ok, since ->vlan_filter is
(bizarrely) 1-indexed. But I'm changing 'if' to 'while' since it's a
bit ugly to rely on MLX4_MAX_PORTS being 2.

[I'm not sure why one even bothers populating the ->vlan_filter array:
mlx4.h isn't #included by anything outside
drivers/net/ethernet/mellanox/mlx4/, and "git grep -C2 -w vlan_filter
drivers/net/ethernet/mellanox/mlx4/" seems to suggest that the
vlan_filter elements aren't used at all.]

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 drivers/net/ethernet/mellanox/mlx4/cmd.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx4/cmd.c b/drivers/net/ethernet/mellanox/mlx4/cmd.c
index d48d5793407d..bfe8234abbba 100644
--- a/drivers/net/ethernet/mellanox/mlx4/cmd.c
+++ b/drivers/net/ethernet/mellanox/mlx4/cmd.c
@@ -2369,7 +2369,7 @@ int mlx4_multi_func_init(struct mlx4_dev *dev)
 					kzalloc(sizeof(struct mlx4_vlan_fltr),
 						GFP_KERNEL);
 				if (!s_state->vlan_filter[port]) {
-					if (--port)
+					while (--port)
 						kfree(s_state->vlan_filter[port]);
 					goto err_slaves;
 				}
@@ -2429,7 +2429,7 @@ err_thread:
 	flush_workqueue(priv->mfunc.master.comm_wq);
 	destroy_workqueue(priv->mfunc.master.comm_wq);
 err_slaves:
-	while (--i) {
+	while (i--) {
 		for (port = 1; port <= MLX4_MAX_PORTS; port++)
 			kfree(priv->mfunc.master.slave_state[i].vlan_filter[port]);
 	}
-- 
2.1.4

Re: [PATCH 3/5] net/mlx4: fix some error handling in mlx4_multi_func_init()

[Yishai Hadas]

On 2/9/2016 10:11 PM, Rasmus Villemoes wrote:
> The while loop after err_slaves should use post-decrement; otherwise
> we'll fail to do the kfrees for i==0, and will run into out-of-bounds
> accesses if the setup above failed already at i==0.
>
> The predecrement in the --port is ok, since ->vlan_filter is
> (bizarrely) 1-indexed. But I'm changing 'if' to 'while' since it's a
> bit ugly to rely on MLX4_MAX_PORTS being 2.
>
> [I'm not sure why one even bothers populating the ->vlan_filter array:
> mlx4.h isn't #included by anything outside
> drivers/net/ethernet/mellanox/mlx4/, and "git grep -C2 -w vlan_filter
> drivers/net/ethernet/mellanox/mlx4/" seems to suggest that the
> vlan_filter elements aren't used at all.]
>
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> ---
>   drivers/net/ethernet/mellanox/mlx4/cmd.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/mellanox/mlx4/cmd.c b/drivers/net/ethernet/mellanox/mlx4/cmd.c
> index d48d5793407d..bfe8234abbba 100644
> --- a/drivers/net/ethernet/mellanox/mlx4/cmd.c
> +++ b/drivers/net/ethernet/mellanox/mlx4/cmd.c
> @@ -2369,7 +2369,7 @@ int mlx4_multi_func_init(struct mlx4_dev *dev)
>   					kzalloc(sizeof(struct mlx4_vlan_fltr),
>   						GFP_KERNEL);
>   				if (!s_state->vlan_filter[port]) {
> -					if (--port)
> +					while (--port)

Prefer to leave as-is. There is no way that mlx4 will ever have more 
than 2 ports.

>   						kfree(s_state->vlan_filter[port]);
>   					goto err_slaves;
>   				}
> @@ -2429,7 +2429,7 @@ err_thread:
>   	flush_workqueue(priv->mfunc.master.comm_wq);
>   	destroy_workqueue(priv->mfunc.master.comm_wq);
>   err_slaves:
> -	while (--i) {
> +	while (i--) {

This fix is wrong as it hits the case that i arrived the last value then 
below code will access to a non valid entry in the array.

The expected fix should be:
while (--i >= 0)

>   		for (port = 1; port <= MLX4_MAX_PORTS; port++)
>   			kfree(priv->mfunc.master.slave_state[i].vlan_filter[port]);
>   	}
>

Re: [PATCH 3/5] net/mlx4: fix some error handling in mlx4_multi_func_init()

[Rasmus Villemoes]

On Wed, Feb 10 2016, Yishai Hadas <yishaih@dev.mellanox.co.il> wrote:

>> @@ -2429,7 +2429,7 @@ err_thread:
>>   	flush_workqueue(priv->mfunc.master.comm_wq);
>>   	destroy_workqueue(priv->mfunc.master.comm_wq);
>>   err_slaves:
>> -	while (--i) {
>> +	while (i--) {
>
> This fix is wrong as it hits the case that i arrived the last value
> then below code will access to a non valid entry in the array.
>
> The expected fix should be:
> while (--i >= 0)
>

Huh? They're completely equivalent (given that i is necessarily
non-negative before we evaluate the loop condition). I don't really care
either way, but git grep says that 'while (i--)' is 5 times more common
than 'while (--i >= 0)'.

Rasmus

Re: [PATCH 3/5] net/mlx4: fix some error handling in mlx4_multi_func_init()

[Jack Morgenstein]

On Wed, 10 Feb 2016 19:15:20 +0100
Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:

> On Wed, Feb 10 2016, Yishai Hadas <yishaih@dev.mellanox.co.il> wrote:
> 
> >> @@ -2429,7 +2429,7 @@ err_thread:
> >>   	flush_workqueue(priv->mfunc.master.comm_wq);
> >>   	destroy_workqueue(priv->mfunc.master.comm_wq);
> >>   err_slaves:
> >> -	while (--i) {
> >> +	while (i--) {
> >
> > This fix is wrong as it hits the case that i arrived the last value
> > then below code will access to a non valid entry in the array.
> >
> > The expected fix should be:
> > while (--i >= 0)
> >
> 
> Huh? They're completely equivalent (given that i is necessarily
> non-negative before we evaluate the loop condition)

No, they are not equivalent.
if i == the max value (dev->num_slaves) when entering your proposed
while loop, the kfree call index (i) will be out of range!  This can
happen, for example, if the failure occurs downstream from the "i"
for-loop (e.g., if the call to mlx4_init_resource_tracker() fails).

Therefore, we DO require the pre-decrement format.  Therefore, the
one-line fix proposed by Yishai is the correct fix.
>. I don't really
> care either way, but git grep says that 'while (i--)' is 5 times more
> common than 'while (--i >= 0)'.
Not relevant, while (i--) is simply not correct, because of the case
where the for-loop involving i completes successfully and an error
occurs later.

FYI, you also had another bug in your solution -- a double-free when
kzalloc for port 2 fails.  For your code, you should also have reset
s_state->vlan_filter[port] to NULL as shown below:
			for (port = 1; port <= MLX4_MAX_PORTS; port++) {
				struct mlx4_vport_state *admin_vport;
				struct mlx4_vport_state *oper_vport;

				s_state->vlan_filter[port] =
					kzalloc(sizeof(struct
				mlx4_vlan_fltr), GFP_KERNEL);
				if (!s_state->vlan_filter[port]) {
					if (--port) {
 						kfree(s_state->vlan_filter[port]);
	==> You should have added this		s_state->vlan_filter[port] = NULL;
					}
					goto err_slaves;
				}

However, again, the correct solution is to do what Yishai suggests:
	while (--i >= 0)
so that if i is already zero the while-loop will not be entered.

-Jack
> 
> Rasmus
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma"
> in the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/5] net/mlx4: fix some error handling in mlx4_multi_func_init()

[Jack Morgenstein]

Ouch! Egg on my face!  Sorry about that.
You are correct!  while (--i >= 0) IS exactly equivalent to
while (i--). (the while condition is fully evaluated before the loop is
entered; pre or post increment only influences which value is tested
for true in the while condition -- the pre-value (with post-increment) 
or the post-value (with pre-increment)).

In that case, my comment below regarding the double-free is also not
correct.  Setting the freed pointer to NULL is not needed.

My bad. We should go with your format:  while (i--)

-Jack

On Thu, 11 Feb 2016 11:29:43 +0200
Jack Morgenstein <jackm@dev.mellanox.co.il> wrote:

> On Wed, 10 Feb 2016 19:15:20 +0100
> Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:
> 
> > On Wed, Feb 10 2016, Yishai Hadas <yishaih@dev.mellanox.co.il>
> > wrote:
> > 
> > >> @@ -2429,7 +2429,7 @@ err_thread:
> > >>   	flush_workqueue(priv->mfunc.master.comm_wq);
> > >>   	destroy_workqueue(priv->mfunc.master.comm_wq);
> > >>   err_slaves:
> > >> -	while (--i) {
> > >> +	while (i--) {
> > >
> > > This fix is wrong as it hits the case that i arrived the last
> > > value then below code will access to a non valid entry in the
> > > array.
> > >
> > > The expected fix should be:
> > > while (--i >= 0)
> > >
> > 
> > Huh? They're completely equivalent (given that i is necessarily
> > non-negative before we evaluate the loop condition)
> 
> No, they are not equivalent.
> if i == the max value (dev->num_slaves) when entering your proposed
> while loop, the kfree call index (i) will be out of range!  This can
> happen, for example, if the failure occurs downstream from the "i"
> for-loop (e.g., if the call to mlx4_init_resource_tracker() fails).
> 
> Therefore, we DO require the pre-decrement format.  Therefore, the
> one-line fix proposed by Yishai is the correct fix.
> >. I don't really
> > care either way, but git grep says that 'while (i--)' is 5 times
> > more common than 'while (--i >= 0)'.
> Not relevant, while (i--) is simply not correct, because of the case
> where the for-loop involving i completes successfully and an error
> occurs later.
> 
> FYI, you also had another bug in your solution -- a double-free when
> kzalloc for port 2 fails.  For your code, you should also have reset
> s_state->vlan_filter[port] to NULL as shown below:
> 			for (port = 1; port <= MLX4_MAX_PORTS;
> port++) { struct mlx4_vport_state *admin_vport;
> 				struct mlx4_vport_state *oper_vport;
> 
> 				s_state->vlan_filter[port] =
> 					kzalloc(sizeof(struct
> 				mlx4_vlan_fltr), GFP_KERNEL);
> 				if (!s_state->vlan_filter[port]) {
> 					if (--port) {
>  						kfree(s_state->vlan_filter[port]);
> 	==> You should have added this
> s_state->vlan_filter[port] = NULL; }
> 					goto err_slaves;
> 				}
> 
> However, again, the correct solution is to do what Yishai suggests:
> 	while (--i >= 0)
> so that if i is already zero the while-loop will not be entered.
> 
> -Jack
> > 
> > Rasmus
> > --
> > To unsubscribe from this list: send the line "unsubscribe
> > linux-rdma" in the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 3/5] net/mlx4: fix some error handling in mlx4_multi_func_init()

[Doug Ledford]

[-- Attachment #1: Type: text/plain, Size: 2106 bytes --]

On 02/09/2016 03:11 PM, Rasmus Villemoes wrote:
> The while loop after err_slaves should use post-decrement; otherwise
> we'll fail to do the kfrees for i==0, and will run into out-of-bounds
> accesses if the setup above failed already at i==0.
> 
> The predecrement in the --port is ok, since ->vlan_filter is
> (bizarrely) 1-indexed. But I'm changing 'if' to 'while' since it's a
> bit ugly to rely on MLX4_MAX_PORTS being 2.
> 
> [I'm not sure why one even bothers populating the ->vlan_filter array:
> mlx4.h isn't #included by anything outside
> drivers/net/ethernet/mellanox/mlx4/, and "git grep -C2 -w vlan_filter
> drivers/net/ethernet/mellanox/mlx4/" seems to suggest that the
> vlan_filter elements aren't used at all.]
> 
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> ---
>  drivers/net/ethernet/mellanox/mlx4/cmd.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mellanox/mlx4/cmd.c b/drivers/net/ethernet/mellanox/mlx4/cmd.c
> index d48d5793407d..bfe8234abbba 100644
> --- a/drivers/net/ethernet/mellanox/mlx4/cmd.c
> +++ b/drivers/net/ethernet/mellanox/mlx4/cmd.c
> @@ -2369,7 +2369,7 @@ int mlx4_multi_func_init(struct mlx4_dev *dev)
>  					kzalloc(sizeof(struct mlx4_vlan_fltr),
>  						GFP_KERNEL);
>  				if (!s_state->vlan_filter[port]) {
> -					if (--port)
> +					while (--port)
>  						kfree(s_state->vlan_filter[port]);
>  					goto err_slaves;
>  				}
> @@ -2429,7 +2429,7 @@ err_thread:
>  	flush_workqueue(priv->mfunc.master.comm_wq);
>  	destroy_workqueue(priv->mfunc.master.comm_wq);
>  err_slaves:
> -	while (--i) {
> +	while (i--) {
>  		for (port = 1; port <= MLX4_MAX_PORTS; port++)
>  			kfree(priv->mfunc.master.slave_state[i].vlan_filter[port]);
>  	}
> 

I'm modifying your patch slightly (dropping the first hunk, it isn't
really necessary as Yishai pointed out in review) and adjusting the
description to compensate.  I'll apply the result to my next for-rc series.

-- 
Doug Ledford <dledford@redhat.com>
              GPG KeyID: 0E572FDD



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]

[PATCH 4/5] net: sxgbe: fix error paths in sxgbe_platform_probe()

[Rasmus Villemoes]

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 <linux@rasmusvillemoes.dk>
---
 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

Re: [PATCH 4/5] net: sxgbe: fix error paths in sxgbe_platform_probe()

[Rasmus Villemoes]

ping

On Tue, Feb 09 2016, Rasmus Villemoes <linux@rasmusvillemoes.dk> 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 <linux@rasmusvillemoes.dk>
> ---
>  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:

Re: [PATCH 4/5] net: sxgbe: fix error paths in sxgbe_platform_probe()

[Rasmus Villemoes]

ping^2

On Tue, Mar 08 2016, Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:

> ping
>
> On Tue, Feb 09 2016, Rasmus Villemoes <linux@rasmusvillemoes.dk> 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 <linux@rasmusvillemoes.dk>
>> ---
>>  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:

[PATCH] net: sxgbe: fix error paths in sxgbe_platform_probe()

[Rasmus Villemoes]

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 <linux@rasmusvillemoes.dk>
---
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

Re: [PATCH] net: sxgbe: fix error paths in sxgbe_platform_probe()

[Francois Romieu]

Rasmus Villemoes <linux@rasmusvillemoes.dk> :
> 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 <linux@rasmusvillemoes.dk>

(ok, i is signed)

Reviewed-by: Francois Romieu <romieu@fr.zoreil.com>

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

Re: [PATCH] net: sxgbe: fix error paths in sxgbe_platform_probe()

[Rasmus Villemoes]

On Sun, Mar 27 2016, Francois Romieu <romieu@fr.zoreil.com> wrote:

> Rasmus Villemoes <linux@rasmusvillemoes.dk> :
>> 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 <linux@rasmusvillemoes.dk>
>
> (ok, i is signed)
>
> Reviewed-by: Francois Romieu <romieu@fr.zoreil.com>
>

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

Re: [PATCH] net: sxgbe: fix error paths in sxgbe_platform_probe()

[David Miller]

From: Francois Romieu <romieu@fr.zoreil.com>
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.

Re: [PATCH] net: sxgbe: fix error paths in sxgbe_platform_probe()

[David Miller]

From: Rasmus Villemoes <linux@rasmusvillemoes.dk>
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 <linux@rasmusvillemoes.dk>

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.