On Wed, 10 Feb 2016 19:15:20 +0100
Rasmus Villemoes <li...@rasmusvillemoes.dk> wrote:
> On Wed, Feb 10 2016, Yishai Hadas <yish...@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 majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
