| Message ID | alpine.LFD.2.20.1609141305360.1727@schleppi (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
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 -- 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
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 -- 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
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 -- 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
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 -- 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
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! -- 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
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. -- 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
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) {
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(-)