* [PATCH] net/mlx4_en: fix off by one in error handling
@ 2016-09-14 11:09 Sebastian Ott
2016-09-14 13:43 ` Tariq Toukan
2016-09-16 8:16 ` David Miller
0 siblings, 2 replies; 7+ messages in thread
From: Sebastian Ott @ 2016-09-14 11:09 UTC (permalink / raw)
To: Yishai Hadas, Tariq Toukan; +Cc: netdev, linux-rdma, linux-kernel
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] net/mlx4_en: fix off by one in error handling
2016-09-14 11:09 [PATCH] net/mlx4_en: fix off by one in error handling Sebastian Ott
@ 2016-09-14 13:43 ` Tariq Toukan
2016-09-14 13:53 ` Sebastian Ott
2016-09-16 8:16 ` David Miller
1 sibling, 1 reply; 7+ messages in thread
From: Tariq Toukan @ 2016-09-14 13:43 UTC (permalink / raw)
To: Sebastian Ott, Yishai Hadas, Tariq Toukan
Cc: netdev, linux-rdma, linux-kernel
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] net/mlx4_en: fix off by one in error handling
2016-09-14 13:43 ` Tariq Toukan
@ 2016-09-14 13:53 ` Sebastian Ott
2016-09-14 14:49 ` Tariq Toukan
0 siblings, 1 reply; 7+ messages in thread
From: Sebastian Ott @ 2016-09-14 13:53 UTC (permalink / raw)
To: Tariq Toukan; +Cc: Yishai Hadas, Tariq Toukan, netdev, linux-rdma, linux-kernel
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] net/mlx4_en: fix off by one in error handling
2016-09-14 13:53 ` Sebastian Ott
@ 2016-09-14 14:49 ` Tariq Toukan
2016-09-14 16:08 ` Sebastian Ott
0 siblings, 1 reply; 7+ messages in thread
From: Tariq Toukan @ 2016-09-14 14:49 UTC (permalink / raw)
To: Sebastian Ott
Cc: Yishai Hadas, Tariq Toukan, netdev, linux-rdma, linux-kernel
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] net/mlx4_en: fix off by one in error handling
2016-09-14 14:49 ` Tariq Toukan
@ 2016-09-14 16:08 ` Sebastian Ott
2016-09-15 12:18 ` Tariq Toukan
0 siblings, 1 reply; 7+ messages in thread
From: Sebastian Ott @ 2016-09-14 16:08 UTC (permalink / raw)
To: Tariq Toukan; +Cc: Yishai Hadas, Tariq Toukan, netdev, linux-rdma, linux-kernel
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] net/mlx4_en: fix off by one in error handling
2016-09-14 16:08 ` Sebastian Ott
@ 2016-09-15 12:18 ` Tariq Toukan
0 siblings, 0 replies; 7+ messages in thread
From: Tariq Toukan @ 2016-09-15 12:18 UTC (permalink / raw)
To: Sebastian Ott
Cc: Yishai Hadas, Tariq Toukan, netdev, linux-rdma, linux-kernel
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!
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] net/mlx4_en: fix off by one in error handling
2016-09-14 11:09 [PATCH] net/mlx4_en: fix off by one in error handling Sebastian Ott
2016-09-14 13:43 ` Tariq Toukan
@ 2016-09-16 8:16 ` David Miller
1 sibling, 0 replies; 7+ messages in thread
From: David Miller @ 2016-09-16 8:16 UTC (permalink / raw)
To: sebott; +Cc: yishaih, tariqt, netdev, linux-rdma, linux-kernel
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2016-09-16 8:16 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-09-14 11:09 [PATCH] net/mlx4_en: fix off by one in error handling Sebastian Ott
2016-09-14 13:43 ` Tariq Toukan
2016-09-14 13:53 ` Sebastian Ott
2016-09-14 14:49 ` Tariq Toukan
2016-09-14 16:08 ` Sebastian Ott
2016-09-15 12:18 ` Tariq Toukan
2016-09-16 8:16 ` David Miller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).