[PATCH net-next] net: sysfs: use ops lock for speed and duplex

[PATCH net-next] net: sysfs: use ops lock for speed and duplex

[Wang Zhan]

Reading /sys/class/net/<dev>/{speed,duplex} takes rtnl_lock() before
calling __ethtool_get_link_ksettings(). For ops-locked devices whose
callbacks do not require RTNL, this unnecessarily serializes sysfs
readers with rtnetlink users. On CPU-throttled hosts, a periodic reader
such as node-exporter can keep RTNL held for hundreds of milliseconds
while an mlx5 callback runs.

Ops-locked devices can run ethtool operations under the netdev instance
lock. The ethtool netlink and ioctl paths use
ETHTOOL_OP_NEEDS_RTNL_LINKSETTINGS to identify callbacks that still need
RTNL, but the sysfs path currently does not. Apply the same rule to sysfs
reads: call netif_get_link_ksettings() under netdev_lock_ops() when the
callback does not require RTNL, and keep sysfs_rtnl_lock() for legacy
devices and callbacks that opt in to RTNL.

Preserve the existing speed and duplex sysfs ABI, including -EINVAL for
devices that are down or callbacks that fail.

Assisted-by: LLM
Signed-off-by: Wang Zhan <wang.zhan@smartx.com>
---
 net/core/net-sysfs.c | 122 +++++++++++++++++++++++++------------------
 1 file changed, 71 insertions(+), 51 deletions(-)

diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
index 352173df75785..3e01e7f3cac79 100644
--- a/net/core/net-sysfs.c
+++ b/net/core/net-sysfs.c
@@ -118,6 +118,55 @@ static int sysfs_rtnl_lock(struct kobject *kobj, struct attribute *attr,
 	return ret;
 }
 
