net: ethtool: make sure __ethtool_get_link_ksettings() is ops-locked
Subsystems (16)
Jakub Kicinski
maintainerv2 cover net: ethtool: make sure __ethtool_get_link_ksettings() is ops-locked
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/[email protected]
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(-)
Signature · 1 line
2.54.0
Jakub Kicinski
maintainerPATCH v2 1/11 net: rename netdev_ops_assert_locked()
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.
---
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(-)
include/net/netdev_lock.h +3 -3
@@ -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)
drivers/net/ethernet/broadcom/bnxt/bnxt.c +1 -1
@@ -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))
net/core/dev.c +13 -13
@@ -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);
net/core/dev_addr_lists.c +1 -1
@@ -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);
net/core/link_watch.c +1 -1
@@ -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);
net/core/lock_debug.c +1 -1
@@ -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 :
net/core/netdev_queues.c +1 -1
@@ -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))
net/ethtool/netlink.c +2 -2
@@ -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);
net/ipv6/addrconf.c +1 -1
@@ -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 );
net/xdp/xsk_buff_pool.c +1 -1
@@ -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;
Signature · 1 line
2.54.0
Re: [PATCH net-next v2 01/11] net: rename netdev_ops_assert_locked()
Hi Jakub
Quoted 9 lines · 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 ofnetdev_lock_ops_compat().
Quoted 8 lines
The reason for naming divergence is likely that
netdev_ops_assert_locked()predated the_compat()helpers.Suggested-by Jakub Sitnicki <[email protected]>Signed-off-by Jakub Kicinski <[email protected]>---
[...]
Thanks
Nicolai
Re: [PATCH net-next v2 01/11] net: rename netdev_ops_assert_locked()
In reply to Jakub KicinskiQuoted 12 lines · 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 ofnetdev_lock_ops_compat().The reason for naming divergence is likely that
netdev_ops_assert_locked()predated the_compat()helpers.
Quoted 6 lines
Suggested-by Jakub Sitnicki <[email protected]>Signed-off-by Jakub Kicinski <[email protected]>---
v2:
- new patch
---
Re: [PATCH net-next v2 01/11] net: rename netdev_ops_assert_locked()
In reply to Jakub KicinskiQuoted 6 lines · 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.
Quoted 8 lines
The existing helper check for either ops lock or
rtnl_lock, which is the locking behavior ofnetdev_lock_ops_compat().The reason for naming divergence is likely that
netdev_ops_assert_locked()predated the_compat()helpers.Suggested-by Jakub Sitnicki <[email protected]>Signed-off-by Jakub Kicinski <[email protected]>
Jakub Kicinski
maintainerPATCH v2 2/11 net: ethtool: cmis_cdb: hold instance lock for ops locked devices
In reply to Jakub KicinskiFW 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.
--- 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(-)
include/net/netdev_lock.h +6 -0
@@ -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))
net/ethtool/cmis_cdb.c +3 -0
@@ -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)
net/ethtool/cmis_fw_update.c +2 -6
@@ -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
net/ethtool/module.c +2 -0
@@ -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);
Signature · 1 line
2.54.0
Re: [PATCH net-next v2 02/11] net: ethtool: cmis_cdb: hold instance lock for ops locked devices
Quoted 9 lines · 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 <[email protected]>Signed-off-by Jakub Kicinski <[email protected]>
Jakub Kicinski
maintainerPATCH v2 3/11 net: document NETDEV_CHANGENAME as ops locked
In reply to Jakub KicinskiNETDEV_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.
--- Documentation/networking/netdevices.rst | 1 + net/core/lock_debug.c | 1 + 2 files changed, 2 insertions(+)
Documentation/networking/netdevices.rst +1 -0
@@ -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``
net/core/lock_debug.c +1 -0
@@ -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 ; }
Signature · 1 line
2.54.0
Re: [PATCH net-next v2 03/11] net: document NETDEV_CHANGENAME as ops locked
Quoted 10 lines · Jakub Kicinski
NETDEV_CHANGENAMEis only emitted fromnetif_change_name().netif_change_name()has two callers both of which holdnetdev_lock_ops()around the call site:
-dev_change_name()
-do_setlink()Document
NETDEV_CHANGENAMEas always ops locked.Signed-off-by Jakub Kicinski <[email protected]>---
Re: [PATCH net-next v2 03/11] net: document NETDEV_CHANGENAME as ops locked
In reply to Jakub KicinskiQuoted 9 lines · Jakub Kicinski
NETDEV_CHANGENAMEis only emitted fromnetif_change_name().netif_change_name()has two callers both of which holdnetdev_lock_ops()around the call site:
-dev_change_name()
-do_setlink()Document
NETDEV_CHANGENAMEas always ops locked.Signed-off-by Jakub Kicinski <[email protected]>
Jakub Kicinski
maintainerPATCH v2 4/11 net: ethtool: add netif_get_link_ksettings() for correct ops-locked use
In reply to 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.
---
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(-)
include/linux/ethtool.h +2 -0
@@ -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);
net/ethtool/ioctl.c +14 -3
@@ -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
net/ethtool/linkinfo.c +2 -2
@@ -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;
net/ethtool/linkmodes.c +2 -2
@@ -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;
Signature · 1 line
2.54.0
Re: [PATCH net-next v2 04/11] net: ethtool: add netif_get_link_ksettings() for correct ops-locked use
Hi Jakub
Quoted 11 lines · Jakub Kicinski
__ethtool_get_link_ksettings()is exported and called from sysfs and many drivers. It invokesethtool_ops->get_link_ksettingsso by our own docs it should be holding netdev lock for ops locked devices. Looks like commit2bcf4772e45a("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).
Quoted 5 lines
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_lockprotection this will become critical.Signed-off-by Jakub Kicinski <[email protected]>
Maxime
Quoted 103 lines
---
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(-)include/linux/ethtool.h +2
-0@@ -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);net/ethtool/ioctl.c +14
-3@@ -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 net/ethtool/linkinfo.c +2
-2@@ -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;net/ethtool/linkmodes.c +2
-2@@ -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 04/11] net: ethtool: add netif_get_link_ksettings() for correct ops-locked use
In reply to Jakub KicinskiQuoted 11 lines · Jakub Kicinski
__ethtool_get_link_ksettings()is exported and called from sysfs and many drivers. It invokesethtool_ops->get_link_ksettingsso by our own docs it should be holding netdev lock for ops locked devices. Looks like commit2bcf4772e45a("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).
Quoted 5 lines
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_lockprotection this will become critical.Signed-off-by Jakub Kicinski <[email protected]>
Jakub Kicinski
maintainerPATCH v2 5/11 net: bonding: don't recurse on the slave's netdev ops lock
In reply to Jakub Kicinskibond_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.
--- drivers/net/bonding/bond_main.c | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-)
drivers/net/bonding/bond_main.c +16 -5
@@ -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,
Signature · 1 line
2.54.0
Re: [PATCH net-next v2 05/11] net: bonding: don't recurse on the slave's netdev ops lock
Hi Jakub
Quoted 15 lines · 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 lockaround ->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).
Quoted 7 lines
The call site which does already hold the ops lock is
bond_slave_netdev_event()viaNETDEV_UP/NETDEV_CHANGEnotifiers, so it stays as-is.Signed-off-by Jakub Kicinski <[email protected]>[...]
Thanks
Nicolai
Jakub Kicinski
maintainerPATCH v2 6/11 net: team: don't recurse on the port's netdev ops lock
In reply to 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.
--- drivers/net/team/team_core.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/net/team/team_core.c +3 -1
@@ -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;
Signature · 1 line
2.54.0
Jakub Kicinski
maintainerPATCH v2 7/11 net: bridge: don't recurse on the port's netdev ops lock
In reply to Jakub Kicinskiport_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.
--- net/bridge/br_if.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
net/bridge/br_if.c +5 -2
@@ -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 ;
Signature · 1 line
2.54.0
Re: [PATCH net-next v2 07/11] net: bridge: don't recurse on the port's netdev ops lock
Hi Jakub
Quoted 53 lines · 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 theNETDEV_CHANGEnotifier 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 tonetif_get_link_ksettings(). The only other caller isnew_nbp(), make sure it takes the lock explicitly.Signed-off-by Jakub Kicinski <[email protected]>--- net/bridge/br_if.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)net/bridge/br_if.c +2
-1@@ -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 intport_cost(struct net_device *dev)
{net/bridge/br_if.c +3
-1struct 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;
Quoted 1 line
p->flags=BR_LEARNING|BR_FLOOD|BR_MCAST_FLOOD|BR_BCAST_FLOOD;
Thanks
Nicolai
Re: [PATCH net-next v2 07/11] net: bridge: don't recurse on the port's netdev ops lock
In reply to Jakub KicinskiQuoted 43 lines · 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 theNETDEV_CHANGEnotifier 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 tonetif_get_link_ksettings(). The only other caller isnew_nbp(), make sure it takes the lock explicitly.Signed-off-by Jakub Kicinski <[email protected]>--- net/bridge/br_if.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)net/bridge/br_if.c +3
-2@@ -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 ;
net/bridge/br_if.c static struct net_bridge_port *new_nbp(struct net_bridge *br, · line 445
Quoted 9 lines
net/bridge/br_if.c +2
-0@@ -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,
Re: [PATCH net-next v2 07/11] net: bridge: don't recurse on the port's netdev ops lock
In reply to Jakub KicinskiQuoted 10 lines · 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 theNETDEV_CHANGEnotifier 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 tonetif_get_link_ksettings(). The only other caller isnew_nbp(), make sure it takes the lock explicitly.Signed-off-by Jakub Kicinski <[email protected]>
Jakub Kicinski
maintainerPATCH v2 8/11 net: sched: don't recurse on the netdev ops lock in qdiscs
In reply to Jakub Kicinskicbs_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.
--- net/sched/sch_cbs.c | 2 +- net/sched/sch_taprio.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
net/sched/sch_cbs.c +1 -1
@@ -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;
net/sched/sch_taprio.c +1 -1
@@ -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;
Signature · 1 line
2.54.0
Re: [PATCH net-next v2 08/11] net: sched: don't recurse on the netdev ops lock in qdiscs
Quoted 6 lines · Jakub Kicinski
cbs_set_port_rate()andtaprio_set_picos_per_byte()are reached from two paths and both already hold the device's ops lock:
*_change(), viatc_modify_qdisc()which callsnetdev_lock_ops(dev)
before dispatching to the qdisc ops.
Quoted 8 lines
*_dev_notifier()onNETDEV_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 <[email protected]>---
Cheers,
Signature · 1 line
Vinicius
Jakub Kicinski
maintainerPATCH v2 9/11 leds: trigger: netdev: don't recurse on the netdev ops lock
In reply to Jakub Kicinskiget_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.
--- drivers/leds/trigger/ledtrig-netdev.c | 37 +++++++++++++++------------ 1 file changed, 21 insertions(+), 16 deletions(-)
drivers/leds/trigger/ledtrig-netdev.c +21 -16
@@ -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 ;
Signature · 1 line
2.54.0
Jakub Kicinski
maintainerPATCH v2 10/11 scsi: fcoe: don't recurse on the netdev's ops lock
In reply to Jakub Kicinskifcoe_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.
--- 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(-)
drivers/scsi/bnx2fc/bnx2fc_fcoe.c +3 -0
@@ -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 ))
drivers/scsi/fcoe/fcoe.c +4 -2
@@ -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)) {
drivers/scsi/fcoe/fcoe_transport.c +3 -1
@@ -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 |
Signature · 1 line
2.54.0
Jakub Kicinski
maintainerPATCH v2 11/11 net: ethtool: make sure __ethtool_get_link_ksettings() is ops-locked
In reply to Jakub KicinskiAll 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().
--- net/ethtool/ioctl.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)
net/ethtool/ioctl.c +7 -3
@@ -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);
Signature · 1 line
2.54.0
Hi Jakub
Quoted 29 lines · 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 innetif_get_link_ksettings().Signed-off-by Jakub Kicinski <[email protected]>--- net/ethtool/ioctl.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)net/ethtool/ioctl.c +3
-1@@ -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; +
net/ethtool/ioctl.c EXPORT_SYMBOL(netif_get_link_ksettings); · line 468
Quoted 10 lines
net/ethtool/ioctl.c +4
-2ASSERT_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);
Thanks
Nicolai
Quoted 6 lines · 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 innetif_get_link_ksettings().Signed-off-by Jakub Kicinski <[email protected]>
This series was applied to netdev/net-next.git (main) by Jakub Kicinski <[email protected]>:
Hello:
This series was applied to netdev/net-next.git (main) by Jakub Kicinski <[email protected]>:
Quoted 10 lines · On Tue, 2 Jun 2026 18:28:29 -0700 you wrote:
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/[email protected][...]
Here is the summary with links:
- [net-next,v2,01/11] net: rename netdev_ops_assert_locked()
https://git.kernel.org/netdev/net-next/c/251bc9db88fb - [net-next,v2,02/11] net: ethtool: cmis_cdb: hold instance lock for ops locked devices https://git.kernel.org/netdev/net-next/c/1d3964e29e53 - [net-next,v2,03/11] net: document NETDEV_CHANGENAME as ops locked https://git.kernel.org/netdev/net-next/c/b8dfa196cb97 - [net-next,v2,04/11] net: ethtool: add netif_get_link_ksettings() for correct ops-locked use https://git.kernel.org/netdev/net-next/c/f32fe1d79a18 - [net-next,v2,05/11] net: bonding: don't recurse on the slave's netdev ops lock https://git.kernel.org/netdev/net-next/c/107b097790e9 - [net-next,v2,06/11] net: team: don't recurse on the port's netdev ops lock https://git.kernel.org/netdev/net-next/c/a6882c158326 - [net-next,v2,07/11] net: bridge: don't recurse on the port's netdev ops lock https://git.kernel.org/netdev/net-next/c/20398c41d001 - [net-next,v2,08/11] net: sched: don't recurse on the netdev ops lock in qdiscs https://git.kernel.org/netdev/net-next/c/e50f6245be53 - [net-next,v2,09/11] leds: trigger: netdev: don't recurse on the netdev ops lock https://git.kernel.org/netdev/net-next/c/ea8ea212d415 - [net-next,v2,10/11] scsi: fcoe: don't recurse on the netdev's ops lock https://git.kernel.org/netdev/net-next/c/af1737dab07c - [net-next,v2,11/11] net: ethtool: make sure __ethtool_get_link_ksettings() is ops-locked https://git.kernel.org/netdev/net-next/c/9f275c2e9020
You are awesome, thank you!
Signature · 2 lines
Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html