net/mlx4_en: Fix a memory leak in case of error in 'mlx4_en_init_netdev()'

If 'kzalloc' fails, we must free some memory before returning.

Fixes: 67f8b1dcb9ee ("net/mlx4_en: Refactor the XDP forwarding rings scheme")
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
 drivers/net/ethernet/mellanox/mlx4/en_netdev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

On 12/03/2018 12:45 AM, Christophe JAILLET wrote:
> If 'kzalloc' fails, we must free some memory before returning.
> 
> Fixes: 67f8b1dcb9ee ("net/mlx4_en: Refactor the XDP forwarding rings scheme")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
>   drivers/net/ethernet/mellanox/mlx4/en_netdev.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
> index 8fc51bc29003..f9db018e858f 100644
> --- a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
> +++ b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
> @@ -3327,7 +3327,7 @@ int mlx4_en_init_netdev(struct mlx4_en_dev *mdev, int port,
>   		if (!priv->tx_cq[t]) {
>   			kfree(priv->tx_ring[t]);
>   			err = -ENOMEM;
> -			goto out;
> +			goto err_free_tx;
>   		}
>   	}
>   	priv->rx_ring_num = prof->rx_ring_num;
> 

Hi Christophe, thanks for spotting this.

However, I think these err_free_tx label and loop are redundant.
Both tx_ring/tx_cq flows should just goto out, as resources are freed 
later in mlx4_en_destroy_netdev() -> mlx4_en_free_resources().

Le 12/03/2018 à 09:42, Tariq Toukan a écrit :
 >
 >
 > On 12/03/2018 12:45 AM, Christophe JAILLET wrote:
 >> If 'kzalloc' fails, we must free some memory before returning.
 >>
 >> Fixes: 67f8b1dcb9ee ("net/mlx4_en: Refactor the XDP forwarding rings
 >> scheme")
 >> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
 >> ---
 >>   drivers/net/ethernet/mellanox/mlx4/en_netdev.c | 2 +-
 >>   1 file changed, 1 insertion(+), 1 deletion(-)
 >>
 >> diff --git a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
 >> b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
 >> index 8fc51bc29003..f9db018e858f 100644
 >> --- a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
 >> +++ b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
 >> @@ -3327,7 +3327,7 @@ int mlx4_en_init_netdev(struct mlx4_en_dev
 >> *mdev, int port,
 >>           if (!priv->tx_cq[t]) {
 >>               kfree(priv->tx_ring[t]);
 >>               err = -ENOMEM;
 >> -            goto out;
 >> +            goto err_free_tx;
 >>           }
 >>       }
 >>       priv->rx_ring_num = prof->rx_ring_num;
 >>
 >
 > Hi Christophe, thanks for spotting this.
 >
 > However, I think these err_free_tx label and loop are redundant.
 > Both tx_ring/tx_cq flows should just goto out, as resources are freed
 > later in mlx4_en_destroy_netdev() -> mlx4_en_free_resources().
 >

Hi,

I do not agree with you and I think that the patch is relevant.

If 'mlx4_en_init_netdev' fails, the only caller, 'mlx4_en_activate()', 
will set:
	mdev->pndev[i] = NULL
(see 
https://elixir.bootlin.com/linux/v4.16-rc5/source/drivers/net/ethernet/mellanox/mlx4/en_main.c#L254)

and 'mlx4_en_destroy_netdev()' is not called in this case.
(see 
https://elixir.bootlin.com/linux/v4.16-rc5/source/drivers/net/ethernet/mellanox/mlx4/en_main.c#L232)

My understanding is that 'mlx4_en_destroy_netdev()' will free resources 
in the normal case but that resources should be freed at allocation time 
if it does not fully succeed.

Best regards,
CJ

---
L'absence de virus dans ce courrier électronique a été vérifiée par le logiciel antivirus Avast.
https://www.avast.com/antivirus