[PATCH net-next] net: ethtool: don't take rtnl_lock for global string dump

[PATCH net-next] net: ethtool: don't take rtnl_lock for global string dump

[Jakub Kicinski]

ETHTOOL_MSG_STRSET_GET is the only op which sets allow_nodev_do.
When no device is provided it dumps static tables, there's no
need to hold rtnl_lock for this.

Not taking rtnl_lock is a minor win in itself so I think this
patch stands on its own merits. Later on it will be useful
to do locking only in paths which have access to a netdev,
so that we can decide which locks to take per-netdev.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: andrew@lunn.ch
CC: maxime.chevallier@bootlin.com
---
 net/ethtool/netlink.h |  4 +++-
 net/ethtool/netlink.c | 10 ++++++----
 2 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/net/ethtool/netlink.h b/net/ethtool/netlink.h
index aaf6f2468768..b31de00fe683 100644
--- a/net/ethtool/netlink.h
+++ b/net/ethtool/netlink.h
@@ -333,7 +333,9 @@ int ethnl_sock_priv_set(struct sk_buff *skb, struct net_device *dev, u32 portid,
  * @hdr_attr:         attribute type for request header
  * @req_info_size:    size of request info
  * @reply_data_size:  size of reply data
- * @allow_nodev_do:   allow non-dump request with no device identification
+ * @allow_nodev_do:
+ *	Allow non-dump request with no device identification.
+ *	Note that locks (rtnl_lock etc.) are only taken if device is set.
  * @set_ntf_cmd:      notification to generate on changes (SET)
  * @parse_request:
  *	Parse request except common header (struct ethnl_req_info). Common
diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
index 5046023a30b1..a42bdd5ded51 100644
--- a/net/ethtool/netlink.c
+++ b/net/ethtool/netlink.c
@@ -526,13 +526,15 @@ static int ethnl_default_doit(struct sk_buff *skb, struct genl_info *info)
 		goto err_free;
 	ethnl_init_reply_data(reply_data, ops, req_info->dev);
 
-	rtnl_lock();
-	if (req_info->dev)
+	if (req_info->dev) {
+		rtnl_lock();
 		netdev_lock_ops(req_info->dev);
+	}
 	ret = ops->prepare_data(req_info, reply_data, info);
-	if (req_info->dev)
+	if (req_info->dev) {
 		netdev_unlock_ops(req_info->dev);
-	rtnl_unlock();
+		rtnl_unlock();
+	}
 	if (ret < 0)
 		goto err_dev;
 	ret = ops->reply_size(req_info, reply_data);
-- 
2.54.0

Re: [PATCH net-next] net: ethtool: don't take rtnl_lock for global string dump

[Vadim Fedorenko]

On 27/05/2026 17:25, Jakub Kicinski wrote:
> ETHTOOL_MSG_STRSET_GET is the only op which sets allow_nodev_do.
> When no device is provided it dumps static tables, there's no
> need to hold rtnl_lock for this.
> 
> Not taking rtnl_lock is a minor win in itself so I think this
> patch stands on its own merits. Later on it will be useful
> to do locking only in paths which have access to a netdev,
> so that we can decide which locks to take per-netdev.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
> CC: andrew@lunn.ch
> CC: maxime.chevallier@bootlin.com
> ---
>   net/ethtool/netlink.h |  4 +++-
>   net/ethtool/netlink.c | 10 ++++++----
>   2 files changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/net/ethtool/netlink.h b/net/ethtool/netlink.h
> index aaf6f2468768..b31de00fe683 100644
> --- a/net/ethtool/netlink.h
> +++ b/net/ethtool/netlink.h
> @@ -333,7 +333,9 @@ int ethnl_sock_priv_set(struct sk_buff *skb, struct net_device *dev, u32 portid,
>    * @hdr_attr:         attribute type for request header
>    * @req_info_size:    size of request info
>    * @reply_data_size:  size of reply data
> - * @allow_nodev_do:   allow non-dump request with no device identification
> + * @allow_nodev_do:
> + *	Allow non-dump request with no device identification.
> + *	Note that locks (rtnl_lock etc.) are only taken if device is set.
>    * @set_ntf_cmd:      notification to generate on changes (SET)
>    * @parse_request:
>    *	Parse request except common header (struct ethnl_req_info). Common
> diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
> index 5046023a30b1..a42bdd5ded51 100644
> --- a/net/ethtool/netlink.c
> +++ b/net/ethtool/netlink.c
> @@ -526,13 +526,15 @@ static int ethnl_default_doit(struct sk_buff *skb, struct genl_info *info)
>   		goto err_free;
>   	ethnl_init_reply_data(reply_data, ops, req_info->dev);
>   
> -	rtnl_lock();
> -	if (req_info->dev)
> +	if (req_info->dev) {
> +		rtnl_lock();
>   		netdev_lock_ops(req_info->dev);
> +	}
>   	ret = ops->prepare_data(req_info, reply_data, info);
> -	if (req_info->dev)
> +	if (req_info->dev) {
>   		netdev_unlock_ops(req_info->dev);
> -	rtnl_unlock();
> +		rtnl_unlock();
> +	}
>   	if (ret < 0)
>   		goto err_dev;
>   	ret = ops->reply_size(req_info, reply_data);

Reviewed-by: Vadim Fedorenko <vadim.fedorenko@linux.dev>