[PATCH net-next 13/14] net: ethtool: optionally skip rtnl_lock on IOCTL path
From: Jakub Kicinski <kuba@kernel.org>
Date: 2026-05-28 23:17:00
Subsystem:
networking [ethtool], networking [general], the rest · Maintainers:
Andrew Lunn, Jakub Kicinski, "David S. Miller", Eric Dumazet, Paolo Abeni, Linus Torvalds
Convert the IOCTL path similarly to how we converted Netlink. The device lookup gets a little hairy. We could take rtnl_lock unconditionally and drop it before calling the driver (this would avoid the reference + liveness check). But I think being able to make progress even if rtnl is dead-locked is quite useful. First extra concern is handling features. List all the cmds which modify features and always take rtnl_lock. We could fold this list into ethtool_ioctl_needs_rtnl() but seems cleaner to keep ethtool_ioctl_needs_rtnl() driver-related. If a driver changed features and we were not holding rtnl_lock - warn about it. It can only happen on buggy ops locked drivers (buggy because they should have set appropriate "I need rtnl for op X" bit). Second wrinkle is the PHY ID hack which drops the locks while sleeping. Convert its static "busy" variable which used to be protected by rtnl_lock to a field in struct ethtool_netdev_state. This feature is about identifying an adapter or a port within a system, so being able to blink multiple LEDs at the same time is likely not very useful in practice. But it's the simplest fix, we can add a mutex if someone thinks a system should only be ID'ing one port at a time. Signed-off-by: Jakub Kicinski <kuba@kernel.org> --- include/linux/ethtool.h | 2 + net/ethtool/ioctl.c | 98 ++++++++++++++++++++++++++++++----------- 2 files changed, 74 insertions(+), 26 deletions(-)
diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 4f15221119e2..35ee57a0e5fa 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h@@ -1375,6 +1375,7 @@ int ethtool_virtdev_set_link_ksettings(struct net_device *dev, * within RTNL. * @rss_indir_user_size: Number of user provided entries for the default * (context 0) indirection table. + * @phys_id_busy: Loop blinking the device LED is running. * @wol_enabled: Wake-on-LAN is enabled * @module_fw_flash_in_progress: Module firmware flashing is in progress. */
@@ -1382,6 +1383,7 @@ struct ethtool_netdev_state { struct xarray rss_ctx; struct mutex rss_lock; u32 rss_indir_user_size; + unsigned phys_id_busy:1; unsigned wol_enabled:1; unsigned module_fw_flash_in_progress:1; };
diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
index 6c3a7e8644ae..aea087d62fe9 100644
--- a/net/ethtool/ioctl.c
+++ b/net/ethtool/ioctl.c@@ -543,7 +543,7 @@ static int ethtool_get_link_ksettings(struct net_device *dev, int err = 0; struct ethtool_link_ksettings link_ksettings; - ASSERT_RTNL(); + netdev_ops_assert_locked(dev); if (!dev->ethtool_ops->get_link_ksettings) return -EOPNOTSUPP;
@@ -600,7 +600,7 @@ static int ethtool_set_link_ksettings(struct net_device *dev, struct ethtool_link_ksettings link_ksettings = {}; int err; - ASSERT_RTNL(); + netdev_ops_assert_locked(dev); if (!dev->ethtool_ops->set_link_ksettings) return -EOPNOTSUPP;
@@ -674,7 +674,7 @@ static int ethtool_get_settings(struct net_device *dev, void __user *useraddr) struct ethtool_cmd cmd; int err; - ASSERT_RTNL(); + netdev_ops_assert_locked(dev); if (!dev->ethtool_ops->get_link_ksettings) return -EOPNOTSUPP;
@@ -710,7 +710,7 @@ static int ethtool_set_settings(struct net_device *dev, void __user *useraddr) struct ethtool_cmd cmd; int ret; - ASSERT_RTNL(); + netdev_ops_assert_locked(dev); if (copy_from_user(&cmd, useraddr, sizeof(cmd))) return -EFAULT;
@@ -2451,10 +2451,10 @@ void ethtool_puts(u8 **data, const char *str) } EXPORT_SYMBOL(ethtool_puts); -static int ethtool_phys_id(struct net_device *dev, void __user *useraddr) +static int ethtool_phys_id(struct net_device *dev, void __user *useraddr, + bool has_rtnl_lock) { struct ethtool_value id; - static bool busy; const struct ethtool_ops *ops = dev->ethtool_ops; netdevice_tracker dev_tracker; int rc;
@@ -2462,7 +2462,7 @@ static int ethtool_phys_id(struct net_device *dev, void __user *useraddr) if (!ops->set_phys_id) return -EOPNOTSUPP; - if (busy) + if (dev->ethtool->phys_id_busy) return -EBUSY; if (copy_from_user(&id, useraddr, sizeof(id)))
@@ -2472,13 +2472,14 @@ static int ethtool_phys_id(struct net_device *dev, void __user *useraddr) if (rc < 0) return rc; - /* Drop the RTNL lock while waiting, but prevent reentry or + /* Drop the locks while waiting, but prevent reentry or * removal of the device. */ - busy = true; + dev->ethtool->phys_id_busy = true; netdev_hold(dev, &dev_tracker, GFP_KERNEL); netdev_unlock_ops(dev); - rtnl_unlock(); + if (has_rtnl_lock) + rtnl_unlock(); if (rc == 0) { /* Driver will handle this itself */
@@ -2491,22 +2492,25 @@ static int ethtool_phys_id(struct net_device *dev, void __user *useraddr) u64 i = 0; do { - rtnl_lock(); + if (has_rtnl_lock) + rtnl_lock(); netdev_lock_ops(dev); rc = ops->set_phys_id(dev, (i++ & 1) ? ETHTOOL_ID_OFF : ETHTOOL_ID_ON); netdev_unlock_ops(dev); - rtnl_unlock(); + if (has_rtnl_lock) + rtnl_unlock(); if (rc) break; schedule_timeout_interruptible(interval); } while (!signal_pending(current) && (!id.data || i < count)); } - rtnl_lock(); + if (has_rtnl_lock) + rtnl_lock(); netdev_lock_ops(dev); netdev_put(dev, &dev_tracker); - busy = false; + dev->ethtool->phys_id_busy = false; (void) ops->set_phys_id(dev, ETHTOOL_ID_INACTIVE); return rc;
@@ -3259,7 +3263,8 @@ static int ethtool_set_fecparam(struct net_device *dev, void __user *useraddr) static int dev_ethtool_locked(struct net *net, struct net_device *dev, void __user *useraddr, - u32 ethcmd, struct ethtool_devlink_compat *devlink_state) + u32 ethcmd, struct ethtool_devlink_compat *devlink_state, + bool has_rtnl_lock) { u32 sub_cmd; int rc;
@@ -3315,6 +3320,8 @@ dev_ethtool_locked(struct net *net, struct net_device *dev, return -EPERM; } + netdev_ops_assert_locked(dev); + if (dev->dev.parent) pm_runtime_get_sync(dev->dev.parent);
@@ -3402,7 +3409,7 @@ dev_ethtool_locked(struct net *net, struct net_device *dev, rc = ethtool_get_strings(dev, useraddr); break; case ETHTOOL_PHYS_ID: - rc = ethtool_phys_id(dev, useraddr); + rc = ethtool_phys_id(dev, useraddr, has_rtnl_lock); break; case ETHTOOL_GSTATS: rc = ethtool_get_stats(dev, useraddr);
@@ -3549,8 +3556,12 @@ dev_ethtool_locked(struct net *net, struct net_device *dev, if (dev->ethtool_ops->complete) dev->ethtool_ops->complete(dev); - if (old_features != dev->features) - netdev_features_change(dev); + if (old_features != dev->features) { + if (has_rtnl_lock) + netdev_features_change(dev); + else + netdev_WARN(dev, "unexpected device features change with ethtool cmd %u", ethcmd); + } out: if (dev->dev.parent) pm_runtime_put(dev->dev.parent);
@@ -3558,25 +3569,60 @@ dev_ethtool_locked(struct net *net, struct net_device *dev, return rc; } +/* Commands that may toggle dev->features in net/ethtool/ioctl.c and so + * call into __netdev_update_features(), which still requires rtnl_lock. + * Driver-decided SET commands that may chain into rtnl-only helpers are + * covered by ethtool_ioctl_needs_rtnl()/ETHTOOL_OP_NEEDS_RTNL_*. + */ +static bool ethtool_cmd_changes_features(u32 ethcmd) +{ + switch (ethcmd) { + case ETHTOOL_SFEATURES: + case ETHTOOL_SFLAGS: + case ETHTOOL_STXCSUM: + case ETHTOOL_SRXCSUM: + case ETHTOOL_SSG: + case ETHTOOL_STSO: + case ETHTOOL_SGSO: + case ETHTOOL_SGRO: + return true; + } + return false; +} + static int __dev_ethtool(struct net *net, struct ifreq *ifr, void __user *useraddr, u32 ethcmd, struct ethtool_devlink_compat *devlink_state) { + netdevice_tracker dev_tracker; struct net_device *dev; + bool need_rtnl; int rc; - rtnl_lock(); - dev = __dev_get_by_name(net, ifr->ifr_name); - if (!dev) { + dev = netdev_get_by_name(net, ifr->ifr_name, &dev_tracker, GFP_KERNEL); + if (!dev) + return -ENODEV; + + need_rtnl = !netdev_need_ops_lock(dev) || + ethtool_cmd_changes_features(ethcmd) || + ethtool_ioctl_needs_rtnl(dev, ethcmd); + if (need_rtnl) + rtnl_lock(); + netdev_lock_ops(dev); + if (dev->reg_state > NETREG_REGISTERED || + dev->moving_ns || !net_eq(dev_net(dev), net)) { rc = -ENODEV; - goto exit_rtnl_unlock; + goto exit_ops_unlock; } - netdev_lock_ops(dev); - rc = dev_ethtool_locked(net, dev, useraddr, ethcmd, devlink_state); + rc = dev_ethtool_locked(net, dev, useraddr, ethcmd, devlink_state, + need_rtnl); + +exit_ops_unlock: netdev_unlock_ops(dev); -exit_rtnl_unlock: - rtnl_unlock(); + if (need_rtnl) + rtnl_unlock(); + netdev_put(dev, &dev_tracker); return rc; }
--
2.54.0