Skip to content

Commit 04efcee

Browse files
Stanislav Fomichevkuba-moo
Stanislav Fomichev
authored and
kuba-moo
committed
net: hold instance lock during NETDEV_CHANGE
Cosmin reports an issue with ipv6_add_dev being called from NETDEV_CHANGE notifier: [ 3455.008776] ? ipv6_add_dev+0x370/0x620 [ 3455.010097] ipv6_find_idev+0x96/0xe0 [ 3455.010725] addrconf_add_dev+0x1e/0xa0 [ 3455.011382] addrconf_init_auto_addrs+0xb0/0x720 [ 3455.013537] addrconf_notify+0x35f/0x8d0 [ 3455.014214] notifier_call_chain+0x38/0xf0 [ 3455.014903] netdev_state_change+0x65/0x90 [ 3455.015586] linkwatch_do_dev+0x5a/0x70 [ 3455.016238] rtnl_getlink+0x241/0x3e0 [ 3455.019046] rtnetlink_rcv_msg+0x177/0x5e0 Similarly, linkwatch might get to ipv6_add_dev without ops lock: [ 3456.656261] ? ipv6_add_dev+0x370/0x620 [ 3456.660039] ipv6_find_idev+0x96/0xe0 [ 3456.660445] addrconf_add_dev+0x1e/0xa0 [ 3456.660861] addrconf_init_auto_addrs+0xb0/0x720 [ 3456.661803] addrconf_notify+0x35f/0x8d0 [ 3456.662236] notifier_call_chain+0x38/0xf0 [ 3456.662676] netdev_state_change+0x65/0x90 [ 3456.663112] linkwatch_do_dev+0x5a/0x70 [ 3456.663529] __linkwatch_run_queue+0xeb/0x200 [ 3456.663990] linkwatch_event+0x21/0x30 [ 3456.664399] process_one_work+0x211/0x610 [ 3456.664828] worker_thread+0x1cc/0x380 [ 3456.665691] kthread+0xf4/0x210 Reclassify NETDEV_CHANGE as a notifier that consistently runs under the instance lock. Link: https://lore.kernel.org/netdev/aac073de8beec3e531c86c101b274d434741c28e.camel@nvidia.com/ Reported-by: Cosmin Ratiu <cratiu@nvidia.com> Tested-by: Cosmin Ratiu <cratiu@nvidia.com> Fixes: ad7c7b2 ("net: hold netdev instance lock during sysfs operations") Signed-off-by: Stanislav Fomichev <sdf@fomichev.me> Link: https://patch.msgid.link/20250404161122.3907628-1-sdf@fomichev.me Signed-off-by: Jakub Kicinski <kuba@kernel.org>
1 parent 54f5faf commit 04efcee

File tree

10 files changed

+63
-31
lines changed

10 files changed

+63
-31
lines changed

‎Documentation/networking/netdevices.rst

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -338,10 +338,11 @@ operations directly under the netdev instance lock.
338338
Devices drivers are encouraged to rely on the instance lock where possible.
339339

340340
For the (mostly software) drivers that need to interact with the core stack,
341-
there are two sets of interfaces: ``dev_xxx`` and ``netif_xxx`` (e.g.,
342-
``dev_set_mtu`` and ``netif_set_mtu``). The ``dev_xxx`` functions handle
343-
acquiring the instance lock themselves, while the ``netif_xxx`` functions
344-
assume that the driver has already acquired the instance lock.
341+
there are two sets of interfaces: ``dev_xxx``/``netdev_xxx`` and ``netif_xxx``
342+
(e.g., ``dev_set_mtu`` and ``netif_set_mtu``). The ``dev_xxx``/``netdev_xxx``
343+
functions handle acquiring the instance lock themselves, while the
344+
``netif_xxx`` functions assume that the driver has already acquired
345+
the instance lock.
345346

346347
Notifiers and netdev instance lock
347348
==================================
@@ -354,6 +355,7 @@ For devices with locked ops, currently only the following notifiers are
354355
running under the lock:
355356
* ``NETDEV_REGISTER``
356357
* ``NETDEV_UP``
358+
* ``NETDEV_CHANGE``
357359

358360
The following notifiers are running without the lock:
359361
* ``NETDEV_UNREGISTER``

‎include/linux/netdevice.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4429,6 +4429,7 @@ void linkwatch_fire_event(struct net_device *dev);
44294429
* pending work list (if queued).
44304430
*/
44314431
void linkwatch_sync_dev(struct net_device *dev);
4432+
void __linkwatch_sync_dev(struct net_device *dev);
44324433

