| Message ID | 1455048677-19882-4-git-send-email-linux@rasmusvillemoes.dk (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
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]); > } > -- 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, 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 -- 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, 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 -- 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
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 > -- 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 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.
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]); }
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(-)