Subject: Re: [PATCH net-next 13/14] net: ethtool: optionally skip rtnl_lock on IOCTL path

From: Stanislav Fomichev <[email-masked]>
Date: Mon, 1 Jun 2026 08:17:56 -0700

On 05/28, Jakub Kicinski wrote:
> 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 <[email-masked]>
> ---
>  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;

GVE's gve_get_ethtool_stats (ETHTOOL_GSTATS) still has ASSERT_RTNL and
is now reachable with only ops lock?