Re: [PATCH] net/mlx4_en: fix off by one in error handling

[David Miller]

From: Sebastian Ott 
Date: Wed, 14 Sep 2016 13:09:24 +0200 (CEST)

> If an error occurs in mlx4_init_eq_table the index used in the
> err_out_unmap label is one too big which results in a panic in
> mlx4_free_eq. This patch fixes the index in the error path.
> 
> Signed-off-by: Sebastian Ott 

Applied.

Re: [PATCH] net/mlx4_en: fix off by one in error handling

[David Miller]

From: Sebastian Ott 
Date: Wed, 14 Sep 2016 13:09:24 +0200 (CEST)

> If an error occurs in mlx4_init_eq_table the index used in the
> err_out_unmap label is one too big which results in a panic in
> mlx4_free_eq. This patch fixes the index in the error path.
> 
> Signed-off-by: Sebastian Ott 

Applied.

Re: [PATCH] net/mlx4_en: fix off by one in error handling

[Tariq Toukan]

On 14/09/2016 7:08 PM, Sebastian Ott wrote:

On Wed, 14 Sep 2016, Tariq Toukan wrote:

On 14/09/2016 4:53 PM, Sebastian Ott wrote:

On Wed, 14 Sep 2016, Tariq Toukan wrote:

On 14/09/2016 2:09 PM, Sebastian Ott wrote:

If an error occurs in mlx4_init_eq_table the index used in the
err_out_unmap label is one too big which results in a panic in
mlx4_free_eq. This patch fixes the index in the error path.

You are right, but your change below does not cover all cases.
The full solution looks like this:

@@ -1260,7 +1260,7 @@ int mlx4_init_eq_table(struct mlx4_dev *dev)
   eq);
  }
  if (err)
-   goto err_out_unmap;
+   goto err_out_unmap_excluded;

In this case a call to mlx4_create_eq failed. Do you really have to call
mlx4_free_eq for this index again?

We agree on this part, that's why here we should goto the _excluded_ label.
For all other parts, we should not exclude the eq in the highest index, and
thus we goto the _non_excluded_ label.

But that's exactly what the original patch does. If the failure is within
the for loop at index i, we do the cleanup starting at index i-1. If the
failure is after the for loop then i == dev->caps.num_comp_vectors + 1
and we do the cleanup starting at index i == dev->caps.num_comp_vectors.

In the latter case your patch would have an out of bounds array access.

Indeed. Agreed.


Regards,
Sebastian



Reviewed-by: Tariq Toukan 

Thanks!

Re: [PATCH] net/mlx4_en: fix off by one in error handling

[Tariq Toukan]

On 14/09/2016 7:08 PM, Sebastian Ott wrote:

On Wed, 14 Sep 2016, Tariq Toukan wrote:

On 14/09/2016 4:53 PM, Sebastian Ott wrote:

On Wed, 14 Sep 2016, Tariq Toukan wrote:

On 14/09/2016 2:09 PM, Sebastian Ott wrote:

If an error occurs in mlx4_init_eq_table the index used in the
err_out_unmap label is one too big which results in a panic in
mlx4_free_eq. This patch fixes the index in the error path.

You are right, but your change below does not cover all cases.
The full solution looks like this:

@@ -1260,7 +1260,7 @@ int mlx4_init_eq_table(struct mlx4_dev *dev)
   eq);
  }
  if (err)
-   goto err_out_unmap;
+   goto err_out_unmap_excluded;

In this case a call to mlx4_create_eq failed. Do you really have to call
mlx4_free_eq for this index again?

We agree on this part, that's why here we should goto the _excluded_ label.
For all other parts, we should not exclude the eq in the highest index, and
thus we goto the _non_excluded_ label.

But that's exactly what the original patch does. If the failure is within
the for loop at index i, we do the cleanup starting at index i-1. If the
failure is after the for loop then i == dev->caps.num_comp_vectors + 1
and we do the cleanup starting at index i == dev->caps.num_comp_vectors.

In the latter case your patch would have an out of bounds array access.

Indeed. Agreed.


Regards,
Sebastian



Reviewed-by: Tariq Toukan 

Thanks!

Re: [PATCH] net/mlx4_en: fix off by one in error handling

[Sebastian Ott]

