* [PATCH net-next 00/11] net: Hold netdev instance lock during ndo operations
@ 2025-02-10 19:20 Stanislav Fomichev
2025-02-10 19:20 ` [PATCH net-next 01/11] net: hold netdev instance lock during ndo_open/ndo_stop Stanislav Fomichev
` (10 more replies)
0 siblings, 11 replies; 22+ messages in thread
From: Stanislav Fomichev @ 2025-02-10 19:20 UTC (permalink / raw)
To: netdev; +Cc: davem, edumazet, kuba, pabeni, Saeed Mahameed
As the gradual purging of rtnl continues, start grabbing netdev
instance lock in more places so we can get to the state where
most paths are working without rtnl. Start with requiring the
drivers that use shaper api (and later queue mgmt api) to work
with both rtnl and netdev instance lock. Eventually we might
attempt to drop rtnl. This mostly affects iavf, gve, bnxt and
netdev sim (as the drivers that implement shaper/queue mgmt)
so those drivers are converted in the process.
call_netdevice_notifiers locking is very inconsistent and might need
a separate follow up. Some notified events are covered by the
instance lock, some are not, which might complicate the driver
expectations.
Changes since RFC:
- other control paths are protected
- bntx has been converted to mostly depend on netdev instance lock
Cc: Saeed Mahameed <saeed@kernel.org>
Jakub Kicinski (1):
net: ethtool: try to protect all callback with netdev instance lock
Stanislav Fomichev (10):
net: hold netdev instance lock during ndo_open/ndo_stop
net: hold netdev instance lock during ndo_setup_tc
net: hold netdev instance lock during queue operations
net: hold netdev instance lock during rtnetlink operations
net: hold netdev instance lock during ioctl operations
net: hold netdev instance lock during sysfs operations
net: hold netdev instance lock during ndo_bpf
net: dummy: add dummy shaper API
docs: net: document new locking reality
eth: bnxt: remove most dependencies on RTNL
Documentation/networking/netdevices.rst | 57 +++-
drivers/net/bonding/bond_main.c | 16 +-
drivers/net/dummy.c | 37 ++
drivers/net/ethernet/broadcom/bnxt/bnxt.c | 138 ++++----
.../net/ethernet/broadcom/bnxt/bnxt_devlink.c | 9 +
.../net/ethernet/broadcom/bnxt/bnxt_sriov.c | 6 +
drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.c | 16 +-
drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c | 18 +-
drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c | 3 +-
drivers/net/ethernet/google/gve/gve_main.c | 8 +-
drivers/net/ethernet/google/gve/gve_utils.c | 8 +-
drivers/net/ethernet/intel/iavf/iavf_main.c | 16 +-
drivers/net/netdevsim/ethtool.c | 2 -
drivers/net/netdevsim/netdev.c | 36 +-
include/linux/netdevice.h | 63 +++-
kernel/bpf/offload.c | 7 +-
net/8021q/vlan_dev.c | 4 +-
net/core/Makefile | 2 +-
net/core/dev.c | 255 +++++---------
net/core/dev.h | 19 +-
net/core/dev_api.c | 315 ++++++++++++++++++
net/core/dev_ioctl.c | 67 ++--
net/core/net-sysfs.c | 2 +
net/core/netdev_rx_queue.c | 5 +
net/core/rtnetlink.c | 47 ++-
net/dsa/conduit.c | 16 +-
net/dsa/user.c | 5 +-
net/ethtool/cabletest.c | 20 +-
net/ethtool/features.c | 6 +-
net/ethtool/ioctl.c | 6 +
net/ethtool/module.c | 8 +-
net/ethtool/netlink.c | 12 +
net/ethtool/phy.c | 20 +-
net/ethtool/rss.c | 2 +
net/ethtool/tsinfo.c | 9 +-
net/netfilter/nf_flow_table_offload.c | 2 +-
net/netfilter/nf_tables_offload.c | 2 +-
net/sched/cls_api.c | 2 +-
net/sched/sch_api.c | 13 +-
net/sched/sch_cbs.c | 9 +-
net/sched/sch_etf.c | 9 +-
net/sched/sch_ets.c | 10 +-
net/sched/sch_fifo.c | 10 +-
net/sched/sch_gred.c | 5 +-
net/sched/sch_htb.c | 2 +-
net/sched/sch_mq.c | 5 +-
net/sched/sch_mqprio.c | 6 +-
net/sched/sch_prio.c | 5 +-
net/sched/sch_red.c | 8 +-
net/sched/sch_taprio.c | 22 +-
net/sched/sch_tbf.c | 10 +-
net/xdp/xsk.c | 3 +
net/xdp/xsk_buff_pool.c | 2 +
53 files changed, 912 insertions(+), 473 deletions(-)
create mode 100644 net/core/dev_api.c
--
2.48.1
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH net-next 01/11] net: hold netdev instance lock during ndo_open/ndo_stop
2025-02-10 19:20 [PATCH net-next 00/11] net: Hold netdev instance lock during ndo operations Stanislav Fomichev
@ 2025-02-10 19:20 ` Stanislav Fomichev
2025-02-12 2:06 ` Jakub Kicinski
2025-02-10 19:20 ` [PATCH net-next 02/11] net: hold netdev instance lock during ndo_setup_tc Stanislav Fomichev
` (9 subsequent siblings)
10 siblings, 1 reply; 22+ messages in thread
From: Stanislav Fomichev @ 2025-02-10 19:20 UTC (permalink / raw)
To: netdev; +Cc: davem, edumazet, kuba, pabeni, Saeed Mahameed
For the drivers that use shaper API, switch to the mode where
core stack holds the netdev lock. This affects two drivers:
* iavf - already grabs netdev lock in ndo_open/ndo_stop, so mostly
remove these
* netdevsim - switch to _locked APIs to avoid deadlock
iavf_close diff is a bit confusing, the existing call looks like this:
iavf_close() {
netdev_lock()
..
netdev_unlock()
wait_event_timeout(down_waitqueue)
}
I change it to the following:
netdev_lock()
iavf_close() {
..
netdev_unlock()
wait_event_timeout(down_waitqueue)
netdev_lock() // reusing this lock call
}
netdev_unlock()
Since I'm reusing existing netdev_lock call, so it looks like I only
add netdev_unlock.
Cc: Saeed Mahameed <saeed@kernel.org>
Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
---
drivers/net/ethernet/intel/iavf/iavf_main.c | 14 ++++++-------
drivers/net/netdevsim/netdev.c | 14 ++++++++-----
include/linux/netdevice.h | 23 +++++++++++++++++++++
net/core/dev.c | 12 +++++++++++
net/core/dev.h | 6 ++++--
5 files changed, 54 insertions(+), 15 deletions(-)
diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
index 2d7a18fcc3be..176f9bb871d0 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_main.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
@@ -4375,22 +4375,21 @@ static int iavf_open(struct net_device *netdev)
struct iavf_adapter *adapter = netdev_priv(netdev);
int err;
+ netdev_assert_locked(netdev);
+
if (adapter->flags & IAVF_FLAG_PF_COMMS_FAILED) {
dev_err(&adapter->pdev->dev, "Unable to open device due to PF driver failure.\n");
return -EIO;
}
- netdev_lock(netdev);
while (!mutex_trylock(&adapter->crit_lock)) {
/* If we are in __IAVF_INIT_CONFIG_ADAPTER state the crit_lock
* is already taken and iavf_open is called from an upper
* device's notifier reacting on NETDEV_REGISTER event.
* We have to leave here to avoid dead lock.
*/
- if (adapter->state == __IAVF_INIT_CONFIG_ADAPTER) {
- netdev_unlock(netdev);
+ if (adapter->state == __IAVF_INIT_CONFIG_ADAPTER)
return -EBUSY;
- }
usleep_range(500, 1000);
}
@@ -4439,7 +4438,6 @@ static int iavf_open(struct net_device *netdev)
iavf_irq_enable(adapter, true);
mutex_unlock(&adapter->crit_lock);
- netdev_unlock(netdev);
return 0;
@@ -4452,7 +4450,6 @@ static int iavf_open(struct net_device *netdev)
iavf_free_all_tx_resources(adapter);
err_unlock:
mutex_unlock(&adapter->crit_lock);
- netdev_unlock(netdev);
return err;
}
@@ -4474,12 +4471,12 @@ static int iavf_close(struct net_device *netdev)
u64 aq_to_restore;
int status;
- netdev_lock(netdev);
+ netdev_assert_locked(netdev);
+
mutex_lock(&adapter->crit_lock);
if (adapter->state <= __IAVF_DOWN_PENDING) {
mutex_unlock(&adapter->crit_lock);
- netdev_unlock(netdev);
return 0;
}
@@ -4532,6 +4529,7 @@ static int iavf_close(struct net_device *netdev)
if (!status)
netdev_warn(netdev, "Device resources not yet released\n");
+ netdev_lock(netdev);
mutex_lock(&adapter->crit_lock);
adapter->aq_required |= aq_to_restore;
mutex_unlock(&adapter->crit_lock);
diff --git a/drivers/net/netdevsim/netdev.c b/drivers/net/netdevsim/netdev.c
index 9b394ddc5206..d857ae3c8f06 100644
--- a/drivers/net/netdevsim/netdev.c
+++ b/drivers/net/netdevsim/netdev.c
@@ -401,7 +401,7 @@ static int nsim_init_napi(struct netdevsim *ns)
for (i = 0; i < dev->num_rx_queues; i++) {
rq = ns->rq[i];
- netif_napi_add_config(dev, &rq->napi, nsim_poll, i);
+ netif_napi_add_config_locked(dev, &rq->napi, nsim_poll, i);
}
for (i = 0; i < dev->num_rx_queues; i++) {
@@ -421,7 +421,7 @@ static int nsim_init_napi(struct netdevsim *ns)
}
for (i = 0; i < dev->num_rx_queues; i++)
- __netif_napi_del(&ns->rq[i]->napi);
+ __netif_napi_del_locked(&ns->rq[i]->napi);
return err;
}
@@ -435,7 +435,7 @@ static void nsim_enable_napi(struct netdevsim *ns)
struct nsim_rq *rq = ns->rq[i];
netif_queue_set_napi(dev, i, NETDEV_QUEUE_TYPE_RX, &rq->napi);
- napi_enable(&rq->napi);
+ napi_enable_locked(&rq->napi);
}
}
@@ -444,6 +444,8 @@ static int nsim_open(struct net_device *dev)
struct netdevsim *ns = netdev_priv(dev);
int err;
+ netdev_assert_locked(dev);
+
err = nsim_init_napi(ns);
if (err)
return err;
@@ -461,8 +463,8 @@ static void nsim_del_napi(struct netdevsim *ns)
for (i = 0; i < dev->num_rx_queues; i++) {
struct nsim_rq *rq = ns->rq[i];
- napi_disable(&rq->napi);
- __netif_napi_del(&rq->napi);
+ napi_disable_locked(&rq->napi);
+ __netif_napi_del_locked(&rq->napi);
}
synchronize_net();
@@ -477,6 +479,8 @@ static int nsim_stop(struct net_device *dev)
struct netdevsim *ns = netdev_priv(dev);
struct netdevsim *peer;
+ netdev_assert_locked(dev);
+
netif_carrier_off(dev);
peer = rtnl_dereference(ns->peer);
if (peer)
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 5429581f2299..308c7fc888b3 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -2718,6 +2718,29 @@ static inline void netdev_assert_locked_or_invisible(struct net_device *dev)
netdev_assert_locked(dev);
}
+static inline bool need_netdev_ops_lock(struct net_device *dev)
+{
+ bool ret = false;
+
+#if IS_ENABLED(CONFIG_NET_SHAPER)
+ ret |= !!(dev)->netdev_ops->net_shaper_ops;
+#endif
+
+ return ret;
+}
+
+static inline void netdev_lock_ops(struct net_device *dev)
+{
+ if (need_netdev_ops_lock(dev))
+ netdev_lock(dev);
+}
+
+static inline void netdev_unlock_ops(struct net_device *dev)
+{
+ if (need_netdev_ops_lock(dev))
+ netdev_unlock(dev);
+}
+
static inline void netif_napi_set_irq_locked(struct napi_struct *napi, int irq)
{
napi->irq = irq;
diff --git a/net/core/dev.c b/net/core/dev.c
index d5ab9a4b318e..23262f1d8839 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1596,6 +1596,8 @@ static int __dev_open(struct net_device *dev, struct netlink_ext_ack *extack)
if (ret)
return ret;
+ netdev_lock_ops(dev);
+
set_bit(__LINK_STATE_START, &dev->state);
if (ops->ndo_validate_addr)
@@ -1615,6 +1617,8 @@ static int __dev_open(struct net_device *dev, struct netlink_ext_ack *extack)
add_device_randomness(dev->dev_addr, dev->addr_len);
}
+ netdev_unlock_ops(dev);
+
return ret;
}
@@ -1685,11 +1689,19 @@ static void __dev_close_many(struct list_head *head)
* We allow it to be called even after a DETACH hot-plug
* event.
*/
+
+ /* TODO: move the lock up before clearing __LINK_STATE_START.
+ * Generates spurious lockdep warning.
+ */
+ netdev_lock_ops(dev);
+
if (ops->ndo_stop)
ops->ndo_stop(dev);
netif_set_up(dev, false);
netpoll_poll_enable(dev);
+
+ netdev_unlock_ops(dev);
}
}
diff --git a/net/core/dev.h b/net/core/dev.h
index caa13e431a6b..a966b6936009 100644
--- a/net/core/dev.h
+++ b/net/core/dev.h
@@ -134,9 +134,11 @@ static inline void netif_set_up(struct net_device *dev, bool value)
else
dev->flags &= ~IFF_UP;
- netdev_lock(dev);
+ if (!need_netdev_ops_lock(dev))
+ netdev_lock(dev);
dev->up = value;
- netdev_unlock(dev);
+ if (!need_netdev_ops_lock(dev))
+ netdev_unlock(dev);
}
static inline void netif_set_gso_max_size(struct net_device *dev,
--
2.48.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH net-next 02/11] net: hold netdev instance lock during ndo_setup_tc
2025-02-10 19:20 [PATCH net-next 00/11] net: Hold netdev instance lock during ndo operations Stanislav Fomichev
2025-02-10 19:20 ` [PATCH net-next 01/11] net: hold netdev instance lock during ndo_open/ndo_stop Stanislav Fomichev
@ 2025-02-10 19:20 ` Stanislav Fomichev
2025-02-12 2:20 ` Jakub Kicinski
2025-02-10 19:20 ` [PATCH net-next 03/11] net: hold netdev instance lock during queue operations Stanislav Fomichev
` (8 subsequent siblings)
10 siblings, 1 reply; 22+ messages in thread
From: Stanislav Fomichev @ 2025-02-10 19:20 UTC (permalink / raw)
To: netdev; +Cc: davem, edumazet, kuba, pabeni, Saeed Mahameed
Introduce new dev_setup_tc that handles the details and call it from
all qdiscs/classifiers. The instance lock is still applied only to
the drivers that implement shaper API so only iavf is affected.
Cc: Saeed Mahameed <saeed@kernel.org>
Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
---
drivers/net/ethernet/intel/iavf/iavf_main.c | 2 --
include/linux/netdevice.h | 2 ++
net/core/dev.c | 22 +++++++++++++++++++++
net/dsa/user.c | 5 +----
net/netfilter/nf_flow_table_offload.c | 2 +-
net/netfilter/nf_tables_offload.c | 2 +-
net/sched/cls_api.c | 2 +-
net/sched/sch_api.c | 13 +++---------
net/sched/sch_cbs.c | 9 ++-------
net/sched/sch_etf.c | 9 ++-------
net/sched/sch_ets.c | 10 ++--------
net/sched/sch_fifo.c | 10 ++--------
net/sched/sch_gred.c | 5 +----
net/sched/sch_htb.c | 2 +-
net/sched/sch_mq.c | 5 +----
net/sched/sch_mqprio.c | 6 ++----
net/sched/sch_prio.c | 5 +----
net/sched/sch_red.c | 8 ++------
net/sched/sch_taprio.c | 17 +++-------------
net/sched/sch_tbf.c | 10 ++--------
20 files changed, 52 insertions(+), 94 deletions(-)
diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
index 176f9bb871d0..4fe481433842 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_main.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
@@ -3707,10 +3707,8 @@ static int __iavf_setup_tc(struct net_device *netdev, void *type_data)
if (test_bit(__IAVF_IN_REMOVE_TASK, &adapter->crit_section))
return 0;
- netdev_lock(netdev);
netif_set_real_num_rx_queues(netdev, total_qps);
netif_set_real_num_tx_queues(netdev, total_qps);
- netdev_unlock(netdev);
return ret;
}
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 308c7fc888b3..06f24dc4f46e 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -3316,6 +3316,8 @@ int dev_alloc_name(struct net_device *dev, const char *name);
int dev_open(struct net_device *dev, struct netlink_ext_ack *extack);
void dev_close(struct net_device *dev);
void dev_close_many(struct list_head *head, bool unlink);
+int dev_setup_tc(struct net_device *dev, enum tc_setup_type type,
+ void *type_data);
void dev_disable_lro(struct net_device *dev);
int dev_loopback_xmit(struct net *net, struct sock *sk, struct sk_buff *newskb);
u16 dev_pick_tx_zero(struct net_device *dev, struct sk_buff *skb,
diff --git a/net/core/dev.c b/net/core/dev.c
index 23262f1d8839..a8df26f8ffe5 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1755,6 +1755,28 @@ void dev_close(struct net_device *dev)
}
EXPORT_SYMBOL(dev_close);
+int dev_setup_tc(struct net_device *dev, enum tc_setup_type type,
+ void *type_data)
+{
+ const struct net_device_ops *ops = dev->netdev_ops;
+
+ ASSERT_RTNL();
+
+ if (tc_can_offload(dev) && ops->ndo_setup_tc) {
+ int ret = -ENODEV;
+
+ if (netif_device_present(dev)) {
+ netdev_lock_ops(dev);
+ ret = ops->ndo_setup_tc(dev, type, type_data);
+ netdev_unlock_ops(dev);
+ }
+
+ return ret;
+ }
+
+ return -EOPNOTSUPP;
+}
+EXPORT_SYMBOL(dev_setup_tc);
/**
* dev_disable_lro - disable Large Receive Offload on a device
diff --git a/net/dsa/user.c b/net/dsa/user.c
index 291ab1b4acc4..f2ac7662e4cc 100644
--- a/net/dsa/user.c
+++ b/net/dsa/user.c
@@ -1729,10 +1729,7 @@ static int dsa_user_setup_ft_block(struct dsa_switch *ds, int port,
{
struct net_device *conduit = dsa_port_to_conduit(dsa_to_port(ds, port));
- if (!conduit->netdev_ops->ndo_setup_tc)
- return -EOPNOTSUPP;
-
- return conduit->netdev_ops->ndo_setup_tc(conduit, TC_SETUP_FT, type_data);
+ return dev_setup_tc(conduit, TC_SETUP_FT, type_data);
}
static int dsa_user_setup_tc(struct net_device *dev, enum tc_setup_type type,
diff --git a/net/netfilter/nf_flow_table_offload.c b/net/netfilter/nf_flow_table_offload.c
index e06bc36f49fe..0ec4abded10d 100644
--- a/net/netfilter/nf_flow_table_offload.c
+++ b/net/netfilter/nf_flow_table_offload.c
@@ -1175,7 +1175,7 @@ static int nf_flow_table_offload_cmd(struct flow_block_offload *bo,
nf_flow_table_block_offload_init(bo, dev_net(dev), cmd, flowtable,
extack);
down_write(&flowtable->flow_block_lock);
- err = dev->netdev_ops->ndo_setup_tc(dev, TC_SETUP_FT, bo);
+ err = dev_setup_tc(dev, TC_SETUP_FT, bo);
up_write(&flowtable->flow_block_lock);
if (err < 0)
return err;
diff --git a/net/netfilter/nf_tables_offload.c b/net/netfilter/nf_tables_offload.c
index 64675f1c7f29..b761899c143c 100644
--- a/net/netfilter/nf_tables_offload.c
+++ b/net/netfilter/nf_tables_offload.c
@@ -390,7 +390,7 @@ static int nft_block_offload_cmd(struct nft_base_chain *chain,
nft_flow_block_offload_init(&bo, dev_net(dev), cmd, chain, &extack);
- err = dev->netdev_ops->ndo_setup_tc(dev, TC_SETUP_BLOCK, &bo);
+ err = dev_setup_tc(dev, TC_SETUP_BLOCK, &bo);
if (err < 0)
return err;
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 8e47e5355be6..082b355be8e6 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -835,7 +835,7 @@ static int tcf_block_offload_cmd(struct tcf_block *block,
if (dev->netdev_ops->ndo_setup_tc) {
int err;
- err = dev->netdev_ops->ndo_setup_tc(dev, TC_SETUP_BLOCK, &bo);
+ err = dev_setup_tc(dev, TC_SETUP_BLOCK, &bo);
if (err < 0) {
if (err != -EOPNOTSUPP)
NL_SET_ERR_MSG(extack, "Driver ndo_setup_tc failed");
diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
index e3e91cf867eb..6d0728cfc19f 100644
--- a/net/sched/sch_api.c
+++ b/net/sched/sch_api.c
@@ -833,10 +833,8 @@ int qdisc_offload_dump_helper(struct Qdisc *sch, enum tc_setup_type type,
int err;
sch->flags &= ~TCQ_F_OFFLOADED;
- if (!tc_can_offload(dev) || !dev->netdev_ops->ndo_setup_tc)
- return 0;
- err = dev->netdev_ops->ndo_setup_tc(dev, type, type_data);
+ err = dev_setup_tc(dev, type, type_data);
if (err == -EOPNOTSUPP)
return 0;
@@ -855,10 +853,7 @@ void qdisc_offload_graft_helper(struct net_device *dev, struct Qdisc *sch,
bool any_qdisc_is_offloaded;
int err;
- if (!tc_can_offload(dev) || !dev->netdev_ops->ndo_setup_tc)
- return;
-
- err = dev->netdev_ops->ndo_setup_tc(dev, type, type_data);
+ err = dev_setup_tc(dev, type, type_data);
/* Don't report error if the graft is part of destroy operation. */
if (!err || !new || new == &noop_qdisc)
@@ -880,7 +875,6 @@ void qdisc_offload_query_caps(struct net_device *dev,
enum tc_setup_type type,
void *caps, size_t caps_len)
{
- const struct net_device_ops *ops = dev->netdev_ops;
struct tc_query_caps_base base = {
.type = type,
.caps = caps,
@@ -888,8 +882,7 @@ void qdisc_offload_query_caps(struct net_device *dev,
memset(caps, 0, caps_len);
- if (ops->ndo_setup_tc)
- ops->ndo_setup_tc(dev, TC_QUERY_CAPS, &base);
+ dev_setup_tc(dev, TC_QUERY_CAPS, &base);
}
EXPORT_SYMBOL(qdisc_offload_query_caps);
diff --git a/net/sched/sch_cbs.c b/net/sched/sch_cbs.c
index 8c9a0400c862..492a98fa209b 100644
--- a/net/sched/sch_cbs.c
+++ b/net/sched/sch_cbs.c
@@ -251,7 +251,6 @@ static void cbs_disable_offload(struct net_device *dev,
struct cbs_sched_data *q)
{
struct tc_cbs_qopt_offload cbs = { };
- const struct net_device_ops *ops;
int err;
if (!q->offload)
@@ -260,14 +259,10 @@ static void cbs_disable_offload(struct net_device *dev,
q->enqueue = cbs_enqueue_soft;
q->dequeue = cbs_dequeue_soft;
- ops = dev->netdev_ops;
- if (!ops->ndo_setup_tc)
- return;
-
cbs.queue = q->queue;
cbs.enable = 0;
- err = ops->ndo_setup_tc(dev, TC_SETUP_QDISC_CBS, &cbs);
+ err = dev_setup_tc(dev, TC_SETUP_QDISC_CBS, &cbs);
if (err < 0)
pr_warn("Couldn't disable CBS offload for queue %d\n",
cbs.queue);
@@ -294,7 +289,7 @@ static int cbs_enable_offload(struct net_device *dev, struct cbs_sched_data *q,
cbs.idleslope = opt->idleslope;
cbs.sendslope = opt->sendslope;
- err = ops->ndo_setup_tc(dev, TC_SETUP_QDISC_CBS, &cbs);
+ err = dev_setup_tc(dev, TC_SETUP_QDISC_CBS, &cbs);
if (err < 0) {
NL_SET_ERR_MSG(extack, "Specified device failed to setup cbs hardware offload");
return err;
diff --git a/net/sched/sch_etf.c b/net/sched/sch_etf.c
index c74d778c32a1..f183c2e9a4e8 100644
--- a/net/sched/sch_etf.c
+++ b/net/sched/sch_etf.c
@@ -297,20 +297,15 @@ static void etf_disable_offload(struct net_device *dev,
struct etf_sched_data *q)
{
struct tc_etf_qopt_offload etf = { };
- const struct net_device_ops *ops;
int err;
if (!q->offload)
return;
- ops = dev->netdev_ops;
- if (!ops->ndo_setup_tc)
- return;
-
etf.queue = q->queue;
etf.enable = 0;
- err = ops->ndo_setup_tc(dev, TC_SETUP_QDISC_ETF, &etf);
+ err = dev_setup_tc(dev, TC_SETUP_QDISC_ETF, &etf);
if (err < 0)
pr_warn("Couldn't disable ETF offload for queue %d\n",
etf.queue);
@@ -331,7 +326,7 @@ static int etf_enable_offload(struct net_device *dev, struct etf_sched_data *q,
etf.queue = q->queue;
etf.enable = 1;
- err = ops->ndo_setup_tc(dev, TC_SETUP_QDISC_ETF, &etf);
+ err = dev_setup_tc(dev, TC_SETUP_QDISC_ETF, &etf);
if (err < 0) {
NL_SET_ERR_MSG(extack, "Specified device failed to setup ETF hardware offload");
return err;
diff --git a/net/sched/sch_ets.c b/net/sched/sch_ets.c
index 516038a44163..1757dd3b0552 100644
--- a/net/sched/sch_ets.c
+++ b/net/sched/sch_ets.c
@@ -117,9 +117,6 @@ static void ets_offload_change(struct Qdisc *sch)
unsigned int weight;
unsigned int i;
- if (!tc_can_offload(dev) || !dev->netdev_ops->ndo_setup_tc)
- return;
-
qopt.command = TC_ETS_REPLACE;
qopt.handle = sch->handle;
qopt.parent = sch->parent;
@@ -142,7 +139,7 @@ static void ets_offload_change(struct Qdisc *sch)
qopt.replace_params.weights[i] = weight;
}
- dev->netdev_ops->ndo_setup_tc(dev, TC_SETUP_QDISC_ETS, &qopt);
+ dev_setup_tc(dev, TC_SETUP_QDISC_ETS, &qopt);
}
static void ets_offload_destroy(struct Qdisc *sch)
@@ -150,13 +147,10 @@ static void ets_offload_destroy(struct Qdisc *sch)
struct net_device *dev = qdisc_dev(sch);
struct tc_ets_qopt_offload qopt;
- if (!tc_can_offload(dev) || !dev->netdev_ops->ndo_setup_tc)
- return;
-
qopt.command = TC_ETS_DESTROY;
qopt.handle = sch->handle;
qopt.parent = sch->parent;
- dev->netdev_ops->ndo_setup_tc(dev, TC_SETUP_QDISC_ETS, &qopt);
+ dev_setup_tc(dev, TC_SETUP_QDISC_ETS, &qopt);
}
static void ets_offload_graft(struct Qdisc *sch, struct Qdisc *new,
diff --git a/net/sched/sch_fifo.c b/net/sched/sch_fifo.c
index e6bfd39ff339..f5c31af3a749 100644
--- a/net/sched/sch_fifo.c
+++ b/net/sched/sch_fifo.c
@@ -61,13 +61,10 @@ static void fifo_offload_init(struct Qdisc *sch)
struct net_device *dev = qdisc_dev(sch);
struct tc_fifo_qopt_offload qopt;
- if (!tc_can_offload(dev) || !dev->netdev_ops->ndo_setup_tc)
- return;
-
qopt.command = TC_FIFO_REPLACE;
qopt.handle = sch->handle;
qopt.parent = sch->parent;
- dev->netdev_ops->ndo_setup_tc(dev, TC_SETUP_QDISC_FIFO, &qopt);
+ dev_setup_tc(dev, TC_SETUP_QDISC_FIFO, &qopt);
}
static void fifo_offload_destroy(struct Qdisc *sch)
@@ -75,13 +72,10 @@ static void fifo_offload_destroy(struct Qdisc *sch)
struct net_device *dev = qdisc_dev(sch);
struct tc_fifo_qopt_offload qopt;
- if (!tc_can_offload(dev) || !dev->netdev_ops->ndo_setup_tc)
- return;
-
qopt.command = TC_FIFO_DESTROY;
qopt.handle = sch->handle;
qopt.parent = sch->parent;
- dev->netdev_ops->ndo_setup_tc(dev, TC_SETUP_QDISC_FIFO, &qopt);
+ dev_setup_tc(dev, TC_SETUP_QDISC_FIFO, &qopt);
}
static int fifo_offload_dump(struct Qdisc *sch)
diff --git a/net/sched/sch_gred.c b/net/sched/sch_gred.c
index ab6234b4fcd5..d1a29842a2b4 100644
--- a/net/sched/sch_gred.c
+++ b/net/sched/sch_gred.c
@@ -314,9 +314,6 @@ static void gred_offload(struct Qdisc *sch, enum tc_gred_command command)
struct net_device *dev = qdisc_dev(sch);
struct tc_gred_qopt_offload *opt = table->opt;
- if (!tc_can_offload(dev) || !dev->netdev_ops->ndo_setup_tc)
- return;
-
memset(opt, 0, sizeof(*opt));
opt->command = command;
opt->handle = sch->handle;
@@ -348,7 +345,7 @@ static void gred_offload(struct Qdisc *sch, enum tc_gred_command command)
opt->set.qstats = &sch->qstats;
}
- dev->netdev_ops->ndo_setup_tc(dev, TC_SETUP_QDISC_GRED, opt);
+ dev_setup_tc(dev, TC_SETUP_QDISC_GRED, opt);
}
static int gred_offload_dump_stats(struct Qdisc *sch)
diff --git a/net/sched/sch_htb.c b/net/sched/sch_htb.c
index c31bc5489bdd..ed406b3ceb7b 100644
--- a/net/sched/sch_htb.c
+++ b/net/sched/sch_htb.c
@@ -1041,7 +1041,7 @@ static void htb_work_func(struct work_struct *work)
static int htb_offload(struct net_device *dev, struct tc_htb_qopt_offload *opt)
{
- return dev->netdev_ops->ndo_setup_tc(dev, TC_SETUP_QDISC_HTB, opt);
+ return dev_setup_tc(dev, TC_SETUP_QDISC_HTB, opt);
}
static int htb_init(struct Qdisc *sch, struct nlattr *opt,
diff --git a/net/sched/sch_mq.c b/net/sched/sch_mq.c
index c860119a8f09..a5ba59728d63 100644
--- a/net/sched/sch_mq.c
+++ b/net/sched/sch_mq.c
@@ -29,10 +29,7 @@ static int mq_offload(struct Qdisc *sch, enum tc_mq_command cmd)
.handle = sch->handle,
};
- if (!tc_can_offload(dev) || !dev->netdev_ops->ndo_setup_tc)
- return -EOPNOTSUPP;
-
- return dev->netdev_ops->ndo_setup_tc(dev, TC_SETUP_QDISC_MQ, &opt);
+ return dev_setup_tc(dev, TC_SETUP_QDISC_MQ, &opt);
}
static int mq_offload_stats(struct Qdisc *sch)
diff --git a/net/sched/sch_mqprio.c b/net/sched/sch_mqprio.c
index 51d4013b6121..2c9a90b83c2b 100644
--- a/net/sched/sch_mqprio.c
+++ b/net/sched/sch_mqprio.c
@@ -67,8 +67,7 @@ static int mqprio_enable_offload(struct Qdisc *sch,
mqprio_fp_to_offload(priv->fp, &mqprio);
- err = dev->netdev_ops->ndo_setup_tc(dev, TC_SETUP_QDISC_MQPRIO,
- &mqprio);
+ err = dev_setup_tc(dev, TC_SETUP_QDISC_MQPRIO, &mqprio);
if (err)
return err;
@@ -86,8 +85,7 @@ static void mqprio_disable_offload(struct Qdisc *sch)
switch (priv->mode) {
case TC_MQPRIO_MODE_DCB:
case TC_MQPRIO_MODE_CHANNEL:
- dev->netdev_ops->ndo_setup_tc(dev, TC_SETUP_QDISC_MQPRIO,
- &mqprio);
+ dev_setup_tc(dev, TC_SETUP_QDISC_MQPRIO, &mqprio);
break;
}
}
diff --git a/net/sched/sch_prio.c b/net/sched/sch_prio.c
index cc30f7a32f1a..276e290b4071 100644
--- a/net/sched/sch_prio.c
+++ b/net/sched/sch_prio.c
@@ -145,9 +145,6 @@ static int prio_offload(struct Qdisc *sch, struct tc_prio_qopt *qopt)
.parent = sch->parent,
};
- if (!tc_can_offload(dev) || !dev->netdev_ops->ndo_setup_tc)
- return -EOPNOTSUPP;
-
if (qopt) {
opt.command = TC_PRIO_REPLACE;
opt.replace_params.bands = qopt->bands;
@@ -158,7 +155,7 @@ static int prio_offload(struct Qdisc *sch, struct tc_prio_qopt *qopt)
opt.command = TC_PRIO_DESTROY;
}
- return dev->netdev_ops->ndo_setup_tc(dev, TC_SETUP_QDISC_PRIO, &opt);
+ return dev_setup_tc(dev, TC_SETUP_QDISC_PRIO, &opt);
}
static void
diff --git a/net/sched/sch_red.c b/net/sched/sch_red.c
index ef8a2afed26b..235ec57b29b8 100644
--- a/net/sched/sch_red.c
+++ b/net/sched/sch_red.c
@@ -192,9 +192,6 @@ static int red_offload(struct Qdisc *sch, bool enable)
.parent = sch->parent,
};
- if (!tc_can_offload(dev) || !dev->netdev_ops->ndo_setup_tc)
- return -EOPNOTSUPP;
-
if (enable) {
opt.command = TC_RED_REPLACE;
opt.set.min = q->parms.qth_min >> q->parms.Wlog;
@@ -209,7 +206,7 @@ static int red_offload(struct Qdisc *sch, bool enable)
opt.command = TC_RED_DESTROY;
}
- return dev->netdev_ops->ndo_setup_tc(dev, TC_SETUP_QDISC_RED, &opt);
+ return dev_setup_tc(dev, TC_SETUP_QDISC_RED, &opt);
}
static void red_destroy(struct Qdisc *sch)
@@ -460,8 +457,7 @@ static int red_dump_stats(struct Qdisc *sch, struct gnet_dump *d)
.xstats = &q->stats,
},
};
- dev->netdev_ops->ndo_setup_tc(dev, TC_SETUP_QDISC_RED,
- &hw_stats_request);
+ dev_setup_tc(dev, TC_SETUP_QDISC_RED, &hw_stats_request);
}
st.early = q->stats.prob_drop + q->stats.forced_drop;
st.pdrop = q->stats.pdrop;
diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
index a68e17891b0b..e35cf13eb4bc 100644
--- a/net/sched/sch_taprio.c
+++ b/net/sched/sch_taprio.c
@@ -1539,7 +1539,7 @@ static int taprio_enable_offload(struct net_device *dev,
for (tc = 0; tc < TC_MAX_QUEUE; tc++)
offload->max_sdu[tc] = q->max_sdu[tc];
- err = ops->ndo_setup_tc(dev, TC_SETUP_QDISC_TAPRIO, offload);
+ err = dev_setup_tc(dev, TC_SETUP_QDISC_TAPRIO, offload);
if (err < 0) {
NL_SET_ERR_MSG_WEAK(extack,
"Device failed to setup taprio offload");
@@ -1564,7 +1564,6 @@ static int taprio_disable_offload(struct net_device *dev,
struct taprio_sched *q,
struct netlink_ext_ack *extack)
{
- const struct net_device_ops *ops = dev->netdev_ops;
struct tc_taprio_qopt_offload *offload;
int err;
@@ -1579,7 +1578,7 @@ static int taprio_disable_offload(struct net_device *dev,
}
offload->cmd = TAPRIO_CMD_DESTROY;
- err = ops->ndo_setup_tc(dev, TC_SETUP_QDISC_TAPRIO, offload);
+ err = dev_setup_tc(dev, TC_SETUP_QDISC_TAPRIO, offload);
if (err < 0) {
NL_SET_ERR_MSG(extack,
"Device failed to disable offload");
@@ -2314,24 +2313,14 @@ static int taprio_dump_xstats(struct Qdisc *sch, struct gnet_dump *d,
struct tc_taprio_qopt_stats *stats)
{
struct net_device *dev = qdisc_dev(sch);
- const struct net_device_ops *ops;
struct sk_buff *skb = d->skb;
struct nlattr *xstats;
int err;
- ops = qdisc_dev(sch)->netdev_ops;
-
- /* FIXME I could use qdisc_offload_dump_helper(), but that messes
- * with sch->flags depending on whether the device reports taprio
- * stats, and I'm not sure whether that's a good idea, considering
- * that stats are optional to the offload itself
- */
- if (!ops->ndo_setup_tc)
- return 0;
memset(stats, 0xff, sizeof(*stats));
- err = ops->ndo_setup_tc(dev, TC_SETUP_QDISC_TAPRIO, offload);
+ err = dev_setup_tc(dev, TC_SETUP_QDISC_TAPRIO, offload);
if (err == -EOPNOTSUPP)
return 0;
if (err)
diff --git a/net/sched/sch_tbf.c b/net/sched/sch_tbf.c
index dc26b22d53c7..5a2133716446 100644
--- a/net/sched/sch_tbf.c
+++ b/net/sched/sch_tbf.c
@@ -145,9 +145,6 @@ static void tbf_offload_change(struct Qdisc *sch)
struct net_device *dev = qdisc_dev(sch);
struct tc_tbf_qopt_offload qopt;
- if (!tc_can_offload(dev) || !dev->netdev_ops->ndo_setup_tc)
- return;
-
qopt.command = TC_TBF_REPLACE;
qopt.handle = sch->handle;
qopt.parent = sch->parent;
@@ -155,7 +152,7 @@ static void tbf_offload_change(struct Qdisc *sch)
qopt.replace_params.max_size = q->max_size;
qopt.replace_params.qstats = &sch->qstats;
- dev->netdev_ops->ndo_setup_tc(dev, TC_SETUP_QDISC_TBF, &qopt);
+ dev_setup_tc(dev, TC_SETUP_QDISC_TBF, &qopt);
}
static void tbf_offload_destroy(struct Qdisc *sch)
@@ -163,13 +160,10 @@ static void tbf_offload_destroy(struct Qdisc *sch)
struct net_device *dev = qdisc_dev(sch);
struct tc_tbf_qopt_offload qopt;
- if (!tc_can_offload(dev) || !dev->netdev_ops->ndo_setup_tc)
- return;
-
qopt.command = TC_TBF_DESTROY;
qopt.handle = sch->handle;
qopt.parent = sch->parent;
- dev->netdev_ops->ndo_setup_tc(dev, TC_SETUP_QDISC_TBF, &qopt);
+ dev_setup_tc(dev, TC_SETUP_QDISC_TBF, &qopt);
}
static int tbf_offload_dump(struct Qdisc *sch)
--
2.48.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH net-next 03/11] net: hold netdev instance lock during queue operations
2025-02-10 19:20 [PATCH net-next 00/11] net: Hold netdev instance lock during ndo operations Stanislav Fomichev
2025-02-10 19:20 ` [PATCH net-next 01/11] net: hold netdev instance lock during ndo_open/ndo_stop Stanislav Fomichev
2025-02-10 19:20 ` [PATCH net-next 02/11] net: hold netdev instance lock during ndo_setup_tc Stanislav Fomichev
@ 2025-02-10 19:20 ` Stanislav Fomichev
2025-02-12 2:21 ` Jakub Kicinski
2025-02-10 19:20 ` [PATCH net-next 04/11] net: hold netdev instance lock during rtnetlink operations Stanislav Fomichev
` (7 subsequent siblings)
10 siblings, 1 reply; 22+ messages in thread
From: Stanislav Fomichev @ 2025-02-10 19:20 UTC (permalink / raw)
To: netdev; +Cc: davem, edumazet, kuba, pabeni, Saeed Mahameed
For the drivers that use queue management API, switch to the mode where
core stack holds the netdev instance lock. This affects the following
drivers:
- bnxt
- gve
- netdevsim
Originally I locked only start/stop, but switched to holding the
lock over all iterations to make them look atomic to the device
(feels like it should be easier to reason about).
Cc: Saeed Mahameed <saeed@kernel.org>
Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
---
drivers/net/ethernet/broadcom/bnxt/bnxt.c | 22 +++++++++++++--------
drivers/net/ethernet/google/gve/gve_main.c | 8 ++++----
drivers/net/ethernet/google/gve/gve_utils.c | 8 +++++---
drivers/net/netdevsim/netdev.c | 22 ++++++++++++---------
include/linux/netdevice.h | 2 +-
net/core/netdev_rx_queue.c | 5 +++++
6 files changed, 42 insertions(+), 25 deletions(-)
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index 7b8b5b39c7bb..082a6c2b5cca 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -11301,7 +11301,7 @@ static int bnxt_request_irq(struct bnxt *bp)
if (rc)
break;
- netif_napi_set_irq(&bp->bnapi[i]->napi, irq->vector);
+ netif_napi_set_irq_locked(&bp->bnapi[i]->napi, irq->vector);
irq->requested = 1;
if (zalloc_cpumask_var(&irq->cpu_mask, GFP_KERNEL)) {
@@ -11337,9 +11337,9 @@ static void bnxt_del_napi(struct bnxt *bp)
for (i = 0; i < bp->cp_nr_rings; i++) {
struct bnxt_napi *bnapi = bp->bnapi[i];
- __netif_napi_del(&bnapi->napi);
+ __netif_napi_del_locked(&bnapi->napi);
}
- /* We called __netif_napi_del(), we need
+ /* We called __netif_napi_del_locked(), we need
* to respect an RCU grace period before freeing napi structures.
*/
synchronize_net();
@@ -11352,18 +11352,20 @@ static void bnxt_init_napi(struct bnxt *bp)
struct bnxt_napi *bnapi;
int i;
+ netdev_assert_locked(bp->dev);
+
if (bp->flags & BNXT_FLAG_CHIP_P5_PLUS)
poll_fn = bnxt_poll_p5;
else if (BNXT_CHIP_TYPE_NITRO_A0(bp))
cp_nr_rings--;
for (i = 0; i < cp_nr_rings; i++) {
bnapi = bp->bnapi[i];
- netif_napi_add_config(bp->dev, &bnapi->napi, poll_fn,
- bnapi->index);
+ netif_napi_add_config_locked(bp->dev, &bnapi->napi, poll_fn,
+ bnapi->index);
}
if (BNXT_CHIP_TYPE_NITRO_A0(bp)) {
bnapi = bp->bnapi[cp_nr_rings];
- netif_napi_add(bp->dev, &bnapi->napi, bnxt_poll_nitroa0);
+ netif_napi_add_locked(bp->dev, &bnapi->napi, bnxt_poll_nitroa0);
}
}
@@ -11375,6 +11377,8 @@ static void bnxt_disable_napi(struct bnxt *bp)
test_and_set_bit(BNXT_STATE_NAPI_DISABLED, &bp->state))
return;
+ netdev_assert_locked(bp->dev);
+
for (i = 0; i < bp->cp_nr_rings; i++) {
struct bnxt_napi *bnapi = bp->bnapi[i];
struct bnxt_cp_ring_info *cpr;
@@ -11384,7 +11388,7 @@ static void bnxt_disable_napi(struct bnxt *bp)
cpr->sw_stats->tx.tx_resets++;
if (bnapi->in_reset)
cpr->sw_stats->rx.rx_resets++;
- napi_disable(&bnapi->napi);
+ napi_disable_locked(&bnapi->napi);
}
}
@@ -11392,6 +11396,8 @@ static void bnxt_enable_napi(struct bnxt *bp)
{
int i;
+ netdev_assert_locked(bp->dev);
+
clear_bit(BNXT_STATE_NAPI_DISABLED, &bp->state);
for (i = 0; i < bp->cp_nr_rings; i++) {
struct bnxt_napi *bnapi = bp->bnapi[i];
@@ -11406,7 +11412,7 @@ static void bnxt_enable_napi(struct bnxt *bp)
INIT_WORK(&cpr->dim.work, bnxt_dim_work);
cpr->dim.mode = DIM_CQ_PERIOD_MODE_START_FROM_EQE;
}
- napi_enable(&bnapi->napi);
+ napi_enable_locked(&bnapi->napi);
}
}
diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
index 533e659b15b3..cf9bd08d04b2 100644
--- a/drivers/net/ethernet/google/gve/gve_main.c
+++ b/drivers/net/ethernet/google/gve/gve_main.c
@@ -1886,7 +1886,7 @@ static void gve_turndown(struct gve_priv *priv)
netif_queue_set_napi(priv->dev, idx,
NETDEV_QUEUE_TYPE_TX, NULL);
- napi_disable(&block->napi);
+ napi_disable_locked(&block->napi);
}
for (idx = 0; idx < priv->rx_cfg.num_queues; idx++) {
int ntfy_idx = gve_rx_idx_to_ntfy(priv, idx);
@@ -1897,7 +1897,7 @@ static void gve_turndown(struct gve_priv *priv)
netif_queue_set_napi(priv->dev, idx, NETDEV_QUEUE_TYPE_RX,
NULL);
- napi_disable(&block->napi);
+ napi_disable_locked(&block->napi);
}
/* Stop tx queues */
@@ -1925,7 +1925,7 @@ static void gve_turnup(struct gve_priv *priv)
if (!gve_tx_was_added_to_block(priv, idx))
continue;
- napi_enable(&block->napi);
+ napi_enable_locked(&block->napi);
if (idx < priv->tx_cfg.num_queues)
netif_queue_set_napi(priv->dev, idx,
@@ -1953,7 +1953,7 @@ static void gve_turnup(struct gve_priv *priv)
if (!gve_rx_was_added_to_block(priv, idx))
continue;
- napi_enable(&block->napi);
+ napi_enable_locked(&block->napi);
netif_queue_set_napi(priv->dev, idx, NETDEV_QUEUE_TYPE_RX,
&block->napi);
diff --git a/drivers/net/ethernet/google/gve/gve_utils.c b/drivers/net/ethernet/google/gve/gve_utils.c
index 30fef100257e..fa21d240806b 100644
--- a/drivers/net/ethernet/google/gve/gve_utils.c
+++ b/drivers/net/ethernet/google/gve/gve_utils.c
@@ -110,13 +110,15 @@ void gve_add_napi(struct gve_priv *priv, int ntfy_idx,
{
struct gve_notify_block *block = &priv->ntfy_blocks[ntfy_idx];
- netif_napi_add(priv->dev, &block->napi, gve_poll);
- netif_napi_set_irq(&block->napi, block->irq);
+ netdev_assert_locked(priv->dev);
+ netif_napi_add_locked(priv->dev, &block->napi, gve_poll);
+ netif_napi_set_irq_locked(&block->napi, block->irq);
}
void gve_remove_napi(struct gve_priv *priv, int ntfy_idx)
{
struct gve_notify_block *block = &priv->ntfy_blocks[ntfy_idx];
- netif_napi_del(&block->napi);
+ netdev_assert_locked(priv->dev);
+ netif_napi_del_locked(&block->napi);
}
diff --git a/drivers/net/netdevsim/netdev.c b/drivers/net/netdevsim/netdev.c
index d857ae3c8f06..07f4670d69d5 100644
--- a/drivers/net/netdevsim/netdev.c
+++ b/drivers/net/netdevsim/netdev.c
@@ -664,7 +664,7 @@ nsim_queue_mem_alloc(struct net_device *dev, void *per_queue_mem, int idx)
goto err_free;
if (!ns->rq_reset_mode)
- netif_napi_add_config(dev, &qmem->rq->napi, nsim_poll, idx);
+ netif_napi_add_config_locked(dev, &qmem->rq->napi, nsim_poll, idx);
return 0;
@@ -681,7 +681,7 @@ static void nsim_queue_mem_free(struct net_device *dev, void *per_queue_mem)
page_pool_destroy(qmem->pp);
if (qmem->rq) {
if (!ns->rq_reset_mode)
- netif_napi_del(&qmem->rq->napi);
+ netif_napi_del_locked(&qmem->rq->napi);
page_pool_destroy(qmem->rq->page_pool);
nsim_queue_free(qmem->rq);
}
@@ -693,9 +693,11 @@ nsim_queue_start(struct net_device *dev, void *per_queue_mem, int idx)
struct nsim_queue_mem *qmem = per_queue_mem;
struct netdevsim *ns = netdev_priv(dev);
+ netdev_assert_locked(dev);
+
if (ns->rq_reset_mode == 1) {
ns->rq[idx]->page_pool = qmem->pp;
- napi_enable(&ns->rq[idx]->napi);
+ napi_enable_locked(&ns->rq[idx]->napi);
return 0;
}
@@ -703,15 +705,15 @@ nsim_queue_start(struct net_device *dev, void *per_queue_mem, int idx)
* here we want to test various call orders.
*/
if (ns->rq_reset_mode == 2) {
- netif_napi_del(&ns->rq[idx]->napi);
- netif_napi_add_config(dev, &qmem->rq->napi, nsim_poll, idx);
+ netif_napi_del_locked(&ns->rq[idx]->napi);
+ netif_napi_add_config_locked(dev, &qmem->rq->napi, nsim_poll, idx);
} else if (ns->rq_reset_mode == 3) {
- netif_napi_add_config(dev, &qmem->rq->napi, nsim_poll, idx);
- netif_napi_del(&ns->rq[idx]->napi);
+ netif_napi_add_config_locked(dev, &qmem->rq->napi, nsim_poll, idx);
+ netif_napi_del_locked(&ns->rq[idx]->napi);
}
ns->rq[idx] = qmem->rq;
- napi_enable(&ns->rq[idx]->napi);
+ napi_enable_locked(&ns->rq[idx]->napi);
return 0;
}
@@ -721,7 +723,9 @@ static int nsim_queue_stop(struct net_device *dev, void *per_queue_mem, int idx)
struct nsim_queue_mem *qmem = per_queue_mem;
struct netdevsim *ns = netdev_priv(dev);
- napi_disable(&ns->rq[idx]->napi);
+ netdev_assert_locked(dev);
+
+ napi_disable_locked(&ns->rq[idx]->napi);
if (ns->rq_reset_mode == 1) {
qmem->pp = ns->rq[idx]->page_pool;
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 06f24dc4f46e..7367e2972026 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -2720,7 +2720,7 @@ static inline void netdev_assert_locked_or_invisible(struct net_device *dev)
static inline bool need_netdev_ops_lock(struct net_device *dev)
{
- bool ret = false;
+ bool ret = !!(dev)->queue_mgmt_ops;
#if IS_ENABLED(CONFIG_NET_SHAPER)
ret |= !!(dev)->netdev_ops->net_shaper_ops;
diff --git a/net/core/netdev_rx_queue.c b/net/core/netdev_rx_queue.c
index ddd54e1e7289..7419c41fd3cb 100644
--- a/net/core/netdev_rx_queue.c
+++ b/net/core/netdev_rx_queue.c
@@ -30,6 +30,8 @@ int netdev_rx_queue_restart(struct net_device *dev, unsigned int rxq_idx)
goto err_free_new_mem;
}
+ netdev_lock(dev);
+
err = qops->ndo_queue_mem_alloc(dev, new_mem, rxq_idx);
if (err)
goto err_free_old_mem;
@@ -52,6 +54,8 @@ int netdev_rx_queue_restart(struct net_device *dev, unsigned int rxq_idx)
qops->ndo_queue_mem_free(dev, old_mem);
+ netdev_unlock(dev);
+
kvfree(old_mem);
kvfree(new_mem);
@@ -76,6 +80,7 @@ int netdev_rx_queue_restart(struct net_device *dev, unsigned int rxq_idx)
qops->ndo_queue_mem_free(dev, new_mem);
err_free_old_mem:
+ netdev_unlock(dev);
kvfree(old_mem);
err_free_new_mem:
--
2.48.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH net-next 04/11] net: hold netdev instance lock during rtnetlink operations
2025-02-10 19:20 [PATCH net-next 00/11] net: Hold netdev instance lock during ndo operations Stanislav Fomichev
` (2 preceding siblings ...)
2025-02-10 19:20 ` [PATCH net-next 03/11] net: hold netdev instance lock during queue operations Stanislav Fomichev
@ 2025-02-10 19:20 ` Stanislav Fomichev
2025-02-11 15:06 ` kernel test robot
2025-02-12 2:33 ` Jakub Kicinski
2025-02-10 19:20 ` [PATCH net-next 05/11] net: hold netdev instance lock during ioctl operations Stanislav Fomichev
` (6 subsequent siblings)
10 siblings, 2 replies; 22+ messages in thread
From: Stanislav Fomichev @ 2025-02-10 19:20 UTC (permalink / raw)
To: netdev; +Cc: davem, edumazet, kuba, pabeni, Saeed Mahameed
To preserve the atomicity, hold the lock while applying multiple
attributes. The major issue with a full conversion to the instance
lock are software nesting devices (bonding/team/vrf/etc). Those
devices call into the core stack for their lower (potentially
real hw) devices. To avoid explicitly wrapping all those places
into instance lock/unlock, introduce new API boundaries:
- (some) existing dev_xxx calls are now considered "external"
(to drivers) APIs and they transparently grab the instance
lock if needed (dev_api.c)
- new netif_xxx calls are internal core stack API (naming is
sketchy, I've tried netdev_xxx_locked per Jakub's suggestion,
but it feels a bit verbose; but happy to get back to this
naming scheme if this is the preference)
This avoids touching most of the existing ioctl/sysfs/drivers paths.
Note the special handling of ndo_xxx_slave operations: I exploit
the fact that none of the drivers that call these functions
need/use instance lock. At the same time, they use dev_xxx
APIs, so the lower device has to be unlocked.
Changes in unregister_netdevice_many_notify (to protect dev->state
with instance lock) trigger lockdep - the loop over close_list
(mostly from cleanup_net) introduces spurious ordering issues.
netdev_lock_cmp_fn has a justification on why it's ok to suppress
for now.
Cc: Saeed Mahameed <saeed@kernel.org>
Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
---
include/linux/netdevice.h | 21 ++--
net/core/Makefile | 2 +-
net/core/dev.c | 166 ++++++++----------------------
net/core/dev.h | 13 ++-
net/core/dev_api.c | 211 ++++++++++++++++++++++++++++++++++++++
net/core/rtnetlink.c | 47 ++++++---
6 files changed, 310 insertions(+), 150 deletions(-)
create mode 100644 net/core/dev_api.c
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 7367e2972026..afc0dba15624 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -2741,6 +2741,12 @@ static inline void netdev_unlock_ops(struct net_device *dev)
netdev_unlock(dev);
}
+static inline void netdev_ops_assert_locked(struct net_device *dev)
+{
+ if (need_netdev_ops_lock(dev))
+ lockdep_assert_held(&dev->lock);
+}
+
static inline void netif_napi_set_irq_locked(struct napi_struct *napi, int irq)
{
napi->irq = irq;
@@ -3313,7 +3319,9 @@ struct net_device *dev_get_by_name_rcu(struct net *net, const char *name);
struct net_device *__dev_get_by_name(struct net *net, const char *name);
bool netdev_name_in_use(struct net *net, const char *name);
int dev_alloc_name(struct net_device *dev, const char *name);
+int netif_open(struct net_device *dev, struct netlink_ext_ack *extack);
int dev_open(struct net_device *dev, struct netlink_ext_ack *extack);
+void netif_close(struct net_device *dev);
void dev_close(struct net_device *dev);
void dev_close_many(struct list_head *head, bool unlink);
int dev_setup_tc(struct net_device *dev, enum tc_setup_type type,
@@ -4169,24 +4177,25 @@ int dev_ethtool(struct net *net, struct ifreq *ifr, void __user *userdata);
unsigned int dev_get_flags(const struct net_device *);
int __dev_change_flags(struct net_device *dev, unsigned int flags,
struct netlink_ext_ack *extack);
+int netif_change_flags(struct net_device *dev, unsigned int flags,
+ struct netlink_ext_ack *extack);
int dev_change_flags(struct net_device *dev, unsigned int flags,
struct netlink_ext_ack *extack);
+int netif_set_alias(struct net_device *dev, const char *alias, size_t len);
int dev_set_alias(struct net_device *, const char *, size_t);
int dev_get_alias(const struct net_device *, char *, size_t);
-int __dev_change_net_namespace(struct net_device *dev, struct net *net,
+int netif_change_net_namespace(struct net_device *dev, struct net *net,
const char *pat, int new_ifindex);
-static inline
int dev_change_net_namespace(struct net_device *dev, struct net *net,
- const char *pat)
-{
- return __dev_change_net_namespace(dev, net, pat, 0);
-}
+ const char *pat);
int __dev_set_mtu(struct net_device *, int);
int dev_set_mtu(struct net_device *, int);
int dev_pre_changeaddr_notify(struct net_device *dev, const char *addr,
struct netlink_ext_ack *extack);
int dev_set_mac_address(struct net_device *dev, struct sockaddr *sa,
struct netlink_ext_ack *extack);
+int netif_set_mac_address_user(struct net_device *dev, struct sockaddr *sa,
+ struct netlink_ext_ack *extack);
int dev_set_mac_address_user(struct net_device *dev, struct sockaddr *sa,
struct netlink_ext_ack *extack);
int dev_get_mac_address(struct sockaddr *sa, struct net *net, char *dev_name);
diff --git a/net/core/Makefile b/net/core/Makefile
index d9326600e289..a10c3bd96798 100644
--- a/net/core/Makefile
+++ b/net/core/Makefile
@@ -9,7 +9,7 @@ obj-y := sock.o request_sock.o skbuff.o datagram.o stream.o scm.o \
obj-$(CONFIG_SYSCTL) += sysctl_net_core.o
-obj-y += dev.o dev_addr_lists.o dst.o netevent.o \
+obj-y += dev.o dev_api.o dev_addr_lists.o dst.o netevent.o \
neighbour.o rtnetlink.o utils.o link_watch.o filter.o \
sock_diag.o dev_ioctl.o tso.o sock_reuseport.o \
fib_notifier.o xdp.o flow_offload.o gro.o \
diff --git a/net/core/dev.c b/net/core/dev.c
index a8df26f8ffe5..f511240fd514 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1340,15 +1340,7 @@ static int dev_get_valid_name(struct net *net, struct net_device *dev,
return ret < 0 ? ret : 0;
}
-/**
- * dev_change_name - change name of a device
- * @dev: device
- * @newname: name (or format string) must be at least IFNAMSIZ
- *
- * Change name of a device, can pass format strings "eth%d".
- * for wildcarding.
- */
-int dev_change_name(struct net_device *dev, const char *newname)
+int netif_change_name(struct net_device *dev, const char *newname)
{
struct net *net = dev_net(dev);
unsigned char old_assign_type;
@@ -1418,15 +1410,7 @@ int dev_change_name(struct net_device *dev, const char *newname)
return err;
}
-/**
- * dev_set_alias - change ifalias of a device
- * @dev: device
- * @alias: name up to IFALIASZ
- * @len: limit of bytes to copy from info
- *
- * Set ifalias for a device,
- */
-int dev_set_alias(struct net_device *dev, const char *alias, size_t len)
+int netif_set_alias(struct net_device *dev, const char *alias, size_t len)
{
struct dev_ifalias *new_alias = NULL;
@@ -1452,7 +1436,6 @@ int dev_set_alias(struct net_device *dev, const char *alias, size_t len)
return len;
}
-EXPORT_SYMBOL(dev_set_alias);
/**
* dev_get_alias - get ifalias of a device
@@ -1596,10 +1579,10 @@ static int __dev_open(struct net_device *dev, struct netlink_ext_ack *extack)
if (ret)
return ret;
- netdev_lock_ops(dev);
-
set_bit(__LINK_STATE_START, &dev->state);
+ netdev_ops_assert_locked(dev);
+
if (ops->ndo_validate_addr)
ret = ops->ndo_validate_addr(dev);
@@ -1617,25 +1600,10 @@ static int __dev_open(struct net_device *dev, struct netlink_ext_ack *extack)
add_device_randomness(dev->dev_addr, dev->addr_len);
}
- netdev_unlock_ops(dev);
-
return ret;
}
-/**
- * dev_open - prepare an interface for use.
- * @dev: device to open
- * @extack: netlink extended ack
- *
- * Takes a device from down to up state. The device's private open
- * function is invoked and then the multicast lists are loaded. Finally
- * the device is moved into the up state and a %NETDEV_UP message is
- * sent to the netdev notifier chain.
- *
- * Calling this function on an active interface is a nop. On a failure
- * a negative errno code is returned.
- */
-int dev_open(struct net_device *dev, struct netlink_ext_ack *extack)
+int netif_open(struct net_device *dev, struct netlink_ext_ack *extack)
{
int ret;
@@ -1651,7 +1619,6 @@ int dev_open(struct net_device *dev, struct netlink_ext_ack *extack)
return ret;
}
-EXPORT_SYMBOL(dev_open);
static void __dev_close_many(struct list_head *head)
{
@@ -1690,18 +1657,13 @@ static void __dev_close_many(struct list_head *head)
* event.
*/
- /* TODO: move the lock up before clearing __LINK_STATE_START.
- * Generates spurious lockdep warning.
- */
- netdev_lock_ops(dev);
+ netdev_ops_assert_locked(dev);
if (ops->ndo_stop)
ops->ndo_stop(dev);
netif_set_up(dev, false);
netpoll_poll_enable(dev);
-
- netdev_unlock_ops(dev);
}
}
@@ -1734,16 +1696,7 @@ void dev_close_many(struct list_head *head, bool unlink)
}
EXPORT_SYMBOL(dev_close_many);
-/**
- * dev_close - shutdown an interface.
- * @dev: device to shutdown
- *
- * This function moves an active device into down state. A
- * %NETDEV_GOING_DOWN is sent to the netdev notifier chain. The device
- * is then deactivated and finally a %NETDEV_DOWN is sent to the notifier
- * chain.
- */
-void dev_close(struct net_device *dev)
+void netif_close(struct net_device *dev)
{
if (dev->flags & IFF_UP) {
LIST_HEAD(single);
@@ -1753,7 +1706,6 @@ void dev_close(struct net_device *dev)
list_del(&single);
}
}
-EXPORT_SYMBOL(dev_close);
int dev_setup_tc(struct net_device *dev, enum tc_setup_type type,
void *type_data)
@@ -9268,17 +9220,8 @@ void __dev_notify_flags(struct net_device *dev, unsigned int old_flags,
}
}
-/**
- * dev_change_flags - change device settings
- * @dev: device
- * @flags: device state flags
- * @extack: netlink extended ack
- *
- * Change settings on device based state flags. The flags are
- * in the userspace exported format.
- */
-int dev_change_flags(struct net_device *dev, unsigned int flags,
- struct netlink_ext_ack *extack)
+int netif_change_flags(struct net_device *dev, unsigned int flags,
+ struct netlink_ext_ack *extack)
{
int ret;
unsigned int changes, old_flags = dev->flags, old_gflags = dev->gflags;
@@ -9291,7 +9234,6 @@ int dev_change_flags(struct net_device *dev, unsigned int flags,
__dev_notify_flags(dev, old_flags, changes, 0, NULL);
return ret;
}
-EXPORT_SYMBOL(dev_change_flags);
int __dev_set_mtu(struct net_device *dev, int new_mtu)
{
@@ -9323,15 +9265,15 @@ int dev_validate_mtu(struct net_device *dev, int new_mtu,
}
/**
- * dev_set_mtu_ext - Change maximum transfer unit
+ * netif_set_mtu_ext - Change maximum transfer unit
* @dev: device
* @new_mtu: new transfer unit
* @extack: netlink extended ack
*
* Change the maximum transfer size of the network device.
*/
-int dev_set_mtu_ext(struct net_device *dev, int new_mtu,
- struct netlink_ext_ack *extack)
+int netif_set_mtu_ext(struct net_device *dev, int new_mtu,
+ struct netlink_ext_ack *extack)
{
int err, orig_mtu;
@@ -9375,19 +9317,14 @@ int dev_set_mtu(struct net_device *dev, int new_mtu)
int err;
memset(&extack, 0, sizeof(extack));
- err = dev_set_mtu_ext(dev, new_mtu, &extack);
+ err = netdev_set_mtu_ext_locked(dev, new_mtu, &extack);
if (err && extack._msg)
net_err_ratelimited("%s: %s\n", dev->name, extack._msg);
return err;
}
EXPORT_SYMBOL(dev_set_mtu);
-/**
- * dev_change_tx_queue_len - Change TX queue length of a netdevice
- * @dev: device
- * @new_len: new tx queue length
- */
-int dev_change_tx_queue_len(struct net_device *dev, unsigned long new_len)
+int netif_change_tx_queue_len(struct net_device *dev, unsigned long new_len)
{
unsigned int orig_len = dev->tx_queue_len;
int res;
@@ -9414,12 +9351,7 @@ int dev_change_tx_queue_len(struct net_device *dev, unsigned long new_len)
return res;
}
-/**
- * dev_set_group - Change group this device belongs to
- * @dev: device
- * @new_group: group this device should belong to
- */
-void dev_set_group(struct net_device *dev, int new_group)
+void netif_set_group(struct net_device *dev, int new_group)
{
dev->group = new_group;
}
@@ -9482,8 +9414,8 @@ EXPORT_SYMBOL(dev_set_mac_address);
DECLARE_RWSEM(dev_addr_sem);
-int dev_set_mac_address_user(struct net_device *dev, struct sockaddr *sa,
- struct netlink_ext_ack *extack)
+int netif_set_mac_address_user(struct net_device *dev, struct sockaddr *sa,
+ struct netlink_ext_ack *extack)
{
int ret;
@@ -9492,7 +9424,6 @@ int dev_set_mac_address_user(struct net_device *dev, struct sockaddr *sa,
up_write(&dev_addr_sem);
return ret;
}
-EXPORT_SYMBOL(dev_set_mac_address_user);
int dev_get_mac_address(struct sockaddr *sa, struct net *net, char *dev_name)
{
@@ -9522,14 +9453,7 @@ int dev_get_mac_address(struct sockaddr *sa, struct net *net, char *dev_name)
}
EXPORT_SYMBOL(dev_get_mac_address);
-/**
- * dev_change_carrier - Change device carrier
- * @dev: device
- * @new_carrier: new value
- *
- * Change device carrier
- */
-int dev_change_carrier(struct net_device *dev, bool new_carrier)
+int netif_change_carrier(struct net_device *dev, bool new_carrier)
{
const struct net_device_ops *ops = dev->netdev_ops;
@@ -9640,13 +9564,7 @@ bool netdev_port_same_parent_id(struct net_device *a, struct net_device *b)
}
EXPORT_SYMBOL(netdev_port_same_parent_id);
-/**
- * dev_change_proto_down - set carrier according to proto_down.
- *
- * @dev: device
- * @proto_down: new value
- */
-int dev_change_proto_down(struct net_device *dev, bool proto_down)
+int netif_change_proto_down(struct net_device *dev, bool proto_down)
{
if (!dev->change_proto_down)
return -EOPNOTSUPP;
@@ -9661,14 +9579,14 @@ int dev_change_proto_down(struct net_device *dev, bool proto_down)
}
/**
- * dev_change_proto_down_reason - proto down reason
+ * netdev_change_proto_down_reason_locked - proto down reason
*
* @dev: device
* @mask: proto down mask
* @value: proto down value
*/
-void dev_change_proto_down_reason(struct net_device *dev, unsigned long mask,
- u32 value)
+void netdev_change_proto_down_reason_locked(struct net_device *dev,
+ unsigned long mask, u32 value)
{
u32 proto_down_reason;
int b;
@@ -10476,6 +10394,7 @@ int __netdev_update_features(struct net_device *dev)
int err = -1;
ASSERT_RTNL();
+ netdev_ops_assert_locked(dev);
features = netdev_get_wanted_features(dev);
@@ -11468,6 +11387,18 @@ void netdev_sw_irq_coalesce_default_on(struct net_device *dev)
}
EXPORT_SYMBOL_GPL(netdev_sw_irq_coalesce_default_on);
+static int netdev_lock_cmp_fn(const struct lockdep_map *a, const struct lockdep_map *b)
+{
+ /* Only lower devices currently grab the instance lock, so no
+ * real ordering issues can occur. In the near future, only
+ * hardware devices will grab instance lock which also does not
+ * involve any ordering. Suppress lockdep ordering warnings
+ * until (if) we start grabbing instance lock on pure SW
+ * devices (bond/team/etc).
+ */
+ return -1;
+}
+
/**
* alloc_netdev_mqs - allocate network device
* @sizeof_priv: size of private data to allocate space for
@@ -11558,6 +11489,7 @@ struct net_device *alloc_netdev_mqs(int sizeof_priv, const char *name,
#endif
mutex_init(&dev->lock);
+ lock_set_cmp_fn(&dev->lock, netdev_lock_cmp_fn, NULL);
dev->priv_flags = IFF_XMIT_DST_RELEASE | IFF_XMIT_DST_RELEASE_PERM;
setup(dev);
@@ -11824,11 +11756,14 @@ void unregister_netdevice_many_notify(struct list_head *head,
}
/* If device is running, close it first. */
- list_for_each_entry(dev, head, unreg_list)
+ list_for_each_entry(dev, head, unreg_list) {
list_add_tail(&dev->close_list, &close_head);
+ netdev_lock_ops(dev);
+ }
dev_close_many(&close_head, true);
list_for_each_entry(dev, head, unreg_list) {
+ netdev_unlock_ops(dev);
/* And unlink it from device chain. */
unlist_netdevice(dev);
netdev_lock(dev);
@@ -11943,23 +11878,7 @@ void unregister_netdev(struct net_device *dev)
}
EXPORT_SYMBOL(unregister_netdev);
-/**
- * __dev_change_net_namespace - move device to different nethost namespace
- * @dev: device
- * @net: network namespace
- * @pat: If not NULL name pattern to try if the current device name
- * is already taken in the destination network namespace.
- * @new_ifindex: If not zero, specifies device index in the target
- * namespace.
- *
- * This function shuts down a device interface and moves it
- * to a new network namespace. On success 0 is returned, on
- * a failure a netagive errno code is returned.
- *
- * Callers must hold the rtnl semaphore.
- */
-
-int __dev_change_net_namespace(struct net_device *dev, struct net *net,
+int netif_change_net_namespace(struct net_device *dev, struct net *net,
const char *pat, int new_ifindex)
{
struct netdev_name_node *name_node;
@@ -12021,7 +11940,7 @@ int __dev_change_net_namespace(struct net_device *dev, struct net *net,
*/
/* If device is running close it first. */
- dev_close(dev);
+ netif_close(dev);
/* And unlink it from device chain */
unlist_netdevice(dev);
@@ -12103,7 +12022,6 @@ int __dev_change_net_namespace(struct net_device *dev, struct net *net,
out:
return err;
}
-EXPORT_SYMBOL_GPL(__dev_change_net_namespace);
static int dev_cpu_dead(unsigned int oldcpu)
{
diff --git a/net/core/dev.h b/net/core/dev.h
index a966b6936009..1f04083bffb5 100644
--- a/net/core/dev.h
+++ b/net/core/dev.h
@@ -85,6 +85,7 @@ struct netdev_name_node {
};
int netdev_get_name(struct net *net, char *name, int ifindex);
+int netif_change_name(struct net_device *dev, const char *newname);
int dev_change_name(struct net_device *dev, const char *newname);
#define netdev_for_each_altname(dev, namenode) \
@@ -98,24 +99,28 @@ int netdev_name_node_alt_destroy(struct net_device *dev, const char *name);
int dev_validate_mtu(struct net_device *dev, int mtu,
struct netlink_ext_ack *extack);
-int dev_set_mtu_ext(struct net_device *dev, int mtu,
- struct netlink_ext_ack *extack);
+int netif_set_mtu_ext(struct net_device *dev, int new_mtu,
+ struct netlink_ext_ack *extack);
int dev_get_phys_port_id(struct net_device *dev,
struct netdev_phys_item_id *ppid);
int dev_get_phys_port_name(struct net_device *dev,
char *name, size_t len);
+int netif_change_proto_down(struct net_device *dev, bool proto_down);
int dev_change_proto_down(struct net_device *dev, bool proto_down);
-void dev_change_proto_down_reason(struct net_device *dev, unsigned long mask,
- u32 value);
+void netdev_change_proto_down_reason_locked(struct net_device *dev,
+ unsigned long mask, u32 value);
typedef int (*bpf_op_t)(struct net_device *dev, struct netdev_bpf *bpf);
int dev_change_xdp_fd(struct net_device *dev, struct netlink_ext_ack *extack,
int fd, int expected_fd, u32 flags);
+int netif_change_tx_queue_len(struct net_device *dev, unsigned long new_len);
int dev_change_tx_queue_len(struct net_device *dev, unsigned long new_len);
+void netif_set_group(struct net_device *dev, int new_group);
void dev_set_group(struct net_device *dev, int new_group);
+int netif_change_carrier(struct net_device *dev, bool new_carrier);
int dev_change_carrier(struct net_device *dev, bool new_carrier);
void __dev_set_rx_mode(struct net_device *dev);
diff --git a/net/core/dev_api.c b/net/core/dev_api.c
new file mode 100644
index 000000000000..1130da03357b
--- /dev/null
+++ b/net/core/dev_api.c
@@ -0,0 +1,211 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+#include <linux/netdevice.h>
+
+#include "dev.h"
+
+/**
+ * dev_change_name - change name of a device
+ * @dev: device
+ * @newname: name (or format string) must be at least IFNAMSIZ
+ *
+ * Change name of a device, can pass format strings "eth%d".
+ * for wildcarding.
+ */
+int dev_change_name(struct net_device *dev, const char *newname)
+{
+ int ret;
+
+ netdev_lock_ops(dev);
+ ret = netif_change_name(dev, newname);
+ netdev_unlock_ops(dev);
+
+ return ret;
+}
+
+/**
+ * dev_set_alias - change ifalias of a device
+ * @dev: device
+ * @alias: name up to IFALIASZ
+ * @len: limit of bytes to copy from info
+ *
+ * Set ifalias for a device,
+ */
+int dev_set_alias(struct net_device *dev, const char *alias, size_t len)
+{
+ int ret;
+
+ netdev_lock_ops(dev);
+ ret = netif_set_alias(dev, alias, len);
+ netdev_unlock_ops(dev);
+
+ return ret;
+}
+EXPORT_SYMBOL(dev_set_alias);
+
+/**
+ * dev_change_flags - change device settings
+ * @dev: device
+ * @flags: device state flags
+ * @extack: netlink extended ack
+ *
+ * Change settings on device based state flags. The flags are
+ * in the userspace exported format.
+ */
+int dev_change_flags(struct net_device *dev, unsigned int flags,
+ struct netlink_ext_ack *extack)
+{
+ int ret;
+
+ netdev_lock_ops(dev);
+ ret = netif_change_flags(dev, flags, extack);
+ netdev_unlock_ops(dev);
+
+ return ret;
+}
+EXPORT_SYMBOL(dev_change_flags);
+
+/**
+ * dev_set_group - Change group this device belongs to
+ * @dev: device
+ * @new_group: group this device should belong to
+ */
+void dev_set_group(struct net_device *dev, int new_group)
+{
+ netdev_lock_ops(dev);
+ netif_set_group(dev, new_group);
+ netdev_unlock_ops(dev);
+}
+
+int dev_set_mac_address_user(struct net_device *dev, struct sockaddr *sa,
+ struct netlink_ext_ack *extack)
+{
+ int ret;
+
+ netdev_lock_ops(dev);
+ ret = netif_set_mac_address_user(dev, sa, extack);
+ netdev_unlock_ops(dev);
+
+ return ret;
+}
+EXPORT_SYMBOL(dev_set_mac_address_user);
+
+/**
+ * dev_change_net_namespace - move device to different nethost namespace
+ * @dev: device
+ * @net: network namespace
+ * @pat: If not NULL name pattern to try if the current device name
+ * is already taken in the destination network namespace.
+ * @new_ifindex: If not zero, specifies device index in the target
+ * namespace.
+ *
+ * This function shuts down a device interface and moves it
+ * to a new network namespace. On success 0 is returned, on
+ * a failure a netagive errno code is returned.
+ *
+ * Callers must hold the rtnl semaphore.
+ */
+int dev_change_net_namespace(struct net_device *dev, struct net *net,
+ const char *pat)
+{
+ int ret;
+
+ netdev_lock_ops(dev);
+ ret = netif_change_net_namespace(dev, net, pat, 0);
+ netdev_unlock_ops(dev);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(dev_change_net_namespace);
+
+/**
+ * dev_change_carrier - Change device carrier
+ * @dev: device
+ * @new_carrier: new value
+ *
+ * Change device carrier
+ */
+int dev_change_carrier(struct net_device *dev, bool new_carrier)
+{
+ int ret;
+
+ netdev_lock_ops(dev);
+ ret = netif_change_carrier(dev, new_carrier);
+ netdev_unlock_ops(dev);
+
+ return ret;
+}
+
+/**
+ * dev_change_tx_queue_len - Change TX queue length of a netdevice
+ * @dev: device
+ * @new_len: new tx queue length
+ */
+int dev_change_tx_queue_len(struct net_device *dev, unsigned long new_len)
+{
+ int ret;
+
+ netdev_lock_ops(dev);
+ ret = netif_change_tx_queue_len(dev, new_len);
+ netdev_unlock_ops(dev);
+
+ return ret;
+}
+
+/**
+ * dev_change_proto_down - set carrier according to proto_down.
+ *
+ * @dev: device
+ * @proto_down: new value
+ */
+int dev_change_proto_down(struct net_device *dev, bool proto_down)
+{
+ int ret;
+
+ netdev_lock_ops(dev);
+ ret = netif_change_proto_down(dev, proto_down);
+ netdev_unlock_ops(dev);
+
+ return ret;
+}
+
+/**
+ * dev_open - prepare an interface for use.
+ * @dev: device to open
+ * @extack: netlink extended ack
+ *
+ * Takes a device from down to up state. The device's private open
+ * function is invoked and then the multicast lists are loaded. Finally
+ * the device is moved into the up state and a %NETDEV_UP message is
+ * sent to the netdev notifier chain.
+ *
+ * Calling this function on an active interface is a nop. On a failure
+ * a negative errno code is returned.
+ */
+int dev_open(struct net_device *dev, struct netlink_ext_ack *extack)
+{
+ int ret;
+
+ netdev_lock_ops(dev);
+ ret = netif_open(dev, extack);
+ netdev_unlock_ops(dev);
+
+ return ret;
+}
+EXPORT_SYMBOL(dev_open);
+
+/**
+ * dev_close - shutdown an interface.
+ * @dev: device to shutdown
+ *
+ * This function moves an active device into down state. A
+ * %NETDEV_GOING_DOWN is sent to the netdev notifier chain. The device
+ * is then deactivated and finally a %NETDEV_DOWN is sent to the notifier
+ * chain.
+ */
+void dev_close(struct net_device *dev)
+{
+ netdev_lock_ops(dev);
+ netif_close(dev);
+ netdev_unlock_ops(dev);
+}
+EXPORT_SYMBOL(dev_close);
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index cb7fad8d1f95..3cdbf5459d29 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -2909,12 +2909,19 @@ static int do_set_master(struct net_device *dev, int ifindex,
const struct net_device_ops *ops;
int err;
+ /* Release the lower lock, the upper is responsible for locking
+ * the lower if needed. None of the existing upper devices
+ * use netdev instance lock, so don't grab it.
+ */
+
if (upper_dev) {
if (upper_dev->ifindex == ifindex)
return 0;
ops = upper_dev->netdev_ops;
if (ops->ndo_del_slave) {
+ netdev_unlock_ops(dev);
err = ops->ndo_del_slave(upper_dev, dev);
+ netdev_lock_ops(dev);
if (err)
return err;
} else {
@@ -2928,7 +2935,9 @@ static int do_set_master(struct net_device *dev, int ifindex,
return -EINVAL;
ops = upper_dev->netdev_ops;
if (ops->ndo_add_slave) {
+ netdev_unlock_ops(dev);
err = ops->ndo_add_slave(upper_dev, dev, extack);
+ netdev_lock_ops(dev);
if (err)
return err;
} else {
@@ -2978,7 +2987,7 @@ static int do_set_proto_down(struct net_device *dev,
if (pdreason[IFLA_PROTO_DOWN_REASON_MASK])
mask = nla_get_u32(pdreason[IFLA_PROTO_DOWN_REASON_MASK]);
- dev_change_proto_down_reason(dev, mask, value);
+ netdev_change_proto_down_reason_locked(dev, mask, value);
}
if (nl_proto_down) {
@@ -2989,8 +2998,7 @@ static int do_set_proto_down(struct net_device *dev,
NL_SET_ERR_MSG(extack, "Cannot clear protodown, active reasons");
return -EBUSY;
}
- err = dev_change_proto_down(dev,
- proto_down);
+ err = netif_change_proto_down(dev, proto_down);
if (err)
return err;
}
@@ -3010,6 +3018,8 @@ static int do_setlink(const struct sk_buff *skb, struct net_device *dev,
char ifname[IFNAMSIZ];
int err;
+ netdev_lock_ops(dev);
+
err = validate_linkmsg(dev, tb, extack);
if (err < 0)
goto errout;
@@ -3025,7 +3035,7 @@ static int do_setlink(const struct sk_buff *skb, struct net_device *dev,
new_ifindex = nla_get_s32_default(tb[IFLA_NEW_IFINDEX], 0);
- err = __dev_change_net_namespace(dev, tgt_net, pat, new_ifindex);
+ err = netif_change_net_namespace(dev, tgt_net, pat, new_ifindex);
if (err)
goto errout;
@@ -3075,7 +3085,7 @@ static int do_setlink(const struct sk_buff *skb, struct net_device *dev,
sa->sa_family = dev->type;
memcpy(sa->sa_data, nla_data(tb[IFLA_ADDRESS]),
dev->addr_len);
- err = dev_set_mac_address_user(dev, sa, extack);
+ err = netif_set_mac_address_user(dev, sa, extack);
kfree(sa);
if (err)
goto errout;
@@ -3083,14 +3093,14 @@ static int do_setlink(const struct sk_buff *skb, struct net_device *dev,
}
if (tb[IFLA_MTU]) {
- err = dev_set_mtu_ext(dev, nla_get_u32(tb[IFLA_MTU]), extack);
+ err = netif_set_mtu_ext(dev, nla_get_u32(tb[IFLA_MTU]), extack);
if (err < 0)
goto errout;
status |= DO_SETLINK_MODIFIED;
}
if (tb[IFLA_GROUP]) {
- dev_set_group(dev, nla_get_u32(tb[IFLA_GROUP]));
+ netif_set_group(dev, nla_get_u32(tb[IFLA_GROUP]));
status |= DO_SETLINK_NOTIFY;
}
@@ -3100,15 +3110,15 @@ static int do_setlink(const struct sk_buff *skb, struct net_device *dev,
* requested.
*/
if (ifm->ifi_index > 0 && ifname[0]) {
- err = dev_change_name(dev, ifname);
+ err = netif_change_name(dev, ifname);
if (err < 0)
goto errout;
status |= DO_SETLINK_MODIFIED;
}
if (tb[IFLA_IFALIAS]) {
- err = dev_set_alias(dev, nla_data(tb[IFLA_IFALIAS]),
- nla_len(tb[IFLA_IFALIAS]));
+ err = netif_set_alias(dev, nla_data(tb[IFLA_IFALIAS]),
+ nla_len(tb[IFLA_IFALIAS]));
if (err < 0)
goto errout;
status |= DO_SETLINK_NOTIFY;
@@ -3120,8 +3130,8 @@ static int do_setlink(const struct sk_buff *skb, struct net_device *dev,
}
if (ifm->ifi_flags || ifm->ifi_change) {
- err = dev_change_flags(dev, rtnl_dev_combine_flags(dev, ifm),
- extack);
+ err = netif_change_flags(dev, rtnl_dev_combine_flags(dev, ifm),
+ extack);
if (err < 0)
goto errout;
}
@@ -3134,7 +3144,7 @@ static int do_setlink(const struct sk_buff *skb, struct net_device *dev,
}
if (tb[IFLA_CARRIER]) {
- err = dev_change_carrier(dev, nla_get_u8(tb[IFLA_CARRIER]));
+ err = netif_change_carrier(dev, nla_get_u8(tb[IFLA_CARRIER]));
if (err)
goto errout;
status |= DO_SETLINK_MODIFIED;
@@ -3143,7 +3153,7 @@ static int do_setlink(const struct sk_buff *skb, struct net_device *dev,
if (tb[IFLA_TXQLEN]) {
unsigned int value = nla_get_u32(tb[IFLA_TXQLEN]);
- err = dev_change_tx_queue_len(dev, value);
+ err = netif_change_tx_queue_len(dev, value);
if (err)
goto errout;
status |= DO_SETLINK_MODIFIED;
@@ -3374,6 +3384,8 @@ static int do_setlink(const struct sk_buff *skb, struct net_device *dev,
dev->name);
}
+ netdev_unlock_ops(dev);
+
return err;
}
@@ -3805,11 +3817,13 @@ static int rtnl_newlink_create(struct sk_buff *skb, struct ifinfomsg *ifm,
goto out;
}
+ netdev_lock_ops(dev);
+
err = rtnl_configure_link(dev, ifm, portid, nlh);
if (err < 0)
goto out_unregister;
if (link_net) {
- err = dev_change_net_namespace(dev, tgt_net, ifname);
+ err = netif_change_net_namespace(dev, tgt_net, ifname, 0);
if (err < 0)
goto out_unregister;
}
@@ -3818,9 +3832,12 @@ static int rtnl_newlink_create(struct sk_buff *skb, struct ifinfomsg *ifm,
if (err)
goto out_unregister;
}
+
+ netdev_unlock_ops(dev);
out:
return err;
out_unregister:
+ netdev_unlock_ops(dev);
if (ops->newlink) {
LIST_HEAD(list_kill);
--
2.48.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH net-next 05/11] net: hold netdev instance lock during ioctl operations
2025-02-10 19:20 [PATCH net-next 00/11] net: Hold netdev instance lock during ndo operations Stanislav Fomichev
` (3 preceding siblings ...)
2025-02-10 19:20 ` [PATCH net-next 04/11] net: hold netdev instance lock during rtnetlink operations Stanislav Fomichev
@ 2025-02-10 19:20 ` Stanislav Fomichev
2025-02-10 19:20 ` [PATCH net-next 06/11] net: hold netdev instance lock during sysfs operations Stanislav Fomichev
` (5 subsequent siblings)
10 siblings, 0 replies; 22+ messages in thread
From: Stanislav Fomichev @ 2025-02-10 19:20 UTC (permalink / raw)
To: netdev; +Cc: davem, edumazet, kuba, pabeni, Saeed Mahameed
Convert all ndo_eth_ioctl invocations to dev_eth_ioctl which does the
locking. Reflow some of the dev_siocxxx to drop else clause.
Cc: Saeed Mahameed <saeed@kernel.org>
Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
---
drivers/net/bonding/bond_main.c | 9 ++---
include/linux/netdevice.h | 3 ++
net/8021q/vlan_dev.c | 4 +-
net/core/dev.c | 4 +-
net/core/dev_api.c | 30 +++++++++++++++
net/core/dev_ioctl.c | 67 ++++++++++++++++++++-------------
6 files changed, 80 insertions(+), 37 deletions(-)
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index f6d0628a36d9..19775e9d7341 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -855,7 +855,6 @@ static int bond_check_dev_link(struct bonding *bond,
struct net_device *slave_dev, int reporting)
{
const struct net_device_ops *slave_ops = slave_dev->netdev_ops;
- int (*ioctl)(struct net_device *, struct ifreq *, int);
struct ifreq ifr;
struct mii_ioctl_data *mii;
@@ -871,8 +870,7 @@ static int bond_check_dev_link(struct bonding *bond,
BMSR_LSTATUS : 0;
/* Ethtool can't be used, fallback to MII ioctls. */
- ioctl = slave_ops->ndo_eth_ioctl;
- if (ioctl) {
+ if (slave_ops->ndo_eth_ioctl) {
/* TODO: set pointer to correct ioctl on a per team member
* bases to make this more efficient. that is, once
* we determine the correct ioctl, we will always
@@ -888,9 +886,10 @@ static int bond_check_dev_link(struct bonding *bond,
/* Yes, the mii is overlaid on the ifreq.ifr_ifru */
strscpy_pad(ifr.ifr_name, slave_dev->name, IFNAMSIZ);
mii = if_mii(&ifr);
- if (ioctl(slave_dev, &ifr, SIOCGMIIPHY) == 0) {
+
+ if (dev_eth_ioctl(slave_dev, &ifr, SIOCGMIIPHY) == 0) {
mii->reg_num = MII_BMSR;
- if (ioctl(slave_dev, &ifr, SIOCGMIIREG) == 0)
+ if (dev_eth_ioctl(slave_dev, &ifr, SIOCGMIIREG) == 0)
return mii->val_out & BMSR_LSTATUS;
}
}
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index afc0dba15624..dc5b95ae58e0 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -4168,6 +4168,8 @@ int put_user_ifreq(struct ifreq *ifr, void __user *arg);
int dev_ioctl(struct net *net, unsigned int cmd, struct ifreq *ifr,
void __user *data, bool *need_copyout);
int dev_ifconf(struct net *net, struct ifconf __user *ifc);
+int dev_eth_ioctl(struct net_device *dev,
+ struct ifreq *ifr, unsigned int cmd);
int generic_hwtstamp_get_lower(struct net_device *dev,
struct kernel_hwtstamp_config *kernel_cfg);
int generic_hwtstamp_set_lower(struct net_device *dev,
@@ -4189,6 +4191,7 @@ int netif_change_net_namespace(struct net_device *dev, struct net *net,
int dev_change_net_namespace(struct net_device *dev, struct net *net,
const char *pat);
int __dev_set_mtu(struct net_device *, int);
+int netif_set_mtu(struct net_device *dev, int new_mtu);
int dev_set_mtu(struct net_device *, int);
int dev_pre_changeaddr_notify(struct net_device *dev, const char *addr,
struct netlink_ext_ack *extack);
diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
index 91d134961357..ee3283400716 100644
--- a/net/8021q/vlan_dev.c
+++ b/net/8021q/vlan_dev.c
@@ -377,7 +377,6 @@ static int vlan_hwtstamp_set(struct net_device *dev,
static int vlan_dev_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
{
struct net_device *real_dev = vlan_dev_priv(dev)->real_dev;
- const struct net_device_ops *ops = real_dev->netdev_ops;
struct ifreq ifrr;
int err = -EOPNOTSUPP;
@@ -388,8 +387,7 @@ static int vlan_dev_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
case SIOCGMIIPHY:
case SIOCGMIIREG:
case SIOCSMIIREG:
- if (netif_device_present(real_dev) && ops->ndo_eth_ioctl)
- err = ops->ndo_eth_ioctl(real_dev, &ifrr, cmd);
+ err = dev_eth_ioctl(real_dev, &ifrr, cmd);
break;
}
diff --git a/net/core/dev.c b/net/core/dev.c
index f511240fd514..ac2f8a9aef50 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -9311,7 +9311,7 @@ int netif_set_mtu_ext(struct net_device *dev, int new_mtu,
return err;
}
-int dev_set_mtu(struct net_device *dev, int new_mtu)
+int netif_set_mtu(struct net_device *dev, int new_mtu)
{
struct netlink_ext_ack extack;
int err;
@@ -9322,7 +9322,7 @@ int dev_set_mtu(struct net_device *dev, int new_mtu)
net_err_ratelimited("%s: %s\n", dev->name, extack._msg);
return err;
}
-EXPORT_SYMBOL(dev_set_mtu);
+EXPORT_SYMBOL(netif_set_mtu);
int netif_change_tx_queue_len(struct net_device *dev, unsigned long new_len)
{
diff --git a/net/core/dev_api.c b/net/core/dev_api.c
index 1130da03357b..cd8cb04b5a0f 100644
--- a/net/core/dev_api.c
+++ b/net/core/dev_api.c
@@ -209,3 +209,33 @@ void dev_close(struct net_device *dev)
netdev_unlock_ops(dev);
}
EXPORT_SYMBOL(dev_close);
+
+int dev_eth_ioctl(struct net_device *dev,
+ struct ifreq *ifr, unsigned int cmd)
+{
+ const struct net_device_ops *ops = dev->netdev_ops;
+ int ret = -ENODEV;
+
+ if (!ops->ndo_eth_ioctl)
+ return -EOPNOTSUPP;
+
+ netdev_lock_ops(dev);
+ if (netif_device_present(dev))
+ ret = ops->ndo_eth_ioctl(dev, ifr, cmd);
+ netdev_unlock_ops(dev);
+
+ return ret;
+}
+EXPORT_SYMBOL(dev_eth_ioctl);
+
+int dev_set_mtu(struct net_device *dev, int new_mtu)
+{
+ int ret;
+
+ netdev_lock_ops(dev);
+ ret = netif_set_mtu(dev, new_mtu);
+ netdev_unlock_ops(dev);
+
+ return ret;
+}
+EXPORT_SYMBOL(dev_set_mtu);
diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c
index 4c2098ac9d72..d9f350593121 100644
--- a/net/core/dev_ioctl.c
+++ b/net/core/dev_ioctl.c
@@ -110,7 +110,7 @@ static int dev_getifmap(struct net_device *dev, struct ifreq *ifr)
return 0;
}
-static int dev_setifmap(struct net_device *dev, struct ifreq *ifr)
+static int netif_setifmap(struct net_device *dev, struct ifreq *ifr)
{
struct compat_ifmap *cifmap = (struct compat_ifmap *)&ifr->ifr_map;
@@ -240,20 +240,6 @@ int net_hwtstamp_validate(const struct kernel_hwtstamp_config *cfg)
return 0;
}
-static int dev_eth_ioctl(struct net_device *dev,
- struct ifreq *ifr, unsigned int cmd)
-{
- const struct net_device_ops *ops = dev->netdev_ops;
-
- if (!ops->ndo_eth_ioctl)
- return -EOPNOTSUPP;
-
- if (!netif_device_present(dev))
- return -ENODEV;
-
- return ops->ndo_eth_ioctl(dev, ifr, cmd);
-}
-
/**
* dev_get_hwtstamp_phylib() - Get hardware timestamping settings of NIC
* or of attached phylib PHY
@@ -305,7 +291,9 @@ static int dev_get_hwtstamp(struct net_device *dev, struct ifreq *ifr)
return -ENODEV;
kernel_cfg.ifr = ifr;
+ netdev_lock_ops(dev);
err = dev_get_hwtstamp_phylib(dev, &kernel_cfg);
+ netdev_unlock_ops(dev);
if (err)
return err;
@@ -429,7 +417,9 @@ static int dev_set_hwtstamp(struct net_device *dev, struct ifreq *ifr)
if (!netif_device_present(dev))
return -ENODEV;
+ netdev_lock_ops(dev);
err = dev_set_hwtstamp_phylib(dev, &kernel_cfg, &extack);
+ netdev_unlock_ops(dev);
if (err)
return err;
@@ -504,10 +494,14 @@ static int dev_siocbond(struct net_device *dev,
const struct net_device_ops *ops = dev->netdev_ops;
if (ops->ndo_siocbond) {
+ int ret = -ENODEV;
+
+ netdev_lock_ops(dev);
if (netif_device_present(dev))
- return ops->ndo_siocbond(dev, ifr, cmd);
- else
- return -ENODEV;
+ ret = ops->ndo_siocbond(dev, ifr, cmd);
+ netdev_unlock_ops(dev);
+
+ return ret;
}
return -EOPNOTSUPP;
@@ -519,10 +513,14 @@ static int dev_siocdevprivate(struct net_device *dev, struct ifreq *ifr,
const struct net_device_ops *ops = dev->netdev_ops;
if (ops->ndo_siocdevprivate) {
+ int ret = -ENODEV;
+
+ netdev_lock_ops(dev);
if (netif_device_present(dev))
- return ops->ndo_siocdevprivate(dev, ifr, data, cmd);
- else
- return -ENODEV;
+ ret = ops->ndo_siocdevprivate(dev, ifr, data, cmd);
+ netdev_unlock_ops(dev);
+
+ return ret;
}
return -EOPNOTSUPP;
@@ -533,10 +531,14 @@ static int dev_siocwandev(struct net_device *dev, struct if_settings *ifs)
const struct net_device_ops *ops = dev->netdev_ops;
if (ops->ndo_siocwandev) {
+ int ret = -ENODEV;
+
+ netdev_lock_ops(dev);
if (netif_device_present(dev))
- return ops->ndo_siocwandev(dev, ifs);
- else
- return -ENODEV;
+ ret = ops->ndo_siocwandev(dev, ifs);
+ netdev_unlock_ops(dev);
+
+ return ret;
}
return -EOPNOTSUPP;
@@ -580,11 +582,16 @@ static int dev_ifsioc(struct net *net, struct ifreq *ifr, void __user *data,
memcpy(dev->broadcast, ifr->ifr_hwaddr.sa_data,
min(sizeof(ifr->ifr_hwaddr.sa_data_min),
(size_t)dev->addr_len));
+ netdev_lock_ops(dev);
call_netdevice_notifiers(NETDEV_CHANGEADDR, dev);
+ netdev_unlock_ops(dev);
return 0;
case SIOCSIFMAP:
- return dev_setifmap(dev, ifr);
+ netdev_lock_ops(dev);
+ err = netif_setifmap(dev, ifr);
+ netdev_unlock_ops(dev);
+ return err;
case SIOCADDMULTI:
if (!ops->ndo_set_rx_mode ||
@@ -592,7 +599,10 @@ static int dev_ifsioc(struct net *net, struct ifreq *ifr, void __user *data,
return -EINVAL;
if (!netif_device_present(dev))
return -ENODEV;
- return dev_mc_add_global(dev, ifr->ifr_hwaddr.sa_data);
+ netdev_lock_ops(dev);
+ err = dev_mc_add_global(dev, ifr->ifr_hwaddr.sa_data);
+ netdev_unlock_ops(dev);
+ return err;
case SIOCDELMULTI:
if (!ops->ndo_set_rx_mode ||
@@ -600,7 +610,10 @@ static int dev_ifsioc(struct net *net, struct ifreq *ifr, void __user *data,
return -EINVAL;
if (!netif_device_present(dev))
return -ENODEV;
- return dev_mc_del_global(dev, ifr->ifr_hwaddr.sa_data);
+ netdev_lock_ops(dev);
+ err = dev_mc_del_global(dev, ifr->ifr_hwaddr.sa_data);
+ netdev_unlock_ops(dev);
+ return err;
case SIOCSIFTXQLEN:
if (ifr->ifr_qlen < 0)
--
2.48.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH net-next 06/11] net: hold netdev instance lock during sysfs operations
2025-02-10 19:20 [PATCH net-next 00/11] net: Hold netdev instance lock during ndo operations Stanislav Fomichev
` (4 preceding siblings ...)
2025-02-10 19:20 ` [PATCH net-next 05/11] net: hold netdev instance lock during ioctl operations Stanislav Fomichev
@ 2025-02-10 19:20 ` Stanislav Fomichev
2025-02-10 19:20 ` [PATCH net-next 07/11] net: hold netdev instance lock during ndo_bpf Stanislav Fomichev
` (4 subsequent siblings)
10 siblings, 0 replies; 22+ messages in thread
From: Stanislav Fomichev @ 2025-02-10 19:20 UTC (permalink / raw)
To: netdev; +Cc: davem, edumazet, kuba, pabeni, Saeed Mahameed
Most of them are already covered by the converted dev_xxx APIs.
Add the locking wrappers for the remaining ones.
Cc: Saeed Mahameed <saeed@kernel.org>
Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
---
drivers/net/bonding/bond_main.c | 7 ++--
include/linux/netdevice.h | 4 +++
net/core/dev.c | 60 +++++++------------------------
net/core/dev_api.c | 62 +++++++++++++++++++++++++++++++++
net/core/net-sysfs.c | 2 ++
5 files changed, 86 insertions(+), 49 deletions(-)
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 19775e9d7341..05afda23829f 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -2644,10 +2644,13 @@ static int __bond_release_one(struct net_device *bond_dev,
dev_set_mac_address(slave_dev, (struct sockaddr *)&ss, NULL);
}
- if (unregister)
+ if (unregister) {
+ netdev_lock_ops(slave_dev);
__dev_set_mtu(slave_dev, slave->original_mtu);
- else
+ netdev_unlock_ops(slave_dev);
+ } else {
dev_set_mtu(slave_dev, slave->original_mtu);
+ }
if (!netif_is_bond_master(slave_dev))
slave_dev->priv_flags &= ~IFF_BONDING;
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index dc5b95ae58e0..c4f182e4563e 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -3326,6 +3326,7 @@ void dev_close(struct net_device *dev);
void dev_close_many(struct list_head *head, bool unlink);
int dev_setup_tc(struct net_device *dev, enum tc_setup_type type,
void *type_data);
+void netif_disable_lro(struct net_device *dev);
void dev_disable_lro(struct net_device *dev);
int dev_loopback_xmit(struct net *net, struct sock *sk, struct sk_buff *newskb);
u16 dev_pick_tx_zero(struct net_device *dev, struct sk_buff *skb,
@@ -4195,6 +4196,8 @@ int netif_set_mtu(struct net_device *dev, int new_mtu);
int dev_set_mtu(struct net_device *, int);
int dev_pre_changeaddr_notify(struct net_device *dev, const char *addr,
struct netlink_ext_ack *extack);
+int netif_set_mac_address(struct net_device *dev, struct sockaddr *sa,
+ struct netlink_ext_ack *extack);
int dev_set_mac_address(struct net_device *dev, struct sockaddr *sa,
struct netlink_ext_ack *extack);
int netif_set_mac_address_user(struct net_device *dev, struct sockaddr *sa,
@@ -4954,6 +4957,7 @@ static inline void __dev_mc_unsync(struct net_device *dev,
/* Functions used for secondary unicast and multicast support */
void dev_set_rx_mode(struct net_device *dev);
int dev_set_promiscuity(struct net_device *dev, int inc);
+int netif_set_allmulti(struct net_device *dev, int inc, bool notify);
int dev_set_allmulti(struct net_device *dev, int inc);
void netdev_state_change(struct net_device *dev);
void __netdev_notify_peers(struct net_device *dev);
diff --git a/net/core/dev.c b/net/core/dev.c
index ac2f8a9aef50..bc027cc8276a 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1730,15 +1730,7 @@ int dev_setup_tc(struct net_device *dev, enum tc_setup_type type,
}
EXPORT_SYMBOL(dev_setup_tc);
-/**
- * dev_disable_lro - disable Large Receive Offload on a device
- * @dev: device
- *
- * Disable Large Receive Offload (LRO) on a net device. Must be
- * called under RTNL. This is needed if received packets may be
- * forwarded to another interface.
- */
-void dev_disable_lro(struct net_device *dev)
+void netif_disable_lro(struct net_device *dev)
{
struct net_device *lower_dev;
struct list_head *iter;
@@ -1749,10 +1741,12 @@ void dev_disable_lro(struct net_device *dev)
if (unlikely(dev->features & NETIF_F_LRO))
netdev_WARN(dev, "failed to disable LRO!\n");
- netdev_for_each_lower_dev(dev, lower_dev, iter)
- dev_disable_lro(lower_dev);
+ netdev_for_each_lower_dev(dev, lower_dev, iter) {
+ netdev_lock_ops(lower_dev);
+ netif_disable_lro(lower_dev);
+ netdev_unlock_ops(lower_dev);
+ }
}
-EXPORT_SYMBOL(dev_disable_lro);
/**
* dev_disable_gro_hw - disable HW Generic Receive Offload on a device
@@ -5976,7 +5970,7 @@ static int generic_xdp_install(struct net_device *dev, struct netdev_bpf *xdp)
static_branch_dec(&generic_xdp_needed_key);
} else if (new && !old) {
static_branch_inc(&generic_xdp_needed_key);
- dev_disable_lro(dev);
+ netif_disable_lro(dev);
dev_disable_gro_hw(dev);
}
break;
@@ -8999,7 +8993,7 @@ int dev_set_promiscuity(struct net_device *dev, int inc)
}
EXPORT_SYMBOL(dev_set_promiscuity);
-static int __dev_set_allmulti(struct net_device *dev, int inc, bool notify)
+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;
@@ -9034,25 +9028,6 @@ static int __dev_set_allmulti(struct net_device *dev, int inc, bool notify)
return 0;
}
-/**
- * dev_set_allmulti - update allmulti count on a device
- * @dev: device
- * @inc: modifier
- *
- * Add or remove reception of all multicast frames to a device. While the
- * count in the device remains above zero the interface remains listening
- * to all interfaces. Once it hits zero the device reverts back to normal
- * filtering operation. A negative @inc value is used to drop the counter
- * when releasing a resource needing all multicasts.
- * Return 0 if successful or a negative errno code on error.
- */
-
-int dev_set_allmulti(struct net_device *dev, int inc)
-{
- return __dev_set_allmulti(dev, inc, true);
-}
-EXPORT_SYMBOL(dev_set_allmulti);
-
/*
* Upload unicast and multicast address lists to device and
* configure RX filtering. When the device doesn't support unicast
@@ -9185,7 +9160,7 @@ int __dev_change_flags(struct net_device *dev, unsigned int flags,
int inc = (flags & IFF_ALLMULTI) ? 1 : -1;
dev->gflags ^= IFF_ALLMULTI;
- __dev_set_allmulti(dev, inc, false);
+ netif_set_allmulti(dev, inc, false);
}
return ret;
@@ -9317,7 +9292,7 @@ int netif_set_mtu(struct net_device *dev, int new_mtu)
int err;
memset(&extack, 0, sizeof(extack));
- err = netdev_set_mtu_ext_locked(dev, new_mtu, &extack);
+ err = netif_set_mtu_ext(dev, new_mtu, &extack);
if (err && extack._msg)
net_err_ratelimited("%s: %s\n", dev->name, extack._msg);
return err;
@@ -9377,16 +9352,8 @@ int dev_pre_changeaddr_notify(struct net_device *dev, const char *addr,
}
EXPORT_SYMBOL(dev_pre_changeaddr_notify);
-/**
- * dev_set_mac_address - Change Media Access Control Address
- * @dev: device
- * @sa: new address
- * @extack: netlink extended ack
- *
- * Change the hardware (MAC) address of the device
- */
-int dev_set_mac_address(struct net_device *dev, struct sockaddr *sa,
- struct netlink_ext_ack *extack)
+int netif_set_mac_address(struct net_device *dev, struct sockaddr *sa,
+ struct netlink_ext_ack *extack)
{
const struct net_device_ops *ops = dev->netdev_ops;
int err;
@@ -9410,7 +9377,6 @@ int dev_set_mac_address(struct net_device *dev, struct sockaddr *sa,
add_device_randomness(dev->dev_addr, dev->addr_len);
return 0;
}
-EXPORT_SYMBOL(dev_set_mac_address);
DECLARE_RWSEM(dev_addr_sem);
@@ -9420,7 +9386,7 @@ int netif_set_mac_address_user(struct net_device *dev, struct sockaddr *sa,
int ret;
down_write(&dev_addr_sem);
- ret = dev_set_mac_address(dev, sa, extack);
+ ret = netif_set_mac_address(dev, sa, extack);
up_write(&dev_addr_sem);
return ret;
}
diff --git a/net/core/dev_api.c b/net/core/dev_api.c
index cd8cb04b5a0f..d654db36ecc9 100644
--- a/net/core/dev_api.c
+++ b/net/core/dev_api.c
@@ -239,3 +239,65 @@ int dev_set_mtu(struct net_device *dev, int new_mtu)
return ret;
}
EXPORT_SYMBOL(dev_set_mtu);
+
+/**
+ * dev_disable_lro - disable Large Receive Offload on a device
+ * @dev: device
+ *
+ * Disable Large Receive Offload (LRO) on a net device. Must be
+ * called under RTNL. This is needed if received packets may be
+ * forwarded to another interface.
+ */
+void dev_disable_lro(struct net_device *dev)
+{
+ netdev_lock_ops(dev);
+ netif_disable_lro(dev);
+ netdev_unlock_ops(dev);
+}
+EXPORT_SYMBOL(dev_disable_lro);
+
+/**
+ * dev_set_allmulti - update allmulti count on a device
+ * @dev: device
+ * @inc: modifier
+ *
+ * Add or remove reception of all multicast frames to a device. While the
+ * count in the device remains above zero the interface remains listening
+ * to all interfaces. Once it hits zero the device reverts back to normal
+ * filtering operation. A negative @inc value is used to drop the counter
+ * when releasing a resource needing all multicasts.
+ * Return 0 if successful or a negative errno code on error.
+ */
+
+int dev_set_allmulti(struct net_device *dev, int inc)
+{
+ int ret;
+
+ netdev_lock_ops(dev);
+ ret = netif_set_allmulti(dev, inc, true);
+ netdev_unlock_ops(dev);
+
+ return ret;
+}
+EXPORT_SYMBOL(dev_set_allmulti);
+
+/**
+ * dev_set_mac_address - Change Media Access Control Address
+ * @dev: device
+ * @sa: new address
+ * @extack: netlink extended ack
+ *
+ * Change the hardware (MAC) address of the device
+ */
+int dev_set_mac_address(struct net_device *dev, struct sockaddr *sa,
+ struct netlink_ext_ack *extack)
+{
+ int ret;
+
+ netdev_lock_ops(dev);
+ ret = netif_set_mac_address(dev, sa, extack);
+ netdev_unlock_ops(dev);
+
+ return ret;
+}
+EXPORT_SYMBOL(dev_set_mac_address);
diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
index 3fe2c521e574..35f79a308d7b 100644
--- a/net/core/net-sysfs.c
+++ b/net/core/net-sysfs.c
@@ -1480,8 +1480,10 @@ static ssize_t tx_maxrate_store(struct kobject *kobj, struct attribute *attr,
return err;
err = -EOPNOTSUPP;
+ netdev_lock_ops(dev);
if (dev->netdev_ops->ndo_set_tx_maxrate)
err = dev->netdev_ops->ndo_set_tx_maxrate(dev, index, rate);
+ netdev_unlock_ops(dev);
if (!err) {
queue->tx_maxrate = rate;
--
2.48.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH net-next 07/11] net: hold netdev instance lock during ndo_bpf
2025-02-10 19:20 [PATCH net-next 00/11] net: Hold netdev instance lock during ndo operations Stanislav Fomichev
` (5 preceding siblings ...)
2025-02-10 19:20 ` [PATCH net-next 06/11] net: hold netdev instance lock during sysfs operations Stanislav Fomichev
@ 2025-02-10 19:20 ` Stanislav Fomichev
2025-02-10 19:20 ` [PATCH net-next 08/11] net: ethtool: try to protect all callback with netdev instance lock Stanislav Fomichev
` (3 subsequent siblings)
10 siblings, 0 replies; 22+ messages in thread
From: Stanislav Fomichev @ 2025-02-10 19:20 UTC (permalink / raw)
To: netdev; +Cc: davem, edumazet, kuba, pabeni, Saeed Mahameed
Cover the paths that come via bpf system call and XSK bind.
Cc: Saeed Mahameed <saeed@kernel.org>
Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
---
include/linux/netdevice.h | 1 +
kernel/bpf/offload.c | 7 ++++++-
net/core/dev.c | 13 +++++++++++--
net/core/dev_api.c | 12 ++++++++++++
net/xdp/xsk.c | 3 +++
net/xdp/xsk_buff_pool.c | 2 ++
6 files changed, 35 insertions(+), 3 deletions(-)
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index c4f182e4563e..675e5dee3219 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -4215,6 +4215,7 @@ struct sk_buff *dev_hard_start_xmit(struct sk_buff *skb, struct net_device *dev,
int bpf_xdp_link_attach(const union bpf_attr *attr, struct bpf_prog *prog);
u8 dev_xdp_prog_count(struct net_device *dev);
+int netif_xdp_propagate(struct net_device *dev, struct netdev_bpf *bpf);
int dev_xdp_propagate(struct net_device *dev, struct netdev_bpf *bpf);
u8 dev_xdp_sb_prog_count(struct net_device *dev);
u32 dev_xdp_prog_id(struct net_device *dev, enum bpf_xdp_mode mode);
diff --git a/kernel/bpf/offload.c b/kernel/bpf/offload.c
index 1a4fec330eaa..a9d370c1b135 100644
--- a/kernel/bpf/offload.c
+++ b/kernel/bpf/offload.c
@@ -122,6 +122,7 @@ static int bpf_map_offload_ndo(struct bpf_offloaded_map *offmap,
{
struct netdev_bpf data = {};
struct net_device *netdev;
+ int ret;
ASSERT_RTNL();
@@ -130,7 +131,11 @@ static int bpf_map_offload_ndo(struct bpf_offloaded_map *offmap,
/* Caller must make sure netdev is valid */
netdev = offmap->netdev;
- return netdev->netdev_ops->ndo_bpf(netdev, &data);
+ netdev_lock_ops(netdev);
+ ret = netdev->netdev_ops->ndo_bpf(netdev, &data);
+ netdev_unlock_ops(netdev);
+
+ return ret;
}
static void __bpf_map_offload_destroy(struct bpf_offloaded_map *offmap)
diff --git a/net/core/dev.c b/net/core/dev.c
index bc027cc8276a..fac1888c187e 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -9641,7 +9641,7 @@ u8 dev_xdp_sb_prog_count(struct net_device *dev)
return count;
}
-int dev_xdp_propagate(struct net_device *dev, struct netdev_bpf *bpf)
+int netif_xdp_propagate(struct net_device *dev, struct netdev_bpf *bpf)
{
if (!dev->netdev_ops->ndo_bpf)
return -EOPNOTSUPP;
@@ -9661,7 +9661,6 @@ int dev_xdp_propagate(struct net_device *dev, struct netdev_bpf *bpf)
return dev->netdev_ops->ndo_bpf(dev, bpf);
}
-EXPORT_SYMBOL_GPL(dev_xdp_propagate);
u32 dev_xdp_prog_id(struct net_device *dev, enum bpf_xdp_mode mode)
{
@@ -9691,6 +9690,8 @@ 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);
+
if (dev->cfg->hds_config == ETHTOOL_TCP_DATA_SPLIT_ENABLED &&
prog && !prog->aux->xdp_has_frags) {
NL_SET_ERR_MSG(extack, "unable to install XDP to device using tcp-data-split");
@@ -9923,7 +9924,9 @@ static void bpf_xdp_link_release(struct bpf_link *link)
* already NULL, in which case link was already auto-detached
*/
if (xdp_link->dev) {
+ netdev_lock_ops(xdp_link->dev);
WARN_ON(dev_xdp_detach_link(xdp_link->dev, NULL, xdp_link));
+ netdev_unlock_ops(xdp_link->dev);
xdp_link->dev = NULL;
}
@@ -10005,10 +10008,12 @@ static int bpf_xdp_link_update(struct bpf_link *link, struct bpf_prog *new_prog,
goto out_unlock;
}
+ netdev_lock_ops(xdp_link->dev);
mode = dev_xdp_mode(xdp_link->dev, xdp_link->flags);
bpf_op = dev_xdp_bpf_op(xdp_link->dev, mode);
err = dev_xdp_install(xdp_link->dev, mode, bpf_op, NULL,
xdp_link->flags, new_prog);
+ netdev_unlock_ops(xdp_link->dev);
if (err)
goto out_unlock;
@@ -10794,7 +10799,9 @@ int register_netdevice(struct net_device *dev)
if (ret)
goto err_uninit_notify;
+ netdev_lock_ops(dev);
__netdev_update_features(dev);
+ netdev_unlock_ops(dev);
/*
* Default initial state at registry is that the
@@ -11746,7 +11753,9 @@ void unregister_netdevice_many_notify(struct list_head *head,
/* Shutdown queueing discipline. */
dev_shutdown(dev);
dev_tcx_uninstall(dev);
+ netdev_lock_ops(dev);
dev_xdp_uninstall(dev);
+ netdev_unlock_ops(dev);
bpf_dev_bound_netdev_unregister(dev);
dev_memory_provider_uninstall(dev);
diff --git a/net/core/dev_api.c b/net/core/dev_api.c
index d654db36ecc9..8dbeeac8d6e7 100644
--- a/net/core/dev_api.c
+++ b/net/core/dev_api.c
@@ -301,3 +301,15 @@ int dev_set_mac_address(struct net_device *dev, struct sockaddr *sa,
return ret;
}
EXPORT_SYMBOL(dev_set_mac_address);
+
+int dev_xdp_propagate(struct net_device *dev, struct netdev_bpf *bpf)
+{
+ int ret;
+
+ netdev_lock_ops(dev);
+ ret = netif_xdp_propagate(dev, bpf);
+ netdev_unlock_ops(dev);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(dev_xdp_propagate);
diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
index 89d2bef96469..277c97c58c09 100644
--- a/net/xdp/xsk.c
+++ b/net/xdp/xsk.c
@@ -1178,6 +1178,8 @@ static int xsk_bind(struct socket *sock, struct sockaddr *addr, int addr_len)
goto out_release;
}
+ netdev_lock_ops(dev);
+
if (!xs->rx && !xs->tx) {
err = -EINVAL;
goto out_unlock;
@@ -1312,6 +1314,7 @@ static int xsk_bind(struct socket *sock, struct sockaddr *addr, int addr_len)
smp_wmb();
WRITE_ONCE(xs->state, XSK_BOUND);
}
+ netdev_unlock_ops(dev);
out_release:
mutex_unlock(&xs->mutex);
rtnl_unlock();
diff --git a/net/xdp/xsk_buff_pool.c b/net/xdp/xsk_buff_pool.c
index 1f7975b49657..87c337f36e66 100644
--- a/net/xdp/xsk_buff_pool.c
+++ b/net/xdp/xsk_buff_pool.c
@@ -1,5 +1,6 @@
// SPDX-License-Identifier: GPL-2.0
+#include <linux/netdevice.h>
#include <net/xsk_buff_pool.h>
#include <net/xdp_sock.h>
#include <net/xdp_sock_drv.h>
@@ -219,6 +220,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);
err = netdev->netdev_ops->ndo_bpf(netdev, &bpf);
if (err)
goto err_unreg_pool;
--
2.48.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH net-next 08/11] net: ethtool: try to protect all callback with netdev instance lock
2025-02-10 19:20 [PATCH net-next 00/11] net: Hold netdev instance lock during ndo operations Stanislav Fomichev
` (6 preceding siblings ...)
2025-02-10 19:20 ` [PATCH net-next 07/11] net: hold netdev instance lock during ndo_bpf Stanislav Fomichev
@ 2025-02-10 19:20 ` Stanislav Fomichev
2025-02-10 19:20 ` [PATCH net-next 09/11] net: dummy: add dummy shaper API Stanislav Fomichev
` (2 subsequent siblings)
10 siblings, 0 replies; 22+ messages in thread
From: Stanislav Fomichev @ 2025-02-10 19:20 UTC (permalink / raw)
To: netdev; +Cc: davem, edumazet, kuba, pabeni, Saeed Mahameed
From: Jakub Kicinski <kuba@kernel.org>
Protect all ethtool callbacks and PHY related state with the netdev
instance lock, for drivers which want / need to have their ops
instance-locked. Basically take the lock everywhere we take rtnl_lock.
It was tempting to take the lock in ethnl_ops_begin(), but turns
out we actually nest those calls (when generating notifications).
Cc: Saeed Mahameed <saeed@kernel.org>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
---
drivers/net/netdevsim/ethtool.c | 2 --
net/dsa/conduit.c | 16 +++++++++++++++-
net/ethtool/cabletest.c | 20 ++++++++++++--------
net/ethtool/features.c | 6 ++++--
net/ethtool/ioctl.c | 6 ++++++
net/ethtool/module.c | 8 +++++---
net/ethtool/netlink.c | 12 ++++++++++++
net/ethtool/phy.c | 20 ++++++++++++++------
| 2 ++
net/ethtool/tsinfo.c | 9 ++++++---
net/sched/sch_taprio.c | 5 ++++-
11 files changed, 80 insertions(+), 26 deletions(-)
diff --git a/drivers/net/netdevsim/ethtool.c b/drivers/net/netdevsim/ethtool.c
index 5c80fbee7913..72a369cd21a2 100644
--- a/drivers/net/netdevsim/ethtool.c
+++ b/drivers/net/netdevsim/ethtool.c
@@ -107,10 +107,8 @@ nsim_set_channels(struct net_device *dev, struct ethtool_channels *ch)
struct netdevsim *ns = netdev_priv(dev);
int err;
- netdev_lock(dev);
err = netif_set_real_num_queues(dev, ch->combined_count,
ch->combined_count);
- netdev_unlock(dev);
if (err)
return err;
diff --git a/net/dsa/conduit.c b/net/dsa/conduit.c
index 3dfdb3cb47dc..f21bb2551bed 100644
--- a/net/dsa/conduit.c
+++ b/net/dsa/conduit.c
@@ -26,7 +26,9 @@ static int dsa_conduit_get_regs_len(struct net_device *dev)
int len;
if (ops->get_regs_len) {
+ netdev_lock_ops(dev);
len = ops->get_regs_len(dev);
+ netdev_unlock_ops(dev);
if (len < 0)
return len;
ret += len;
@@ -57,11 +59,15 @@ static void dsa_conduit_get_regs(struct net_device *dev,
int len;
if (ops->get_regs_len && ops->get_regs) {
+ netdev_lock_ops(dev);
len = ops->get_regs_len(dev);
- if (len < 0)
+ if (len < 0) {
+ netdev_unlock_ops(dev);
return;
+ }
regs->len = len;
ops->get_regs(dev, regs, data);
+ netdev_unlock_ops(dev);
data += regs->len;
}
@@ -91,8 +97,10 @@ static void dsa_conduit_get_ethtool_stats(struct net_device *dev,
int count = 0;
if (ops->get_sset_count && ops->get_ethtool_stats) {
+ netdev_lock_ops(dev);
count = ops->get_sset_count(dev, ETH_SS_STATS);
ops->get_ethtool_stats(dev, stats, data);
+ netdev_unlock_ops(dev);
}
if (ds->ops->get_ethtool_stats)
@@ -114,8 +122,10 @@ static void dsa_conduit_get_ethtool_phy_stats(struct net_device *dev,
if (count >= 0)
phy_ethtool_get_stats(dev->phydev, stats, data);
} else if (ops->get_sset_count && ops->get_ethtool_phy_stats) {
+ netdev_lock_ops(dev);
count = ops->get_sset_count(dev, ETH_SS_PHY_STATS);
ops->get_ethtool_phy_stats(dev, stats, data);
+ netdev_unlock_ops(dev);
}
if (count < 0)
@@ -132,11 +142,13 @@ static int dsa_conduit_get_sset_count(struct net_device *dev, int sset)
struct dsa_switch *ds = cpu_dp->ds;
int count = 0;
+ netdev_lock_ops(dev);
if (sset == ETH_SS_PHY_STATS && dev->phydev &&
!ops->get_ethtool_phy_stats)
count = phy_ethtool_get_sset_count(dev->phydev);
else if (ops->get_sset_count)
count = ops->get_sset_count(dev, sset);
+ netdev_unlock_ops(dev);
if (count < 0)
count = 0;
@@ -163,6 +175,7 @@ static void dsa_conduit_get_strings(struct net_device *dev, uint32_t stringset,
/* We do not want to be NULL-terminated, since this is a prefix */
pfx[sizeof(pfx) - 1] = '_';
+ netdev_lock_ops(dev);
if (stringset == ETH_SS_PHY_STATS && dev->phydev &&
!ops->get_ethtool_phy_stats) {
mcount = phy_ethtool_get_sset_count(dev->phydev);
@@ -176,6 +189,7 @@ static void dsa_conduit_get_strings(struct net_device *dev, uint32_t stringset,
mcount = 0;
ops->get_strings(dev, stringset, data);
}
+ netdev_unlock_ops(dev);
if (ds->ops->get_strings) {
ndata = data + mcount * len;
diff --git a/net/ethtool/cabletest.c b/net/ethtool/cabletest.c
index f22051f33868..d4a79310b33f 100644
--- a/net/ethtool/cabletest.c
+++ b/net/ethtool/cabletest.c
@@ -72,23 +72,24 @@ int ethnl_act_cable_test(struct sk_buff *skb, struct genl_info *info)
dev = req_info.dev;
rtnl_lock();
+ netdev_lock_ops(dev);
phydev = ethnl_req_get_phydev(&req_info,
tb[ETHTOOL_A_CABLE_TEST_HEADER],
info->extack);
if (IS_ERR_OR_NULL(phydev)) {
ret = -EOPNOTSUPP;
- goto out_rtnl;
+ goto out_unlock;
}
ops = ethtool_phy_ops;
if (!ops || !ops->start_cable_test) {
ret = -EOPNOTSUPP;
- goto out_rtnl;
+ goto out_unlock;
}
ret = ethnl_ops_begin(dev);
if (ret < 0)
- goto out_rtnl;
+ goto out_unlock;
ret = ops->start_cable_test(phydev, info->extack);
@@ -97,7 +98,8 @@ int ethnl_act_cable_test(struct sk_buff *skb, struct genl_info *info)
if (!ret)
ethnl_cable_test_started(phydev, ETHTOOL_MSG_CABLE_TEST_NTF);
-out_rtnl:
+out_unlock:
+ netdev_unlock_ops(dev);
rtnl_unlock();
ethnl_parse_header_dev_put(&req_info);
return ret;
@@ -339,23 +341,24 @@ int ethnl_act_cable_test_tdr(struct sk_buff *skb, struct genl_info *info)
goto out_dev_put;
rtnl_lock();
+ netdev_lock_ops(dev);
phydev = ethnl_req_get_phydev(&req_info,
tb[ETHTOOL_A_CABLE_TEST_TDR_HEADER],
info->extack);
if (IS_ERR_OR_NULL(phydev)) {
ret = -EOPNOTSUPP;
- goto out_rtnl;
+ goto out_unlock;
}
ops = ethtool_phy_ops;
if (!ops || !ops->start_cable_test_tdr) {
ret = -EOPNOTSUPP;
- goto out_rtnl;
+ goto out_unlock;
}
ret = ethnl_ops_begin(dev);
if (ret < 0)
- goto out_rtnl;
+ goto out_unlock;
ret = ops->start_cable_test_tdr(phydev, info->extack, &cfg);
@@ -365,7 +368,8 @@ int ethnl_act_cable_test_tdr(struct sk_buff *skb, struct genl_info *info)
ethnl_cable_test_started(phydev,
ETHTOOL_MSG_CABLE_TEST_TDR_NTF);
-out_rtnl:
+out_unlock:
+ netdev_unlock_ops(dev);
rtnl_unlock();
out_dev_put:
ethnl_parse_header_dev_put(&req_info);
diff --git a/net/ethtool/features.c b/net/ethtool/features.c
index b6cb101d7f19..ccffd64d5a87 100644
--- a/net/ethtool/features.c
+++ b/net/ethtool/features.c
@@ -234,9 +234,10 @@ int ethnl_set_features(struct sk_buff *skb, struct genl_info *info)
dev = req_info.dev;
rtnl_lock();
+ netdev_lock_ops(dev);
ret = ethnl_ops_begin(dev);
if (ret < 0)
- goto out_rtnl;
+ goto out_unlock;
ethnl_features_to_bitmap(old_active, dev->features);
ethnl_features_to_bitmap(old_wanted, dev->wanted_features);
ret = ethnl_parse_bitset(req_wanted, req_mask, NETDEV_FEATURE_COUNT,
@@ -286,7 +287,8 @@ int ethnl_set_features(struct sk_buff *skb, struct genl_info *info)
out_ops:
ethnl_ops_complete(dev);
-out_rtnl:
+out_unlock:
+ netdev_unlock_ops(dev);
rtnl_unlock();
ethnl_parse_header_dev_put(&req_info);
return ret;
diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
index 7609ce2b2c5e..e776e4444247 100644
--- a/net/ethtool/ioctl.c
+++ b/net/ethtool/ioctl.c
@@ -2311,6 +2311,7 @@ static int ethtool_phys_id(struct net_device *dev, void __user *useraddr)
*/
busy = true;
netdev_hold(dev, &dev_tracker, GFP_KERNEL);
+ netdev_unlock_ops(dev);
rtnl_unlock();
if (rc == 0) {
@@ -2325,8 +2326,10 @@ static int ethtool_phys_id(struct net_device *dev, void __user *useraddr)
do {
rtnl_lock();
+ netdev_lock_ops(dev);
rc = ops->set_phys_id(dev,
(i++ & 1) ? ETHTOOL_ID_OFF : ETHTOOL_ID_ON);
+ netdev_unlock_ops(dev);
rtnl_unlock();
if (rc)
break;
@@ -2335,6 +2338,7 @@ static int ethtool_phys_id(struct net_device *dev, void __user *useraddr)
}
rtnl_lock();
+ netdev_lock_ops(dev);
netdev_put(dev, &dev_tracker);
busy = false;
@@ -3134,6 +3138,7 @@ __dev_ethtool(struct net *net, struct ifreq *ifr, void __user *useraddr,
return -EPERM;
}
+ netdev_lock_ops(dev);
if (dev->dev.parent)
pm_runtime_get_sync(dev->dev.parent);
@@ -3367,6 +3372,7 @@ __dev_ethtool(struct net *net, struct ifreq *ifr, void __user *useraddr,
out:
if (dev->dev.parent)
pm_runtime_put(dev->dev.parent);
+ netdev_unlock_ops(dev);
return rc;
}
diff --git a/net/ethtool/module.c b/net/ethtool/module.c
index 6988e07bdcd6..d3d2e135e45e 100644
--- a/net/ethtool/module.c
+++ b/net/ethtool/module.c
@@ -419,19 +419,21 @@ int ethnl_act_module_fw_flash(struct sk_buff *skb, struct genl_info *info)
dev = req_info.dev;
rtnl_lock();
+ netdev_lock_ops(dev);
ret = ethnl_ops_begin(dev);
if (ret < 0)
- goto out_rtnl;
+ goto out_unlock;
ret = ethnl_module_fw_flash_validate(dev, info->extack);
if (ret < 0)
- goto out_rtnl;
+ goto out_unlock;
ret = module_flash_fw(dev, tb, skb, info);
ethnl_ops_complete(dev);
-out_rtnl:
+out_unlock:
+ netdev_unlock_ops(dev);
rtnl_unlock();
ethnl_parse_header_dev_put(&req_info);
return ret;
diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
index b4c45207fa32..dee36f5cc228 100644
--- a/net/ethtool/netlink.c
+++ b/net/ethtool/netlink.c
@@ -90,6 +90,8 @@ int ethnl_ops_begin(struct net_device *dev)
if (dev->dev.parent)
pm_runtime_get_sync(dev->dev.parent);
+ netdev_ops_assert_locked(dev);
+
if (!netif_device_present(dev) ||
dev->reg_state >= NETREG_UNREGISTERING) {
ret = -ENODEV;
@@ -490,7 +492,11 @@ static int ethnl_default_doit(struct sk_buff *skb, struct genl_info *info)
ethnl_init_reply_data(reply_data, ops, req_info->dev);
rtnl_lock();
+ if (req_info->dev)
+ netdev_lock_ops(req_info->dev);
ret = ops->prepare_data(req_info, reply_data, info);
+ if (req_info->dev)
+ netdev_unlock_ops(req_info->dev);
rtnl_unlock();
if (ret < 0)
goto err_cleanup;
@@ -548,7 +554,9 @@ static int ethnl_default_dump_one(struct sk_buff *skb, struct net_device *dev,
ethnl_init_reply_data(ctx->reply_data, ctx->ops, dev);
rtnl_lock();
+ netdev_lock_ops(ctx->req_info->dev);
ret = ctx->ops->prepare_data(ctx->req_info, ctx->reply_data, info);
+ netdev_unlock_ops(ctx->req_info->dev);
rtnl_unlock();
if (ret < 0)
goto out;
@@ -693,6 +701,7 @@ static int ethnl_default_set_doit(struct sk_buff *skb, struct genl_info *info)
dev = req_info.dev;
rtnl_lock();
+ netdev_lock_ops(dev);
dev->cfg_pending = kmemdup(dev->cfg, sizeof(*dev->cfg),
GFP_KERNEL_ACCOUNT);
if (!dev->cfg_pending) {
@@ -720,6 +729,7 @@ static int ethnl_default_set_doit(struct sk_buff *skb, struct genl_info *info)
kfree(dev->cfg_pending);
out_tie_cfg:
dev->cfg_pending = dev->cfg;
+ netdev_unlock_ops(dev);
rtnl_unlock();
out_dev:
ethnl_parse_header_dev_put(&req_info);
@@ -777,6 +787,8 @@ static void ethnl_default_notify(struct net_device *dev, unsigned int cmd,
req_info->dev = dev;
req_info->flags |= ETHTOOL_FLAG_COMPACT_BITSETS;
+ netdev_ops_assert_locked(dev);
+
ethnl_init_reply_data(reply_data, ops, dev);
ret = ops->prepare_data(req_info, reply_data, &info);
if (ret < 0)
diff --git a/net/ethtool/phy.c b/net/ethtool/phy.c
index ed8f690f6bac..2b428bc80c9b 100644
--- a/net/ethtool/phy.c
+++ b/net/ethtool/phy.c
@@ -158,18 +158,19 @@ int ethnl_phy_doit(struct sk_buff *skb, struct genl_info *info)
return ret;
rtnl_lock();
+ netdev_lock_ops(req_info.base.dev);
ret = ethnl_phy_parse_request(&req_info.base, tb, info->extack);
if (ret < 0)
- goto err_unlock_rtnl;
+ goto err_unlock;
/* No PHY, return early */
if (!req_info.pdn)
- goto err_unlock_rtnl;
+ goto err_unlock;
ret = ethnl_phy_reply_size(&req_info.base, info->extack);
if (ret < 0)
- goto err_unlock_rtnl;
+ goto err_unlock;
reply_len = ret + ethnl_reply_header_size();
rskb = ethnl_reply_init(reply_len, req_info.base.dev,
@@ -178,13 +179,14 @@ int ethnl_phy_doit(struct sk_buff *skb, struct genl_info *info)
info, &reply_payload);
if (!rskb) {
ret = -ENOMEM;
- goto err_unlock_rtnl;
+ goto err_unlock;
}
ret = ethnl_phy_fill_reply(&req_info.base, rskb);
if (ret)
goto err_free_msg;
+ netdev_unlock_ops(req_info.base.dev);
rtnl_unlock();
ethnl_parse_header_dev_put(&req_info.base);
genlmsg_end(rskb, reply_payload);
@@ -193,7 +195,8 @@ int ethnl_phy_doit(struct sk_buff *skb, struct genl_info *info)
err_free_msg:
nlmsg_free(rskb);
-err_unlock_rtnl:
+err_unlock:
+ netdev_unlock_ops(req_info.base.dev);
rtnl_unlock();
ethnl_parse_header_dev_put(&req_info.base);
return ret;
@@ -290,10 +293,15 @@ int ethnl_phy_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
rtnl_lock();
if (ctx->phy_req_info->base.dev) {
- ret = ethnl_phy_dump_one_dev(skb, ctx->phy_req_info->base.dev, cb);
+ dev = ctx->phy_req_info->base.dev;
+ netdev_lock_ops(dev);
+ ret = ethnl_phy_dump_one_dev(skb, dev, cb);
+ netdev_unlock_ops(dev);
} else {
for_each_netdev_dump(net, dev, ctx->ifindex) {
+ netdev_lock_ops(dev);
ret = ethnl_phy_dump_one_dev(skb, dev, cb);
+ netdev_unlock_ops(dev);
if (ret)
break;
--git a/net/ethtool/rss.c b/net/ethtool/rss.c
index 58df9ad02ce8..ec41d1d7eefe 100644
--- a/net/ethtool/rss.c
+++ b/net/ethtool/rss.c
@@ -345,7 +345,9 @@ int ethnl_rss_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
if (ctx->match_ifindex && ctx->match_ifindex != ctx->ifindex)
break;
+ netdev_lock_ops(dev);
ret = rss_dump_one_dev(skb, cb, dev);
+ netdev_unlock_ops(dev);
if (ret)
break;
}
diff --git a/net/ethtool/tsinfo.c b/net/ethtool/tsinfo.c
index 691be6c445b3..73b6a89b8731 100644
--- a/net/ethtool/tsinfo.c
+++ b/net/ethtool/tsinfo.c
@@ -448,12 +448,15 @@ int ethnl_tsinfo_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
rtnl_lock();
if (ctx->req_info->base.dev) {
- ret = ethnl_tsinfo_dump_one_net_topo(skb,
- ctx->req_info->base.dev,
- cb);
+ dev = ctx->req_info->base.dev;
+ netdev_lock_ops(dev);
+ ret = ethnl_tsinfo_dump_one_net_topo(skb, dev, cb);
+ netdev_unlock_ops(dev);
} else {
for_each_netdev_dump(net, dev, ctx->pos_ifindex) {
+ netdev_lock_ops(dev);
ret = ethnl_tsinfo_dump_one_net_topo(skb, dev, cb);
+ netdev_unlock_ops(dev);
if (ret < 0 && ret != -EOPNOTSUPP)
break;
ctx->pos_phyindex = 0;
diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
index e35cf13eb4bc..583485d6cce8 100644
--- a/net/sched/sch_taprio.c
+++ b/net/sched/sch_taprio.c
@@ -1620,8 +1620,11 @@ static int taprio_parse_clockid(struct Qdisc *sch, struct nlattr **tb,
goto out;
}
- if (ops && ops->get_ts_info)
+ if (ops && ops->get_ts_info) {
+ netdev_lock_ops(dev);
err = ops->get_ts_info(dev, &info);
+ netdev_unlock_ops(dev);
+ }
if (err || info.phc_index < 0) {
NL_SET_ERR_MSG(extack,
--
2.48.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH net-next 09/11] net: dummy: add dummy shaper API
2025-02-10 19:20 [PATCH net-next 00/11] net: Hold netdev instance lock during ndo operations Stanislav Fomichev
` (7 preceding siblings ...)
2025-02-10 19:20 ` [PATCH net-next 08/11] net: ethtool: try to protect all callback with netdev instance lock Stanislav Fomichev
@ 2025-02-10 19:20 ` Stanislav Fomichev
2025-02-11 10:41 ` kernel test robot
2025-02-11 15:29 ` kernel test robot
2025-02-10 19:20 ` [PATCH net-next 10/11] docs: net: document new locking reality Stanislav Fomichev
2025-02-10 19:20 ` [PATCH net-next 11/11] eth: bnxt: remove most dependencies on RTNL Stanislav Fomichev
10 siblings, 2 replies; 22+ messages in thread
From: Stanislav Fomichev @ 2025-02-10 19:20 UTC (permalink / raw)
To: netdev; +Cc: davem, edumazet, kuba, pabeni, Saeed Mahameed
A lot of selftests are using dummy module, convert it to netdev
instance lock to expand the test coverage.
Cc: Saeed Mahameed <saeed@kernel.org>
Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
---
drivers/net/dummy.c | 37 +++++++++++++++++++++++++++++++++++++
1 file changed, 37 insertions(+)
diff --git a/drivers/net/dummy.c b/drivers/net/dummy.c
index 005d79975f3b..52d68246dc11 100644
--- a/drivers/net/dummy.c
+++ b/drivers/net/dummy.c
@@ -38,6 +38,7 @@
#include <linux/moduleparam.h>
#include <linux/rtnetlink.h>
#include <linux/net_tstamp.h>
+#include <net/net_shaper.h>
#include <net/rtnetlink.h>
#include <linux/u64_stats_sync.h>
@@ -82,6 +83,41 @@ static int dummy_change_carrier(struct net_device *dev, bool new_carrier)
return 0;
}
+static int dummy_shaper_set(struct net_shaper_binding *binding,
+ const struct net_shaper *shaper,
+ struct netlink_ext_ack *extack)
+{
+ return 0;
+}
+
+static int dummy_shaper_del(struct net_shaper_binding *binding,
+ const struct net_shaper_handle *handle,
+ struct netlink_ext_ack *extack)
+{
+ return 0;
+}
+
+static int dummy_shaper_group(struct net_shaper_binding *binding,
+ int leaves_count, const struct net_shaper *leaves,
+ const struct net_shaper *root,
+ struct netlink_ext_ack *extack)
+{
+ return 0;
+}
+
+static void dummy_shaper_cap(struct net_shaper_binding *binding,
+ enum net_shaper_scope scope, unsigned long *flags)
+{
+ *flags = ULONG_MAX;
+}
+
+static const struct net_shaper_ops dummy_shaper_ops = {
+ .set = dummy_shaper_set,
+ .delete = dummy_shaper_del,
+ .group = dummy_shaper_group,
+ .capabilities = dummy_shaper_cap,
+};
+
static const struct net_device_ops dummy_netdev_ops = {
.ndo_init = dummy_dev_init,
.ndo_start_xmit = dummy_xmit,
@@ -90,6 +126,7 @@ static const struct net_device_ops dummy_netdev_ops = {
.ndo_set_mac_address = eth_mac_addr,
.ndo_get_stats64 = dummy_get_stats64,
.ndo_change_carrier = dummy_change_carrier,
+ .net_shaper_ops = &dummy_shaper_ops,
};
static const struct ethtool_ops dummy_ethtool_ops = {
--
2.48.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH net-next 10/11] docs: net: document new locking reality
2025-02-10 19:20 [PATCH net-next 00/11] net: Hold netdev instance lock during ndo operations Stanislav Fomichev
` (8 preceding siblings ...)
2025-02-10 19:20 ` [PATCH net-next 09/11] net: dummy: add dummy shaper API Stanislav Fomichev
@ 2025-02-10 19:20 ` Stanislav Fomichev
2025-02-10 19:20 ` [PATCH net-next 11/11] eth: bnxt: remove most dependencies on RTNL Stanislav Fomichev
10 siblings, 0 replies; 22+ messages in thread
From: Stanislav Fomichev @ 2025-02-10 19:20 UTC (permalink / raw)
To: netdev; +Cc: davem, edumazet, kuba, pabeni, Saeed Mahameed
Also clarify ndo_get_stats (that reads and write paths can run
concurrently) and mention only RCU.
Cc: Saeed Mahameed <saeed@kernel.org>
Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
---
Documentation/networking/netdevices.rst | 57 +++++++++++++++++++------
include/linux/netdevice.h | 4 ++
2 files changed, 48 insertions(+), 13 deletions(-)
diff --git a/Documentation/networking/netdevices.rst b/Documentation/networking/netdevices.rst
index 1d37038e9fbe..0d2ab558f86a 100644
--- a/Documentation/networking/netdevices.rst
+++ b/Documentation/networking/netdevices.rst
@@ -210,49 +210,55 @@ packets is preferred.
struct net_device synchronization rules
=======================================
ndo_open:
- Synchronization: rtnl_lock() semaphore.
+ Synchronization: rtnl_lock() semaphore. In addition, netdev instance
+ lock if the driver implements queue management or shaper API.
Context: process
ndo_stop:
- Synchronization: rtnl_lock() semaphore.
+ Synchronization: rtnl_lock() semaphore. In addition, netdev instance
+ lock if the driver implements queue management or shaper API.
Context: process
Note: netif_running() is guaranteed false
ndo_do_ioctl:
Synchronization: rtnl_lock() semaphore.
- Context: process
- This is only called by network subsystems internally,
- not by user space calling ioctl as it was in before
- linux-5.14.
+ This is only called by network subsystems internally,
+ not by user space calling ioctl as it was in before
+ linux-5.14.
ndo_siocbond:
- Synchronization: rtnl_lock() semaphore.
+ Synchronization: rtnl_lock() semaphore. In addition, netdev instance
+ lock if the driver implements queue management or shaper API.
Context: process
- Used by the bonding driver for the SIOCBOND family of
- ioctl commands.
+ Used by the bonding driver for the SIOCBOND family of
+ ioctl commands.
ndo_siocwandev:
- Synchronization: rtnl_lock() semaphore.
+ Synchronization: rtnl_lock() semaphore. In addition, netdev instance
+ lock if the driver implements queue management or shaper API.
Context: process
Used by the drivers/net/wan framework to handle
the SIOCWANDEV ioctl with the if_settings structure.
ndo_siocdevprivate:
- Synchronization: rtnl_lock() semaphore.
+ Synchronization: rtnl_lock() semaphore. In addition, netdev instance
+ lock if the driver implements queue management or shaper API.
Context: process
This is used to implement SIOCDEVPRIVATE ioctl helpers.
These should not be added to new drivers, so don't use.
ndo_eth_ioctl:
- Synchronization: rtnl_lock() semaphore.
+ Synchronization: rtnl_lock() semaphore. In addition, netdev instance
+ lock if the driver implements queue management or shaper API.
Context: process
ndo_get_stats:
- Synchronization: rtnl_lock() semaphore, or RCU.
+ Synchronization: RCU (can be called concurrently with the stats
+ update path).
Context: atomic (can't sleep under RCU)
ndo_start_xmit:
@@ -284,6 +290,10 @@ struct net_device synchronization rules
Synchronization: netif_addr_lock spinlock.
Context: BHs disabled
+Most ndo callbacks not specified in the list above are running
+under RTNL. In addition, netdev instance lock is taken as well if
+the driver implements queue management or shaper API.
+
struct napi_struct synchronization rules
========================================
napi->poll:
@@ -298,6 +308,27 @@ struct napi_struct synchronization rules
softirq
will be called with interrupts disabled by netconsole.
+struct netdev_queue_mgmt_ops synchronization rules
+==================================================
+
+All queue management ndo callbacks are holding netdev instance lock.
+
+RTNL and netdev instance lock
+=============================
+
+Historically, all networking control operations were protected by a single
+global lock known as RTNL. There is an ongoing effort to replace this global
+lock with separate locks for each network namespace. The netdev instance lock
+represents another step towards making the locking mechanism more granular.
+
+For device drivers that implement shaping or queue management APIs, all control
+operations will be performed under the netdev instance lock. Currently, this
+instance lock is acquired within the context of RTNL. In the future, there will
+be an option for individual drivers to opt out of using RTNL and instead
+perform their control operations directly under the netdev instance lock.
+
+Devices drivers are encouraged to rely on the instance lock where possible.
+
NETDEV_INTERNAL symbol namespace
================================
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 675e5dee3219..59c7c35bae33 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -2470,6 +2470,10 @@ struct net_device {
*
* Also protects some fields in struct napi_struct.
*
+ * For the drivers that implement shaper or queue API, the scope
+ * of this lock is expanded to cover most ndo/queue/ethtool/sysfs
+ * operations.
+ *
* Ordering: take after rtnl_lock.
*/
struct mutex lock;
--
2.48.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH net-next 11/11] eth: bnxt: remove most dependencies on RTNL
2025-02-10 19:20 [PATCH net-next 00/11] net: Hold netdev instance lock during ndo operations Stanislav Fomichev
` (9 preceding siblings ...)
2025-02-10 19:20 ` [PATCH net-next 10/11] docs: net: document new locking reality Stanislav Fomichev
@ 2025-02-10 19:20 ` Stanislav Fomichev
10 siblings, 0 replies; 22+ messages in thread
From: Stanislav Fomichev @ 2025-02-10 19:20 UTC (permalink / raw)
To: netdev; +Cc: davem, edumazet, kuba, pabeni, Saeed Mahameed
Only devlink and sriov paths are grabbing rtnl explicitly. The rest is
covered by netdev instance lock which the core now grabs, so there is
no need to manage rtnl in most places anymore.
On the core side we can now try to drop rtnl in some places
(do_setlink for example) for the drivers that signal non-rtnl
mode (TBD).
Boot-tested and with `ethtool -L eth1 combined 24` to trigger reset.
Cc: Saeed Mahameed <saeed@kernel.org>
Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
---
drivers/net/ethernet/broadcom/bnxt/bnxt.c | 116 +++++++++---------
.../net/ethernet/broadcom/bnxt/bnxt_devlink.c | 9 ++
.../net/ethernet/broadcom/bnxt/bnxt_sriov.c | 6 +
drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.c | 16 ++-
drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c | 18 ++-
drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c | 3 +-
include/linux/netdevice.h | 5 +
7 files changed, 100 insertions(+), 73 deletions(-)
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index 082a6c2b5cca..a32dab09763b 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -5236,8 +5236,10 @@ static void bnxt_free_ntp_fltrs(struct bnxt *bp, bool all)
{
int i;
- /* Under rtnl_lock and all our NAPIs have been disabled. It's
- * safe to delete the hash table.
+ netdev_assert_locked(bp->dev);
+
+ /* Under netdev instance lock and all our NAPIs have been disabled.
+ * It's safe to delete the hash table.
*/
for (i = 0; i < BNXT_NTP_FLTR_HASH_SIZE; i++) {
struct hlist_head *head;
@@ -12575,7 +12577,6 @@ static int __bnxt_open_nic(struct bnxt *bp, bool irq_re_init, bool link_re_init)
return rc;
}
-/* rtnl_lock held */
int bnxt_open_nic(struct bnxt *bp, bool irq_re_init, bool link_re_init)
{
int rc = 0;
@@ -12591,9 +12592,9 @@ int bnxt_open_nic(struct bnxt *bp, bool irq_re_init, bool link_re_init)
return rc;
}
-/* rtnl_lock held, open the NIC half way by allocating all resources, but
- * NAPI, IRQ, and TX are not enabled. This is mainly used for offline
- * self tests.
+/* netdev instance lock held, open the NIC half way by allocating all
+ * resources, but NAPI, IRQ, and TX are not enabled. This is mainly used
+ * for offline self tests.
*/
int bnxt_half_open_nic(struct bnxt *bp)
{
@@ -12628,8 +12629,8 @@ int bnxt_half_open_nic(struct bnxt *bp)
return rc;
}
-/* rtnl_lock held, this call can only be made after a previous successful
- * call to bnxt_half_open_nic().
+/* netdev instance lock held, this call can only be made after a previous
+ * successful call to bnxt_half_open_nic().
*/
void bnxt_half_close_nic(struct bnxt *bp)
{
@@ -12738,10 +12739,11 @@ void bnxt_close_nic(struct bnxt *bp, bool irq_re_init, bool link_re_init)
if (test_bit(BNXT_STATE_IN_FW_RESET, &bp->state)) {
/* If we get here, it means firmware reset is in progress
* while we are trying to close. We can safely proceed with
- * the close because we are holding rtnl_lock(). Some firmware
- * messages may fail as we proceed to close. We set the
- * ABORT_ERR flag here so that the FW reset thread will later
- * abort when it gets the rtnl_lock() and sees the flag.
+ * the close because we are holding netdev instance lock.
+ * Some firmware messages may fail as we proceed to close.
+ * We set the ABORT_ERR flag here so that the FW reset thread
+ * will later abort when it gets the netdev instance lock
+ * and sees the flag.
*/
netdev_warn(bp->dev, "FW reset in progress during close, FW reset will be aborted\n");
set_bit(BNXT_STATE_ABORT_ERR, &bp->state);
@@ -12832,7 +12834,7 @@ static int bnxt_hwrm_port_phy_write(struct bnxt *bp, u16 phy_addr, u16 reg,
return hwrm_req_send(bp, req);
}
-/* rtnl_lock held */
+/* netdev instance lock held */
static int bnxt_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
{
struct mii_ioctl_data *mdio = if_mii(ifr);
@@ -13751,30 +13753,30 @@ static void bnxt_timer(struct timer_list *t)
mod_timer(&bp->timer, jiffies + bp->current_interval);
}
-static void bnxt_rtnl_lock_sp(struct bnxt *bp)
+static void bnxt_lock_sp(struct bnxt *bp)
{
/* We are called from bnxt_sp_task which has BNXT_STATE_IN_SP_TASK
* set. If the device is being closed, bnxt_close() may be holding
- * rtnl() and waiting for BNXT_STATE_IN_SP_TASK to clear. So we
- * must clear BNXT_STATE_IN_SP_TASK before holding rtnl().
+ * netdev instance lock and waiting for BNXT_STATE_IN_SP_TASK to clear.
+ * So we must clear BNXT_STATE_IN_SP_TASK before holding rtnl().
*/
clear_bit(BNXT_STATE_IN_SP_TASK, &bp->state);
- rtnl_lock();
+ netdev_lock(bp->dev);
}
-static void bnxt_rtnl_unlock_sp(struct bnxt *bp)
+static void bnxt_unlock_sp(struct bnxt *bp)
{
set_bit(BNXT_STATE_IN_SP_TASK, &bp->state);
- rtnl_unlock();
+ netdev_unlock(bp->dev);
}
/* Only called from bnxt_sp_task() */
static void bnxt_reset(struct bnxt *bp, bool silent)
{
- bnxt_rtnl_lock_sp(bp);
+ bnxt_lock_sp(bp);
if (test_bit(BNXT_STATE_OPEN, &bp->state))
bnxt_reset_task(bp, silent);
- bnxt_rtnl_unlock_sp(bp);
+ bnxt_unlock_sp(bp);
}
/* Only called from bnxt_sp_task() */
@@ -13782,9 +13784,9 @@ static void bnxt_rx_ring_reset(struct bnxt *bp)
{
int i;
- bnxt_rtnl_lock_sp(bp);
+ bnxt_lock_sp(bp);
if (!test_bit(BNXT_STATE_OPEN, &bp->state)) {
- bnxt_rtnl_unlock_sp(bp);
+ bnxt_unlock_sp(bp);
return;
}
/* Disable and flush TPA before resetting the RX ring */
@@ -13823,7 +13825,7 @@ static void bnxt_rx_ring_reset(struct bnxt *bp)
}
if (bp->flags & BNXT_FLAG_TPA)
bnxt_set_tpa(bp, true);
- bnxt_rtnl_unlock_sp(bp);
+ bnxt_unlock_sp(bp);
}
static void bnxt_fw_fatal_close(struct bnxt *bp)
@@ -13879,7 +13881,7 @@ static bool is_bnxt_fw_ok(struct bnxt *bp)
return false;
}
-/* rtnl_lock is acquired before calling this function */
+/* netdev instance lock is acquired before calling this function */
static void bnxt_force_fw_reset(struct bnxt *bp)
{
struct bnxt_fw_health *fw_health = bp->fw_health;
@@ -13922,9 +13924,9 @@ void bnxt_fw_exception(struct bnxt *bp)
netdev_warn(bp->dev, "Detected firmware fatal condition, initiating reset\n");
set_bit(BNXT_STATE_FW_FATAL_COND, &bp->state);
bnxt_ulp_stop(bp);
- bnxt_rtnl_lock_sp(bp);
+ bnxt_lock_sp(bp);
bnxt_force_fw_reset(bp);
- bnxt_rtnl_unlock_sp(bp);
+ bnxt_unlock_sp(bp);
}
/* Returns the number of registered VFs, or 1 if VF configuration is pending, or
@@ -13954,7 +13956,7 @@ static int bnxt_get_registered_vfs(struct bnxt *bp)
void bnxt_fw_reset(struct bnxt *bp)
{
bnxt_ulp_stop(bp);
- bnxt_rtnl_lock_sp(bp);
+ bnxt_lock_sp(bp);
if (test_bit(BNXT_STATE_OPEN, &bp->state) &&
!test_bit(BNXT_STATE_IN_FW_RESET, &bp->state)) {
struct bnxt_ptp_cfg *ptp = bp->ptp_cfg;
@@ -14000,7 +14002,7 @@ void bnxt_fw_reset(struct bnxt *bp)
bnxt_queue_fw_reset_work(bp, tmo);
}
fw_reset_exit:
- bnxt_rtnl_unlock_sp(bp);
+ bnxt_unlock_sp(bp);
}
static void bnxt_chk_missed_irq(struct bnxt *bp)
@@ -14199,7 +14201,7 @@ static void bnxt_sp_task(struct work_struct *work)
static void _bnxt_get_max_rings(struct bnxt *bp, int *max_rx, int *max_tx,
int *max_cp);
-/* Under rtnl_lock */
+/* Under netdev instance lock */
int bnxt_check_rings(struct bnxt *bp, int tx, int rx, bool sh, int tcs,
int tx_xdp)
{
@@ -14627,10 +14629,10 @@ static void bnxt_fw_reset_task(struct work_struct *work)
return;
}
bp->fw_reset_timestamp = jiffies;
- rtnl_lock();
+ netdev_lock(bp->dev);
if (test_bit(BNXT_STATE_ABORT_ERR, &bp->state)) {
bnxt_fw_reset_abort(bp, rc);
- rtnl_unlock();
+ netdev_unlock(bp->dev);
goto ulp_start;
}
bnxt_fw_reset_close(bp);
@@ -14641,7 +14643,7 @@ static void bnxt_fw_reset_task(struct work_struct *work)
bp->fw_reset_state = BNXT_FW_RESET_STATE_ENABLE_DEV;
tmo = bp->fw_reset_min_dsecs * HZ / 10;
}
- rtnl_unlock();
+ netdev_unlock(bp->dev);
bnxt_queue_fw_reset_work(bp, tmo);
return;
}
@@ -14715,7 +14717,7 @@ static void bnxt_fw_reset_task(struct work_struct *work)
bp->fw_reset_state = BNXT_FW_RESET_STATE_OPENING;
fallthrough;
case BNXT_FW_RESET_STATE_OPENING:
- while (!rtnl_trylock()) {
+ while (!netdev_trylock(bp->dev)) {
bnxt_queue_fw_reset_work(bp, HZ / 10);
return;
}
@@ -14723,7 +14725,7 @@ static void bnxt_fw_reset_task(struct work_struct *work)
if (rc) {
netdev_err(bp->dev, "bnxt_open() failed during FW reset\n");
bnxt_fw_reset_abort(bp, rc);
- rtnl_unlock();
+ netdev_unlock(bp->dev);
goto ulp_start;
}
@@ -14742,13 +14744,13 @@ static void bnxt_fw_reset_task(struct work_struct *work)
bnxt_dl_health_fw_recovery_done(bp);
bnxt_dl_health_fw_status_update(bp, true);
}
- rtnl_unlock();
+ netdev_unlock(bp->dev);
bnxt_ulp_start(bp, 0);
bnxt_reenable_sriov(bp);
- rtnl_lock();
+ netdev_lock(bp->dev);
bnxt_vf_reps_alloc(bp);
bnxt_vf_reps_open(bp);
- rtnl_unlock();
+ netdev_unlock(bp->dev);
break;
}
return;
@@ -14761,9 +14763,9 @@ static void bnxt_fw_reset_task(struct work_struct *work)
netdev_err(bp->dev, "fw_health_status 0x%x\n", sts);
}
fw_reset_abort:
- rtnl_lock();
+ netdev_lock(bp->dev);
bnxt_fw_reset_abort(bp, rc);
- rtnl_unlock();
+ netdev_unlock(bp->dev);
ulp_start:
bnxt_ulp_start(bp, rc);
}
@@ -14855,13 +14857,14 @@ static int bnxt_init_board(struct pci_dev *pdev, struct net_device *dev)
return rc;
}
-/* rtnl_lock held */
static int bnxt_change_mac_addr(struct net_device *dev, void *p)
{
struct sockaddr *addr = p;
struct bnxt *bp = netdev_priv(dev);
int rc = 0;
+ netdev_assert_locked(dev);
+
if (!is_valid_ether_addr(addr->sa_data))
return -EADDRNOTAVAIL;
@@ -14882,11 +14885,12 @@ static int bnxt_change_mac_addr(struct net_device *dev, void *p)
return rc;
}
-/* rtnl_lock held */
static int bnxt_change_mtu(struct net_device *dev, int new_mtu)
{
struct bnxt *bp = netdev_priv(dev);
+ netdev_assert_locked(dev);
+
if (netif_running(dev))
bnxt_close_nic(bp, true, false);
@@ -16001,7 +16005,7 @@ int bnxt_restore_pf_fw_resources(struct bnxt *bp)
{
int rc;
- ASSERT_RTNL();
+ netdev_ops_assert_locked(bp->dev);
bnxt_hwrm_func_qcaps(bp);
if (netif_running(bp->dev))
@@ -16401,7 +16405,7 @@ static void bnxt_shutdown(struct pci_dev *pdev)
if (!dev)
return;
- rtnl_lock();
+ netdev_lock(dev);
bp = netdev_priv(dev);
if (!bp)
goto shutdown_exit;
@@ -16419,7 +16423,7 @@ static void bnxt_shutdown(struct pci_dev *pdev)
}
shutdown_exit:
- rtnl_unlock();
+ netdev_unlock(dev);
}
#ifdef CONFIG_PM_SLEEP
@@ -16431,7 +16435,7 @@ static int bnxt_suspend(struct device *device)
bnxt_ulp_stop(bp);
- rtnl_lock();
+ netdev_lock(dev);
if (netif_running(dev)) {
netif_device_detach(dev);
rc = bnxt_close(dev);
@@ -16440,7 +16444,7 @@ static int bnxt_suspend(struct device *device)
bnxt_ptp_clear(bp);
pci_disable_device(bp->pdev);
bnxt_free_ctx_mem(bp, false);
- rtnl_unlock();
+ netdev_unlock(dev);
return rc;
}
@@ -16450,7 +16454,7 @@ static int bnxt_resume(struct device *device)
struct bnxt *bp = netdev_priv(dev);
int rc = 0;
- rtnl_lock();
+ netdev_lock(dev);
rc = pci_enable_device(bp->pdev);
if (rc) {
netdev_err(dev, "Cannot re-enable PCI device during resume, err = %d\n",
@@ -16493,7 +16497,7 @@ static int bnxt_resume(struct device *device)
}
resume_exit:
- rtnl_unlock();
+ netdev_unlock(bp->dev);
bnxt_ulp_start(bp, rc);
if (!rc)
bnxt_reenable_sriov(bp);
@@ -16528,7 +16532,7 @@ static pci_ers_result_t bnxt_io_error_detected(struct pci_dev *pdev,
bnxt_ulp_stop(bp);
- rtnl_lock();
+ netdev_lock(netdev);
netif_device_detach(netdev);
if (test_and_set_bit(BNXT_STATE_IN_FW_RESET, &bp->state)) {
@@ -16537,7 +16541,7 @@ static pci_ers_result_t bnxt_io_error_detected(struct pci_dev *pdev,
}
if (abort || state == pci_channel_io_perm_failure) {
- rtnl_unlock();
+ netdev_unlock(netdev);
return PCI_ERS_RESULT_DISCONNECT;
}
@@ -16556,7 +16560,7 @@ static pci_ers_result_t bnxt_io_error_detected(struct pci_dev *pdev,
if (pci_is_enabled(pdev))
pci_disable_device(pdev);
bnxt_free_ctx_mem(bp, false);
- rtnl_unlock();
+ netdev_unlock(netdev);
/* Request a slot slot reset. */
return PCI_ERS_RESULT_NEED_RESET;
@@ -16586,7 +16590,7 @@ static pci_ers_result_t bnxt_io_slot_reset(struct pci_dev *pdev)
test_bit(BNXT_STATE_PCI_CHANNEL_IO_FROZEN, &bp->state))
msleep(900);
- rtnl_lock();
+ netdev_lock(netdev);
if (pci_enable_device(pdev)) {
dev_err(&pdev->dev,
@@ -16641,7 +16645,7 @@ static pci_ers_result_t bnxt_io_slot_reset(struct pci_dev *pdev)
reset_exit:
clear_bit(BNXT_STATE_IN_FW_RESET, &bp->state);
bnxt_clear_reservations(bp, true);
- rtnl_unlock();
+ netdev_unlock(netdev);
return result;
}
@@ -16660,7 +16664,7 @@ static void bnxt_io_resume(struct pci_dev *pdev)
int err;
netdev_info(bp->dev, "PCI Slot Resume\n");
- rtnl_lock();
+ netdev_lock(netdev);
err = bnxt_hwrm_func_qcaps(bp);
if (!err) {
@@ -16673,7 +16677,7 @@ static void bnxt_io_resume(struct pci_dev *pdev)
if (!err)
netif_device_attach(netdev);
- rtnl_unlock();
+ netdev_unlock(netdev);
bnxt_ulp_start(bp, err);
if (!err)
bnxt_reenable_sriov(bp);
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
index ef8288fd68f4..b06fcddfc81c 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c
@@ -439,14 +439,17 @@ static int bnxt_dl_reload_down(struct devlink *dl, bool netns_change,
case DEVLINK_RELOAD_ACTION_DRIVER_REINIT: {
bnxt_ulp_stop(bp);
rtnl_lock();
+ netdev_lock(bp->dev);
if (bnxt_sriov_cfg(bp)) {
NL_SET_ERR_MSG_MOD(extack,
"reload is unsupported while VFs are allocated or being configured");
+ netdev_unlock(bp->dev);
rtnl_unlock();
bnxt_ulp_start(bp, 0);
return -EOPNOTSUPP;
}
if (bp->dev->reg_state == NETREG_UNREGISTERED) {
+ netdev_unlock(bp->dev);
rtnl_unlock();
bnxt_ulp_start(bp, 0);
return -ENODEV;
@@ -459,6 +462,7 @@ static int bnxt_dl_reload_down(struct devlink *dl, bool netns_change,
NL_SET_ERR_MSG_MOD(extack, "Failed to deregister");
if (netif_running(bp->dev))
dev_close(bp->dev);
+ netdev_unlock(bp->dev);
rtnl_unlock();
break;
}
@@ -479,7 +483,9 @@ static int bnxt_dl_reload_down(struct devlink *dl, bool netns_change,
return -EPERM;
}
rtnl_lock();
+ netdev_lock(bp->dev);
if (bp->dev->reg_state == NETREG_UNREGISTERED) {
+ netdev_unlock(bp->dev);
rtnl_unlock();
return -ENODEV;
}
@@ -493,6 +499,7 @@ static int bnxt_dl_reload_down(struct devlink *dl, bool netns_change,
if (rc) {
NL_SET_ERR_MSG_MOD(extack, "Failed to activate firmware");
clear_bit(BNXT_STATE_FW_ACTIVATE, &bp->state);
+ netdev_unlock(bp->dev);
rtnl_unlock();
}
break;
@@ -568,7 +575,9 @@ static int bnxt_dl_reload_up(struct devlink *dl, enum devlink_reload_action acti
}
*actions_performed |= BIT(action);
} else if (netif_running(bp->dev)) {
+ netdev_lock(bp->dev);
dev_close(bp->dev);
+ netdev_unlock(bp->dev);
}
rtnl_unlock();
if (action == DEVLINK_RELOAD_ACTION_DRIVER_REINIT)
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_sriov.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_sriov.c
index 12b6ed51fd88..5ddddd89052f 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_sriov.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_sriov.c
@@ -946,7 +946,9 @@ void bnxt_sriov_disable(struct bnxt *bp)
/* Reclaim all resources for the PF. */
rtnl_lock();
+ netdev_lock(bp->dev);
bnxt_restore_pf_fw_resources(bp);
+ netdev_unlock(bp->dev);
rtnl_unlock();
}
@@ -956,17 +958,21 @@ int bnxt_sriov_configure(struct pci_dev *pdev, int num_vfs)
struct bnxt *bp = netdev_priv(dev);
rtnl_lock();
+ netdev_lock(dev);
if (!netif_running(dev)) {
netdev_warn(dev, "Reject SRIOV config request since if is down!\n");
+ netdev_unlock(dev);
rtnl_unlock();
return 0;
}
if (test_bit(BNXT_STATE_IN_FW_RESET, &bp->state)) {
netdev_warn(dev, "Reject SRIOV config request when FW reset is in progress\n");
+ netdev_unlock(dev);
rtnl_unlock();
return 0;
}
bp->sriov_cfg = true;
+ netdev_unlock(dev);
rtnl_unlock();
if (pci_vfs_assigned(bp->pdev)) {
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.c
index e4a7f37036ed..a8e930d5dbb0 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.c
@@ -112,7 +112,7 @@ int bnxt_register_dev(struct bnxt_en_dev *edev,
struct bnxt_ulp *ulp;
int rc = 0;
- rtnl_lock();
+ netdev_lock(dev);
mutex_lock(&edev->en_dev_lock);
if (!bp->irq_tbl) {
rc = -ENODEV;
@@ -138,7 +138,7 @@ int bnxt_register_dev(struct bnxt_en_dev *edev,
edev->flags |= BNXT_EN_FLAG_MSIX_REQUESTED;
exit:
mutex_unlock(&edev->en_dev_lock);
- rtnl_unlock();
+ netdev_unlock(dev);
return rc;
}
EXPORT_SYMBOL(bnxt_register_dev);
@@ -151,7 +151,7 @@ void bnxt_unregister_dev(struct bnxt_en_dev *edev)
int i = 0;
ulp = edev->ulp_tbl;
- rtnl_lock();
+ netdev_lock(dev);
mutex_lock(&edev->en_dev_lock);
if (ulp->msix_requested)
edev->flags &= ~BNXT_EN_FLAG_MSIX_REQUESTED;
@@ -169,7 +169,7 @@ void bnxt_unregister_dev(struct bnxt_en_dev *edev)
i++;
}
mutex_unlock(&edev->en_dev_lock);
- rtnl_unlock();
+ netdev_unlock(dev);
return;
}
EXPORT_SYMBOL(bnxt_unregister_dev);
@@ -309,12 +309,14 @@ void bnxt_ulp_irq_stop(struct bnxt *bp)
if (!ulp->msix_requested)
return;
- ops = rtnl_dereference(ulp->ulp_ops);
+ netdev_lock(bp->dev);
+ ops = rcu_dereference(ulp->ulp_ops);
if (!ops || !ops->ulp_irq_stop)
return;
if (test_bit(BNXT_STATE_FW_RESET_DET, &bp->state))
reset = true;
ops->ulp_irq_stop(ulp->handle, reset);
+ netdev_unlock(bp->dev);
}
}
@@ -333,7 +335,8 @@ void bnxt_ulp_irq_restart(struct bnxt *bp, int err)
if (!ulp->msix_requested)
return;
- ops = rtnl_dereference(ulp->ulp_ops);
+ netdev_lock(bp->dev);
+ ops = rcu_dereference(ulp->ulp_ops);
if (!ops || !ops->ulp_irq_restart)
return;
@@ -345,6 +348,7 @@ void bnxt_ulp_irq_restart(struct bnxt *bp, int err)
bnxt_fill_msix_vecs(bp, ent);
}
ops->ulp_irq_restart(ulp->handle, ent);
+ netdev_unlock(bp->dev);
kfree(ent);
}
}
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c
index 1467b94a6427..619f0844e778 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_vfr.c
@@ -257,8 +257,7 @@ bool bnxt_dev_is_vf_rep(struct net_device *dev)
/* Called when the parent PF interface is closed:
* As the mode transition from SWITCHDEV to LEGACY
- * happens under the rtnl_lock() this routine is safe
- * under the rtnl_lock()
+ * happens under the netdev instance lock this routine is safe
*/
void bnxt_vf_reps_close(struct bnxt *bp)
{
@@ -278,8 +277,7 @@ void bnxt_vf_reps_close(struct bnxt *bp)
/* Called when the parent PF interface is opened (re-opened):
* As the mode transition from SWITCHDEV to LEGACY
- * happen under the rtnl_lock() this routine is safe
- * under the rtnl_lock()
+ * happen under the netdev instance lock this routine is safe
*/
void bnxt_vf_reps_open(struct bnxt *bp)
{
@@ -348,7 +346,7 @@ void bnxt_vf_reps_destroy(struct bnxt *bp)
/* Ensure that parent PF's and VF-reps' RX/TX has been quiesced
* before proceeding with VF-rep cleanup.
*/
- rtnl_lock();
+ netdev_lock(bp->dev);
if (netif_running(bp->dev)) {
bnxt_close_nic(bp, false, false);
closed = true;
@@ -365,10 +363,10 @@ void bnxt_vf_reps_destroy(struct bnxt *bp)
bnxt_open_nic(bp, false, false);
bp->eswitch_mode = DEVLINK_ESWITCH_MODE_SWITCHDEV;
}
- rtnl_unlock();
+ netdev_unlock(bp->dev);
- /* Need to call vf_reps_destroy() outside of rntl_lock
- * as unregister_netdev takes rtnl_lock
+ /* Need to call vf_reps_destroy() outside of netdev instance lock
+ * as unregister_netdev takes it
*/
__bnxt_vf_reps_destroy(bp);
}
@@ -376,7 +374,7 @@ void bnxt_vf_reps_destroy(struct bnxt *bp)
/* Free the VF-Reps in firmware, during firmware hot-reset processing.
* Note that the VF-Rep netdevs are still active (not unregistered) during
* this process. As the mode transition from SWITCHDEV to LEGACY happens
- * under the rtnl_lock() this routine is safe under the rtnl_lock().
+ * under the netdev instance lock this routine is safe.
*/
void bnxt_vf_reps_free(struct bnxt *bp)
{
@@ -413,7 +411,7 @@ static int bnxt_alloc_vf_rep(struct bnxt *bp, struct bnxt_vf_rep *vf_rep,
/* Allocate the VF-Reps in firmware, during firmware hot-reset processing.
* Note that the VF-Rep netdevs are still active (not unregistered) during
* this process. As the mode transition from SWITCHDEV to LEGACY happens
- * under the rtnl_lock() this routine is safe under the rtnl_lock().
+ * under the netdev instance lock this routine is safe.
*/
int bnxt_vf_reps_alloc(struct bnxt *bp)
{
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c
index e6c64e4bd66c..0caf6e9bccb8 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c
@@ -382,13 +382,14 @@ int bnxt_xdp_xmit(struct net_device *dev, int num_frames,
return nxmit;
}
-/* Under rtnl_lock */
static int bnxt_xdp_set(struct bnxt *bp, struct bpf_prog *prog)
{
struct net_device *dev = bp->dev;
int tx_xdp = 0, tx_cp, rc, tc;
struct bpf_prog *old;
+ netdev_assert_locked(dev);
+
if (prog && !prog->aux->xdp_has_frags &&
bp->dev->mtu > BNXT_MAX_PAGE_MODE_MTU) {
netdev_warn(dev, "MTU %d larger than %d without XDP frag support.\n",
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 59c7c35bae33..3f168ed35b69 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -2705,6 +2705,11 @@ static inline void netdev_lock(struct net_device *dev)
mutex_lock(&dev->lock);
}
+static inline bool netdev_trylock(struct net_device *dev)
+{
+ return mutex_trylock(&dev->lock);
+}
+
static inline void netdev_unlock(struct net_device *dev)
{
mutex_unlock(&dev->lock);
--
2.48.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH net-next 09/11] net: dummy: add dummy shaper API
2025-02-10 19:20 ` [PATCH net-next 09/11] net: dummy: add dummy shaper API Stanislav Fomichev
@ 2025-02-11 10:41 ` kernel test robot
2025-02-11 15:29 ` kernel test robot
1 sibling, 0 replies; 22+ messages in thread
From: kernel test robot @ 2025-02-11 10:41 UTC (permalink / raw)
To: Stanislav Fomichev, netdev
Cc: oe-kbuild-all, davem, edumazet, kuba, pabeni, Saeed Mahameed
Hi Stanislav,
kernel test robot noticed the following build errors:
[auto build test ERROR on net-next/main]
url: https://github.com/intel-lab-lkp/linux/commits/Stanislav-Fomichev/net-hold-netdev-instance-lock-during-ndo_open-ndo_stop/20250211-032336
base: net-next/main
patch link: https://lore.kernel.org/r/20250210192043.439074-10-sdf%40fomichev.me
patch subject: [PATCH net-next 09/11] net: dummy: add dummy shaper API
config: i386-randconfig-013-20250211 (https://download.01.org/0day-ci/archive/20250211/202502111855.5U4I6OK5-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250211/202502111855.5U4I6OK5-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202502111855.5U4I6OK5-lkp@intel.com/
All errors (new ones prefixed by >>):
>> drivers/net/dummy.c:129:10: error: 'const struct net_device_ops' has no member named 'net_shaper_ops'
129 | .net_shaper_ops = &dummy_shaper_ops,
| ^~~~~~~~~~~~~~
>> drivers/net/dummy.c:129:35: error: initialization of 'int (*)(struct net_device *, struct netdev_phys_item_id *)' from incompatible pointer type 'const struct net_shaper_ops *' [-Werror=incompatible-pointer-types]
129 | .net_shaper_ops = &dummy_shaper_ops,
| ^
drivers/net/dummy.c:129:35: note: (near initialization for 'dummy_netdev_ops.ndo_get_phys_port_id')
cc1: some warnings being treated as errors
vim +129 drivers/net/dummy.c
120
121 static const struct net_device_ops dummy_netdev_ops = {
122 .ndo_init = dummy_dev_init,
123 .ndo_start_xmit = dummy_xmit,
124 .ndo_validate_addr = eth_validate_addr,
125 .ndo_set_rx_mode = set_multicast_list,
126 .ndo_set_mac_address = eth_mac_addr,
127 .ndo_get_stats64 = dummy_get_stats64,
128 .ndo_change_carrier = dummy_change_carrier,
> 129 .net_shaper_ops = &dummy_shaper_ops,
130 };
131
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH net-next 04/11] net: hold netdev instance lock during rtnetlink operations
2025-02-10 19:20 ` [PATCH net-next 04/11] net: hold netdev instance lock during rtnetlink operations Stanislav Fomichev
@ 2025-02-11 15:06 ` kernel test robot
2025-02-11 16:32 ` Stanislav Fomichev
2025-02-12 2:33 ` Jakub Kicinski
1 sibling, 1 reply; 22+ messages in thread
From: kernel test robot @ 2025-02-11 15:06 UTC (permalink / raw)
To: Stanislav Fomichev, netdev
Cc: oe-kbuild-all, davem, edumazet, kuba, pabeni, Saeed Mahameed
Hi Stanislav,
kernel test robot noticed the following build warnings:
[auto build test WARNING on net-next/main]
url: https://github.com/intel-lab-lkp/linux/commits/Stanislav-Fomichev/net-hold-netdev-instance-lock-during-ndo_open-ndo_stop/20250211-032336
base: net-next/main
patch link: https://lore.kernel.org/r/20250210192043.439074-5-sdf%40fomichev.me
patch subject: [PATCH net-next 04/11] net: hold netdev instance lock during rtnetlink operations
config: arc-randconfig-001-20250211 (https://download.01.org/0day-ci/archive/20250211/202502112254.DdkYlmMx-lkp@intel.com/config)
compiler: arceb-elf-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250211/202502112254.DdkYlmMx-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202502112254.DdkYlmMx-lkp@intel.com/
All warnings (new ones prefixed by >>):
net/core/dev.c: In function 'dev_set_mtu':
net/core/dev.c:9320:15: error: implicit declaration of function 'netdev_set_mtu_ext_locked'; did you mean 'netdev_ops_assert_locked'? [-Werror=implicit-function-declaration]
9320 | err = netdev_set_mtu_ext_locked(dev, new_mtu, &extack);
| ^~~~~~~~~~~~~~~~~~~~~~~~~
| netdev_ops_assert_locked
net/core/dev.c: At top level:
>> net/core/dev.c:11390:12: warning: 'netdev_lock_cmp_fn' defined but not used [-Wunused-function]
11390 | static int netdev_lock_cmp_fn(const struct lockdep_map *a, const struct lockdep_map *b)
| ^~~~~~~~~~~~~~~~~~
cc1: some warnings being treated as errors
--
>> net/core/dev_api.c:109: warning: Excess function parameter 'new_ifindex' description in 'dev_change_net_namespace'
vim +/netdev_lock_cmp_fn +11390 net/core/dev.c
11389
11390 static int netdev_lock_cmp_fn(const struct lockdep_map *a, const struct lockdep_map *b)
11391 {
11392 /* Only lower devices currently grab the instance lock, so no
11393 * real ordering issues can occur. In the near future, only
11394 * hardware devices will grab instance lock which also does not
11395 * involve any ordering. Suppress lockdep ordering warnings
11396 * until (if) we start grabbing instance lock on pure SW
11397 * devices (bond/team/etc).
11398 */
11399 return -1;
11400 }
11401
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH net-next 09/11] net: dummy: add dummy shaper API
2025-02-10 19:20 ` [PATCH net-next 09/11] net: dummy: add dummy shaper API Stanislav Fomichev
2025-02-11 10:41 ` kernel test robot
@ 2025-02-11 15:29 ` kernel test robot
1 sibling, 0 replies; 22+ messages in thread
From: kernel test robot @ 2025-02-11 15:29 UTC (permalink / raw)
To: Stanislav Fomichev, netdev
Cc: llvm, oe-kbuild-all, davem, edumazet, kuba, pabeni,
Saeed Mahameed
Hi Stanislav,
kernel test robot noticed the following build errors:
[auto build test ERROR on net-next/main]
url: https://github.com/intel-lab-lkp/linux/commits/Stanislav-Fomichev/net-hold-netdev-instance-lock-during-ndo_open-ndo_stop/20250211-032336
base: net-next/main
patch link: https://lore.kernel.org/r/20250210192043.439074-10-sdf%40fomichev.me
patch subject: [PATCH net-next 09/11] net: dummy: add dummy shaper API
config: i386-randconfig-001-20250211 (https://download.01.org/0day-ci/archive/20250211/202502112354.4t60yYpw-lkp@intel.com/config)
compiler: clang version 19.1.3 (https://github.com/llvm/llvm-project ab51eccf88f5321e7c60591c5546b254b6afab99)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250211/202502112354.4t60yYpw-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202502112354.4t60yYpw-lkp@intel.com/
All errors (new ones prefixed by >>):
>> drivers/net/dummy.c:129:3: error: field designator 'net_shaper_ops' does not refer to any field in type 'const struct net_device_ops'
129 | .net_shaper_ops = &dummy_shaper_ops,
| ~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 error generated.
vim +129 drivers/net/dummy.c
120
121 static const struct net_device_ops dummy_netdev_ops = {
122 .ndo_init = dummy_dev_init,
123 .ndo_start_xmit = dummy_xmit,
124 .ndo_validate_addr = eth_validate_addr,
125 .ndo_set_rx_mode = set_multicast_list,
126 .ndo_set_mac_address = eth_mac_addr,
127 .ndo_get_stats64 = dummy_get_stats64,
128 .ndo_change_carrier = dummy_change_carrier,
> 129 .net_shaper_ops = &dummy_shaper_ops,
130 };
131
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH net-next 04/11] net: hold netdev instance lock during rtnetlink operations
2025-02-11 15:06 ` kernel test robot
@ 2025-02-11 16:32 ` Stanislav Fomichev
0 siblings, 0 replies; 22+ messages in thread
From: Stanislav Fomichev @ 2025-02-11 16:32 UTC (permalink / raw)
To: kernel test robot
Cc: Stanislav Fomichev, netdev, oe-kbuild-all, davem, edumazet, kuba,
pabeni, Saeed Mahameed
On 02/11, kernel test robot wrote:
> Hi Stanislav,
>
> kernel test robot noticed the following build warnings:
>
> [auto build test WARNING on net-next/main]
>
> url: https://github.com/intel-lab-lkp/linux/commits/Stanislav-Fomichev/net-hold-netdev-instance-lock-during-ndo_open-ndo_stop/20250211-032336
> base: net-next/main
> patch link: https://lore.kernel.org/r/20250210192043.439074-5-sdf%40fomichev.me
> patch subject: [PATCH net-next 04/11] net: hold netdev instance lock during rtnetlink operations
> config: arc-randconfig-001-20250211 (https://download.01.org/0day-ci/archive/20250211/202502112254.DdkYlmMx-lkp@intel.com/config)
> compiler: arceb-elf-gcc (GCC) 13.2.0
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250211/202502112254.DdkYlmMx-lkp@intel.com/reproduce)
>
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202502112254.DdkYlmMx-lkp@intel.com/
>
> All warnings (new ones prefixed by >>):
>
> net/core/dev.c: In function 'dev_set_mtu':
> net/core/dev.c:9320:15: error: implicit declaration of function 'netdev_set_mtu_ext_locked'; did you mean 'netdev_ops_assert_locked'? [-Werror=implicit-function-declaration]
> 9320 | err = netdev_set_mtu_ext_locked(dev, new_mtu, &extack);
> | ^~~~~~~~~~~~~~~~~~~~~~~~~
> | netdev_ops_assert_locked
> net/core/dev.c: At top level:
Looks like my rebase error, will fix it in v2. Should hopefully not
affect the review overall..
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH net-next 01/11] net: hold netdev instance lock during ndo_open/ndo_stop
2025-02-10 19:20 ` [PATCH net-next 01/11] net: hold netdev instance lock during ndo_open/ndo_stop Stanislav Fomichev
@ 2025-02-12 2:06 ` Jakub Kicinski
0 siblings, 0 replies; 22+ messages in thread
From: Jakub Kicinski @ 2025-02-12 2:06 UTC (permalink / raw)
To: Stanislav Fomichev; +Cc: netdev, davem, edumazet, pabeni, Saeed Mahameed
On Mon, 10 Feb 2025 11:20:33 -0800 Stanislav Fomichev wrote:
> +static inline bool need_netdev_ops_lock(struct net_device *dev)
nit: netdev_need_ops_lock() ?
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH net-next 02/11] net: hold netdev instance lock during ndo_setup_tc
2025-02-10 19:20 ` [PATCH net-next 02/11] net: hold netdev instance lock during ndo_setup_tc Stanislav Fomichev
@ 2025-02-12 2:20 ` Jakub Kicinski
2025-02-12 3:48 ` Stanislav Fomichev
0 siblings, 1 reply; 22+ messages in thread
From: Jakub Kicinski @ 2025-02-12 2:20 UTC (permalink / raw)
To: Stanislav Fomichev; +Cc: netdev, davem, edumazet, pabeni, Saeed Mahameed
On Mon, 10 Feb 2025 11:20:34 -0800 Stanislav Fomichev wrote:
> Introduce new dev_setup_tc that handles the details and call it from
> all qdiscs/classifiers. The instance lock is still applied only to
> the drivers that implement shaper API so only iavf is affected.
> +int dev_setup_tc(struct net_device *dev, enum tc_setup_type type,
> + void *type_data)
> +{
> + const struct net_device_ops *ops = dev->netdev_ops;
> +
> + ASSERT_RTNL();
> +
> + if (tc_can_offload(dev) && ops->ndo_setup_tc) {
> + int ret = -ENODEV;
> +
> + if (netif_device_present(dev)) {
> + netdev_lock_ops(dev);
> + ret = ops->ndo_setup_tc(dev, type, type_data);
> + netdev_unlock_ops(dev);
> + }
> +
> + return ret;
> + }
> +
> + return -EOPNOTSUPP;
Why the indent? IMHO this would be cleaner:
if (!tc_can_offload || !ops...
return -ENOPNOTSUPP;
if (!netif_device_present(dev))
return -ENODEV:
netdev_lock_ops(dev);
...
> diff --git a/net/dsa/user.c b/net/dsa/user.c
> index 291ab1b4acc4..f2ac7662e4cc 100644
> --- a/net/dsa/user.c
> +++ b/net/dsa/user.c
> @@ -1729,10 +1729,7 @@ static int dsa_user_setup_ft_block(struct dsa_switch *ds, int port,
> {
> struct net_device *conduit = dsa_port_to_conduit(dsa_to_port(ds, port));
>
> - if (!conduit->netdev_ops->ndo_setup_tc)
> - return -EOPNOTSUPP;
> -
> - return conduit->netdev_ops->ndo_setup_tc(conduit, TC_SETUP_FT, type_data);
> + return dev_setup_tc(conduit, TC_SETUP_FT, type_data);
The netfilter / flow table offloads don't seem to test tc_can_offload(),
should we make that part of the check optional in dev_setup_tc() ?
Add a bool argument to ignore tc_can_offload() ?
> @@ -855,10 +853,7 @@ void qdisc_offload_graft_helper(struct net_device *dev, struct Qdisc *sch,
> bool any_qdisc_is_offloaded;
> int err;
>
> - if (!tc_can_offload(dev) || !dev->netdev_ops->ndo_setup_tc)
> - return;
> -
> - err = dev->netdev_ops->ndo_setup_tc(dev, type, type_data);
> + err = dev_setup_tc(dev, type, type_data);
Probably need to handle -EOPNOTSUPP here now?
> /* Don't report error if the graft is part of destroy operation. */
> if (!err || !new || new == &noop_qdisc)
> - err = ops->ndo_setup_tc(dev, TC_SETUP_QDISC_CBS, &cbs);
> + err = dev_setup_tc(dev, TC_SETUP_QDISC_CBS, &cbs);
> if (err < 0)
> pr_warn("Couldn't disable CBS offload for queue %d\n",
> cbs.queue);
> @@ -294,7 +289,7 @@ static int cbs_enable_offload(struct net_device *dev, struct cbs_sched_data *q,
> cbs.idleslope = opt->idleslope;
> cbs.sendslope = opt->sendslope;
>
> - err = ops->ndo_setup_tc(dev, TC_SETUP_QDISC_CBS, &cbs);
> + err = dev_setup_tc(dev, TC_SETUP_QDISC_CBS, &cbs);
$ for f in $(git grep --files-with-matches TC_SETUP_QDISC_CBS -- drivers/ ); do \
d=$(dirname $f); \
git grep HW_TC -- $d || echo No match in $d; \
done
No match in drivers/net/dsa/microchip
No match in drivers/net/dsa/ocelot
No match in drivers/net/dsa/sja1105
drivers/net/ethernet/freescale/enetc/enetc_pf.c: if (changed & NETIF_F_HW_TC) {
drivers/net/ethernet/freescale/enetc/enetc_pf.c: err = enetc_set_psfp(ndev, !!(features & NETIF_F_HW_TC));
drivers/net/ethernet/freescale/enetc/enetc_pf_common.c: ndev->features |= NETIF_F_HW_TC;
drivers/net/ethernet/freescale/enetc/enetc_pf_common.c: ndev->hw_features |= NETIF_F_HW_TC;
drivers/net/ethernet/intel/igb/igb_main.c: netdev->features |= NETIF_F_HW_TC;
drivers/net/ethernet/intel/igc/igc_main.c: netdev->features |= NETIF_F_HW_TC;
drivers/net/ethernet/microchip/lan966x/lan966x_main.c: NETIF_F_HW_TC;
drivers/net/ethernet/microchip/lan966x/lan966x_main.c: dev->hw_features |= NETIF_F_HW_TC;
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c: ndev->hw_features |= NETIF_F_HW_TC;
drivers/net/ethernet/ti/am65-cpsw-nuss.c: NETIF_F_HW_TC;
drivers/net/ethernet/ti/cpsw_new.c: NETIF_F_HW_VLAN_CTAG_RX | NETIF_F_HW_TC;
Looks like some the these qdiscs will need to ignore the features too :(
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH net-next 03/11] net: hold netdev instance lock during queue operations
2025-02-10 19:20 ` [PATCH net-next 03/11] net: hold netdev instance lock during queue operations Stanislav Fomichev
@ 2025-02-12 2:21 ` Jakub Kicinski
0 siblings, 0 replies; 22+ messages in thread
From: Jakub Kicinski @ 2025-02-12 2:21 UTC (permalink / raw)
To: Stanislav Fomichev; +Cc: netdev, davem, edumazet, pabeni, Saeed Mahameed
On Mon, 10 Feb 2025 11:20:35 -0800 Stanislav Fomichev wrote:
> - netif_napi_add(priv->dev, &block->napi, gve_poll);
> - netif_napi_set_irq(&block->napi, block->irq);
> + netdev_assert_locked(priv->dev);
> + netif_napi_add_locked(priv->dev, &block->napi, gve_poll);
> + netif_napi_set_irq_locked(&block->napi, block->irq);
> }
>
> void gve_remove_napi(struct gve_priv *priv, int ntfy_idx)
> {
> struct gve_notify_block *block = &priv->ntfy_blocks[ntfy_idx];
>
> - netif_napi_del(&block->napi);
> + netdev_assert_locked(priv->dev);
> + netif_napi_del_locked(&block->napi);
nit: probably don't need the asserts right above _locked() calls
I think the _locked() calls already assert
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH net-next 04/11] net: hold netdev instance lock during rtnetlink operations
2025-02-10 19:20 ` [PATCH net-next 04/11] net: hold netdev instance lock during rtnetlink operations Stanislav Fomichev
2025-02-11 15:06 ` kernel test robot
@ 2025-02-12 2:33 ` Jakub Kicinski
1 sibling, 0 replies; 22+ messages in thread
From: Jakub Kicinski @ 2025-02-12 2:33 UTC (permalink / raw)
To: Stanislav Fomichev; +Cc: netdev, davem, edumazet, pabeni, Saeed Mahameed
On Mon, 10 Feb 2025 11:20:36 -0800 Stanislav Fomichev wrote:
> @@ -11468,6 +11387,18 @@ void netdev_sw_irq_coalesce_default_on(struct net_device *dev)
> }
> EXPORT_SYMBOL_GPL(netdev_sw_irq_coalesce_default_on);
>
> +static int netdev_lock_cmp_fn(const struct lockdep_map *a, const struct lockdep_map *b)
over 80 chars, fwiw
> +{
> + /* Only lower devices currently grab the instance lock, so no
> + * real ordering issues can occur. In the near future, only
> + * hardware devices will grab instance lock which also does not
> + * involve any ordering. Suppress lockdep ordering warnings
> + * until (if) we start grabbing instance lock on pure SW
> + * devices (bond/team/etc).
mentioning bond and team here may be misleading, add veth or dummy
into this list? Otherwise it may seem like it's about uppers.
For other devices we could probably sort the shutdown list, and
we would be good. But yeah, we can defer.
> +/**
> + * dev_change_name - change name of a device
> + * @dev: device
> + * @newname: name (or format string) must be at least IFNAMSIZ
> + *
> + * Change name of a device, can pass format strings "eth%d".
> + * for wildcarding.
Would you mind making these kdocs suck a bit less while we move them?
No tab indent.
round brackets and no tabs in first line, just:
* function() - short description
Document the return value in the kdoc way:
* Return: 0 on success, -errno on failure.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH net-next 02/11] net: hold netdev instance lock during ndo_setup_tc
2025-02-12 2:20 ` Jakub Kicinski
@ 2025-02-12 3:48 ` Stanislav Fomichev
2025-02-12 3:57 ` Jakub Kicinski
0 siblings, 1 reply; 22+ messages in thread
From: Stanislav Fomichev @ 2025-02-12 3:48 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Stanislav Fomichev, netdev, davem, edumazet, pabeni,
Saeed Mahameed
On 02/11, Jakub Kicinski wrote:
> On Mon, 10 Feb 2025 11:20:34 -0800 Stanislav Fomichev wrote:
> > Introduce new dev_setup_tc that handles the details and call it from
> > all qdiscs/classifiers. The instance lock is still applied only to
> > the drivers that implement shaper API so only iavf is affected.
>
> > +int dev_setup_tc(struct net_device *dev, enum tc_setup_type type,
> > + void *type_data)
> > +{
> > + const struct net_device_ops *ops = dev->netdev_ops;
> > +
> > + ASSERT_RTNL();
> > +
> > + if (tc_can_offload(dev) && ops->ndo_setup_tc) {
> > + int ret = -ENODEV;
> > +
> > + if (netif_device_present(dev)) {
> > + netdev_lock_ops(dev);
> > + ret = ops->ndo_setup_tc(dev, type, type_data);
> > + netdev_unlock_ops(dev);
> > + }
> > +
> > + return ret;
> > + }
> > +
> > + return -EOPNOTSUPP;
>
> Why the indent? IMHO this would be cleaner:
>
> if (!tc_can_offload || !ops...
> return -ENOPNOTSUPP;
> if (!netif_device_present(dev))
> return -ENODEV:
>
> netdev_lock_ops(dev);
> ...
I was trying to keep consistent style with existing
dev_siocbond/dev_siocdevprivate/dev_siocwandev. Agreed that non-indented
flow looks much nicer, will use it for dev_setup_tc.
> > diff --git a/net/dsa/user.c b/net/dsa/user.c
> > index 291ab1b4acc4..f2ac7662e4cc 100644
> > --- a/net/dsa/user.c
> > +++ b/net/dsa/user.c
> > @@ -1729,10 +1729,7 @@ static int dsa_user_setup_ft_block(struct dsa_switch *ds, int port,
> > {
> > struct net_device *conduit = dsa_port_to_conduit(dsa_to_port(ds, port));
> >
> > - if (!conduit->netdev_ops->ndo_setup_tc)
> > - return -EOPNOTSUPP;
> > -
> > - return conduit->netdev_ops->ndo_setup_tc(conduit, TC_SETUP_FT, type_data);
> > + return dev_setup_tc(conduit, TC_SETUP_FT, type_data);
>
> The netfilter / flow table offloads don't seem to test tc_can_offload(),
> should we make that part of the check optional in dev_setup_tc() ?
> Add a bool argument to ignore tc_can_offload() ?
Let me dig into it... I was assuming that tc_can_offload() is basically
a runtime way to signal to the core that even though the device has
ndo_setup_tc defined, the feature can't be used.
I don't understand why some places care only about ndo_setup_tc
while other test for both ndo_setup_tc/tc_can_offload. Do you by
chance have any context on this? Does tc_can_offload cover only
a subset of (TC_SETUP_BLOCK) the offload types?
The easiest way is probably just to keep calls to tc_can_offload outside,
as is, but I was thinking that doing both ndo_setup_tc and tc_can_offload
is a bit more safe.
> > @@ -855,10 +853,7 @@ void qdisc_offload_graft_helper(struct net_device *dev, struct Qdisc *sch,
> > bool any_qdisc_is_offloaded;
> > int err;
> >
> > - if (!tc_can_offload(dev) || !dev->netdev_ops->ndo_setup_tc)
> > - return;
> > -
> > - err = dev->netdev_ops->ndo_setup_tc(dev, type, type_data);
> > + err = dev_setup_tc(dev, type, type_data);
>
> Probably need to handle -EOPNOTSUPP here now?
Ah, yes, thanks!
> > /* Don't report error if the graft is part of destroy operation. */
> > if (!err || !new || new == &noop_qdisc)
>
> > - err = ops->ndo_setup_tc(dev, TC_SETUP_QDISC_CBS, &cbs);
> > + err = dev_setup_tc(dev, TC_SETUP_QDISC_CBS, &cbs);
> > if (err < 0)
> > pr_warn("Couldn't disable CBS offload for queue %d\n",
> > cbs.queue);
> > @@ -294,7 +289,7 @@ static int cbs_enable_offload(struct net_device *dev, struct cbs_sched_data *q,
> > cbs.idleslope = opt->idleslope;
> > cbs.sendslope = opt->sendslope;
> >
> > - err = ops->ndo_setup_tc(dev, TC_SETUP_QDISC_CBS, &cbs);
> > + err = dev_setup_tc(dev, TC_SETUP_QDISC_CBS, &cbs);
>
> $ for f in $(git grep --files-with-matches TC_SETUP_QDISC_CBS -- drivers/ ); do \
> d=$(dirname $f); \
> git grep HW_TC -- $d || echo No match in $d; \
> done
>
> No match in drivers/net/dsa/microchip
> No match in drivers/net/dsa/ocelot
> No match in drivers/net/dsa/sja1105
> drivers/net/ethernet/freescale/enetc/enetc_pf.c: if (changed & NETIF_F_HW_TC) {
> drivers/net/ethernet/freescale/enetc/enetc_pf.c: err = enetc_set_psfp(ndev, !!(features & NETIF_F_HW_TC));
> drivers/net/ethernet/freescale/enetc/enetc_pf_common.c: ndev->features |= NETIF_F_HW_TC;
> drivers/net/ethernet/freescale/enetc/enetc_pf_common.c: ndev->hw_features |= NETIF_F_HW_TC;
> drivers/net/ethernet/intel/igb/igb_main.c: netdev->features |= NETIF_F_HW_TC;
> drivers/net/ethernet/intel/igc/igc_main.c: netdev->features |= NETIF_F_HW_TC;
> drivers/net/ethernet/microchip/lan966x/lan966x_main.c: NETIF_F_HW_TC;
> drivers/net/ethernet/microchip/lan966x/lan966x_main.c: dev->hw_features |= NETIF_F_HW_TC;
> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c: ndev->hw_features |= NETIF_F_HW_TC;
> drivers/net/ethernet/ti/am65-cpsw-nuss.c: NETIF_F_HW_TC;
> drivers/net/ethernet/ti/cpsw_new.c: NETIF_F_HW_VLAN_CTAG_RX | NETIF_F_HW_TC;
>
>
> Looks like some the these qdiscs will need to ignore the features too :(
Hmm, so TC_SETUP_QDISC_CBS is not influenced by NETIF_F_HW_TC? I guess
it's the same discussion as above... LMK if you have more details.
Thank you for the review, will incorporate the rest of the feedback
from the other replies as well!
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH net-next 02/11] net: hold netdev instance lock during ndo_setup_tc
2025-02-12 3:48 ` Stanislav Fomichev
@ 2025-02-12 3:57 ` Jakub Kicinski
0 siblings, 0 replies; 22+ messages in thread
From: Jakub Kicinski @ 2025-02-12 3:57 UTC (permalink / raw)
To: Stanislav Fomichev
Cc: Stanislav Fomichev, netdev, davem, edumazet, pabeni,
Saeed Mahameed
On Tue, 11 Feb 2025 19:48:50 -0800 Stanislav Fomichev wrote:
> > The netfilter / flow table offloads don't seem to test tc_can_offload(),
> > should we make that part of the check optional in dev_setup_tc() ?
> > Add a bool argument to ignore tc_can_offload() ?
>
> Let me dig into it... I was assuming that tc_can_offload() is basically
> a runtime way to signal to the core that even though the device has
> ndo_setup_tc defined, the feature can't be used.
>
> I don't understand why some places care only about ndo_setup_tc
> while other test for both ndo_setup_tc/tc_can_offload. Do you by
> chance have any context on this? Does tc_can_offload cover only
> a subset of (TC_SETUP_BLOCK) the offload types?
>
> The easiest way is probably just to keep calls to tc_can_offload outside,
> as is, but I was thinking that doing both ndo_setup_tc and tc_can_offload
> is a bit more safe.
Off the top of my head the feature flag was added when John pioneered
the opportunistic offload of TC classifiers. It seems to have also
propagated to switch-like Qdisc offloads. The answer is probably some
mix of historic precedent (non-switch qdisc offload like mqprio exited
way before), features which are unambiguously HW-facing, and plain
omissions.
Let's do whatever is easiest here, the series touches half of the
stack, we can't possibly clean up all encountered.. mysteries.
^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2025-02-12 3:57 UTC | newest]
Thread overview: 22+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-02-10 19:20 [PATCH net-next 00/11] net: Hold netdev instance lock during ndo operations Stanislav Fomichev
2025-02-10 19:20 ` [PATCH net-next 01/11] net: hold netdev instance lock during ndo_open/ndo_stop Stanislav Fomichev
2025-02-12 2:06 ` Jakub Kicinski
2025-02-10 19:20 ` [PATCH net-next 02/11] net: hold netdev instance lock during ndo_setup_tc Stanislav Fomichev
2025-02-12 2:20 ` Jakub Kicinski
2025-02-12 3:48 ` Stanislav Fomichev
2025-02-12 3:57 ` Jakub Kicinski
2025-02-10 19:20 ` [PATCH net-next 03/11] net: hold netdev instance lock during queue operations Stanislav Fomichev
2025-02-12 2:21 ` Jakub Kicinski
2025-02-10 19:20 ` [PATCH net-next 04/11] net: hold netdev instance lock during rtnetlink operations Stanislav Fomichev
2025-02-11 15:06 ` kernel test robot
2025-02-11 16:32 ` Stanislav Fomichev
2025-02-12 2:33 ` Jakub Kicinski
2025-02-10 19:20 ` [PATCH net-next 05/11] net: hold netdev instance lock during ioctl operations Stanislav Fomichev
2025-02-10 19:20 ` [PATCH net-next 06/11] net: hold netdev instance lock during sysfs operations Stanislav Fomichev
2025-02-10 19:20 ` [PATCH net-next 07/11] net: hold netdev instance lock during ndo_bpf Stanislav Fomichev
2025-02-10 19:20 ` [PATCH net-next 08/11] net: ethtool: try to protect all callback with netdev instance lock Stanislav Fomichev
2025-02-10 19:20 ` [PATCH net-next 09/11] net: dummy: add dummy shaper API Stanislav Fomichev
2025-02-11 10:41 ` kernel test robot
2025-02-11 15:29 ` kernel test robot
2025-02-10 19:20 ` [PATCH net-next 10/11] docs: net: document new locking reality Stanislav Fomichev
2025-02-10 19:20 ` [PATCH net-next 11/11] eth: bnxt: remove most dependencies on RTNL Stanislav Fomichev
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).