| Message ID | 20250327135659.2057487-7-sdf@fomichev.me (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: hold instance lock during NETDEV_UP/REGISTER/UNREGISTER | expand |
On Thu, 27 Mar 2025 06:56:54 -0700 Stanislav Fomichev wrote: > In order to exercise and verify notifiers' locking assumptions, > register dummy notifiers and assert that netdev is ops locked > for REGISTER/UNREGISTER/UP. > +static int nsim_net_event(struct notifier_block *this, unsigned long event, > + void *ptr) > +{ > + struct net_device *dev = netdev_notifier_info_to_dev(ptr); > + > + switch (event) { > + case NETDEV_REGISTER: > + case NETDEV_UNREGISTER: > + case NETDEV_UP: > + netdev_ops_assert_locked(dev); > + break; > + default: > + break; > + } > + > + return NOTIFY_DONE; > +} Can we register empty notifiers in nsim (just to make sure it has a callback) but do the validation in rtnl_net_debug.c I guess we'd need to transform rtnl_net_debug.c a little, make it less rtnl specific, compile under DEBUG_NET and ifdef out the small rtnl parts? That way we'll have coverage without netdevsim loaded which would be all HW testing.
On 03/27, Jakub Kicinski wrote: > On Thu, 27 Mar 2025 06:56:54 -0700 Stanislav Fomichev wrote: > > In order to exercise and verify notifiers' locking assumptions, > > register dummy notifiers and assert that netdev is ops locked > > for REGISTER/UNREGISTER/UP. > > > +static int nsim_net_event(struct notifier_block *this, unsigned long event, > > + void *ptr) > > +{ > > + struct net_device *dev = netdev_notifier_info_to_dev(ptr); > > + > > + switch (event) { > > + case NETDEV_REGISTER: > > + case NETDEV_UNREGISTER: > > + case NETDEV_UP: > > + netdev_ops_assert_locked(dev); > > + break; > > + default: > > + break; > > + } > > + > > + return NOTIFY_DONE; > > +} > > Can we register empty notifiers in nsim (just to make sure it has > a callback) but do the validation in rtnl_net_debug.c > I guess we'd need to transform rtnl_net_debug.c a little, > make it less rtnl specific, compile under DEBUG_NET and ifdef > out the small rtnl parts? s/rtnl_net_debug.c/notifiers_debug.c/ + DEBUG_NET? Or I can keep the name and only do the DEBUG_NET part. Not sure what needs to be ifdef-ed out, but will take a look (probably just enough to make it compile with !CONFIG_DEBUG_NET_SMALL_RTNL ?). That should work for the regular notifiers, but I think register_netdevice_notifier_dev_net needs a netdev?
On Thu, 27 Mar 2025 14:04:06 -0700 Stanislav Fomichev wrote: > > Can we register empty notifiers in nsim (just to make sure it has > > a callback) but do the validation in rtnl_net_debug.c > > I guess we'd need to transform rtnl_net_debug.c a little, > > make it less rtnl specific, compile under DEBUG_NET and ifdef > > out the small rtnl parts? > > s/rtnl_net_debug.c/notifiers_debug.c/ + DEBUG_NET? Or I can keep the > name and only do the DEBUG_NET part. I was thinking lock or locking as in net/core/lock_debug.c But yeah, it's locking in notifier locking, maybe net/core/notifier_lock_debug.c then? No strong feelings. > Not sure what needs to be ifdef-ed out, > but will take a look (probably just enough to make it compile with > !CONFIG_DEBUG_NET_SMALL_RTNL ?). You're right, looking at the code we need all of it. Somehow I thought its doing extra netns related stuff but it just register a notifier in each ns. I guess we may not need any ifdef at all. > That should work for the regular notifiers, > but I think register_netdevice_notifier_dev_net needs a netdev? Hm. Yes. Not sure if we need anything extra in the notifier for nsim or we just want to make make sure it registers one. If the latter I guess we could export rtnl_net_debug_event (modulo rename) and call it from netdevsim? I mean - we would probably have the same exact asserts in both?
From: Jakub Kicinski <kuba@kernel.org> Date: Thu, 27 Mar 2025 14:46:09 -0700 > On Thu, 27 Mar 2025 14:04:06 -0700 Stanislav Fomichev wrote: > > > Can we register empty notifiers in nsim (just to make sure it has > > > a callback) but do the validation in rtnl_net_debug.c > > > I guess we'd need to transform rtnl_net_debug.c a little, > > > make it less rtnl specific, compile under DEBUG_NET and ifdef > > > out the small rtnl parts? > > > > s/rtnl_net_debug.c/notifiers_debug.c/ + DEBUG_NET? Or I can keep the > > name and only do the DEBUG_NET part. > > I was thinking lock or locking as in net/core/lock_debug.c Maybe lock.c (or netdev_lock.c like netdev_lock.h) and move all locking stuff (netdev_lock_type[], netdev_lock_pos(), etc) there later + ifdef where necessary ? > But yeah, it's locking in notifier locking, maybe > net/core/notifier_lock_debug.c then? No strong feelings. > > > Not sure what needs to be ifdef-ed out, > > but will take a look (probably just enough to make it compile with > > !CONFIG_DEBUG_NET_SMALL_RTNL ?). > > You're right, looking at the code we need all of it. > Somehow I thought its doing extra netns related stuff but it just > register a notifier in each ns. > I guess we may not need any ifdef at all. > > > That should work for the regular notifiers, > > but I think register_netdevice_notifier_dev_net needs a netdev? > > Hm. Yes. Not sure if we need anything extra in the notifier for nsim > or we just want to make make sure it registers one. If the latter > I guess we could export rtnl_net_debug_event (modulo rename) and > call it from netdevsim? I mean - we would probably have the same > exact asserts in both? >
On 03/27, Jakub Kicinski wrote: > On Thu, 27 Mar 2025 14:04:06 -0700 Stanislav Fomichev wrote: > > > Can we register empty notifiers in nsim (just to make sure it has > > > a callback) but do the validation in rtnl_net_debug.c > > > I guess we'd need to transform rtnl_net_debug.c a little, > > > make it less rtnl specific, compile under DEBUG_NET and ifdef > > > out the small rtnl parts? > > > > s/rtnl_net_debug.c/notifiers_debug.c/ + DEBUG_NET? Or I can keep the > > name and only do the DEBUG_NET part. > > I was thinking lock or locking as in net/core/lock_debug.c > But yeah, it's locking in notifier locking, maybe > net/core/notifier_lock_debug.c then? No strong feelings. > > > Not sure what needs to be ifdef-ed out, > > but will take a look (probably just enough to make it compile with > > !CONFIG_DEBUG_NET_SMALL_RTNL ?). > > You're right, looking at the code we need all of it. > Somehow I thought its doing extra netns related stuff but it just > register a notifier in each ns. > I guess we may not need any ifdef at all. > > > That should work for the regular notifiers, > > but I think register_netdevice_notifier_dev_net needs a netdev? > > Hm. Yes. Not sure if we need anything extra in the notifier for nsim > or we just want to make make sure it registers one. If the latter > I guess we could export rtnl_net_debug_event (modulo rename) and > call it from netdevsim? I mean - we would probably have the same > exact asserts in both? SG, reusing the notifier handler makes sense, will do!
On 03/27, Kuniyuki Iwashima wrote: > From: Jakub Kicinski <kuba@kernel.org> > Date: Thu, 27 Mar 2025 14:46:09 -0700 > > On Thu, 27 Mar 2025 14:04:06 -0700 Stanislav Fomichev wrote: > > > > Can we register empty notifiers in nsim (just to make sure it has > > > > a callback) but do the validation in rtnl_net_debug.c > > > > I guess we'd need to transform rtnl_net_debug.c a little, > > > > make it less rtnl specific, compile under DEBUG_NET and ifdef > > > > out the small rtnl parts? > > > > > > s/rtnl_net_debug.c/notifiers_debug.c/ + DEBUG_NET? Or I can keep the > > > name and only do the DEBUG_NET part. > > > > I was thinking lock or locking as in net/core/lock_debug.c > > Maybe lock.c (or netdev_lock.c like netdev_lock.h) and move all > locking stuff (netdev_lock_type[], netdev_lock_pos(), etc) there > later + ifdef where necessary ? That might work as well, but will require move moving. Maybe I can start with s/rtnl_net_debug.c/lock.c/ and the we can move the other stuff separately?
diff --git a/drivers/net/netdevsim/netdev.c b/drivers/net/netdevsim/netdev.c index b67af4651185..fdf10dd3df0b 100644 --- a/drivers/net/netdevsim/netdev.c +++ b/drivers/net/netdevsim/netdev.c @@ -926,6 +926,24 @@ static void nsim_queue_uninit(struct netdevsim *ns) ns->rq = NULL; } +static int nsim_net_event(struct notifier_block *this, unsigned long event, + void *ptr) +{ + struct net_device *dev = netdev_notifier_info_to_dev(ptr); + + switch (event) { + case NETDEV_REGISTER: + case NETDEV_UNREGISTER: + case NETDEV_UP: + netdev_ops_assert_locked(dev); + break; + default: + break; + } + + return NOTIFY_DONE; +} + static int nsim_init_netdevsim(struct netdevsim *ns) { struct mock_phc *phc; @@ -939,6 +957,7 @@ static int nsim_init_netdevsim(struct netdevsim *ns) ns->netdev->netdev_ops = &nsim_netdev_ops; ns->netdev->stat_ops = &nsim_stat_ops; ns->netdev->queue_mgmt_ops = &nsim_queue_mgmt_ops; + netdev_lockdep_set_classes(ns->netdev); err = nsim_udp_tunnels_info_create(ns->nsim_dev, ns->netdev); if (err) @@ -959,7 +978,13 @@ static int nsim_init_netdevsim(struct netdevsim *ns) err = register_netdevice(ns->netdev); if (err) goto err_ipsec_teardown; + rtnl_unlock(); + + ns->nb.notifier_call = nsim_net_event; + if (register_netdevice_notifier_dev_net(ns->netdev, &ns->nb, &ns->nn)) + ns->nb.notifier_call = NULL; + return 0; err_ipsec_teardown: @@ -1043,6 +1068,10 @@ void nsim_destroy(struct netdevsim *ns) debugfs_remove(ns->qr_dfs); debugfs_remove(ns->pp_dfs); + if (ns->nb.notifier_call) + unregister_netdevice_notifier_dev_net(ns->netdev, &ns->nb, + &ns->nn); + rtnl_lock(); peer = rtnl_dereference(ns->peer); if (peer) @@ -1086,6 +1115,28 @@ static struct rtnl_link_ops nsim_link_ops __read_mostly = { .validate = nsim_validate, }; +static int nsim_device_event(struct notifier_block *this, unsigned long event, + void *ptr) +{ + struct net_device *dev = netdev_notifier_info_to_dev(ptr); + + switch (event) { + case NETDEV_REGISTER: + case NETDEV_UNREGISTER: + case NETDEV_UP: + netdev_ops_assert_locked(dev); + break; + default: + break; + } + + return NOTIFY_DONE; +} + +static struct notifier_block nsim_notifier = { + .notifier_call = nsim_device_event, +}; + static int __init nsim_module_init(void) { int err; @@ -1102,8 +1153,14 @@ static int __init nsim_module_init(void) if (err) goto err_bus_exit; + err = register_netdevice_notifier(&nsim_notifier); + if (err) + goto err_notifier_exit; + return 0; +err_notifier_exit: + rtnl_link_unregister(&nsim_link_ops); err_bus_exit: nsim_bus_exit(); err_dev_exit: @@ -1113,6 +1170,7 @@ static int __init nsim_module_init(void) static void __exit nsim_module_exit(void) { + unregister_netdevice_notifier(&nsim_notifier); rtnl_link_unregister(&nsim_link_ops); nsim_bus_exit(); nsim_dev_exit(); diff --git a/drivers/net/netdevsim/netdevsim.h b/drivers/net/netdevsim/netdevsim.h index 665020d18f29..d04401f0bdf7 100644 --- a/drivers/net/netdevsim/netdevsim.h +++ b/drivers/net/netdevsim/netdevsim.h @@ -144,6 +144,9 @@ struct netdevsim { struct nsim_ethtool ethtool; struct netdevsim __rcu *peer; + + struct notifier_block nb; + struct netdev_net_notifier nn; }; struct netdevsim *
In order to exercise and verify notifiers' locking assumptions, register dummy notifiers and assert that netdev is ops locked for REGISTER/UNREGISTER/UP. Signed-off-by: Stanislav Fomichev <sdf@fomichev.me> --- drivers/net/netdevsim/netdev.c | 58 +++++++++++++++++++++++++++++++ drivers/net/netdevsim/netdevsim.h | 3 ++ 2 files changed, 61 insertions(+)