44334434
/**
44344435
* netif_carrier_ok - test if carrier present
@@ -4974,6 +4975,7 @@ void dev_set_rx_mode(struct net_device *dev);
49744975
int dev_set_promiscuity(struct net_device *dev, int inc);
49754976
int netif_set_allmulti(struct net_device *dev, int inc, bool notify);
49764977
int dev_set_allmulti(struct net_device *dev, int inc);
4978+
void netif_state_change(struct net_device *dev);
49774979
void netdev_state_change(struct net_device *dev);
49784980
void __netdev_notify_peers(struct net_device *dev);
49794981
void netdev_notify_peers(struct net_device *dev);

‎include/linux/rtnetlink.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -240,6 +240,6 @@ rtnl_notify_needed(const struct net *net, u16 nlflags, u32 group)
240240
return (nlflags & NLM_F_ECHO) || rtnl_has_listeners(net, group);
241241
}
242242

243-
void netdev_set_operstate(struct net_device *dev, int newstate);
243+
void netif_set_operstate(struct net_device *dev, int newstate);
244244

245245
#endif /* __LINUX_RTNETLINK_H */

‎net/core/dev.c

Lines changed: 1 addition & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1518,15 +1518,7 @@ void netdev_features_change(struct net_device *dev)
15181518
}
15191519
EXPORT_SYMBOL(netdev_features_change);
15201520

1521-
/**
1522-
* netdev_state_change - device changes state
1523-
* @dev: device to cause notification
1524-
*
1525-
* Called to indicate a device has changed state. This function calls
1526-
* the notifier chains for netdev_chain and sends a NEWLINK message
1527-
* to the routing socket.
1528-
*/
1529-
void netdev_state_change(struct net_device *dev)
1521+
void netif_state_change(struct net_device *dev)
15301522
{
15311523
if (dev->flags & IFF_UP) {
15321524
struct netdev_notifier_change_info change_info = {
@@ -1538,7 +1530,6 @@ void netdev_state_change(struct net_device *dev)
15381530
rtmsg_ifinfo(RTM_NEWLINK, dev, 0, GFP_KERNEL, 0, NULL);
15391531
}
15401532
}
1541-
EXPORT_SYMBOL(netdev_state_change);
15421533

15431534
/**
15441535
* __netdev_notify_peers - notify network peers about existence of @dev,

‎net/core/dev_api.c

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -327,3 +327,19 @@ int dev_xdp_propagate(struct net_device *dev, struct netdev_bpf *bpf)
327327
return ret;
328328
}
329329
EXPORT_SYMBOL_GPL(dev_xdp_propagate);
330+
331+
/**
332+
* netdev_state_change() - device changes state
333+
* @dev: device to cause notification
334+
*
335+
* Called to indicate a device has changed state. This function calls
336+
* the notifier chains for netdev_chain and sends a NEWLINK message
337+
* to the routing socket.
338+
*/
339+
void netdev_state_change(struct net_device *dev)
340+
{
341+
netdev_lock_ops(dev);
342+
netif_state_change(dev);
343+
netdev_unlock_ops(dev);
344+
}
345+
EXPORT_SYMBOL(netdev_state_change);

‎net/core/link_watch.c

Lines changed: 23 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -183,7 +183,7 @@ static void linkwatch_do_dev(struct net_device *dev)
183183
else
184184
dev_deactivate(dev);
185185

186-
netdev_state_change(dev);
186+
netif_state_change(dev);
187187
}
188188
/* Note: our callers are responsible for calling netdev_tracker_free().
189189
* This is the reason we use __dev_put() instead of dev_put().
@@ -240,7 +240,9 @@ static void __linkwatch_run_queue(int urgent_only)
240240
*/
241241
netdev_tracker_free(dev, &dev->linkwatch_dev_tracker);
242242
spin_unlock_irq(&lweventlist_lock);
243+
netdev_lock_ops(dev);
243244
linkwatch_do_dev(dev);
245+
netdev_unlock_ops(dev);
244246
do_dev--;
245247
spin_lock_irq(&lweventlist_lock);
246248
}
@@ -253,25 +255,41 @@ static void __linkwatch_run_queue(int urgent_only)
253255
spin_unlock_irq(&lweventlist_lock);
254256
}
255257

256-
void linkwatch_sync_dev(struct net_device *dev)
258+
static bool linkwatch_clean_dev(struct net_device *dev)
257259
{
258260
unsigned long flags;
259-
int clean = 0;
261+
bool clean = false;
260262

261263
spin_lock_irqsave(&lweventlist_lock, flags);
262264
if (!list_empty(&dev->link_watch_list)) {
263265
list_del_init(&dev->link_watch_list);
264-
clean = 1;
266+
clean = true;
265267
/* We must release netdev tracker under
266268
* the spinlock protection.
267269
*/
268270
netdev_tracker_free(dev, &dev->linkwatch_dev_tracker);
269271
}
270272
spin_unlock_irqrestore(&lweventlist_lock, flags);
271-
if (clean)
273+
274+
return clean;
275+
}
276+
277+
void __linkwatch_sync_dev(struct net_device *dev)
278+
{
279+
netdev_ops_assert_locked(dev);
280+
281+
if (linkwatch_clean_dev(dev))
272282
linkwatch_do_dev(dev);
273283
}
274284

285+
void linkwatch_sync_dev(struct net_device *dev)
286+
{
287+
if (linkwatch_clean_dev(dev)) {
288+
netdev_lock_ops(dev);
289+
linkwatch_do_dev(dev);
290+
netdev_unlock_ops(dev);
291+
}
292+
}
275293

