netdev - Re: [PATCH net-next v2 7/8] docs: netdev: break down the instance locking info per ops struct

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: <119bf05d-17c6-4327-a79b-31e3e2838abe@intel.com>
Date: Wed, 9 Apr 2025 23:01:18 -0700
From: Jacob Keller <jacob.e.keller@...el.com>
To: Jakub Kicinski <kuba@...nel.org>, <davem@...emloft.net>
CC: <netdev@...r.kernel.org>, <edumazet@...gle.com>, <pabeni@...hat.com>,
	<andrew+netdev@...n.ch>, <horms@...nel.org>, <sdf@...ichev.me>,
	<hramamurthy@...gle.com>, <kuniyu@...zon.com>, <jdamato@...tly.com>
Subject: Re: [PATCH net-next v2 7/8] docs: netdev: break down the instance
 locking info per ops struct



On 4/8/2025 12:59 PM, Jakub Kicinski wrote:
> Explicitly list all the ops structs and what locking they provide.
> Use "ops locked" as a term for drivers which have ops called under
> the instance lock.
> 
> Acked-by: Stanislav Fomichev <sdf@...ichev.me>
> Signed-off-by: Jakub Kicinski <kuba@...nel.org>
> ---
> v2:
>  - an exception -> exceptions
> v1: https://lore.kernel.org/20250407190117.16528-8-kuba@kernel.org
> ---
>  Documentation/networking/netdevices.rst | 54 +++++++++++++++++++------
>  1 file changed, 42 insertions(+), 12 deletions(-)
> 
> diff --git a/Documentation/networking/netdevices.rst b/Documentation/networking/netdevices.rst
> index d6357472d3f1..7ae28c5fb835 100644
> --- a/Documentation/networking/netdevices.rst
> +++ b/Documentation/networking/netdevices.rst
> @@ -314,13 +314,8 @@ struct napi_struct synchronization rules
>  		 softirq
>  		 will be called with interrupts disabled by netconsole.
>  
> -struct netdev_queue_mgmt_ops synchronization rules
> -==================================================
> -
> -All queue management ndo callbacks are holding netdev instance lock.
> -
> -RTNL and netdev instance lock
> -=============================
> +netdev instance lock
> +====================
>  
>  Historically, all networking control operations were protected by a single
>  global lock known as ``rtnl_lock``. There is an ongoing effort to replace this
> @@ -328,10 +323,13 @@ global lock with separate locks for each network namespace. Additionally,
>  properties of individual netdev are increasingly protected by per-netdev locks.
>  
>  For device drivers that implement shaping or queue management APIs, all control
> -operations will be performed under the netdev instance lock. Currently, this
> -instance lock is acquired within the context of ``rtnl_lock``. The drivers
> -can also explicitly request instance lock to be acquired via
> -``request_ops_lock``. In the future, there will be an option for individual
> +operations will be performed under the netdev instance lock.
> +Drivers can also explicitly request instance lock to be held during ops
> +by setting ``request_ops_lock`` to true. Code comments and docs refer
> +to drivers which have ops called under the instance lock as "ops locked".
> +See also the documentation of the ``lock`` member of struct net_device.
> +
> +In the future, there will be an option for individual
>  drivers to opt out of using ``rtnl_lock`` and instead perform their control
>  operations directly under the netdev instance lock.
>  
> @@ -343,8 +341,40 @@ there are two sets of interfaces: ``dev_xxx`` and ``netif_xxx`` (e.g.,
>  acquiring the instance lock themselves, while the ``netif_xxx`` functions
>  assume that the driver has already acquired the instance lock.
>  
> +struct net_device_ops
> +---------------------
> +
> +``ndos`` are called without holding the instance lock for most drivers.
> +
> +"Ops locked" drivers will have most of the ``ndos`` invoked under
> +the instance lock.
> +
> +struct ethtool_ops
> +------------------
> +
> +Similarly to ``ndos`` the instance lock is only held for select drivers.
> +For "ops locked" drivers all ethtool ops without exceptions should
> +be called under the instance lock.
> +
> +struct net_shaper_ops
> +---------------------
> +
> +All net shaper callbacks are invoked while holding the netdev instance
> +lock. ``rtnl_lock`` may or may not be held.
> +
> +Note that supporting net shapers automatically enables "ops locking".
> +
> +struct netdev_queue_mgmt_ops
> +----------------------------
> +
> +All queue management callbacks are invoked while holding the netdev instance
> +lock. ``rtnl_lock`` may or may not be held.
> +
> +Note that supporting struct netdev_queue_mgmt_ops automatically enables
> +"ops locking".
> +

Does this mean we don't allow drivers which support
netdev_queue_mgmt_ops but don't set request_ops_lock? Or does it mean
that supporting netdev_queue_mgmt_ops and/or netdev shapers
automatically implies request_ops_lock? Or is there some other
behavioral difference?

>From the wording this sounds like its enforced via code, and it seems
reasonable to me that we wouldn't allow these without setting
request_ops_lock to true...