netdev - Re: [PATCH net-next v2 02/11] net: make netdev_lock() protect netdev->reg

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250115083023.31347-1-kuniyu@amazon.com>
Date: Wed, 15 Jan 2025 17:30:23 +0900
From: Kuniyuki Iwashima <kuniyu@...zon.com>
To: <kuba@...nel.org>
CC: <andrew+netdev@...n.ch>, <davem@...emloft.net>, <edumazet@...gle.com>,
	<horms@...nel.org>, <jdamato@...tly.com>, <netdev@...r.kernel.org>,
	<pabeni@...hat.com>, Kuniyuki Iwashima <kuniyu@...zon.com>
Subject: Re: [PATCH net-next v2 02/11] net: make netdev_lock() protect netdev->reg_state

From: Jakub Kicinski <kuba@...nel.org>
Date: Tue, 14 Jan 2025 19:53:10 -0800
> Protect writes to netdev->reg_state with netdev_lock().
> From now on holding netdev_lock() is sufficient to prevent
> the net_device from getting unregistered, so code which
> wants to hold just a single netdev around no longer needs
> to hold rtnl_lock.
> 
> We do not protect the NETREG_UNREGISTERED -> NETREG_RELEASED
> transition. We'd need to move mutex_destroy(netdev->lock)
> to .release, but the real reason is that trying to stop
> the unregistration process mid-way would be unsafe / crazy.
> Taking references on such devices is not safe, either.
> So the intended semantics are to lock REGISTERED devices.
> 
> Reviewed-by: Joe Damato <jdamato@...tly.com>
> Reviewed-by: Eric Dumazet <edumazet@...gle.com>
> Signed-off-by: Jakub Kicinski <kuba@...nel.org>
> ---
> v2:
>  - reorder with next patch
> v1: https://lore.kernel.org/20250114035118.110297-4-kuba@kernel.org
> ---
>  include/linux/netdevice.h | 2 +-
>  net/core/dev.c            | 6 ++++++
>  2 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 891c5bdb894c..30963c5d409b 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -2448,7 +2448,7 @@ struct net_device {
>  	 * Should always be taken using netdev_lock() / netdev_unlock() helpers.
>  	 * Drivers are free to use it for other protection.
>  	 *
> -	 * Protects: @net_shaper_hierarchy.
> +	 * Protects: @reg_state, @net_shaper_hierarchy.
>  	 * Ordering: take after rtnl_lock.
>  	 */
>  	struct mutex		lock;
> diff --git a/net/core/dev.c b/net/core/dev.c
> index fda4e1039bf0..6603c08768f6 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -10668,7 +10668,9 @@ int register_netdevice(struct net_device *dev)
>  
>  	ret = netdev_register_kobject(dev);
>  
> +	netdev_lock(dev);
>  	WRITE_ONCE(dev->reg_state, ret ? NETREG_UNREGISTERED : NETREG_REGISTERED);
> +	netdev_unlock(dev);

Do we need the lock before list_netdevice() ?

It's not a big deal, so

Reviewed-by: Kuniyuki Iwashima <kuniyu@...zon.com>