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

[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 <sebott@linux.vnet.ibm.com>
---
 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, &priv->eq_table.eq[i--]);
+	while (i > 0)
+		mlx4_free_eq(dev, &priv->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

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, &priv->eq_table.eq[i--]);
+       mlx4_free_eq(dev, &priv->eq_table.eq[i]);
+err_out_unmap_excluded:
+       while (i > 0)
+               mlx4_free_eq(dev, &priv->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 <sebott@linux.vnet.ibm.com>
> ---
>   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, &priv->eq_table.eq[i--]);
> +	while (i > 0)
> +		mlx4_free_eq(dev, &priv->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

[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

[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 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 <tariqt@mellanox.com>

Thanks!

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

[David Miller]

From: Sebastian Ott <sebott@linux.vnet.ibm.com>
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 <sebott@linux.vnet.ibm.com>

Applied.