[3/5] net/mlx4: fix some error handling in mlx4_multi_func_init()

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(-)

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]);
>   	}
>

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

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

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
>

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.