On Wed, 14 Sep 2016, Tariq Toukan wrote:
> On 14/09/2016 4:53 PM, Sebastian Ott wrote:
> > On Wed, 14 Sep 2016, Tariq Toukan wrote:
> > > On 14/09/2016 2:09 PM, Sebastian Ott wrote:
> > > > If an error occurs in mlx4_init_eq_table the index used in the
> > > > err_out_unmap label is one too big which results in a panic in
> > > > mlx4_free_eq. This patch fixes the index in the error path.
> > > You are right, but your change below does not cover all cases.
> > > The full solution looks like this:
> > >
> > > @@ -1260,7 +1260,7 @@ int mlx4_init_eq_table(struct mlx4_dev *dev)
> > >   eq);
> > >  }
> > >  if (err)
> > > -   goto err_out_unmap;
> > > +   goto err_out_unmap_excluded;
> > In this case a call to mlx4_create_eq failed. Do you really have to call
> > mlx4_free_eq for this index again?
>
> We agree on this part, that's why here we should goto the _excluded_ label.
> For all other parts, we should not exclude the eq in the highest index, and
> thus we goto the _non_excluded_ label.

But that's exactly what the original patch does. If the failure is within
the for loop at index i, we do the cleanup starting at index i-1. If the
failure is after the for loop then i == dev->caps.num_comp_vectors + 1
and we do the cleanup starting at index i == dev->caps.num_comp_vectors.

In the latter case your patch would have an out of bounds array access.

Regards,
Sebastian

Re: [PATCH] net/mlx4_en: fix off by one in error handling

[Sebastian Ott]

On Wed, 14 Sep 2016, Tariq Toukan wrote:
> On 14/09/2016 4:53 PM, Sebastian Ott wrote:
> > On Wed, 14 Sep 2016, Tariq Toukan wrote:
> > > On 14/09/2016 2:09 PM, Sebastian Ott wrote:
> > > > If an error occurs in mlx4_init_eq_table the index used in the
> > > > err_out_unmap label is one too big which results in a panic in
> > > > mlx4_free_eq. This patch fixes the index in the error path.
> > > You are right, but your change below does not cover all cases.
> > > The full solution looks like this:
> > >
> > > @@ -1260,7 +1260,7 @@ int mlx4_init_eq_table(struct mlx4_dev *dev)
> > >   eq);
> > >  }
> > >  if (err)
> > > -   goto err_out_unmap;
> > > +   goto err_out_unmap_excluded;
> > In this case a call to mlx4_create_eq failed. Do you really have to call
> > mlx4_free_eq for this index again?
>
> We agree on this part, that's why here we should goto the _excluded_ label.
> For all other parts, we should not exclude the eq in the highest index, and
> thus we goto the _non_excluded_ label.

But that's exactly what the original patch does. If the failure is within
the for loop at index i, we do the cleanup starting at index i-1. If the
failure is after the for loop then i == dev->caps.num_comp_vectors + 1
and we do the cleanup starting at index i == dev->caps.num_comp_vectors.

In the latter case your patch would have an out of bounds array access.

Regards,
Sebastian

Re: [PATCH] net/mlx4_en: fix off by one in error handling

[Tariq Toukan]

On 14/09/2016 4:53 PM, Sebastian Ott wrote:

Hello Tariq,

On Wed, 14 Sep 2016, Tariq Toukan wrote:

On 14/09/2016 2:09 PM, Sebastian Ott wrote:

If an error occurs in mlx4_init_eq_table the index used in the
err_out_unmap label is one too big which results in a panic in
mlx4_free_eq. This patch fixes the index in the error path.

You are right, but your change below does not cover all cases.
The full solution looks like this:

@@ -1260,7 +1260,7 @@ int mlx4_init_eq_table(struct mlx4_dev *dev)
  eq);
 }
 if (err)
-   goto err_out_unmap;
+   goto err_out_unmap_excluded;

In this case a call to mlx4_create_eq failed. Do you really have to call
mlx4_free_eq for this index again?

We agree on this part, that's why here we should goto the _excluded_ label.
For all other parts, we should not exclude the eq in the highest index, 
and thus we goto the _non_excluded_ label.

  As far as I understood this code
mlx4_create_eq cleans up when it fails and thus there is no need for an
additional mlx4_free_eq call.

Regards,
Sebastian


Regards,
Tariq

Re: [PATCH] net/mlx4_en: fix off by one in error handling

[Tariq Toukan]

On 14/09/2016 4:53 PM, Sebastian Ott wrote:

Hello Tariq,

On Wed, 14 Sep 2016, Tariq Toukan wrote:

On 14/09/2016 2:09 PM, Sebastian Ott wrote:

If an error occurs in mlx4_init_eq_table the index used in the
err_out_unmap label is one too big which results in a panic in
mlx4_free_eq. This patch fixes the index in the error path.

You are right, but your change below does not cover all cases.
The full solution looks like this:

@@ -1260,7 +1260,7 @@ int mlx4_init_eq_table(struct mlx4_dev *dev)
  eq);
 }
 if (err)
-   goto err_out_unmap;
+   goto err_out_unmap_excluded;

In this case a call to mlx4_create_eq failed. Do you really have to call
mlx4_free_eq for this index again?

We agree on this part, that's why here we should goto the _excluded_ label.
For all other parts, we should not exclude the eq in the highest index, 
and thus we goto the _non_excluded_ label.

  As far as I understood this code
