| Message ID | 20230206094151.2557264-1-jiri@resnulli.us (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 565b4824c39fa335cba2028a09d7beb7112f3c9a |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] devlink: change port event netdev notifier from per-net to global | expand |
On 2/6/2023 1:41 AM, Jiri Pirko wrote: > From: Jiri Pirko <jiri@nvidia.com> > > Currently only the network namespace of devlink instance is monitored > for port events. If netdev is moved to a different namespace and then > unregistered, NETDEV_PRE_UNINIT is missed which leads to trigger > following WARN_ON in devl_port_unregister(). > WARN_ON(devlink_port->type != DEVLINK_PORT_TYPE_NOTSET); > > Fix this by changing the netdev notifier from per-net to global so no > event is missed. > > Fixes: 02a68a47eade ("net: devlink: track netdev with devlink_port assigned") > Signed-off-by: Jiri Pirko <jiri@nvidia.com> Seems reasonable. Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>
Hello: This patch was applied to netdev/net.git (master) by Paolo Abeni <pabeni@redhat.com>: On Mon, 6 Feb 2023 10:41:51 +0100 you wrote: > From: Jiri Pirko <jiri@nvidia.com> > > Currently only the network namespace of devlink instance is monitored > for port events. If netdev is moved to a different namespace and then > unregistered, NETDEV_PRE_UNINIT is missed which leads to trigger > following WARN_ON in devl_port_unregister(). > WARN_ON(devlink_port->type != DEVLINK_PORT_TYPE_NOTSET); > > [...] Here is the summary with links: - [net] devlink: change port event netdev notifier from per-net to global https://git.kernel.org/netdev/net/c/565b4824c39f You are awesome, thank you!
On Mon, Feb 06, 2023 at 10:41:51AM +0100, Jiri Pirko wrote: > From: Jiri Pirko <jiri@nvidia.com> > > Currently only the network namespace of devlink instance is monitored > for port events. If netdev is moved to a different namespace and then > unregistered, NETDEV_PRE_UNINIT is missed which leads to trigger > following WARN_ON in devl_port_unregister(). > WARN_ON(devlink_port->type != DEVLINK_PORT_TYPE_NOTSET); > > Fix this by changing the netdev notifier from per-net to global so no > event is missed. > > Fixes: 02a68a47eade ("net: devlink: track netdev with devlink_port assigned") > Signed-off-by: Jiri Pirko <jiri@nvidia.com> > --- > net/core/devlink.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/net/core/devlink.c b/net/core/devlink.c > index 032d6d0a5ce6..909a10e4b0dd 100644 > --- a/net/core/devlink.c > +++ b/net/core/devlink.c > @@ -9979,7 +9979,7 @@ struct devlink *devlink_alloc_ns(const struct devlink_ops *ops, > goto err_xa_alloc; > > devlink->netdevice_nb.notifier_call = devlink_netdevice_event; > - ret = register_netdevice_notifier_net(net, &devlink->netdevice_nb); > + ret = register_netdevice_notifier(&devlink->netdevice_nb); > if (ret) > goto err_register_netdevice_notifier; > > @@ -10171,8 +10171,7 @@ void devlink_free(struct devlink *devlink) > xa_destroy(&devlink->snapshot_ids); > xa_destroy(&devlink->ports); > > - WARN_ON_ONCE(unregister_netdevice_notifier_net(devlink_net(devlink), > - &devlink->netdevice_nb)); > + WARN_ON_ONCE(unregister_netdevice_notifier(&devlink->netdevice_nb)); > > xa_erase(&devlinks, devlink->index); > > @@ -10503,6 +10502,8 @@ static int devlink_netdevice_event(struct notifier_block *nb, > break; > case NETDEV_REGISTER: > case NETDEV_CHANGENAME: > + if (devlink_net(devlink) != dev_net(netdev)) > + return NOTIFY_OK; > /* Set the netdev on top of previously set type. Note this > * event happens also during net namespace change so here > * we take into account netdev pointer appearing in this > @@ -10512,6 +10513,8 @@ static int devlink_netdevice_event(struct notifier_block *nb, > netdev); > break; > case NETDEV_UNREGISTER: > + if (devlink_net(devlink) != dev_net(netdev)) > + return NOTIFY_OK; > /* Clear netdev pointer, but not the type. This event happens > * also during net namespace change so we need to clear > * pointer to netdev that is going to another net namespace. Since the notifier block is no longer registered per-netns it shouldn't be moved to a different netns on reload. I'm testing the following diff [1] against net-next (although it should be eventually submitted to net). [1] diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h index d9cdbc047b49..efbee940bb03 100644 --- a/include/linux/netdevice.h +++ b/include/linux/netdevice.h @@ -2858,8 +2858,6 @@ int unregister_netdevice_notifier(struct notifier_block *nb); int register_netdevice_notifier_net(struct net *net, struct notifier_block *nb); int unregister_netdevice_notifier_net(struct net *net, struct notifier_block *nb); -void move_netdevice_notifier_net(struct net *src_net, struct net *dst_net, - struct notifier_block *nb); int register_netdevice_notifier_dev_net(struct net_device *dev, struct notifier_block *nb, struct netdev_net_notifier *nn); diff --git a/net/core/dev.c b/net/core/dev.c index 7307a0c15c9f..709b1a02820b 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -1870,14 +1870,6 @@ static void __move_netdevice_notifier_net(struct net *src_net, __register_netdevice_notifier_net(dst_net, nb, true); } -void move_netdevice_notifier_net(struct net *src_net, struct net *dst_net, - struct notifier_block *nb) -{ - rtnl_lock(); - __move_netdevice_notifier_net(src_net, dst_net, nb); - rtnl_unlock(); -} - int register_netdevice_notifier_dev_net(struct net_device *dev, struct notifier_block *nb, struct netdev_net_notifier *nn) diff --git a/net/devlink/dev.c b/net/devlink/dev.c index ab4e0f3c4e3d..c879c3c78e18 100644 --- a/net/devlink/dev.c +++ b/net/devlink/dev.c @@ -343,8 +343,6 @@ static void devlink_reload_netns_change(struct devlink *devlink, * reload process so the notifications are generated separatelly. */ devlink_notify_unregister(devlink); - move_netdevice_notifier_net(curr_net, dest_net, - &devlink->netdevice_nb); write_pnet(&devlink->_net, dest_net); devlink_notify_register(devlink); }
Tue, Feb 14, 2023 at 09:41:36AM CET, idosch@idosch.org wrote: >On Mon, Feb 06, 2023 at 10:41:51AM +0100, Jiri Pirko wrote: >> From: Jiri Pirko <jiri@nvidia.com> >> >> Currently only the network namespace of devlink instance is monitored >> for port events. If netdev is moved to a different namespace and then >> unregistered, NETDEV_PRE_UNINIT is missed which leads to trigger >> following WARN_ON in devl_port_unregister(). >> WARN_ON(devlink_port->type != DEVLINK_PORT_TYPE_NOTSET); >> >> Fix this by changing the netdev notifier from per-net to global so no >> event is missed. >> >> Fixes: 02a68a47eade ("net: devlink: track netdev with devlink_port assigned") >> Signed-off-by: Jiri Pirko <jiri@nvidia.com> >> --- >> net/core/devlink.c | 9 ++++++--- >> 1 file changed, 6 insertions(+), 3 deletions(-) >> >> diff --git a/net/core/devlink.c b/net/core/devlink.c >> index 032d6d0a5ce6..909a10e4b0dd 100644 >> --- a/net/core/devlink.c >> +++ b/net/core/devlink.c >> @@ -9979,7 +9979,7 @@ struct devlink *devlink_alloc_ns(const struct devlink_ops *ops, >> goto err_xa_alloc; >> >> devlink->netdevice_nb.notifier_call = devlink_netdevice_event; >> - ret = register_netdevice_notifier_net(net, &devlink->netdevice_nb); >> + ret = register_netdevice_notifier(&devlink->netdevice_nb); >> if (ret) >> goto err_register_netdevice_notifier; >> >> @@ -10171,8 +10171,7 @@ void devlink_free(struct devlink *devlink) >> xa_destroy(&devlink->snapshot_ids); >> xa_destroy(&devlink->ports); >> >> - WARN_ON_ONCE(unregister_netdevice_notifier_net(devlink_net(devlink), >> - &devlink->netdevice_nb)); >> + WARN_ON_ONCE(unregister_netdevice_notifier(&devlink->netdevice_nb)); >> >> xa_erase(&devlinks, devlink->index); >> >> @@ -10503,6 +10502,8 @@ static int devlink_netdevice_event(struct notifier_block *nb, >> break; >> case NETDEV_REGISTER: >> case NETDEV_CHANGENAME: >> + if (devlink_net(devlink) != dev_net(netdev)) >> + return NOTIFY_OK; >> /* Set the netdev on top of previously set type. Note this >> * event happens also during net namespace change so here >> * we take into account netdev pointer appearing in this >> @@ -10512,6 +10513,8 @@ static int devlink_netdevice_event(struct notifier_block *nb, >> netdev); >> break; >> case NETDEV_UNREGISTER: >> + if (devlink_net(devlink) != dev_net(netdev)) >> + return NOTIFY_OK; >> /* Clear netdev pointer, but not the type. This event happens >> * also during net namespace change so we need to clear >> * pointer to netdev that is going to another net namespace. > >Since the notifier block is no longer registered per-netns it shouldn't >be moved to a different netns on reload. I'm testing the following diff >[1] against net-next (although it should be eventually submitted to >net). Oh yeah. That is needed. Thanks! > >[1] >diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h >index d9cdbc047b49..efbee940bb03 100644 >--- a/include/linux/netdevice.h >+++ b/include/linux/netdevice.h >@@ -2858,8 +2858,6 @@ int unregister_netdevice_notifier(struct notifier_block *nb); > int register_netdevice_notifier_net(struct net *net, struct notifier_block *nb); > int unregister_netdevice_notifier_net(struct net *net, > struct notifier_block *nb); >-void move_netdevice_notifier_net(struct net *src_net, struct net *dst_net, >- struct notifier_block *nb); > int register_netdevice_notifier_dev_net(struct net_device *dev, > struct notifier_block *nb, > struct netdev_net_notifier *nn); >diff --git a/net/core/dev.c b/net/core/dev.c >index 7307a0c15c9f..709b1a02820b 100644 >--- a/net/core/dev.c >+++ b/net/core/dev.c >@@ -1870,14 +1870,6 @@ static void __move_netdevice_notifier_net(struct net *src_net, > __register_netdevice_notifier_net(dst_net, nb, true); > } > >-void move_netdevice_notifier_net(struct net *src_net, struct net *dst_net, >- struct notifier_block *nb) >-{ >- rtnl_lock(); >- __move_netdevice_notifier_net(src_net, dst_net, nb); >- rtnl_unlock(); >-} >- > int register_netdevice_notifier_dev_net(struct net_device *dev, > struct notifier_block *nb, > struct netdev_net_notifier *nn) >diff --git a/net/devlink/dev.c b/net/devlink/dev.c >index ab4e0f3c4e3d..c879c3c78e18 100644 >--- a/net/devlink/dev.c >+++ b/net/devlink/dev.c >@@ -343,8 +343,6 @@ static void devlink_reload_netns_change(struct devlink *devlink, > * reload process so the notifications are generated separatelly. > */ > devlink_notify_unregister(devlink); >- move_netdevice_notifier_net(curr_net, dest_net, >- &devlink->netdevice_nb); > write_pnet(&devlink->_net, dest_net); > devlink_notify_register(devlink); > }
diff --git a/net/core/devlink.c b/net/core/devlink.c index 032d6d0a5ce6..909a10e4b0dd 100644 --- a/net/core/devlink.c +++ b/net/core/devlink.c @@ -9979,7 +9979,7 @@ struct devlink *devlink_alloc_ns(const struct devlink_ops *ops, goto err_xa_alloc; devlink->netdevice_nb.notifier_call = devlink_netdevice_event; - ret = register_netdevice_notifier_net(net, &devlink->netdevice_nb); + ret = register_netdevice_notifier(&devlink->netdevice_nb); if (ret) goto err_register_netdevice_notifier; @@ -10171,8 +10171,7 @@ void devlink_free(struct devlink *devlink) xa_destroy(&devlink->snapshot_ids); xa_destroy(&devlink->ports); - WARN_ON_ONCE(unregister_netdevice_notifier_net(devlink_net(devlink), - &devlink->netdevice_nb)); + WARN_ON_ONCE(unregister_netdevice_notifier(&devlink->netdevice_nb)); xa_erase(&devlinks, devlink->index); @@ -10503,6 +10502,8 @@ static int devlink_netdevice_event(struct notifier_block *nb, break; case NETDEV_REGISTER: case NETDEV_CHANGENAME: + if (devlink_net(devlink) != dev_net(netdev)) + return NOTIFY_OK; /* Set the netdev on top of previously set type. Note this * event happens also during net namespace change so here * we take into account netdev pointer appearing in this @@ -10512,6 +10513,8 @@ static int devlink_netdevice_event(struct notifier_block *nb, netdev); break; case NETDEV_UNREGISTER: + if (devlink_net(devlink) != dev_net(netdev)) + return NOTIFY_OK; /* Clear netdev pointer, but not the type. This event happens * also during net namespace change so we need to clear * pointer to netdev that is going to another net namespace.