276294
/* Must be called with the rtnl semaphore held */
277295
void linkwatch_run_queue(void)

‎net/core/lock_debug.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,11 +20,11 @@ int netdev_debug_event(struct notifier_block *nb, unsigned long event,
2020
switch (cmd) {
2121
case NETDEV_REGISTER:
2222
case NETDEV_UP:
23+
case NETDEV_CHANGE:
2324
netdev_ops_assert_locked(dev);
2425
fallthrough;
2526
case NETDEV_DOWN:
2627
case NETDEV_REBOOT:
27-
case NETDEV_CHANGE:
2828
case NETDEV_UNREGISTER:
2929
case NETDEV_CHANGEMTU:
3030
case NETDEV_CHANGEADDR:

‎net/core/rtnetlink.c

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1043,7 +1043,7 @@ int rtnl_put_cacheinfo(struct sk_buff *skb, struct dst_entry *dst, u32 id,
10431043
}
10441044
EXPORT_SYMBOL_GPL(rtnl_put_cacheinfo);
10451045

1046-
void netdev_set_operstate(struct net_device *dev, int newstate)
1046+
void netif_set_operstate(struct net_device *dev, int newstate)
10471047
{
10481048
unsigned int old = READ_ONCE(dev->operstate);
10491049

@@ -1052,9 +1052,9 @@ void netdev_set_operstate(struct net_device *dev, int newstate)
10521052
return;
10531053
} while (!try_cmpxchg(&dev->operstate, &old, newstate));
10541054

1055-
netdev_state_change(dev);
1055+
netif_state_change(dev);
10561056
}
1057-
EXPORT_SYMBOL(netdev_set_operstate);
1057+
EXPORT_SYMBOL(netif_set_operstate);
10581058

10591059
static void set_operstate(struct net_device *dev, unsigned char transition)
10601060
{
@@ -1080,7 +1080,7 @@ static void set_operstate(struct net_device *dev, unsigned char transition)
10801080
break;
10811081
}
10821082

1083-
netdev_set_operstate(dev, operstate);
1083+
netif_set_operstate(dev, operstate);
10841084
}
10851085

10861086
static unsigned int rtnl_dev_get_flags(const struct net_device *dev)
@@ -3396,7 +3396,7 @@ static int do_setlink(const struct sk_buff *skb, struct net_device *dev,
33963396
errout:
33973397
if (status & DO_SETLINK_MODIFIED) {
33983398
if ((status & DO_SETLINK_NOTIFY) == DO_SETLINK_NOTIFY)
3399-
netdev_state_change(dev);
3399+
netif_state_change(dev);
34003400

34013401
if (err < 0)
34023402
net_warn_ratelimited("A link change request failed with some changes committed already. Interface %s may have been left with an inconsistent configuration, please check.\n",
@@ -3676,8 +3676,11 @@ struct net_device *rtnl_create_link(struct net *net, const char *ifname,
36763676
nla_len(tb[IFLA_BROADCAST]));
36773677
if (tb[IFLA_TXQLEN])
36783678
dev->tx_queue_len = nla_get_u32(tb[IFLA_TXQLEN]);
3679-
if (tb[IFLA_OPERSTATE])
3679+
if (tb[IFLA_OPERSTATE]) {
3680+
netdev_lock_ops(dev);
36803681
set_operstate(dev, nla_get_u8(tb[IFLA_OPERSTATE]));
3682+
netdev_unlock_ops(dev);
3683+
}
36813684
if (tb[IFLA_LINKMODE])
36823685
dev->link_mode = nla_get_u8(tb[IFLA_LINKMODE]);
36833686
if (tb[IFLA_GROUP])

‎net/ethtool/ioctl.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -60,7 +60,7 @@ static struct devlink *netdev_to_devlink_get(struct net_device *dev)
6060
u32 ethtool_op_get_link(struct net_device *dev)
6161
{
6262
/* Synchronize carrier state with link watch, see also rtnl_getlink() */
63-
linkwatch_sync_dev(dev);
63+
__linkwatch_sync_dev(dev);
6464

6565
return netif_carrier_ok(dev) ? 1 : 0;
6666
}

‎net/hsr/hsr_device.c

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -33,14 +33,14 @@ static void hsr_set_operstate(struct hsr_port *master, bool has_carrier)
3333
struct net_device *dev = master->dev;
3434

3535
if (!is_admin_up(dev)) {
36-
netdev_set_operstate(dev, IF_OPER_DOWN);
36+
netif_set_operstate(dev, IF_OPER_DOWN);
3737
return;
3838
}
3939

4040
if (has_carrier)
41-
netdev_set_operstate(dev, IF_OPER_UP);
41+
netif_set_operstate(dev, IF_OPER_UP);
4242
else
43-
netdev_set_operstate(dev, IF_OPER_LOWERLAYERDOWN);
43+
netif_set_operstate(dev, IF_OPER_LOWERLAYERDOWN);
4444
}
4545

4646
static bool hsr_check_carrier(struct hsr_port *master)

0 commit comments

Comments
 (0)