netdev - Re: [PATCH net-next v3 09/12] net: ethernet: aquantia: Call netdev

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-Id: <20170220.112820.161041046228087344.davem@davemloft.net>
Date:   Mon, 20 Feb 2017 11:28:20 -0500 (EST)
From:   David Miller <davem@...emloft.net>
To:     Pavel.Belous@...antia.com
Cc:     netdev@...r.kernel.org, Simon.Edelhaus@...antia.com,
        Alexey.Andriyanov@...antia.com, LinoSanfilippo@....de
Subject: Re: [PATCH net-next v3 09/12] net: ethernet: aquantia: Call
 netdev_register after all initialized.

From: Pavel Belous <Pavel.Belous@...antia.com>
Date: Mon, 20 Feb 2017 14:41:51 +0300

> From: Pavel Belous <pavel.belous@...antia.com>
> 
> netdev_register should be called when everything is initialized.
> 
> Signed-off-by: Pavel Belous <pavel.belous@...antia.com>
> Reviewed-by: Lino Sanfilippo <LinoSanfilippo@....de>
> ---
>  drivers/net/ethernet/aquantia/atlantic/aq_nic.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
> index a8a27c5..fdd7d72 100644
> --- a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
> +++ b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
> @@ -261,16 +261,18 @@ int aq_nic_ndev_register(struct aq_nic_s *self)
>  		ether_addr_copy(self->ndev->dev_addr, mac_addr_permanent);
>  	}
>  #endif
> -	err = register_netdev(self->ndev);
> -	if (err < 0)
> -		goto err_exit;
>  
> -	self->is_ndev_registered = true;
>  	netif_carrier_off(self->ndev);
>  
>  	for (i = AQ_CFG_VECS_MAX; i--;)
>  		aq_nic_ndev_queue_stop(self, i);
>  
> +	err = register_netdev(self->ndev);
> +	if (err < 0)
> +		goto err_exit;
> +
> +	self->is_ndev_registered = true;
> +

Please get rid of this self->is_ndev_registered state, it is not
necessary and it is racey.

The very moment you call register_netdev(), your ->open() method
along with the rest of your driver can be invoked.

Therefore any dependency of state changes after register_netdev()
cannot be depended upon by the rest of the driver, code is going
to run immediately elsewhere in your driver as soon as that
register_netdev() is invoked, and it is going to test that
->is_ndev_registered() value, and it's going to be wrong.

Instead, test netdev->reg_state if you absolutely must have this
piece of information.