netdev - Re: [PATCH net-next v2] netdev: fix the locking for netdev notifications

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 for Android: free password hash cracker in your pocket

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

Message-ID: <Z_2yMjGtbQ0ehtDN@mini-arch>
Date: Mon, 14 Apr 2025 18:11:14 -0700
From: Stanislav Fomichev <stfomichev@...il.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: davem@...emloft.net, netdev@...r.kernel.org, edumazet@...gle.com,
	pabeni@...hat.com, andrew+netdev@...n.ch, horms@...nel.org,
	syzkaller <syzkaller@...glegroups.com>,
	Kuniyuki Iwashima <kuniyu@...zon.com>, sdf@...ichev.me,
	jdamato@...tly.com, almasrymina@...gle.com
Subject: Re: [PATCH net-next v2] netdev: fix the locking for netdev
 notifications

On 04/14, Jakub Kicinski wrote:
> Kuniyuki reports that the assert for netdev lock fires when
> there are netdev event listeners (otherwise we skip the netlink
> event generation).
> 
> Correct the locking when coming from the notifier.
> 
> The NETDEV_XDP_FEAT_CHANGE notifier is already fully locked,
> it's the documentation that's incorrect.
> 
> Fixes: 99e44f39a8f7 ("netdev: depend on netdev->lock for xdp features")
> Reported-by: syzkaller <syzkaller@...glegroups.com>
> Reported-by: Kuniyuki Iwashima <kuniyu@...zon.com>
> Link: https://lore.kernel.org/20250410171019.62128-1-kuniyu@amazon.com
> Signed-off-by: Jakub Kicinski <kuba@...nel.org>

Acked-by: Stanislav Fomichev <sdf@...ichev.me>

> ---
> v2:
>  - rebase vs net merge which brought in
>    commit 04efcee6ef8d ("net: hold instance lock during NETDEV_CHANGE")
> v1: https://lore.kernel.org/20250411204629.128669-1-kuba@kernel.org
> 
> CC: kuniyu@...zon.com
> CC: sdf@...ichev.me
> CC: jdamato@...tly.com
> CC: almasrymina@...gle.com
> ---
>  Documentation/networking/netdevices.rst |  4 +++-
>  include/linux/netdevice.h               |  2 +-
>  include/net/netdev_lock.h               | 12 ++++++++++++
>  net/core/lock_debug.c                   |  4 +++-
>  net/core/netdev-genl.c                  |  4 ++++
>  5 files changed, 23 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/networking/netdevices.rst b/Documentation/networking/netdevices.rst
> index f87bb55b4afe..a73a39b206e3 100644
> --- a/Documentation/networking/netdevices.rst
> +++ b/Documentation/networking/netdevices.rst
> @@ -387,12 +387,14 @@ For device drivers that implement shaping or queue management APIs,
>  some of the notifiers (``enum netdev_cmd``) are running under the netdev
>  instance lock.
>  
> +The following netdev notifiers are always run under the instance lock:
> +* ``NETDEV_XDP_FEAT_CHANGE``
> +
>  For devices with locked ops, currently only the following notifiers are
>  running under the lock:
>  * ``NETDEV_CHANGE``
>  * ``NETDEV_REGISTER``
>  * ``NETDEV_UP``
> -* ``NETDEV_XDP_FEAT_CHANGE``
>  
>  The following notifiers are running without the lock:
>  * ``NETDEV_UNREGISTER``
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index e6036b82ef4c..0321fd952f70 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -2520,7 +2520,7 @@ struct net_device {
>  	 *	@net_shaper_hierarchy, @reg_state, @threaded
>  	 *
>  	 * Double protects:
> -	 *	@up, @moving_ns, @nd_net, @xdp_flags
> +	 *	@up, @moving_ns, @nd_net, @xdp_features
>  	 *
>  	 * Double ops protects:
>  	 *	@real_num_rx_queues, @real_num_tx_queues
> diff --git a/include/net/netdev_lock.h b/include/net/netdev_lock.h
> index 5706835a660c..c63448b17f9e 100644
> --- a/include/net/netdev_lock.h
> +++ b/include/net/netdev_lock.h
> @@ -48,6 +48,18 @@ static inline void netdev_unlock_ops(struct net_device *dev)
>  		netdev_unlock(dev);
>  }
>  
> +static inline void netdev_lock_ops_to_full(struct net_device *dev)
> +{
> +	if (!netdev_need_ops_lock(dev))
> +		netdev_lock(dev);

Optional nit: I'm getting lost in all the helpers, I'd add the following here:

else
	netdev_ops_assert_locked(dev);

Or maybe even:

if (netdev_need_ops_lock)
	netdev_ops_assert_locked
else
	netdev_lock

To express the constraints better.