[PATCH net-next v2 00/11] net: ethtool: make sure __ethtool_get_link_ksettings() is ops-locked

[PATCH net-next v2 00/11] net: ethtool: make sure __ethtool_get_link_ksettings() is ops-locked

[Jakub Kicinski]

This is prep for the series which will make most of the ethtool ops
run without rtnl_lock. The AI bots surfaced a number of callers of
__ethtool_get_link_ksettings() which need fixing, so I decided to
send that as a smaller prep-series. Each driver changed separately
for ease of review.

Full series unlocking ethtool ops AKA v1:: 
https://lore.kernel.org/20260528231637.251822-1-kuba@kernel.org

Jakub Kicinski (11):
  net: rename netdev_ops_assert_locked()
  net: ethtool: cmis_cdb: hold instance lock for ops locked devices
  net: document NETDEV_CHANGENAME as ops locked
  net: ethtool: add netif_get_link_ksettings() for correct ops-locked
    use
  net: bonding: don't recurse on the slave's netdev ops lock
  net: team: don't recurse on the port's netdev ops lock
  net: bridge: don't recurse on the port's netdev ops lock
  net: sched: don't recurse on the netdev ops lock in qdiscs
  leds: trigger: netdev: don't recurse on the netdev ops lock
  scsi: fcoe: don't recurse on the netdev's ops lock
  net: ethtool: make sure __ethtool_get_link_ksettings() is ops-locked

 Documentation/networking/netdevices.rst   |  1 +
 include/linux/ethtool.h                   |  2 ++
 include/net/netdev_lock.h                 | 12 ++++++--
 drivers/leds/trigger/ledtrig-netdev.c     | 37 +++++++++++++----------
 drivers/net/bonding/bond_main.c           | 21 ++++++++++---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c |  2 +-
 drivers/net/team/team_core.c              |  4 ++-
 drivers/scsi/bnx2fc/bnx2fc_fcoe.c         |  3 ++
 drivers/scsi/fcoe/fcoe.c                  |  6 ++--
 drivers/scsi/fcoe/fcoe_transport.c        |  4 ++-
 net/bridge/br_if.c                        |  7 +++--
 net/core/dev.c                            | 26 ++++++++--------
 net/core/dev_addr_lists.c                 |  2 +-
 net/core/link_watch.c                     |  2 +-
 net/core/lock_debug.c                     |  3 +-
 net/core/netdev_queues.c                  |  2 +-
 net/ethtool/cmis_cdb.c                    |  3 ++
 net/ethtool/cmis_fw_update.c              |  8 ++---
 net/ethtool/ioctl.c                       | 21 +++++++++++--
 net/ethtool/linkinfo.c                    |  4 +--
 net/ethtool/linkmodes.c                   |  4 +--
 net/ethtool/module.c                      |  2 ++
 net/ethtool/netlink.c                     |  4 +--
 net/ipv6/addrconf.c                       |  2 +-
 net/sched/sch_cbs.c                       |  2 +-
 net/sched/sch_taprio.c                    |  2 +-
 net/xdp/xsk_buff_pool.c                   |  2 +-
 27 files changed, 121 insertions(+), 67 deletions(-)

-- 
2.54.0

[PATCH net-next v2 01/11] net: rename netdev_ops_assert_locked()

[Jakub Kicinski]

Jakub suggests renaming the existing assert to match
the netdev_lock_ops_compat() semantics.

We want netdev_assert_locked_ops() to mean - if the driver
is ops locked - check that it's holding the device lock.

The existing helper check for either ops lock or rtnl_lock,
which is the locking behavior of netdev_lock_ops_compat().

The reason for naming divergence is likely that
netdev_ops_assert_locked() predated the _compat() helpers.

Suggested-by: Jakub Sitnicki <jakub@cloudflare.com>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
v2:
 - new patch
---
 include/net/netdev_lock.h                 |  6 +++---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c |  2 +-
 net/core/dev.c                            | 26 +++++++++++------------
 net/core/dev_addr_lists.c                 |  2 +-
 net/core/link_watch.c                     |  2 +-
 net/core/lock_debug.c                     |  2 +-
 net/core/netdev_queues.c                  |  2 +-
 net/ethtool/netlink.c                     |  4 ++--
 net/ipv6/addrconf.c                       |  2 +-
 net/xdp/xsk_buff_pool.c                   |  2 +-
 10 files changed, 25 insertions(+), 25 deletions(-)

diff --git a/include/net/netdev_lock.h b/include/net/netdev_lock.h
index 3d3aef80beac..8e84d29b0bfb 100644
--- a/include/net/netdev_lock.h
+++ b/include/net/netdev_lock.h
@@ -64,7 +64,7 @@ static inline void netdev_unlock_full_to_ops(struct net_device *dev)
 		netdev_unlock(dev);
 }
 
-static inline void netdev_ops_assert_locked(const struct net_device *dev)
+static inline void netdev_assert_locked_ops_compat(const struct net_device *dev)
 {
 	if (netdev_need_ops_lock(dev))
 		lockdep_assert_held(&dev->lock);
@@ -73,11 +73,11 @@ static inline void netdev_ops_assert_locked(const struct net_device *dev)
 }
 
 static inline void
