[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 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

[PATCH 1/5] drm/gma500: fix error path in gma_intel_setup_gmbus()

[Rasmus Villemoes]

The current code fails to call i2c_del_adapter on
dev_prev->gmbus[0].adapter, and if the for loop above failed already
at i==0, all hell breaks loose when we do the loop body for
i = -1,-2,...

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 drivers/gpu/drm/gma500/intel_gmbus.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/gma500/intel_gmbus.c b/drivers/gpu/drm/gma500/intel_gmbus.c
index 566d330aaeea..e7e22187c539 100644
--- a/drivers/gpu/drm/gma500/intel_gmbus.c
+++ b/drivers/gpu/drm/gma500/intel_gmbus.c
@@ -436,7 +436,7 @@ int gma_intel_setup_gmbus(struct drm_device *dev)
 	return 0;
 
 err:
-	while (--i) {
+	while (i--) {
 		struct intel_gmbus *bus = &dev_priv->gmbus[i];
 		i2c_del_adapter(&bus->adapter);
 	}
-- 
2.1.4

[PATCH 2/5] drm/i915: fix error path in intel_setup_gmbus()

[Rasmus Villemoes]

This fails to undo the setup for pin==0; moreover, something
interesting happens if the setup failed already at pin==0.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 drivers/gpu/drm/i915/intel_i2c.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
index 25254b5c1ac5..deb8282c26d8 100644
--- a/drivers/gpu/drm/i915/intel_i2c.c
+++ b/drivers/gpu/drm/i915/intel_i2c.c
@@ -683,7 +683,7 @@ int intel_setup_gmbus(struct drm_device *dev)
 	return 0;
 
 err:
-	while (--pin) {
+	while (pin--) {
 		if (!intel_gmbus_is_valid_pin(dev_priv, pin))
 			continue;
 
-- 
2.1.4

[PATCH 2/5] drm/i915: fix error path in intel_setup_gmbus()

[Rasmus Villemoes]

This fails to undo the setup for pin==0; moreover, something
interesting happens if the setup failed already at pin==0.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 drivers/gpu/drm/i915/intel_i2c.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
index 25254b5c1ac5..deb8282c26d8 100644
--- a/drivers/gpu/drm/i915/intel_i2c.c
+++ b/drivers/gpu/drm/i915/intel_i2c.c
@@ -683,7 +683,7 @@ int intel_setup_gmbus(struct drm_device *dev)
 	return 0;
 
 err:
-	while (--pin) {
+	while (pin--) {
 		if (!intel_gmbus_is_valid_pin(dev_priv, pin))
 			continue;
 
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[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

[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

[PATCH 5/5] mm/backing-dev.c: fix error path in wb_init()

[Rasmus Villemoes]

We need to use post-decrement to get percpu_counter_destroy() called
on &wb->stat[0]. Moreover, the pre-decremebt would cause infinite
out-of-bounds accesses if the setup code failed at i==0.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 mm/backing-dev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/backing-dev.c b/mm/backing-dev.c
index cc5d29d2da9b..723f3e624b9a 100644
--- a/mm/backing-dev.c
+++ b/mm/backing-dev.c
@@ -328,7 +328,7 @@ static int wb_init(struct bdi_writeback *wb, struct backing_dev_info *bdi,
 	return 0;
 
 out_destroy_stat:
-	while (--i)
+	while (i--)
 		percpu_counter_destroy(&wb->stat[i]);
 	fprop_local_destroy_percpu(&wb->completions);
 out_put_cong:
-- 
2.1.4

[PATCH 5/5] mm/backing-dev.c: fix error path in wb_init()

[Rasmus Villemoes]

We need to use post-decrement to get percpu_counter_destroy() called
on &wb->stat[0]. Moreover, the pre-decremebt would cause infinite
out-of-bounds accesses if the setup code failed at i==0.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 mm/backing-dev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/backing-dev.c b/mm/backing-dev.c
index cc5d29d2da9b..723f3e624b9a 100644
--- a/mm/backing-dev.c
+++ b/mm/backing-dev.c
@@ -328,7 +328,7 @@ static int wb_init(struct bdi_writeback *wb, struct backing_dev_info *bdi,
 	return 0;
 
 out_destroy_stat:
-	while (--i)
+	while (i--)
 		percpu_counter_destroy(&wb->stat[i]);
 	fprop_local_destroy_percpu(&wb->completions);
 out_put_cong:
-- 
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>

Re: [PATCH 2/5] drm/i915: fix error path in intel_setup_gmbus()

[Jani Nikula]

On Tue, 09 Feb 2016, Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:
> This fails to undo the setup for pin==0; moreover, something
> interesting happens if the setup failed already at pin==0.
>
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> ---
>  drivers/gpu/drm/i915/intel_i2c.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
> index 25254b5c1ac5..deb8282c26d8 100644
> --- a/drivers/gpu/drm/i915/intel_i2c.c
> +++ b/drivers/gpu/drm/i915/intel_i2c.c
> @@ -683,7 +683,7 @@ int intel_setup_gmbus(struct drm_device *dev)
>  	return 0;
>  
>  err:
> -	while (--pin) {
> +	while (pin--) {
>  		if (!intel_gmbus_is_valid_pin(dev_priv, pin))
>  			continue;

Reviewed-by: Jani Nikula <jani.nikula@intel.com>
Fixes: f899fc64cda8 ("drm/i915: use GMBUS to manage i2c links")
Cc: stable@vger.kernel.org


-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH 2/5] drm/i915: fix error path in intel_setup_gmbus()

[Jani Nikula]

On Tue, 09 Feb 2016, Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:
> This fails to undo the setup for pin==0; moreover, something
> interesting happens if the setup failed already at pin==0.
>
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> ---
>  drivers/gpu/drm/i915/intel_i2c.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
> index 25254b5c1ac5..deb8282c26d8 100644
> --- a/drivers/gpu/drm/i915/intel_i2c.c
> +++ b/drivers/gpu/drm/i915/intel_i2c.c
> @@ -683,7 +683,7 @@ int intel_setup_gmbus(struct drm_device *dev)
>  	return 0;
>  
>  err:
> -	while (--pin) {
> +	while (pin--) {
>  		if (!intel_gmbus_is_valid_pin(dev_priv, pin))
>  			continue;

Reviewed-by: Jani Nikula <jani.nikula@intel.com>
Fixes: f899fc64cda8 ("drm/i915: use GMBUS to manage i2c links")
Cc: stable@vger.kernel.org


-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/5] drm/gma500: fix error path in gma_intel_setup_gmbus()

[Andy Shevchenko]

On Tue, Feb 9, 2016 at 10:11 PM, Rasmus Villemoes
<linux@rasmusvillemoes.dk> wrote:
> The current code fails to call i2c_del_adapter on
> dev_prev->gmbus[0].adapter, and if the for loop above failed already
> at i==0, all hell breaks loose when we do the loop body for
> i = -1,-2,...
>

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> ---
>  drivers/gpu/drm/gma500/intel_gmbus.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/gma500/intel_gmbus.c b/drivers/gpu/drm/gma500/intel_gmbus.c
> index 566d330aaeea..e7e22187c539 100644
> --- a/drivers/gpu/drm/gma500/intel_gmbus.c
> +++ b/drivers/gpu/drm/gma500/intel_gmbus.c
> @@ -436,7 +436,7 @@ int gma_intel_setup_gmbus(struct drm_device *dev)
>         return 0;
>
>  err:
> -       while (--i) {
> +       while (i--) {
>                 struct intel_gmbus *bus = &dev_priv->gmbus[i];
>                 i2c_del_adapter(&bus->adapter);
>         }
> --
> 2.1.4
>



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/5] drm/gma500: fix error path in gma_intel_setup_gmbus()

[Andy Shevchenko]

On Tue, Feb 9, 2016 at 10:11 PM, Rasmus Villemoes
<linux@rasmusvillemoes.dk> wrote:
> The current code fails to call i2c_del_adapter on
> dev_prev->gmbus[0].adapter, and if the for loop above failed already
> at i==0, all hell breaks loose when we do the loop body for
> i = -1,-2,...
>

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> ---
>  drivers/gpu/drm/gma500/intel_gmbus.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/gma500/intel_gmbus.c b/drivers/gpu/drm/gma500/intel_gmbus.c
> index 566d330aaeea..e7e22187c539 100644
> --- a/drivers/gpu/drm/gma500/intel_gmbus.c
> +++ b/drivers/gpu/drm/gma500/intel_gmbus.c
> @@ -436,7 +436,7 @@ int gma_intel_setup_gmbus(struct drm_device *dev)
>         return 0;
>
>  err:
> -       while (--i) {
> +       while (i--) {
>                 struct intel_gmbus *bus = &dev_priv->gmbus[i];
>                 i2c_del_adapter(&bus->adapter);
>         }
> --
> 2.1.4
>



-- 
With Best Regards,
Andy Shevchenko
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/5] drm/gma500: fix error path in gma_intel_setup_gmbus()

[Daniel Vetter]

On Wed, Feb 10, 2016 at 08:41:17AM +0200, Andy Shevchenko wrote:
> On Tue, Feb 9, 2016 at 10:11 PM, Rasmus Villemoes
> <linux@rasmusvillemoes.dk> wrote:
> > The current code fails to call i2c_del_adapter on
> > dev_prev->gmbus[0].adapter, and if the for loop above failed already
> > at i==0, all hell breaks loose when we do the loop body for
> > i = -1,-2,...
> >
> 
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

Applied to drm-misc, thanks.
-Daniel

> 
> > Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> > ---
> >  drivers/gpu/drm/gma500/intel_gmbus.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/gma500/intel_gmbus.c b/drivers/gpu/drm/gma500/intel_gmbus.c
> > index 566d330aaeea..e7e22187c539 100644
> > --- a/drivers/gpu/drm/gma500/intel_gmbus.c
> > +++ b/drivers/gpu/drm/gma500/intel_gmbus.c
> > @@ -436,7 +436,7 @@ int gma_intel_setup_gmbus(struct drm_device *dev)
> >         return 0;
> >
> >  err:
> > -       while (--i) {
> > +       while (i--) {
> >                 struct intel_gmbus *bus = &dev_priv->gmbus[i];
> >                 i2c_del_adapter(&bus->adapter);
> >         }
> > --
> > 2.1.4
> >
> 
> 
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 1/5] drm/gma500: fix error path in gma_intel_setup_gmbus()

[Daniel Vetter]

On Wed, Feb 10, 2016 at 08:41:17AM +0200, Andy Shevchenko wrote:
> On Tue, Feb 9, 2016 at 10:11 PM, Rasmus Villemoes
> <linux@rasmusvillemoes.dk> wrote:
> > The current code fails to call i2c_del_adapter on
> > dev_prev->gmbus[0].adapter, and if the for loop above failed already
> > at i==0, all hell breaks loose when we do the loop body for
> > i = -1,-2,...
> >
> 
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

Applied to drm-misc, thanks.
-Daniel

> 
> > Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> > ---
> >  drivers/gpu/drm/gma500/intel_gmbus.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/gma500/intel_gmbus.c b/drivers/gpu/drm/gma500/intel_gmbus.c
> > index 566d330aaeea..e7e22187c539 100644
> > --- a/drivers/gpu/drm/gma500/intel_gmbus.c
> > +++ b/drivers/gpu/drm/gma500/intel_gmbus.c
> > @@ -436,7 +436,7 @@ int gma_intel_setup_gmbus(struct drm_device *dev)
> >         return 0;
> >
> >  err:
> > -       while (--i) {
> > +       while (i--) {
> >                 struct intel_gmbus *bus = &dev_priv->gmbus[i];
> >                 i2c_del_adapter(&bus->adapter);
> >         }
> > --
> > 2.1.4
> >
> 
> 
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/5] drm/i915: fix error path in intel_setup_gmbus()

[Jani Nikula]

On Tue, 09 Feb 2016, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> On Tue, 09 Feb 2016, Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:
>> This fails to undo the setup for pin==0; moreover, something
>> interesting happens if the setup failed already at pin==0.
>>
>> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
>> ---
>>  drivers/gpu/drm/i915/intel_i2c.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
>> index 25254b5c1ac5..deb8282c26d8 100644
>> --- a/drivers/gpu/drm/i915/intel_i2c.c
>> +++ b/drivers/gpu/drm/i915/intel_i2c.c
>> @@ -683,7 +683,7 @@ int intel_setup_gmbus(struct drm_device *dev)
>>  	return 0;
>>  
>>  err:
>> -	while (--pin) {
>> +	while (pin--) {
>>  		if (!intel_gmbus_is_valid_pin(dev_priv, pin))
>>  			continue;
>
> Reviewed-by: Jani Nikula <jani.nikula@intel.com>
> Fixes: f899fc64cda8 ("drm/i915: use GMBUS to manage i2c links")
> Cc: stable@vger.kernel.org

And picked up to drm-intel-next-queued, thanks for the patch.

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH 2/5] drm/i915: fix error path in intel_setup_gmbus()

[Jani Nikula]

On Tue, 09 Feb 2016, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> On Tue, 09 Feb 2016, Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:
>> This fails to undo the setup for pin==0; moreover, something
>> interesting happens if the setup failed already at pin==0.
>>
>> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
>> ---
>>  drivers/gpu/drm/i915/intel_i2c.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
>> index 25254b5c1ac5..deb8282c26d8 100644
>> --- a/drivers/gpu/drm/i915/intel_i2c.c
>> +++ b/drivers/gpu/drm/i915/intel_i2c.c
>> @@ -683,7 +683,7 @@ int intel_setup_gmbus(struct drm_device *dev)
>>  	return 0;
>>  
>>  err:
>> -	while (--pin) {
>> +	while (pin--) {
>>  		if (!intel_gmbus_is_valid_pin(dev_priv, pin))
>>  			continue;
>
> Reviewed-by: Jani Nikula <jani.nikula@intel.com>
> Fixes: f899fc64cda8 ("drm/i915: use GMBUS to manage i2c links")
> Cc: stable@vger.kernel.org

And picked up to drm-intel-next-queued, thanks for the patch.

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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()

[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]

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 --]

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.