+/*
+ * Use the per-device ops lock without RTNL when the device supports it.
+ * Legacy devices and callbacks which explicitly require RTNL retain the
+ * existing locking.
+ */
+static int sysfs_get_link_ksettings(struct device *dev,
+				    struct device_attribute *attr,
+				    struct ethtool_link_ksettings *cmd)
+{
+	struct net_device *netdev = to_net_dev(dev);
+	int ret = -EINVAL;
+	bool need_rtnl;
+
+	/*
+	 * The check is also done in netif_get_link_ksettings(); this helps
+	 * returning early without hitting the locking section below.
+	 */
+	if (!netdev->ethtool_ops->get_link_ksettings)
+		return ret;
+
+	need_rtnl = !netdev_need_ops_lock(netdev) ||
+		    (netdev->ethtool_ops->op_needs_rtnl &
+		     ETHTOOL_OP_NEEDS_RTNL_LINKSETTINGS);
+	if (need_rtnl) {
+		ret = sysfs_rtnl_lock(&dev->kobj, &attr->attr, netdev);
+		if (ret)
+			return ret;
+	}
+	netdev_lock_ops(netdev);
+
+	ret = -EINVAL;
+	if (!dev_isalive(netdev)) {
+		ret = -ENODEV;
+	} else if (netif_running(netdev)) {
+		/*
+		 * Keep the sysfs ABI: report any callback error as -EINVAL
+		 * instead of propagating the driver's error code.
+		 */
+		if (!netif_get_link_ksettings(netdev, cmd))
+			ret = 0;
+	}
+
+	netdev_unlock_ops(netdev);
+	if (need_rtnl)
+		rtnl_unlock();
+
+	return ret;
+}
+
 /* use same locking rules as GIF* ioctl's */
 static ssize_t netdev_show(const struct device *dev,
 			   struct device_attribute *attr, char *buf,
@@ -332,27 +381,13 @@ static DEVICE_ATTR_RW(carrier);
 static ssize_t speed_show(struct device *dev,
 			  struct device_attribute *attr, char *buf)
 {
-	struct net_device *netdev = to_net_dev(dev);
-	int ret = -EINVAL;
-
-	/* The check is also done in __ethtool_get_link_ksettings; this helps
-	 * returning early without hitting the locking section below.
-	 */
-	if (!netdev->ethtool_ops->get_link_ksettings)
-		return ret;
-
-	ret = sysfs_rtnl_lock(&dev->kobj, &attr->attr, netdev);
-	if (ret)
-		return ret;
+	struct ethtool_link_ksettings cmd;
+	int ret;
 
-	ret = -EINVAL;
-	if (netif_running(netdev)) {
-		struct ethtool_link_ksettings cmd;
+	ret = sysfs_get_link_ksettings(dev, attr, &cmd);
+	if (!ret)
+		ret = sysfs_emit(buf, fmt_dec, cmd.base.speed);
 
-		if (!__ethtool_get_link_ksettings(netdev, &cmd))
-			ret = sysfs_emit(buf, fmt_dec, cmd.base.speed);
-	}
-	rtnl_unlock();
 	return ret;
 }
 static DEVICE_ATTR_RO(speed);
@@ -360,41 +395,26 @@ static DEVICE_ATTR_RO(speed);
 static ssize_t duplex_show(struct device *dev,
 			   struct device_attribute *attr, char *buf)
 {
-	struct net_device *netdev = to_net_dev(dev);
-	int ret = -EINVAL;
-
-	/* The check is also done in __ethtool_get_link_ksettings; this helps
-	 * returning early without hitting the locking section below.
-	 */
-	if (!netdev->ethtool_ops->get_link_ksettings)
-		return ret;
-
-	ret = sysfs_rtnl_lock(&dev->kobj, &attr->attr, netdev);
-	if (ret)
-		return ret;
-
-	ret = -EINVAL;
-	if (netif_running(netdev)) {
-		struct ethtool_link_ksettings cmd;
-
-		if (!__ethtool_get_link_ksettings(netdev, &cmd)) {
-			const char *duplex;
+	struct ethtool_link_ksettings cmd;
+	const char *duplex;
+	int ret;
 
-			switch (cmd.base.duplex) {
-			case DUPLEX_HALF:
-				duplex = "half";
-				break;
-			case DUPLEX_FULL:
-				duplex = "full";
-				break;
-			default:
-				duplex = "unknown";
-				break;
-			}
-			ret = sysfs_emit(buf, "%s\n", duplex);
+	ret = sysfs_get_link_ksettings(dev, attr, &cmd);
+	if (!ret) {
+		switch (cmd.base.duplex) {
+		case DUPLEX_HALF:
+			duplex = "half";
+			break;
+		case DUPLEX_FULL:
+			duplex = "full";
+			break;
+		default:
+			duplex = "unknown";
+			break;
 		}
+		ret = sysfs_emit(buf, "%s\n", duplex);
 	}
-	rtnl_unlock();
+
 	return ret;
 }
 static DEVICE_ATTR_RO(duplex);

base-commit: 1bb784eb6e38fd73143f021608e4ef3095d0c0d7
-- 
2.47.3

Re: [net-next] net: sysfs: use ops lock for speed and duplex

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 1 · Low: 0

- [Medium] In the new non-RTNL fast path of sysfs_get_link_ksettings()
  (net/core/net-sysfs.c:141-149), a read of…

--

net: sysfs: use ops lock for speed and duplex

This moves the speed and duplex sysfs reads off rtnl_lock() for ops-locked
devices whose get_link_ksettings callback does not set
ETHTOOL_OP_NEEDS_RTNL_LINKSETTINGS, taking the netdev instance lock
instead.  The new helper sysfs_get_link_ksettings() keeps sysfs_rtnl_lock()
for legacy devices and for callbacks that opt into RTNL.

> diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
> index 352173df75785..3e01e7f3cac79 100644
> --- a/net/core/net-sysfs.c
> +++ b/net/core/net-sysfs.c
> @@ -118,6 +118,55 @@ static int sysfs_rtnl_lock(struct kobject *kobj, struct attribute *attr,
>  	return ret;
>  }
>  
> +/*
> + * Use the per-device ops lock without RTNL when the device supports it.
> + * Legacy devices and callbacks which explicitly require RTNL retain the
> + * existing locking.
> + */
> +static int sysfs_get_link_ksettings(struct device *dev,
> +				    struct device_attribute *attr,
> +				    struct ethtool_link_ksettings *cmd)
> +{
> +	struct net_device *netdev = to_net_dev(dev);
> +	int ret = -EINVAL;
> +	bool need_rtnl;
> +
> +	/*
> +	 * The check is also done in netif_get_link_ksettings(); this helps
> +	 * returning early without hitting the locking section below.
> +	 */
> +	if (!netdev->ethtool_ops->get_link_ksettings)
> +		return ret;
> +
> +	need_rtnl = !netdev_need_ops_lock(netdev) ||
> +		    (netdev->ethtool_ops->op_needs_rtnl &
> +		     ETHTOOL_OP_NEEDS_RTNL_LINKSETTINGS);
> +	if (need_rtnl) {
> +		ret = sysfs_rtnl_lock(&dev->kobj, &attr->attr, netdev);
> +		if (ret)
> +			return ret;
> +	}
> +	netdev_lock_ops(netdev);

[Severity: Medium]
When need_rtnl is false, sysfs_get_link_ksettings() skips sysfs_rtnl_lock()
and therefore also skips the dev_hold() and
sysfs_break_active_protection() that it performs, then sleeps in
netdev_lock_ops() -> mutex_lock(&dev->lock) while the kernfs active
reference on the speed/duplex attribute is still held.

Can that delay the unregistration path?  unregister_netdevice_many_notify()
runs with rtnl_lock held (ASSERT_RTNL at its entry) and reaches:

net/core/dev.c:unregister_netdevice_many_notify() {
	...
		/* Remove entries from kobject tree */
		netdev_unregister_kobject(dev);
	...
}

device_del() -> kernfs_drain() on the speed node then waits for the sysfs
reader to return, so RTNL stays held for as long as some other thread holds
the instance lock, for example an mlx5 channel or ring reconfiguration.
That is the same multi-hundred-millisecond work the commit message wants to
keep out of RTNL.  Before this patch the reader had already broken active
protection before blocking, so it could not hold up the drain.

The second half is the interruptibility change.  sysfs_rtnl_lock() waits
like this:

net/core/net-sysfs.c:sysfs_rtnl_lock() {
	...
	if (rtnl_lock_interruptible()) {
		ret = -ERESTARTSYS;
		goto unbreak;
	}
	...
}

mutex_lock(&dev->lock) in netdev_lock_ops() has no interruptible variant
here, so a world-readable poller of /sys/class/net/<dev>/speed can end up
uninterruptible in D state for the duration of a driver reconfiguration.
Would it make sense to keep sysfs_break_active_protection() and dev_hold()
around the instance-lock acquisition, or to use netdev_trylock() with a
retry that drops the active reference first, so sysfs readers still cannot
stall the rtnl-holding teardown?

For context, netdev_lock_store() already takes netdev_lock() from a sysfs
handler without breaking active protection, so the pattern itself is not
new; the difference here is that it now applies to a world-readable
attribute that monitoring agents poll continuously rather than to a rare
CAP_NET_ADMIN write.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260831080623.1064001-1-wang.zhan%40smartx.com

Re: [PATCH net-next] net: sysfs: use ops lock for speed and duplex

[Jakub Kicinski]

On Mon, 31 Aug 2026 16:06:23 +0800 Wang Zhan wrote:
> Reading /sys/class/net/<dev>/{speed,duplex} takes rtnl_lock() before
> calling __ethtool_get_link_ksettings(). For ops-locked devices whose
> callbacks do not require RTNL, this unnecessarily serializes sysfs
> readers with rtnetlink users. On CPU-throttled hosts, a periodic reader
> such as node-exporter can keep RTNL held for hundreds of milliseconds
> while an mlx5 callback runs.
> 
> Ops-locked devices can run ethtool operations under the netdev instance
> lock. The ethtool netlink and ioctl paths use
> ETHTOOL_OP_NEEDS_RTNL_LINKSETTINGS to identify callbacks that still need
> RTNL, but the sysfs path currently does not. Apply the same rule to sysfs
> reads: call netif_get_link_ksettings() under netdev_lock_ops() when the
> callback does not require RTNL, and keep sysfs_rtnl_lock() for legacy
> devices and callbacks that opt in to RTNL.
> 
> Preserve the existing speed and duplex sysfs ABI, including -EINVAL for
> devices that are down or callbacks that fail.
> 
> Assisted-by: LLM

The diff is too ugly to look at. Learn how to break up your changes to
make them reviewable.
-- 
pw-bot: cr
pv-bot: slop