mlx4_create_eq cleans up when it fails and thus there is no need for an
additional mlx4_free_eq call.

Regards,
Sebastian


Regards,
Tariq

Re: [PATCH] net/mlx4_en: fix off by one in error handling

[Tariq Toukan]

Hi Sebastian,

Thanks for this fix.

On 14/09/2016 2:09 PM, Sebastian Ott wrote:

If an error occurs in mlx4_init_eq_table the index used in the
err_out_unmap label is one too big which results in a panic in
mlx4_free_eq. This patch fixes the index in the error path.

You are right, but your change below does not cover all cases.
The full solution looks like this:

@@ -1260,7 +1260,7 @@ int mlx4_init_eq_table(struct mlx4_dev *dev)
 eq);
}
if (err)
-   goto err_out_unmap;
+   goto err_out_unmap_excluded;
}

if (dev->flags & MLX4_FLAG_MSI_X) {
@@ -1306,8 +1306,10 @@ int mlx4_init_eq_table(struct mlx4_dev *dev)
return 0;

 err_out_unmap:
-   while (i >= 0)
-   mlx4_free_eq(dev, >eq_table.eq[i--]);
+   mlx4_free_eq(dev, >eq_table.eq[i]);
+err_out_unmap_excluded:
+   while (i > 0)
+   mlx4_free_eq(dev, >eq_table.eq[--i]);
 #ifdef CONFIG_RFS_ACCEL
for (i = 1; i <= dev->caps.num_ports; i++) {
if (mlx4_priv(dev)->port[i].rmap) {




Signed-off-by: Sebastian Ott 
---
  drivers/net/ethernet/mellanox/mlx4/eq.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx4/eq.c 
b/drivers/net/ethernet/mellanox/mlx4/eq.c
index f613977..cf8f8a7 100644
--- a/drivers/net/ethernet/mellanox/mlx4/eq.c
+++ b/drivers/net/ethernet/mellanox/mlx4/eq.c
@@ -1305,8 +1305,8 @@ int mlx4_init_eq_table(struct mlx4_dev *dev)
return 0;
  
  err_out_unmap:

-   while (i >= 0)
-   mlx4_free_eq(dev, >eq_table.eq[i--]);
+   while (i > 0)
+   mlx4_free_eq(dev, >eq_table.eq[--i]);
  #ifdef CONFIG_RFS_ACCEL
for (i = 1; i <= dev->caps.num_ports; i++) {
if (mlx4_priv(dev)->port[i].rmap) {
You can choose to submit again, or we can take it from here. Whatever 
you prefer.


Regards,
Tariq

Re: [PATCH] net/mlx4_en: fix off by one in error handling

[Tariq Toukan]

Hi Sebastian,

Thanks for this fix.

On 14/09/2016 2:09 PM, Sebastian Ott wrote:

If an error occurs in mlx4_init_eq_table the index used in the
err_out_unmap label is one too big which results in a panic in
mlx4_free_eq. This patch fixes the index in the error path.

You are right, but your change below does not cover all cases.
The full solution looks like this:

@@ -1260,7 +1260,7 @@ int mlx4_init_eq_table(struct mlx4_dev *dev)
 eq);
}
if (err)
-   goto err_out_unmap;
+   goto err_out_unmap_excluded;
}

if (dev->flags & MLX4_FLAG_MSI_X) {
@@ -1306,8 +1306,10 @@ int mlx4_init_eq_table(struct mlx4_dev *dev)
return 0;

 err_out_unmap:
-   while (i >= 0)
-   mlx4_free_eq(dev, >eq_table.eq[i--]);
+   mlx4_free_eq(dev, >eq_table.eq[i]);
+err_out_unmap_excluded:
+   while (i > 0)
+   mlx4_free_eq(dev, >eq_table.eq[--i]);
 #ifdef CONFIG_RFS_ACCEL
for (i = 1; i <= dev->caps.num_ports; i++) {
if (mlx4_priv(dev)->port[i].rmap) {




Signed-off-by: Sebastian Ott 
---
  drivers/net/ethernet/mellanox/mlx4/eq.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx4/eq.c 
b/drivers/net/ethernet/mellanox/mlx4/eq.c
index f613977..cf8f8a7 100644
--- a/drivers/net/ethernet/mellanox/mlx4/eq.c
+++ b/drivers/net/ethernet/mellanox/mlx4/eq.c
@@ -1305,8 +1305,8 @@ int mlx4_init_eq_table(struct mlx4_dev *dev)
return 0;
  
  err_out_unmap:

-   while (i >= 0)
-   mlx4_free_eq(dev, >eq_table.eq[i--]);
+   while (i > 0)
+   mlx4_free_eq(dev, >eq_table.eq[--i]);
  #ifdef CONFIG_RFS_ACCEL
for (i = 1; i <= dev->caps.num_ports; i++) {
if (mlx4_priv(dev)->port[i].rmap) {
You can choose to submit again, or we can take it from here. Whatever 
you prefer.


Regards,
Tariq

Re: [PATCH] net/mlx4_en: fix off by one in error handling

[Sebastian Ott]

Hello Tariq,

On Wed, 14 Sep 2016, Tariq Toukan wrote:
> On 14/09/2016 2:09 PM, Sebastian Ott wrote:
> > If an error occurs in mlx4_init_eq_table the index used in the
> > err_out_unmap label is one too big which results in a panic in
> > mlx4_free_eq. This patch fixes the index in the error path.
> You are right, but your change below does not cover all cases.
> The full solution looks like this:
> 
> @@ -1260,7 +1260,7 @@ int mlx4_init_eq_table(struct mlx4_dev *dev)
>  eq);
> }
> if (err)
> -   goto err_out_unmap;
> +   goto err_out_unmap_excluded;

In this case a call to mlx4_create_eq failed. Do you really have to call
mlx4_free_eq for this index again? As far as I understood this code
mlx4_create_eq cleans up when it fails and thus there is no need for an
additional mlx4_free_eq call.

Regards,
Sebastian

Re: [PATCH] net/mlx4_en: fix off by one in error handling

[Sebastian Ott]

Hello Tariq,

On Wed, 14 Sep 2016, Tariq Toukan wrote:
> On 14/09/2016 2:09 PM, Sebastian Ott wrote:
> > If an error occurs in mlx4_init_eq_table the index used in the
> > err_out_unmap label is one too big which results in a panic in
> > mlx4_free_eq. This patch fixes the index in the error path.
> You are right, but your change below does not cover all cases.
> The full solution looks like this:
> 
> @@ -1260,7 +1260,7 @@ int mlx4_init_eq_table(struct mlx4_dev *dev)
>  eq);
> }
> if (err)
> -   goto err_out_unmap;
> +   goto err_out_unmap_excluded;

In this case a call to mlx4_create_eq failed. Do you really have to call
mlx4_free_eq for this index again? As far as I understood this code
mlx4_create_eq cleans up when it fails and thus there is no need for an
additional mlx4_free_eq call.

Regards,
Sebastian

[PATCH] net/mlx4_en: fix off by one in error handling

[Sebastian Ott]

If an error occurs in mlx4_init_eq_table the index used in the
err_out_unmap label is one too big which results in a panic in
mlx4_free_eq. This patch fixes the index in the error path.

Signed-off-by: Sebastian Ott 
---
 drivers/net/ethernet/mellanox/mlx4/eq.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx4/eq.c 
b/drivers/net/ethernet/mellanox/mlx4/eq.c
index f613977..cf8f8a7 100644
--- a/drivers/net/ethernet/mellanox/mlx4/eq.c
+++ b/drivers/net/ethernet/mellanox/mlx4/eq.c
@@ -1305,8 +1305,8 @@ int mlx4_init_eq_table(struct mlx4_dev *dev)
return 0;
 
 err_out_unmap:
-   while (i >= 0)
-   mlx4_free_eq(dev, >eq_table.eq[i--]);
+   while (i > 0)
+   mlx4_free_eq(dev, >eq_table.eq[--i]);
 #ifdef CONFIG_RFS_ACCEL
for (i = 1; i <= dev->caps.num_ports; i++) {
if (mlx4_priv(dev)->port[i].rmap) {
-- 
2.5.5

[PATCH] net/mlx4_en: fix off by one in error handling

[Sebastian Ott]

If an error occurs in mlx4_init_eq_table the index used in the
err_out_unmap label is one too big which results in a panic in
mlx4_free_eq. This patch fixes the index in the error path.

Signed-off-by: Sebastian Ott 
---
 drivers/net/ethernet/mellanox/mlx4/eq.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx4/eq.c 
b/drivers/net/ethernet/mellanox/mlx4/eq.c
index f613977..cf8f8a7 100644
--- a/drivers/net/ethernet/mellanox/mlx4/eq.c
+++ b/drivers/net/ethernet/mellanox/mlx4/eq.c
@@ -1305,8 +1305,8 @@ int mlx4_init_eq_table(struct mlx4_dev *dev)
return 0;
 
 err_out_unmap:
-   while (i >= 0)
-   mlx4_free_eq(dev, >eq_table.eq[i--]);
+   while (i > 0)
+   mlx4_free_eq(dev, >eq_table.eq[--i]);
 #ifdef CONFIG_RFS_ACCEL
for (i = 1; i <= dev->caps.num_ports; i++) {
if (mlx4_priv(dev)->port[i].rmap) {
-- 
2.5.5