-netdev_ops_assert_locked_or_invisible(const struct net_device *dev)
+netdev_assert_locked_ops_compat_or_invisible(const struct net_device *dev)
 {
 	if (dev->reg_state == NETREG_REGISTERED ||
 	    dev->reg_state == NETREG_UNREGISTERING)
-		netdev_ops_assert_locked(dev);
+		netdev_assert_locked_ops_compat(dev);
 }
 
 static inline void netdev_lock_ops_compat(struct net_device *dev)
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index d4f93e62f583..e27187ef6d63 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -16762,7 +16762,7 @@ int bnxt_restore_pf_fw_resources(struct bnxt *bp)
 {
 	int rc;
 
-	netdev_ops_assert_locked(bp->dev);
+	netdev_assert_locked_ops_compat(bp->dev);
 	bnxt_hwrm_func_qcaps(bp);
 
 	if (netif_running(bp->dev))
diff --git a/net/core/dev.c b/net/core/dev.c
index 804e8ad25010..1ecd5691992e 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1593,7 +1593,7 @@ EXPORT_SYMBOL(netdev_features_change);
 
 void netif_state_change(struct net_device *dev)
 {
-	netdev_ops_assert_locked_or_invisible(dev);
+	netdev_assert_locked_ops_compat_or_invisible(dev);
 
 	if (dev->flags & IFF_UP) {
 		struct netdev_notifier_change_info change_info = {
@@ -1693,7 +1693,7 @@ static int __dev_open(struct net_device *dev, struct netlink_ext_ack *extack)
 
 	set_bit(__LINK_STATE_START, &dev->state);
 
-	netdev_ops_assert_locked(dev);
+	netdev_assert_locked_ops_compat(dev);
 
 	if (ops->ndo_validate_addr)
 		ret = ops->ndo_validate_addr(dev);
@@ -1770,7 +1770,7 @@ static void __dev_close_many(struct list_head *head)
 		 *	event.
 		 */
 
-		netdev_ops_assert_locked(dev);
+		netdev_assert_locked_ops_compat(dev);
 
 		if (ops->ndo_stop)
 			ops->ndo_stop(dev);
@@ -3198,7 +3198,7 @@ int netif_set_real_num_tx_queues(struct net_device *dev, unsigned int txq)
 
 	if (dev->reg_state == NETREG_REGISTERED ||
 	    dev->reg_state == NETREG_UNREGISTERING) {
-		netdev_ops_assert_locked(dev);
+		netdev_assert_locked_ops_compat(dev);
 
 		rc = netdev_queue_update_kobjects(dev, dev->real_num_tx_queues,
 						  txq);
@@ -3247,7 +3247,7 @@ int netif_set_real_num_rx_queues(struct net_device *dev, unsigned int rxq)
 		return -EINVAL;
 
 	if (dev->reg_state == NETREG_REGISTERED) {
-		netdev_ops_assert_locked(dev);
+		netdev_assert_locked_ops_compat(dev);
 
 		rc = net_rx_queue_update_kobjects(dev, dev->real_num_rx_queues,
 						  rxq);
@@ -7294,7 +7294,7 @@ void netif_queue_set_napi(struct net_device *dev, unsigned int queue_index,
 
 	if (WARN_ON_ONCE(napi && !napi->dev))
 		return;
-	netdev_ops_assert_locked_or_invisible(dev);
+	netdev_assert_locked_ops_compat_or_invisible(dev);
 
 	switch (type) {
 	case NETDEV_QUEUE_TYPE_RX:
@@ -9589,7 +9589,7 @@ int __dev_set_promiscuity(struct net_device *dev, int inc, bool notify)
 	kuid_t uid;
 	kgid_t gid;
 
-	netdev_ops_assert_locked(dev);
+	netdev_assert_locked_ops_compat(dev);
 
 	promiscuity = dev->promiscuity + inc;
 	if (promiscuity == 0) {
@@ -9648,7 +9648,7 @@ int netif_set_allmulti(struct net_device *dev, int inc, bool notify)
 	unsigned int old_flags = dev->flags, old_gflags = dev->gflags;
 	unsigned int allmulti, flags;
 
-	netdev_ops_assert_locked(dev);
+	netdev_assert_locked_ops_compat(dev);
 
 	allmulti = dev->allmulti + inc;
 	if (allmulti == 0) {
@@ -9716,7 +9716,7 @@ int __dev_change_flags(struct net_device *dev, unsigned int flags,
 	unsigned int old_flags = dev->flags;
 	int ret;
 
-	netdev_ops_assert_locked(dev);
+	netdev_assert_locked_ops_compat(dev);
 
 	/*
 	 *	Set the flags on our device.
@@ -9864,7 +9864,7 @@ int netif_set_mtu_ext(struct net_device *dev, int new_mtu,
 {
 	int err, orig_mtu;
 
-	netdev_ops_assert_locked(dev);
+	netdev_assert_locked_ops_compat(dev);
 
 	if (new_mtu == dev->mtu)
 		return 0;
@@ -10317,7 +10317,7 @@ static int dev_xdp_install(struct net_device *dev, enum bpf_xdp_mode mode,
 	struct netdev_bpf xdp;
 	int err;
 
-	netdev_ops_assert_locked(dev);
+	netdev_assert_locked_ops_compat(dev);
 
 	if (dev->cfg->hds_config == ETHTOOL_TCP_DATA_SPLIT_ENABLED &&
 	    prog && !prog->aux->xdp_has_frags) {
@@ -10769,7 +10769,7 @@ u32 dev_get_min_mp_channel_count(const struct net_device *dev)
 {
 	int i;
 
-	netdev_ops_assert_locked(dev);
+	netdev_assert_locked_ops_compat(dev);
 
 	for (i = dev->real_num_rx_queues - 1; i >= 0; i--)
 		if (dev->_rx[i].mp_params.mp_priv)
@@ -10997,7 +10997,7 @@ int __netdev_update_features(struct net_device *dev)
 	int err = -1;
 
 	ASSERT_RTNL();
-	netdev_ops_assert_locked(dev);
+	netdev_assert_locked_ops_compat(dev);
 
 	features = netdev_get_wanted_features(dev);
 
diff --git a/net/core/dev_addr_lists.c b/net/core/dev_addr_lists.c
index d73fcb0c6785..6b493af8dc8b 100644
--- a/net/core/dev_addr_lists.c
+++ b/net/core/dev_addr_lists.c
@@ -1260,7 +1260,7 @@ static void netif_rx_mode_run(struct net_device *dev)
 	int err;
 
 	might_sleep();
-	netdev_ops_assert_locked(dev);
+	netdev_assert_locked_ops_compat(dev);
 
 	__hw_addr_init(&uc_snap);
 	__hw_addr_init(&mc_snap);
diff --git a/net/core/link_watch.c b/net/core/link_watch.c
index ff2c1d4538ef..9c35aac8b2e9 100644
--- a/net/core/link_watch.c
+++ b/net/core/link_watch.c
@@ -277,7 +277,7 @@ static bool linkwatch_clean_dev(struct net_device *dev)
 
 void __linkwatch_sync_dev(struct net_device *dev)
 {
-	netdev_ops_assert_locked(dev);
+	netdev_assert_locked_ops_compat(dev);
 
 	if (linkwatch_clean_dev(dev)) {
 		linkwatch_do_dev(dev);
diff --git a/net/core/lock_debug.c b/net/core/lock_debug.c
index 9e9fb25314b9..14fd8fcdcd56 100644
--- a/net/core/lock_debug.c
+++ b/net/core/lock_debug.c
@@ -24,7 +24,7 @@ int netdev_debug_event(struct notifier_block *nb, unsigned long event,
 	case NETDEV_CHANGE:
 	case NETDEV_REGISTER:
 	case NETDEV_UP:
-		netdev_ops_assert_locked(dev);
+		netdev_assert_locked_ops_compat(dev);
 		fallthrough;
 	case NETDEV_DOWN:
 	case NETDEV_REBOOT:
diff --git a/net/core/netdev_queues.c b/net/core/netdev_queues.c
index 73fb28087a93..4d6864bb4f6b 100644
--- a/net/core/netdev_queues.c
+++ b/net/core/netdev_queues.c
@@ -40,7 +40,7 @@ struct device *netdev_queue_get_dma_dev(struct net_device *dev,
 	struct netdev_rx_queue *hw_rxq;
 	struct device *dma_dev;
 
-	netdev_ops_assert_locked(dev);
+	netdev_assert_locked_ops_compat(dev);
 
 	/* Only RX side supports queue leasing today. */
 	if (type != NETDEV_QUEUE_TYPE_RX || !netif_rxq_is_leased(dev, idx))
diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
index 6cbd13b61bd1..25e22c48060a 100644
--- a/net/ethtool/netlink.c
+++ b/net/ethtool/netlink.c
@@ -98,7 +98,7 @@ int ethnl_ops_begin(struct net_device *dev)
 	if (dev->dev.parent)
 		pm_runtime_get_sync(dev->dev.parent);
 
-	netdev_ops_assert_locked(dev);
+	netdev_assert_locked_ops_compat(dev);
 
 	if (!netif_device_present(dev) ||
 	    dev->reg_state >= NETREG_UNREGISTERING) {
@@ -1005,7 +1005,7 @@ static void ethnl_default_notify(struct net_device *dev, unsigned int cmd,
 		       ops->req_info_size - sizeof(*req_info));
 	}
 
-	netdev_ops_assert_locked(dev);
+	netdev_assert_locked_ops_compat(dev);
 
 	ethnl_init_reply_data(reply_data, ops, dev);
 	ret = ops->prepare_data(req_info, reply_data, &info);
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index d7b03196725f..968bb844b1b8 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -380,7 +380,7 @@ static struct inet6_dev *ipv6_add_dev(struct net_device *dev)
 	int err = -ENOMEM;
 
 	ASSERT_RTNL();
-	netdev_ops_assert_locked(dev);
+	netdev_assert_locked_ops_compat(dev);
 
 	if (dev->mtu < IPV6_MIN_MTU && dev != blackhole_netdev)
 		return ERR_PTR(-EINVAL);
diff --git a/net/xdp/xsk_buff_pool.c b/net/xdp/xsk_buff_pool.c
index d981cfdd8535..1f28a9641571 100644
--- a/net/xdp/xsk_buff_pool.c
+++ b/net/xdp/xsk_buff_pool.c
@@ -239,7 +239,7 @@ int xp_assign_dev(struct xsk_buff_pool *pool,
 	bpf.xsk.pool = pool;
 	bpf.xsk.queue_id = queue_id;
 
-	netdev_ops_assert_locked(netdev);
+	netdev_assert_locked_ops_compat(netdev);
 	err = netdev->netdev_ops->ndo_bpf(netdev, &bpf);
 	if (err)
 		goto err_unreg_pool;
-- 
2.54.0

[PATCH net-next v2 02/11] net: ethtool: cmis_cdb: hold instance lock for ops locked devices

[Jakub Kicinski]

FW module flashing was written so that the flashing happens
without holding rtnl_lock. This allows flashing multiple modules
at once. Current drivers can handle that well, but we should
let drivers depend on the netdev instance lock. Instance lock
is per netdev, and so is the module so we won't break parallel
updates.

Reviewed-by: Jakub Sitnicki <jakub@cloudflare.com>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 include/net/netdev_lock.h    | 6 ++++++
 net/ethtool/cmis_cdb.c       | 3 +++
 net/ethtool/cmis_fw_update.c | 8 ++------
 net/ethtool/module.c         | 2 ++
 4 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/include/net/netdev_lock.h b/include/net/netdev_lock.h
index 8e84d29b0bfb..d3daec4e93f3 100644
--- a/include/net/netdev_lock.h
+++ b/include/net/netdev_lock.h
@@ -80,6 +80,12 @@ netdev_assert_locked_ops_compat_or_invisible(const struct net_device *dev)
 		netdev_assert_locked_ops_compat(dev);
 }
 
+static inline void netdev_assert_locked_ops(const struct net_device *dev)
+{
+	if (netdev_need_ops_lock(dev))
+		netdev_assert_locked(dev);
+}
+
 static inline void netdev_lock_ops_compat(struct net_device *dev)
 {
 	if (netdev_need_ops_lock(dev))
diff --git a/net/ethtool/cmis_cdb.c b/net/ethtool/cmis_cdb.c
index f3a53a984460..a4e8f4b3fb75 100644
--- a/net/ethtool/cmis_cdb.c
+++ b/net/ethtool/cmis_cdb.c
@@ -2,6 +2,7 @@
 
 #include <linux/ethtool.h>
 #include <linux/jiffies.h>
+#include <net/netdev_lock.h>
 
 #include "common.h"
 #include "module_fw.h"
@@ -179,6 +180,7 @@ cmis_cdb_validate_password(struct ethtool_cmis_cdb *cdb,
 
 	pe_pl = *((struct cmis_password_entry_pl *)page_data.data);
 	pe_pl.password = params->password;
+	netdev_assert_locked_ops(dev);
 	err = ops->set_module_eeprom_by_page(dev, &page_data, &extack);
 	if (err < 0) {
 		if (extack._msg)
@@ -546,6 +548,7 @@ __ethtool_cmis_cdb_execute_cmd(struct net_device *dev,
 	if (!page_data->data)
 		return -ENOMEM;
 
+	netdev_assert_locked_ops(dev);
 	err = ops->set_module_eeprom_by_page(dev, page_data, &extack);
 	if (err < 0) {
 		if (extack._msg)
diff --git a/net/ethtool/cmis_fw_update.c b/net/ethtool/cmis_fw_update.c
index 291d04d2776a..dff83807e975 100644
--- a/net/ethtool/cmis_fw_update.c
+++ b/net/ethtool/cmis_fw_update.c
@@ -435,13 +435,9 @@ cmis_fw_update_commit_image(struct ethtool_cmis_cdb *cdb,
 static int cmis_fw_update_reset(struct net_device *dev)
 {
 	__u32 reset_data = ETH_RESET_PHY;
-	int ret;
 
-	netdev_lock_ops(dev);
-	ret = dev->ethtool_ops->reset(dev, &reset_data);
-	netdev_unlock_ops(dev);
-
-	return ret;
+	netdev_assert_locked_ops(dev);
+	return dev->ethtool_ops->reset(dev, &reset_data);
 }
 
 void
diff --git a/net/ethtool/module.c b/net/ethtool/module.c
index ea4fb2a76650..c3388e6d7ec8 100644
--- a/net/ethtool/module.c
+++ b/net/ethtool/module.c
@@ -226,7 +226,9 @@ static void module_flash_fw_work(struct work_struct *work)
 	module_fw = container_of(work, struct ethtool_module_fw_flash, work);
 	dev = module_fw->fw_update.dev;
 
+	netdev_lock_ops(dev);
 	ethtool_cmis_fw_update(&module_fw->fw_update);
+	netdev_unlock_ops(dev);
 
 	module_flash_fw_work_list_del(&module_fw->list);
 
-- 
2.54.0

[PATCH net-next v2 03/11] net: document NETDEV_CHANGENAME as ops locked

[Jakub Kicinski]

NETDEV_CHANGENAME is only emitted from netif_change_name().
netif_change_name() has two callers both of which hold netdev_lock_ops()
around the call site:
 - dev_change_name()
 - do_setlink()

Document NETDEV_CHANGENAME as always ops locked.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 Documentation/networking/netdevices.rst | 1 +
 net/core/lock_debug.c                   | 1 +
 2 files changed, 2 insertions(+)

diff --git a/Documentation/networking/netdevices.rst b/Documentation/networking/netdevices.rst
index 60492d4df2ee..8fc96975b3bd 100644
--- a/Documentation/networking/netdevices.rst
+++ b/Documentation/networking/netdevices.rst
@@ -415,6 +415,7 @@ instance lock.
 For devices with locked ops, currently only the following notifiers are
 running under the lock:
 * ``NETDEV_CHANGE``
+* ``NETDEV_CHANGENAME``
 * ``NETDEV_REGISTER``
 * ``NETDEV_UP``
 
diff --git a/net/core/lock_debug.c b/net/core/lock_debug.c
index 14fd8fcdcd56..8a81c5430705 100644
--- a/net/core/lock_debug.c
+++ b/net/core/lock_debug.c
@@ -65,6 +65,7 @@ int netdev_debug_event(struct notifier_block *nb, unsigned long event,
 		break;
 
 	case NETDEV_CHANGENAME:
+		netdev_assert_locked_ops(dev);
 		ASSERT_RTNL_NET(net);
 		break;
 	}
-- 
2.54.0

[PATCH net-next v2 04/11] net: ethtool: add netif_get_link_ksettings() for correct ops-locked use

[Jakub Kicinski]

__ethtool_get_link_ksettings() is exported and called from sysfs
and many drivers. It invokes ethtool_ops->get_link_ksettings
so by our own docs it should be holding netdev lock for ops locked
devices. Looks like commit 2bcf4772e45a ("net: ethtool:
try to protect all callback with netdev instance lock")
missed adding the ops lock here.

There's a number of callers we need to fix up so let's add the
netif_get_link_ksettings() helper first, without any actual
locking changes (this commit is a nop).

Not treating this as a fix because I don't think any driver cares
at this point, but if we want to remove the rtnl_lock protection
this will become critical.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
v2:
 - split into multiple patches
---
 include/linux/ethtool.h |  2 ++
 net/ethtool/ioctl.c     | 17 ++++++++++++++---
 net/ethtool/linkinfo.c  |  4 ++--
 net/ethtool/linkmodes.c |  4 ++--
 4 files changed, 20 insertions(+), 7 deletions(-)

diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 1cb0740ba331..f51346a6a686 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -325,6 +325,8 @@ struct ethtool_link_ksettings {
 extern int
 __ethtool_get_link_ksettings(struct net_device *dev,
 			     struct ethtool_link_ksettings *link_ksettings);
+int netif_get_link_ksettings(struct net_device *dev,
+			     struct ethtool_link_ksettings *link_ksettings);
 
 struct ethtool_keee {
 	__ETHTOOL_DECLARE_LINK_MODE_MASK(supported);
diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
index bd97f9b9bf18..49da873b673d 100644
--- a/net/ethtool/ioctl.c
+++ b/net/ethtool/ioctl.c
@@ -436,10 +436,10 @@ struct ethtool_link_usettings {
 };
 
 /* Internal kernel helper to query a device ethtool_link_settings. */
-int __ethtool_get_link_ksettings(struct net_device *dev,
-				 struct ethtool_link_ksettings *link_ksettings)
+int netif_get_link_ksettings(struct net_device *dev,
+			     struct ethtool_link_ksettings *link_ksettings)
 {
-	ASSERT_RTNL();
+	/* once callers fixed - assert ops locked */
 
 	if (!dev->ethtool_ops->get_link_ksettings)
 		return -EOPNOTSUPP;
@@ -450,6 +450,17 @@ int __ethtool_get_link_ksettings(struct net_device *dev,
 	memset(link_ksettings, 0, sizeof(*link_ksettings));
 	return dev->ethtool_ops->get_link_ksettings(dev, link_ksettings);
 }
+EXPORT_SYMBOL(netif_get_link_ksettings);
+
+/* Convenience helper for callers that hold only rtnl_lock(). */
+int __ethtool_get_link_ksettings(struct net_device *dev,
+				 struct ethtool_link_ksettings *link_ksettings)
+{
+	ASSERT_RTNL();
+
+	/* once callers fixed - take the ops lock around this call */
+	return netif_get_link_ksettings(dev, link_ksettings);
+}
 EXPORT_SYMBOL(__ethtool_get_link_ksettings);
 
 /* convert ethtool_link_usettings in user space to a kernel internal
diff --git a/net/ethtool/linkinfo.c b/net/ethtool/linkinfo.c
index 244ff92e2ff9..d5b3dbc53c5f 100644
--- a/net/ethtool/linkinfo.c
+++ b/net/ethtool/linkinfo.c
@@ -34,7 +34,7 @@ static int linkinfo_prepare_data(const struct ethnl_req_info *req_base,
 	ret = ethnl_ops_begin(dev);
 	if (ret < 0)
 		return ret;
-	ret = __ethtool_get_link_ksettings(dev, &data->ksettings);
+	ret = netif_get_link_ksettings(dev, &data->ksettings);
 	if (ret < 0)
 		GENL_SET_ERR_MSG(info, "failed to retrieve link settings");
 	ethnl_ops_complete(dev);
@@ -104,7 +104,7 @@ ethnl_set_linkinfo(struct ethnl_req_info *req_info, struct genl_info *info)
 	bool mod = false;
 	int ret;
 
-	ret = __ethtool_get_link_ksettings(dev, &ksettings);
+	ret = netif_get_link_ksettings(dev, &ksettings);
 	if (ret < 0) {
 		GENL_SET_ERR_MSG(info, "failed to retrieve link settings");
 		return ret;
diff --git a/net/ethtool/linkmodes.c b/net/ethtool/linkmodes.c
index 30d703531652..a6d32f0d9fcc 100644
--- a/net/ethtool/linkmodes.c
+++ b/net/ethtool/linkmodes.c
@@ -39,7 +39,7 @@ static int linkmodes_prepare_data(const struct ethnl_req_info *req_base,
 	if (ret < 0)
 		return ret;
 
-	ret = __ethtool_get_link_ksettings(dev, &data->ksettings);
+	ret = netif_get_link_ksettings(dev, &data->ksettings);
 	if (ret < 0) {
 		GENL_SET_ERR_MSG(info, "failed to retrieve link settings");
 		goto out;
@@ -324,7 +324,7 @@ ethnl_set_linkmodes(struct ethnl_req_info *req_info, struct genl_info *info)
 	bool mod = false;
 	int ret;
 
-	ret = __ethtool_get_link_ksettings(dev, &ksettings);
+	ret = netif_get_link_ksettings(dev, &ksettings);
 	if (ret < 0) {
 		GENL_SET_ERR_MSG(info, "failed to retrieve link settings");
 		return ret;
-- 
2.54.0

[PATCH net-next v2 05/11] net: bonding: don't recurse on the slave's netdev ops lock

[Jakub Kicinski]

bond_update_speed_duplex() calls __ethtool_get_link_ksettings() on
the slave, which will soon take the slave's ops lock. One of its
callers already holds it and the other three don't, so the function
would either deadlock or run unprotected depending on the path.

Make the helper expect the slave's ops lock held and switch to
netif_get_link_ksettings(). Wrap the three call sites that don't
already hold it:

  * bond_enslave() (rtnl held; core drops the lower's ops lock
    around ->ndo_add_slave).
  * bond_miimon_commit() (rtnl_trylock'd from the mii workqueue).
  * bond_ethtool_get_link_ksettings() (rtnl held via ethtool layer,
    bond device itself is not ops locked).

The call site which does already hold the ops lock is
bond_slave_netdev_event() via NETDEV_UP / NETDEV_CHANGE notifiers,
so it stays as-is.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 drivers/net/bonding/bond_main.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 82e779f7916b..0bcab797e468 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -787,6 +787,10 @@ int bond_set_carrier(struct bonding *bond)
  * values are invalid, set speed and duplex to -1,
  * and return. Return 1 if speed or duplex settings are
  * UNKNOWN; 0 otherwise.
+ *
+ * Caller must hold the slave's netdev ops lock. The notifier path
+ * (bond_netdev_event NETDEV_CHANGE/UP) reaches us with the slave's ops
+ * lock held; other call sites take it explicitly.
  */
 static int bond_update_speed_duplex(struct slave *slave)
 {
@@ -794,7 +798,7 @@ static int bond_update_speed_duplex(struct slave *slave)
 	struct ethtool_link_ksettings ecmd;
 	int res;
 
-	res = __ethtool_get_link_ksettings(slave_dev, &ecmd);
+	res = netif_get_link_ksettings(slave_dev, &ecmd);
 	if (res < 0)
 		goto speed_duplex_unknown;
 	if (ecmd.base.speed == 0 || ecmd.base.speed == ((__u32)-1))
@@ -2112,8 +2116,10 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
 	new_slave->delay = 0;
 	new_slave->link_failure_count = 0;
 
-	if (bond_update_speed_duplex(new_slave) &&
-	    bond_needs_speed_duplex(bond))
+	netdev_lock_ops(slave_dev);
+	res = bond_update_speed_duplex(new_slave);
+	netdev_unlock_ops(slave_dev);
+	if (res && bond_needs_speed_duplex(bond))
 		new_slave->link = BOND_LINK_DOWN;
 
 	new_slave->last_rx = jiffies -
@@ -2780,6 +2786,7 @@ static void bond_miimon_commit(struct bonding *bond)
 	struct slave *slave, *primary, *active;
 	bool do_failover = false;
 	struct list_head *iter;
+	int err;
 
 	ASSERT_RTNL();
 
@@ -2798,8 +2805,10 @@ static void bond_miimon_commit(struct bonding *bond)
 			continue;
 
 		case BOND_LINK_UP:
-			if (bond_update_speed_duplex(slave) &&
-			    bond_needs_speed_duplex(bond)) {
+			netdev_lock_ops(slave->dev);
+			err = bond_update_speed_duplex(slave);
+			netdev_unlock_ops(slave->dev);
+			if (err && bond_needs_speed_duplex(bond)) {
 				slave->link = BOND_LINK_DOWN;
 				if (net_ratelimit())
 					slave_warn(bond->dev, slave->dev,
@@ -5861,7 +5870,9 @@ static int bond_ethtool_get_link_ksettings(struct net_device *bond_dev,
 	 */
 	bond_for_each_slave(bond, slave, iter) {
 		if (bond_slave_can_tx(slave)) {
+			netdev_lock_ops(slave->dev);
 			bond_update_speed_duplex(slave);
+			netdev_unlock_ops(slave->dev);
 			if (slave->speed != SPEED_UNKNOWN) {
 				if (BOND_MODE(bond) == BOND_MODE_BROADCAST)
 					speed = bond_mode_bcast_speed(slave,
-- 
2.54.0

[PATCH net-next v2 06/11] net: team: don't recurse on the port's netdev ops lock

[Jakub Kicinski]

__team_port_change_send() calls __ethtool_get_link_ksettings() on
the port, which will soon take the port's ops lock. The notifier
caller already holds it while the slave-add/del callers do not,
so the function would either deadlock or run unprotected depending
on the path.

Make __team_port_change_send() expect the port's ops lock held and
switch to netif_get_link_ksettings(). team_device_event()'s NETDEV_UP /
NETDEV_CHANGE already arrive with the port's ops lock held.
team_port_add() now take it explicitly.

Note that NETDEV_DOWN and team_port_del() will pass false as @linkup
so they will not execute netif_get_link_ksettings(). This is fortunate
as NETDEV_DOWN has somewhat mixed locking right now.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 drivers/net/team/team_core.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/team/team_core.c b/drivers/net/team/team_core.c
index f51388d50307..feaa75fbf8fc 100644
--- a/drivers/net/team/team_core.c
+++ b/drivers/net/team/team_core.c
@@ -1375,7 +1375,9 @@ static int team_port_add(struct team *team, struct net_device *port_dev,
 	list_add_tail_rcu(&port->list, &team->port_list);
 	team_port_enable(team, port);
 	netdev_compute_master_upper_features(dev, true);
+	netdev_lock_ops(port_dev);
 	__team_port_change_port_added(port, !!netif_oper_up(port_dev));
+	netdev_unlock_ops(port_dev);
 	__team_options_change_check(team);
 
 	netdev_info(dev, "Port device %s added\n", portname);
@@ -3090,7 +3092,7 @@ static void __team_port_change_send(struct team_port *port, bool linkup)
 	if (linkup) {
 		struct ethtool_link_ksettings ecmd;
 
-		err = __ethtool_get_link_ksettings(port->dev, &ecmd);
+		err = netif_get_link_ksettings(port->dev, &ecmd);
 		if (!err) {
 			port->state.speed = ecmd.base.speed;
 			port->state.duplex = ecmd.base.duplex;
-- 
2.54.0

[PATCH net-next v2 07/11] net: bridge: don't recurse on the port's netdev ops lock

[Jakub Kicinski]

port_cost() calls __ethtool_get_link_ksettings() on the port device,
which will soon take the port's ops lock. br_port_carrier_check()
is reached via the NETDEV_CHANGE notifier from linkwatch, which
already holds the port's ops lock, so the call would deadlock.

Make port_cost() expect the port's ops lock held and switch to
netif_get_link_ksettings(). The only other caller is new_nbp(),
make sure it takes the lock explicitly.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 net/bridge/br_if.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
index d39571e13744..049d1d25bc26 100644
--- a/net/bridge/br_if.c
+++ b/net/bridge/br_if.c
@@ -19,6 +19,7 @@
 #include <linux/if_ether.h>
 #include <linux/slab.h>
 #include <net/dsa.h>
+#include <net/netdev_lock.h>
 #include <net/sock.h>
 #include <linux/if_vlan.h>
 #include <net/switchdev.h>
@@ -30,13 +31,13 @@
  * Determine initial path cost based on speed.
  * using recommendations from 802.1d standard
  *
- * Since driver might sleep need to not be holding any locks.
+ * Since driver might sleep, we need to not be holding any bridge spinlocks.
  */
 static int port_cost(struct net_device *dev)
 {
 	struct ethtool_link_ksettings ecmd;
 
-	if (!__ethtool_get_link_ksettings(dev, &ecmd)) {
+	if (!netif_get_link_ksettings(dev, &ecmd)) {
 		switch (ecmd.base.speed) {
 		case SPEED_10000:
 			return 2;
@@ -436,7 +437,9 @@ static struct net_bridge_port *new_nbp(struct net_bridge *br,
 	p->br = br;
 	netdev_hold(dev, &p->dev_tracker, GFP_KERNEL);
 	p->dev = dev;
+	netdev_lock_ops(dev);
 	p->path_cost = port_cost(dev);
+	netdev_unlock_ops(dev);
 	p->priority = 0x8000 >> BR_PORT_BITS;
 	p->port_no = index;
 	p->flags = BR_LEARNING | BR_FLOOD | BR_MCAST_FLOOD | BR_BCAST_FLOOD;
-- 
2.54.0

[PATCH net-next v2 08/11] net: sched: don't recurse on the netdev ops lock in qdiscs

[Jakub Kicinski]

cbs_set_port_rate() and taprio_set_picos_per_byte() are reached from
two paths and both already hold the device's ops lock:

 *_change(), via tc_modify_qdisc() which calls netdev_lock_ops(dev)
    before dispatching to the qdisc ops.

 *_dev_notifier() on NETDEV_UP / NETDEV_CHANGE, where caller
    holds the ops lock across the notifier chain.

Switch to netif_get_link_ksettings() to avoid deadlock once
__ethtool_get_link_ksettings() starts taking the netdev lock.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 net/sched/sch_cbs.c    | 2 +-
 net/sched/sch_taprio.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/sched/sch_cbs.c b/net/sched/sch_cbs.c
index b952353ec2a5..85b41ffc63ff 100644
--- a/net/sched/sch_cbs.c
+++ b/net/sched/sch_cbs.c
@@ -327,7 +327,7 @@ static void cbs_set_port_rate(struct net_device *dev, struct cbs_sched_data *q)
 	s64 port_rate;
 	int err;
 
-	err = __ethtool_get_link_ksettings(dev, &ecmd);
+	err = netif_get_link_ksettings(dev, &ecmd);
 	if (err < 0)
 		goto skip;
 
diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
index d6b981e5df11..e83cbce62a54 100644
--- a/net/sched/sch_taprio.c
+++ b/net/sched/sch_taprio.c
@@ -1299,7 +1299,7 @@ static void taprio_set_picos_per_byte(struct net_device *dev,
 	int picos_per_byte;
 	int err;
 
-	err = __ethtool_get_link_ksettings(dev, &ecmd);
+	err = netif_get_link_ksettings(dev, &ecmd);
 	if (err < 0)
 		goto skip;
 
-- 
2.54.0

[PATCH net-next v2 09/11] leds: trigger: netdev: don't recurse on the netdev ops lock

[Jakub Kicinski]

get_device_state() calls __ethtool_get_link_ksettings() on the trigger's
netdev, which will soon take the dev's ops lock. Three of its callers
already hold that lock and one doesn't, so the function would either
deadlock or run unprotected depending on the path.

Make get_device_state() expect the dev's ops lock held and switch to
netif_get_link_ksettings():

  * netdev_trig_notify() NETDEV_UP / NETDEV_CHANGE / NETDEV_CHANGENAME
    arrive with the dev's ops lock held (per netdevices.rst).
  * set_device_name() does not hold the lock, take it explicitly.

Due to lock ordering we need to reshuffle the code in set_device_name()
a little bit. We need to find the device earlier on, so that we can
lock it before we take trigger_data->lock.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 drivers/leds/trigger/ledtrig-netdev.c | 37 +++++++++++++++------------
 1 file changed, 21 insertions(+), 16 deletions(-)

diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
index 12cb3311ea22..64c078e997f2 100644
--- a/drivers/leds/trigger/ledtrig-netdev.c
+++ b/drivers/leds/trigger/ledtrig-netdev.c
@@ -26,6 +26,7 @@
 #include <linux/phy.h>
 #include <linux/rtnetlink.h>
 #include <linux/timer.h>
+#include <net/netdev_lock.h>
 #include "../leds.h"
 
 #define NETDEV_LED_DEFAULT_INTERVAL	50
@@ -228,7 +229,7 @@ static void get_device_state(struct led_netdev_data *trigger_data)
 
 	trigger_data->carrier_link_up = netif_carrier_ok(trigger_data->net_dev);
 
-	if (__ethtool_get_link_ksettings(trigger_data->net_dev, &cmd))
+	if (netif_get_link_ksettings(trigger_data->net_dev, &cmd))
 		return;
 
 	if (trigger_data->carrier_link_up) {
@@ -259,31 +260,33 @@ static ssize_t device_name_show(struct device *dev,
 static int set_device_name(struct led_netdev_data *trigger_data,
 			   const char *name, size_t size)
 {
+	struct net_device *new_dev = NULL;
+	char device_name[IFNAMSIZ];
+
 	if (size >= IFNAMSIZ)
 		return -EINVAL;
 
 	cancel_delayed_work_sync(&trigger_data->work);
 
+	memcpy(device_name, name, size);
+	device_name[size] = 0;
+	if (size > 0 && device_name[size - 1] == '\n')
+		device_name[size - 1] = 0;
+
 	/*
-	 * Take RTNL lock before trigger_data lock to prevent potential
-	 * deadlock with netdev notifier registration.
+	 * Lock order: rtnl_lock -> netdev instance lock -> trigger_data lock.
 	 */
 	rtnl_lock();
+	if (device_name[0]) {
+		new_dev = dev_get_by_name(&init_net, device_name);
+		if (new_dev)
+			netdev_lock_ops(new_dev);
+	}
 	mutex_lock(&trigger_data->lock);
 
-	if (trigger_data->net_dev) {
-		dev_put(trigger_data->net_dev);
-		trigger_data->net_dev = NULL;
-	}
-
-	memcpy(trigger_data->device_name, name, size);
-	trigger_data->device_name[size] = 0;
-	if (size > 0 && trigger_data->device_name[size - 1] == '\n')
-		trigger_data->device_name[size - 1] = 0;
-
-	if (trigger_data->device_name[0] != 0)
-		trigger_data->net_dev =
-		    dev_get_by_name(&init_net, trigger_data->device_name);
+	dev_put(trigger_data->net_dev);
+	trigger_data->net_dev = new_dev;
+	strscpy(trigger_data->device_name, device_name);
 
 	trigger_data->carrier_link_up = false;
 	trigger_data->link_speed = SPEED_UNKNOWN;
@@ -298,6 +301,8 @@ static int set_device_name(struct led_netdev_data *trigger_data,
 		set_baseline_state(trigger_data);
 
 	mutex_unlock(&trigger_data->lock);
+	if (new_dev)
+		netdev_unlock_ops(new_dev);
 	rtnl_unlock();
 
 	return 0;
-- 
2.54.0

[PATCH net-next v2 10/11] scsi: fcoe: don't recurse on the netdev's ops lock

[Jakub Kicinski]

fcoe_link_speed_update() calls __ethtool_get_link_ksettings() on the
lport's netdev, which will soon take the dev's ops lock. Some notifier
callers already arrive with this lock held. Switch to
netif_get_link_ksettings() and adjust the explicit call sites to take
the netdev lock explicitly.

Within fcoe_device_notification() try to only query the link speed
from notifiers which announce link state change (UP / CHANGE),
DOWN / GOING_DOWN notifiers are slightly sketchy when it comes
to ops locking right now, and the code already special-cases
those by maintaining the local link_possible variable.

Also take the lock in bnx2fc_net_config(), even though I think
that bnx2fc call sites are largely irrelevant since it's not
an ops-locked driver.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 drivers/scsi/bnx2fc/bnx2fc_fcoe.c  | 3 +++
 drivers/scsi/fcoe/fcoe.c           | 6 ++++--
 drivers/scsi/fcoe/fcoe_transport.c | 4 +++-
 3 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
index 26e0ff380860..c95b084cad69 100644
--- a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
+++ b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
@@ -17,6 +17,7 @@
 #include "bnx2fc.h"
 
 #include <linux/ethtool.h>
+#include <net/netdev_lock.h>
 
 static struct list_head adapter_list;
 static struct list_head if_list;
@@ -815,7 +816,9 @@ static int bnx2fc_net_config(struct fc_lport *lport, struct net_device *netdev)
 	port->fcoe_pending_queue_active = 0;
 	timer_setup(&port->timer, fcoe_queue_timer, 0);
 
+	netdev_lock_ops(netdev);
 	fcoe_link_speed_update(lport);
+	netdev_unlock_ops(netdev);
 
 	if (!lport->vport) {
 		if (fcoe_get_wwn(netdev, &wwnn, NETDEV_FCOE_WWNN))
diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c
index 534596c6d76c..438ac7c3a9e3 100644
--- a/drivers/scsi/fcoe/fcoe.c
+++ b/drivers/scsi/fcoe/fcoe.c
@@ -25,6 +25,7 @@
 #include <scsi/scsicam.h>
 #include <scsi/scsi_transport.h>
 #include <scsi/scsi_transport_fc.h>
+#include <net/netdev_lock.h>
 #include <net/rtnetlink.h>
 
 #include <scsi/fc/fc_encaps.h>
@@ -737,7 +738,9 @@ static int fcoe_netdev_config(struct fc_lport *lport, struct net_device *netdev)
 	port->fcoe_pending_queue_active = 0;
 	timer_setup(&port->timer, fcoe_queue_timer, 0);
 
+	netdev_lock_ops(netdev);
 	fcoe_link_speed_update(lport);
+	netdev_unlock_ops(netdev);
 
 	if (!lport->vport) {
 		if (fcoe_get_wwn(netdev, &wwnn, NETDEV_FCOE_WWNN))
@@ -1841,6 +1844,7 @@ static int fcoe_device_notification(struct notifier_block *notifier,
 		break;
 	case NETDEV_UP:
 	case NETDEV_CHANGE:
+		fcoe_link_speed_update(lport);
 		break;
 	case NETDEV_CHANGEMTU:
 		if (netdev->fcoe_mtu)
@@ -1871,8 +1875,6 @@ static int fcoe_device_notification(struct notifier_block *notifier,
 				"from netdev netlink\n", event);
 	}
 
-	fcoe_link_speed_update(lport);
-
 	cdev = fcoe_ctlr_to_ctlr_dev(ctlr);
 
 	if (link_possible && !fcoe_link_ok(lport)) {
diff --git a/drivers/scsi/fcoe/fcoe_transport.c b/drivers/scsi/fcoe/fcoe_transport.c
index 88d85fc9a52a..2bbb9a38e61d 100644
--- a/drivers/scsi/fcoe/fcoe_transport.c
+++ b/drivers/scsi/fcoe/fcoe_transport.c
@@ -111,6 +111,8 @@ static inline u32 eth2fc_speed(u32 eth_port_speed)
  * fcoe_link_speed_update() - Update the supported and actual link speeds
  * @lport: The local port to update speeds for
  *
+ * Caller must hold the netdev's ops lock.
+ *
  * Returns: 0 if the ethtool query was successful
  *          -1 if the ethtool query failed
  */
@@ -119,7 +121,7 @@ int fcoe_link_speed_update(struct fc_lport *lport)
 	struct net_device *netdev = fcoe_get_netdev(lport);
 	struct ethtool_link_ksettings ecmd;
 
-	if (!__ethtool_get_link_ksettings(netdev, &ecmd)) {
+	if (!netif_get_link_ksettings(netdev, &ecmd)) {
 		lport->link_supported_speeds &= ~(FC_PORTSPEED_1GBIT  |
 		                                  FC_PORTSPEED_10GBIT |
 		                                  FC_PORTSPEED_20GBIT |
-- 
2.54.0

[PATCH net-next v2 11/11] net: ethtool: make sure __ethtool_get_link_ksettings() is ops-locked

[Jakub Kicinski]

All drivers which may call *_get_link_ksettings() on ops-locked
devices from paths already holding the ops lock are ready now.
Make __ethtool_get_link_ksettings() take the ops lock, and assert
that it's held in netif_get_link_ksettings().

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 net/ethtool/ioctl.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
index 49da873b673d..a4b0cbae4063 100644
--- a/net/ethtool/ioctl.c
+++ b/net/ethtool/ioctl.c
@@ -439,7 +439,7 @@ struct ethtool_link_usettings {
 int netif_get_link_ksettings(struct net_device *dev,
 			     struct ethtool_link_ksettings *link_ksettings)
 {
-	/* once callers fixed - assert ops locked */
+	netdev_assert_locked_ops_compat(dev);
 
 	if (!dev->ethtool_ops->get_link_ksettings)
 		return -EOPNOTSUPP;
@@ -456,10 +456,14 @@ EXPORT_SYMBOL(netif_get_link_ksettings);
 int __ethtool_get_link_ksettings(struct net_device *dev,
 				 struct ethtool_link_ksettings *link_ksettings)
 {
+	int ret;
+
 	ASSERT_RTNL();
 
-	/* once callers fixed - take the ops lock around this call */
-	return netif_get_link_ksettings(dev, link_ksettings);
+	netdev_lock_ops(dev);
+	ret = netif_get_link_ksettings(dev, link_ksettings);
+	netdev_unlock_ops(dev);
+	return ret;
 }
 EXPORT_SYMBOL(__ethtool_get_link_ksettings);
 
-- 
2.54.0

Re: [PATCH net-next v2 04/11] net: ethtool: add netif_get_link_ksettings() for correct ops-locked use

[Maxime Chevallier]

Hi Jakub

On 6/3/26 03:28, Jakub Kicinski wrote:
> __ethtool_get_link_ksettings() is exported and called from sysfs
> and many drivers. It invokes ethtool_ops->get_link_ksettings
> so by our own docs it should be holding netdev lock for ops locked
> devices. Looks like commit 2bcf4772e45a ("net: ethtool:
> try to protect all callback with netdev instance lock")
> missed adding the ops lock here.
> 
> There's a number of callers we need to fix up so let's add the
> netif_get_link_ksettings() helper first, without any actual
> locking changes (this commit is a nop).
> 
> Not treating this as a fix because I don't think any driver cares
> at this point, but if we want to remove the rtnl_lock protection
> this will become critical.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

Maxime

> ---
> v2:
>  - split into multiple patches
> ---
>  include/linux/ethtool.h |  2 ++
>  net/ethtool/ioctl.c     | 17 ++++++++++++++---
>  net/ethtool/linkinfo.c  |  4 ++--
>  net/ethtool/linkmodes.c |  4 ++--
>  4 files changed, 20 insertions(+), 7 deletions(-)
> 
> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
> index 1cb0740ba331..f51346a6a686 100644
> --- a/include/linux/ethtool.h
> +++ b/include/linux/ethtool.h
> @@ -325,6 +325,8 @@ struct ethtool_link_ksettings {
>  extern int
>  __ethtool_get_link_ksettings(struct net_device *dev,
>  			     struct ethtool_link_ksettings *link_ksettings);
> +int netif_get_link_ksettings(struct net_device *dev,
> +			     struct ethtool_link_ksettings *link_ksettings);
>  
>  struct ethtool_keee {
>  	__ETHTOOL_DECLARE_LINK_MODE_MASK(supported);
> diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
> index bd97f9b9bf18..49da873b673d 100644
> --- a/net/ethtool/ioctl.c
> +++ b/net/ethtool/ioctl.c
> @@ -436,10 +436,10 @@ struct ethtool_link_usettings {
>  };
>  
>  /* Internal kernel helper to query a device ethtool_link_settings. */
> -int __ethtool_get_link_ksettings(struct net_device *dev,
> -				 struct ethtool_link_ksettings *link_ksettings)
> +int netif_get_link_ksettings(struct net_device *dev,
> +			     struct ethtool_link_ksettings *link_ksettings)
>  {
> -	ASSERT_RTNL();
> +	/* once callers fixed - assert ops locked */
>  
>  	if (!dev->ethtool_ops->get_link_ksettings)
>  		return -EOPNOTSUPP;
> @@ -450,6 +450,17 @@ int __ethtool_get_link_ksettings(struct net_device *dev,
>  	memset(link_ksettings, 0, sizeof(*link_ksettings));
>  	return dev->ethtool_ops->get_link_ksettings(dev, link_ksettings);
>  }
> +EXPORT_SYMBOL(netif_get_link_ksettings);
> +
> +/* Convenience helper for callers that hold only rtnl_lock(). */
> +int __ethtool_get_link_ksettings(struct net_device *dev,
> +				 struct ethtool_link_ksettings *link_ksettings)
> +{
> +	ASSERT_RTNL();
> +
> +	/* once callers fixed - take the ops lock around this call */
> +	return netif_get_link_ksettings(dev, link_ksettings);
> +}
>  EXPORT_SYMBOL(__ethtool_get_link_ksettings);
>  
>  /* convert ethtool_link_usettings in user space to a kernel internal
> diff --git a/net/ethtool/linkinfo.c b/net/ethtool/linkinfo.c
> index 244ff92e2ff9..d5b3dbc53c5f 100644
> --- a/net/ethtool/linkinfo.c
> +++ b/net/ethtool/linkinfo.c
> @@ -34,7 +34,7 @@ static int linkinfo_prepare_data(const struct ethnl_req_info *req_base,
>  	ret = ethnl_ops_begin(dev);
>  	if (ret < 0)
>  		return ret;
> -	ret = __ethtool_get_link_ksettings(dev, &data->ksettings);
> +	ret = netif_get_link_ksettings(dev, &data->ksettings);
>  	if (ret < 0)
>  		GENL_SET_ERR_MSG(info, "failed to retrieve link settings");
>  	ethnl_ops_complete(dev);
> @@ -104,7 +104,7 @@ ethnl_set_linkinfo(struct ethnl_req_info *req_info, struct genl_info *info)
>  	bool mod = false;
>  	int ret;
>  
> -	ret = __ethtool_get_link_ksettings(dev, &ksettings);
> +	ret = netif_get_link_ksettings(dev, &ksettings);
>  	if (ret < 0) {
>  		GENL_SET_ERR_MSG(info, "failed to retrieve link settings");
>  		return ret;
> diff --git a/net/ethtool/linkmodes.c b/net/ethtool/linkmodes.c
> index 30d703531652..a6d32f0d9fcc 100644
> --- a/net/ethtool/linkmodes.c
> +++ b/net/ethtool/linkmodes.c
> @@ -39,7 +39,7 @@ static int linkmodes_prepare_data(const struct ethnl_req_info *req_base,
>  	if (ret < 0)
>  		return ret;
>  
> -	ret = __ethtool_get_link_ksettings(dev, &data->ksettings);
> +	ret = netif_get_link_ksettings(dev, &data->ksettings);
>  	if (ret < 0) {
>  		GENL_SET_ERR_MSG(info, "failed to retrieve link settings");
>  		goto out;
> @@ -324,7 +324,7 @@ ethnl_set_linkmodes(struct ethnl_req_info *req_info, struct genl_info *info)
>  	bool mod = false;
>  	int ret;
>  
> -	ret = __ethtool_get_link_ksettings(dev, &ksettings);
> +	ret = netif_get_link_ksettings(dev, &ksettings);
>  	if (ret < 0) {
>  		GENL_SET_ERR_MSG(info, "failed to retrieve link settings");
>  		return ret;

Re: [PATCH net-next v2 07/11] net: bridge: don't recurse on the port's netdev ops lock

[Nicolai Buchwitz]

Hi Jakub

On 3.6.2026 03:28, Jakub Kicinski wrote:
> port_cost() calls __ethtool_get_link_ksettings() on the port device,
> which will soon take the port's ops lock. br_port_carrier_check()
> is reached via the NETDEV_CHANGE notifier from linkwatch, which
> already holds the port's ops lock, so the call would deadlock.
> 
> Make port_cost() expect the port's ops lock held and switch to
> netif_get_link_ksettings(). The only other caller is new_nbp(),
> make sure it takes the lock explicitly.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
>  net/bridge/br_if.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
> index d39571e13744..049d1d25bc26 100644
> --- a/net/bridge/br_if.c
> +++ b/net/bridge/br_if.c
> @@ -19,6 +19,7 @@
>  #include <linux/if_ether.h>
>  #include <linux/slab.h>
>  #include <net/dsa.h>
> +#include <net/netdev_lock.h>
>  #include <net/sock.h>
>  #include <linux/if_vlan.h>
>  #include <net/switchdev.h>
> @@ -30,13 +31,13 @@
>   * Determine initial path cost based on speed.
>   * using recommendations from 802.1d standard
>   *
> - * Since driver might sleep need to not be holding any locks.
> + * Since driver might sleep, we need to not be holding any bridge 
> spinlocks.
>   */
>  static int port_cost(struct net_device *dev)
>  {
>  	struct ethtool_link_ksettings ecmd;
> 
> -	if (!__ethtool_get_link_ksettings(dev, &ecmd)) {
> +	if (!netif_get_link_ksettings(dev, &ecmd)) {
>  		switch (ecmd.base.speed) {
>  		case SPEED_10000:
>  			return 2;
> @@ -436,7 +437,9 @@ static struct net_bridge_port *new_nbp(struct 
> net_bridge *br,
>  	p->br = br;
>  	netdev_hold(dev, &p->dev_tracker, GFP_KERNEL);
>  	p->dev = dev;
> +	netdev_lock_ops(dev);
>  	p->path_cost = port_cost(dev);
> +	netdev_unlock_ops(dev);
>  	p->priority = 0x8000 >> BR_PORT_BITS;
>  	p->port_no = index;
>  	p->flags = BR_LEARNING | BR_FLOOD | BR_MCAST_FLOOD | BR_BCAST_FLOOD;

Reviewed-by: Nicolai Buchwitz <nb@tipi-net.de>

Thanks
Nicolai

Re: [PATCH net-next v2 11/11] net: ethtool: make sure __ethtool_get_link_ksettings() is ops-locked

[Nicolai Buchwitz]

Hi Jakub

On 3.6.2026 03:28, Jakub Kicinski wrote:
> All drivers which may call *_get_link_ksettings() on ops-locked
> devices from paths already holding the ops lock are ready now.
> Make __ethtool_get_link_ksettings() take the ops lock, and assert
> that it's held in netif_get_link_ksettings().
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
>  net/ethtool/ioctl.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
> index 49da873b673d..a4b0cbae4063 100644
> --- a/net/ethtool/ioctl.c
> +++ b/net/ethtool/ioctl.c
> @@ -439,7 +439,7 @@ struct ethtool_link_usettings {
>  int netif_get_link_ksettings(struct net_device *dev,
>  			     struct ethtool_link_ksettings *link_ksettings)
>  {
> -	/* once callers fixed - assert ops locked */
> +	netdev_assert_locked_ops_compat(dev);
> 
>  	if (!dev->ethtool_ops->get_link_ksettings)
>  		return -EOPNOTSUPP;
> @@ -456,10 +456,14 @@ EXPORT_SYMBOL(netif_get_link_ksettings);
>  int __ethtool_get_link_ksettings(struct net_device *dev,
>  				 struct ethtool_link_ksettings *link_ksettings)
>  {
> +	int ret;
> +
>  	ASSERT_RTNL();
> 
> -	/* once callers fixed - take the ops lock around this call */
> -	return netif_get_link_ksettings(dev, link_ksettings);
> +	netdev_lock_ops(dev);
> +	ret = netif_get_link_ksettings(dev, link_ksettings);
> +	netdev_unlock_ops(dev);
> +	return ret;
>  }
>  EXPORT_SYMBOL(__ethtool_get_link_ksettings);

Reviewed-by: Nicolai Buchwitz <nb@tipi-net.de>

Thanks
Nicolai

Re: [PATCH net-next v2 05/11] net: bonding: don't recurse on the slave's netdev ops lock

[Nicolai Buchwitz]

Hi Jakub

On 3.6.2026 03:28, Jakub Kicinski wrote:
> bond_update_speed_duplex() calls __ethtool_get_link_ksettings() on
> the slave, which will soon take the slave's ops lock. One of its
> callers already holds it and the other three don't, so the function
> would either deadlock or run unprotected depending on the path.
> 
> Make the helper expect the slave's ops lock held and switch to
> netif_get_link_ksettings(). Wrap the three call sites that don't
> already hold it:
> 
>   * bond_enslave() (rtnl held; core drops the lower's ops lock
>     around ->ndo_add_slave).
>   * bond_miimon_commit() (rtnl_trylock'd from the mii workqueue).
>   * bond_ethtool_get_link_ksettings() (rtnl held via ethtool layer,
>     bond device itself is not ops locked).
> 
> The call site which does already hold the ops lock is
> bond_slave_netdev_event() via NETDEV_UP / NETDEV_CHANGE notifiers,
> so it stays as-is.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

> [...]

Reviewed-by: Nicolai Buchwitz <nb@tipi-net.de>

Thanks
Nicolai

Re: [PATCH net-next v2 01/11] net: rename netdev_ops_assert_locked()

[Nicolai Buchwitz]

Hi Jakub

On 3.6.2026 03:28, Jakub Kicinski wrote:
> Jakub suggests renaming the existing assert to match
> the netdev_lock_ops_compat() semantics.
> 
> We want netdev_assert_locked_ops() to mean - if the driver
> is ops locked - check that it's holding the device lock.
> 
> The existing helper check for either ops lock or rtnl_lock,
> which is the locking behavior of netdev_lock_ops_compat().
> 
> The reason for naming divergence is likely that
> netdev_ops_assert_locked() predated the _compat() helpers.
> 
> Suggested-by: Jakub Sitnicki <jakub@cloudflare.com>
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---

> [...]

Reviewed-by: Nicolai Buchwitz <nb@tipi-net.de>

Thanks
Nicolai

Re: [PATCH net-next v2 07/11] net: bridge: don't recurse on the port's netdev ops lock

[Nikolay Aleksandrov]

On 03/06/2026 04:28, Jakub Kicinski wrote:
> port_cost() calls __ethtool_get_link_ksettings() on the port device,
> which will soon take the port's ops lock. br_port_carrier_check()
> is reached via the NETDEV_CHANGE notifier from linkwatch, which
> already holds the port's ops lock, so the call would deadlock.
> 
> Make port_cost() expect the port's ops lock held and switch to
> netif_get_link_ksettings(). The only other caller is new_nbp(),
> make sure it takes the lock explicitly.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
>   net/bridge/br_if.c | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
> index d39571e13744..049d1d25bc26 100644
> --- a/net/bridge/br_if.c
> +++ b/net/bridge/br_if.c
> @@ -19,6 +19,7 @@
>   #include <linux/if_ether.h>
>   #include <linux/slab.h>
>   #include <net/dsa.h>
> +#include <net/netdev_lock.h>
>   #include <net/sock.h>
>   #include <linux/if_vlan.h>
>   #include <net/switchdev.h>
> @@ -30,13 +31,13 @@
>    * Determine initial path cost based on speed.
>    * using recommendations from 802.1d standard
>    *
> - * Since driver might sleep need to not be holding any locks.
> + * Since driver might sleep, we need to not be holding any bridge spinlocks.
>    */
>   static int port_cost(struct net_device *dev)
>   {
>   	struct ethtool_link_ksettings ecmd;
>   
> -	if (!__ethtool_get_link_ksettings(dev, &ecmd)) {
> +	if (!netif_get_link_ksettings(dev, &ecmd)) {
>   		switch (ecmd.base.speed) {
>   		case SPEED_10000:
>   			return 2;
> @@ -436,7 +437,9 @@ static struct net_bridge_port *new_nbp(struct net_bridge *br,
>   	p->br = br;
>   	netdev_hold(dev, &p->dev_tracker, GFP_KERNEL);
>   	p->dev = dev;
> +	netdev_lock_ops(dev);
>   	p->path_cost = port_cost(dev);
> +	netdev_unlock_ops(dev);
>   	p->priority = 0x8000 >> BR_PORT_BITS;
>   	p->port_no = index;
>   	p->flags = BR_LEARNING | BR_FLOOD | BR_MCAST_FLOOD | BR_BCAST_FLOOD;

LGTM,
Acked-by: Nikolay Aleksandrov <razor@blackwall.org>