Skip to content

Commit f994752

Browse files
kuba-mookuba-moo
committed
net: ethtool: optionally skip rtnl_lock on IOCTL path
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. Reviewed-by: Eric Dumazet <edumazet@google.com> Acked-by: Stanislav Fomichev <sdf@fomichev.me> Reviewed-by: Jacob Keller <jacob.e.keller@intel.com> Link: https://patch.msgid.link/20260605002912.3456868-12-kuba@kernel.org Signed-off-by: Jakub Kicinski <kuba@kernel.org>
1 parent f58a40d commit f994752

2 files changed

Lines changed: 82 additions & 26 deletions

File tree

‎include/linux/ethtool.h‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1375,13 +1375,15 @@ int ethtool_virtdev_set_link_ksettings(struct net_device *dev,
13751375
* within RTNL.
13761376
* @rss_indir_user_size: Number of user provided entries for the default
13771377
* (context 0) indirection table.
1378+
* @phys_id_busy: Loop blinking the device LED is running.
13781379
* @wol_enabled: Wake-on-LAN is enabled
13791380
* @module_fw_flash_in_progress: Module firmware flashing is in progress.
13801381
*/
13811382
struct ethtool_netdev_state {
13821383
struct xarray rss_ctx;
13831384
struct mutex rss_lock;
13841385
u32 rss_indir_user_size;
1386+
unsigned phys_id_busy:1;
13851387
unsigned wol_enabled:1;
13861388
unsigned module_fw_flash_in_progress:1;
13871389
};

‎net/ethtool/ioctl.c‎

Lines changed: 80 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -544,7 +544,7 @@ static int ethtool_get_link_ksettings(struct net_device *dev,
544544
int err = 0;
545545
struct ethtool_link_ksettings link_ksettings;
546546

547-
ASSERT_RTNL();
547+
netdev_assert_locked_ops_compat(dev);
548548
if (!dev->ethtool_ops->get_link_ksettings)
549549
return -EOPNOTSUPP;
550550

@@ -601,7 +601,7 @@ static int ethtool_set_link_ksettings(struct net_device *dev,
601601
struct ethtool_link_ksettings link_ksettings = {};
602602
int err;
603603

604-
ASSERT_RTNL();
604+
netdev_assert_locked_ops_compat(dev);
605605

606606
if (!dev->ethtool_ops->set_link_ksettings)
607607
return -EOPNOTSUPP;
@@ -675,7 +675,7 @@ static int ethtool_get_settings(struct net_device *dev, void __user *useraddr)
675675
struct ethtool_cmd cmd;
676676
int err;
677677

678-
ASSERT_RTNL();
678+
netdev_assert_locked_ops_compat(dev);
679679
if (!dev->ethtool_ops->get_link_ksettings)
680680
return -EOPNOTSUPP;
681681

@@ -711,7 +711,7 @@ static int ethtool_set_settings(struct net_device *dev, void __user *useraddr)
711711
struct ethtool_cmd cmd;
712712
int ret;
713713

714-
ASSERT_RTNL();
714+
netdev_assert_locked_ops_compat(dev);
715715

716716
if (copy_from_user(&cmd, useraddr, sizeof(cmd)))
717717
return -EFAULT;
@@ -2452,18 +2452,18 @@ void ethtool_puts(u8 **data, const char *str)
24522452
}
24532453
EXPORT_SYMBOL(ethtool_puts);
24542454

2455-
static int ethtool_phys_id(struct net_device *dev, void __user *useraddr)
2455+
static int ethtool_phys_id(struct net_device *dev, void __user *useraddr,
2456+
bool has_rtnl_lock)
24562457
{
24572458
struct ethtool_value id;
2458-
static bool busy;
24592459
const struct ethtool_ops *ops = dev->ethtool_ops;
24602460
netdevice_tracker dev_tracker;
24612461
int rc;
24622462

24632463
if (!ops->set_phys_id)
24642464
return -EOPNOTSUPP;
24652465

2466-
if (busy)
2466+
if (dev->ethtool->phys_id_busy)
24672467
return -EBUSY;
24682468

24692469
if (copy_from_user(&id, useraddr, sizeof(id)))
@@ -2473,13 +2473,14 @@ static int ethtool_phys_id(struct net_device *dev, void __user *useraddr)
24732473
if (rc < 0)
24742474
return rc;
24752475

2476-
/* Drop the RTNL lock while waiting, but prevent reentry or
2476+
/* Drop the locks while waiting, but prevent reentry or
24772477
* removal of the device.
24782478
*/
2479-
busy = true;
2479+
dev->ethtool->phys_id_busy = true;
24802480
netdev_hold(dev, &dev_tracker, GFP_KERNEL);
24812481
netdev_unlock_ops(dev);
2482-
rtnl_unlock();
2482+
if (has_rtnl_lock)
2483+
rtnl_unlock();
24832484

24842485
if (rc == 0) {
24852486
/* Driver will handle this itself */
@@ -2492,22 +2493,25 @@ static int ethtool_phys_id(struct net_device *dev, void __user *useraddr)
24922493
u64 i = 0;
24932494

24942495
do {
2495-
rtnl_lock();
2496+
if (has_rtnl_lock)
2497+
rtnl_lock();
24962498
netdev_lock_ops(dev);
24972499
rc = ops->set_phys_id(dev,
24982500
(i++ & 1) ? ETHTOOL_ID_OFF : ETHTOOL_ID_ON);
24992501
netdev_unlock_ops(dev);
2500-
rtnl_unlock();
2502+
if (has_rtnl_lock)
2503+
rtnl_unlock();
25012504
if (rc)
25022505
break;
25032506
schedule_timeout_interruptible(interval);
25042507
} while (!signal_pending(current) && (!id.data || i < count));
25052508
}
25062509

2507-
rtnl_lock();
2510+
if (has_rtnl_lock)
2511+
rtnl_lock();
25082512
netdev_lock_ops(dev);
25092513
netdev_put(dev, &dev_tracker);
2510-
busy = false;
2514+
dev->ethtool->phys_id_busy = false;
25112515

25122516
(void) ops->set_phys_id(dev, ETHTOOL_ID_INACTIVE);
25132517
return rc;
@@ -3260,7 +3264,8 @@ static int ethtool_set_fecparam(struct net_device *dev, void __user *useraddr)
32603264
static int
32613265
dev_ethtool_locked(struct net *net, struct net_device *dev,
32623266
void __user *useraddr,
3263-
u32 ethcmd, struct ethtool_devlink_compat *devlink_state)
3267+
u32 ethcmd, struct ethtool_devlink_compat *devlink_state,
3268+
bool has_rtnl_lock)
32643269
{
32653270
u32 sub_cmd;
32663271
int rc;
@@ -3316,6 +3321,8 @@ dev_ethtool_locked(struct net *net, struct net_device *dev,
33163321
return -EPERM;
33173322
}
33183323

3324+
netdev_assert_locked_ops_compat(dev);
3325+
33193326
if (dev->dev.parent)
33203327
pm_runtime_get_sync(dev->dev.parent);
33213328

@@ -3403,7 +3410,7 @@ dev_ethtool_locked(struct net *net, struct net_device *dev,
34033410
rc = ethtool_get_strings(dev, useraddr);
34043411
break;
34053412
case ETHTOOL_PHYS_ID:
3406-
rc = ethtool_phys_id(dev, useraddr);
3413+
rc = ethtool_phys_id(dev, useraddr, has_rtnl_lock);
34073414
break;
34083415
case ETHTOOL_GSTATS:
34093416
rc = ethtool_get_stats(dev, useraddr);
@@ -3550,34 +3557,81 @@ dev_ethtool_locked(struct net *net, struct net_device *dev,
35503557
if (dev->ethtool_ops->complete)
35513558
dev->ethtool_ops->complete(dev);
35523559

3553-
if (old_features != dev->features)
3554-
netdev_features_change(dev);
3560+
switch (ethcmd) {
3561+
case ETHTOOL_PHYS_ID:
3562+
/* Don't check features if operation drops the locks.
3563+
* Someone else may have changed features in parallel.
3564+
*/
3565+
break;
3566+
default:
3567+
if (old_features != dev->features) {
3568+
if (has_rtnl_lock)
3569+
netdev_features_change(dev);
3570+
else
3571+
netdev_WARN(dev, "ethtool cmd %u changed features without rtnl_lock", ethcmd);
3572+
}
3573+
}
35553574
out:
35563575
if (dev->dev.parent)
35573576
pm_runtime_put(dev->dev.parent);
35583577

35593578
return rc;
35603579
}
35613580

3581+
/* Commands that may toggle dev->features in net/ethtool/ioctl.c and so
3582+
* call into __netdev_update_features(), which still requires rtnl_lock.
3583+
* Driver-decided SET commands that may chain into rtnl-only helpers are
3584+
* covered by ethtool_ioctl_needs_rtnl()/ETHTOOL_OP_NEEDS_RTNL_*.
3585+
*/
3586+
static bool ethtool_cmd_changes_features(u32 ethcmd)
3587+
{
3588+
switch (ethcmd) {
3589+
case ETHTOOL_SFEATURES:
3590+
case ETHTOOL_SFLAGS:
3591+
case ETHTOOL_STXCSUM:
3592+
case ETHTOOL_SRXCSUM:
3593+
case ETHTOOL_SSG:
3594+
case ETHTOOL_STSO:
3595+
case ETHTOOL_SGSO:
3596+
case ETHTOOL_SGRO:
3597+
return true;
3598+
}
3599+
return false;
3600+
}
3601+
35623602
static int
35633603
__dev_ethtool(struct net *net, struct ifreq *ifr, void __user *useraddr,
35643604
u32 ethcmd, struct ethtool_devlink_compat *devlink_state)
35653605
{
3606+
netdevice_tracker dev_tracker;
35663607
struct net_device *dev;
3608+
bool need_rtnl;
35673609
int rc;
35683610

3569-
rtnl_lock();
3570-
dev = __dev_get_by_name(net, ifr->ifr_name);
3571-
if (!dev) {
3611+
dev = netdev_get_by_name(net, ifr->ifr_name, &dev_tracker, GFP_KERNEL);
3612+
if (!dev)
3613+
return -ENODEV;
3614+
3615+
need_rtnl = !netdev_need_ops_lock(dev) ||
3616+
ethtool_cmd_changes_features(ethcmd) ||
3617+
ethtool_ioctl_needs_rtnl(dev, ethcmd);
3618+
if (need_rtnl)
3619+
rtnl_lock();
3620+
netdev_lock_ops(dev);
3621+
if (dev->reg_state > NETREG_REGISTERED ||
3622+
dev->moving_ns || !net_eq(dev_net(dev), net)) {
35723623
rc = -ENODEV;
3573-
goto exit_rtnl_unlock;
3624+
goto exit_ops_unlock;
35743625
}
35753626

3576-
netdev_lock_ops(dev);
3577-
rc = dev_ethtool_locked(net, dev, useraddr, ethcmd, devlink_state);
3627+
rc = dev_ethtool_locked(net, dev, useraddr, ethcmd, devlink_state,
3628+
need_rtnl);
3629+
3630+
exit_ops_unlock:
35783631
netdev_unlock_ops(dev);
3579-
exit_rtnl_unlock:
3580-
rtnl_unlock();
3632+
if (need_rtnl)
3633+
rtnl_unlock();
3634+
netdev_put(dev, &dev_tracker);
35813635

35823636
return rc;
35833637
}

0 commit comments

Comments
 (0)