From: Stanislav Fomichev <stfomichev@gmail.com>
To: Cosmin Ratiu <cratiu@nvidia.com>
Cc: "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
"sdf@fomichev.me" <sdf@fomichev.me>,
"edumazet@google.com" <edumazet@google.com>,
"davem@davemloft.net" <davem@davemloft.net>,
"kuba@kernel.org" <kuba@kernel.org>,
"pabeni@redhat.com" <pabeni@redhat.com>
Subject: Re: [PATCH net-next 2/9] net: hold instance lock during NETDEV_REGISTER/UP/UNREGISTER
Date: Wed, 26 Mar 2025 08:23:59 -0700 [thread overview]
Message-ID: <Z-QcD5BXD5mY3BA_@mini-arch> (raw)
In-Reply-To: <86b753c439badc25968a01d03ed59b734886ad9b.camel@nvidia.com>
On 03/26, Cosmin Ratiu wrote:
> On Tue, 2025-03-25 at 14:30 -0700, Stanislav Fomichev wrote:
> > @@ -2072,8 +2087,8 @@ static void
> > __move_netdevice_notifier_net(struct net *src_net,
> > struct net *dst_net,
> > struct notifier_block *nb)
> > {
> > - __unregister_netdevice_notifier_net(src_net, nb);
> > - __register_netdevice_notifier_net(dst_net, nb, true);
> > + __unregister_netdevice_notifier_net(src_net, nb, false);
> > + __register_netdevice_notifier_net(dst_net, nb, true, false);
> > }
>
> I tested with your (and the rest of Jakub's) patches.
> The problem with this approach is that when a netdev's net is changed,
> its lock will be acquired, but the notifiers for ALL netdevs in the old
> and the new namespace will be called, which will result in correct
> behavior for that device and lockdep_assert_held failure for all
> others.
Perfect, thanks for testing! At least we don't deadlock anymore, that's
progress :-) So looks like we need to do something like the following
below, maybe you can give it a run on your side? Since we don't
have any locking hierarchy (yet), we should be able to lock all
other netdevs besides the one that's already locked at netlink level.
diff --git a/net/core/dev.c b/net/core/dev.c
index afee19245401..125af0fc25d3 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1895,32 +1895,32 @@ static int call_netdevice_register_notifiers(struct notifier_block *nb,
static void call_netdevice_unregister_notifiers(struct notifier_block *nb,
struct net_device *dev,
- bool lock)
+ struct net_device *locked)
{
if (dev->flags & IFF_UP) {
call_netdevice_notifier(nb, NETDEV_GOING_DOWN,
dev);
call_netdevice_notifier(nb, NETDEV_DOWN, dev);
}
- if (lock)
+ if (dev != locked)
netdev_lock_ops(dev);
call_netdevice_notifier(nb, NETDEV_UNREGISTER, dev);
- if (lock)
+ if (dev != locked)
netdev_unlock_ops(dev);
}
static int call_netdevice_register_net_notifiers(struct notifier_block *nb,
struct net *net,
- bool lock)
+ struct net_device *locked)
{
struct net_device *dev;
int err;
for_each_netdev(net, dev) {
- if (lock)
+ if (locked != dev)
netdev_lock_ops(dev);
err = call_netdevice_register_notifiers(nb, dev);
- if (lock)
+ if (locked != dev)
netdev_unlock_ops(dev);
if (err)
goto rollback;
@@ -1929,18 +1929,18 @@ static int call_netdevice_register_net_notifiers(struct notifier_block *nb,
rollback:
for_each_netdev_continue_reverse(net, dev)
- call_netdevice_unregister_notifiers(nb, dev, lock);
+ call_netdevice_unregister_notifiers(nb, dev, locked);
return err;
}
static void call_netdevice_unregister_net_notifiers(struct notifier_block *nb,
struct net *net,
- bool lock)
+ struct net_device *locked)
{
struct net_device *dev;
for_each_netdev(net, dev)
- call_netdevice_unregister_notifiers(nb, dev, lock);
+ call_netdevice_unregister_notifiers(nb, dev, locked);
}
static int dev_boot_phase = 1;
@@ -1977,7 +1977,7 @@ int register_netdevice_notifier(struct notifier_block *nb)
goto unlock;
for_each_net(net) {
__rtnl_net_lock(net);
- err = call_netdevice_register_net_notifiers(nb, net, true);
+ err = call_netdevice_register_net_notifiers(nb, net, NULL);
__rtnl_net_unlock(net);
if (err)
goto rollback;
@@ -1991,7 +1991,7 @@ int register_netdevice_notifier(struct notifier_block *nb)
rollback:
for_each_net_continue_reverse(net) {
__rtnl_net_lock(net);
- call_netdevice_unregister_net_notifiers(nb, net, true);
+ call_netdevice_unregister_net_notifiers(nb, net, NULL);
__rtnl_net_unlock(net);
}
@@ -2028,7 +2028,7 @@ int unregister_netdevice_notifier(struct notifier_block *nb)
for_each_net(net) {
__rtnl_net_lock(net);
- call_netdevice_unregister_net_notifiers(nb, net, true);
+ call_netdevice_unregister_net_notifiers(nb, net, NULL);
__rtnl_net_unlock(net);
}
@@ -2042,7 +2042,7 @@ EXPORT_SYMBOL(unregister_netdevice_notifier);
static int __register_netdevice_notifier_net(struct net *net,
struct notifier_block *nb,
bool ignore_call_fail,
- bool lock)
+ struct net_device *locked)
{
int err;
@@ -2052,7 +2052,7 @@ static int __register_netdevice_notifier_net(struct net *net,
if (dev_boot_phase)
return 0;
- err = call_netdevice_register_net_notifiers(nb, net, lock);
+ err = call_netdevice_register_net_notifiers(nb, net, locked);
if (err && !ignore_call_fail)
goto chain_unregister;
@@ -2065,7 +2065,7 @@ static int __register_netdevice_notifier_net(struct net *net,
static int __unregister_netdevice_notifier_net(struct net *net,
struct notifier_block *nb,
- bool lock)
+ struct net_device *locked)
{
int err;
@@ -2073,7 +2073,7 @@ static int __unregister_netdevice_notifier_net(struct net *net,
if (err)
return err;
- call_netdevice_unregister_net_notifiers(nb, net, lock);
+ call_netdevice_unregister_net_notifiers(nb, net, locked);
return 0;
}
@@ -2097,7 +2097,7 @@ int register_netdevice_notifier_net(struct net *net, struct notifier_block *nb)
int err;
rtnl_net_lock(net);
- err = __register_netdevice_notifier_net(net, nb, false, true);
+ err = __register_netdevice_notifier_net(net, nb, false, NULL);
rtnl_net_unlock(net);
return err;
@@ -2126,7 +2126,7 @@ int unregister_netdevice_notifier_net(struct net *net,
int err;
rtnl_net_lock(net);
- err = __unregister_netdevice_notifier_net(net, nb, true);
+ err = __unregister_netdevice_notifier_net(net, nb, NULL);
rtnl_net_unlock(net);
return err;
@@ -2135,10 +2135,11 @@ EXPORT_SYMBOL(unregister_netdevice_notifier_net);
static void __move_netdevice_notifier_net(struct net *src_net,
struct net *dst_net,
- struct notifier_block *nb)
+ struct notifier_block *nb,
+ struct net_device *locked)
{
- __unregister_netdevice_notifier_net(src_net, nb, false);
- __register_netdevice_notifier_net(dst_net, nb, true, false);
+ __unregister_netdevice_notifier_net(src_net, nb, locked);
+ __register_netdevice_notifier_net(dst_net, nb, true, locked);
}
static void rtnl_net_dev_lock(struct net_device *dev)
@@ -2184,7 +2185,7 @@ int register_netdevice_notifier_dev_net(struct net_device *dev,
int err;
rtnl_net_dev_lock(dev);
- err = __register_netdevice_notifier_net(dev_net(dev), nb, false, true);
+ err = __register_netdevice_notifier_net(dev_net(dev), nb, false, NULL);
if (!err) {
nn->nb = nb;
list_add(&nn->list, &dev->net_notifier_list);
@@ -2203,7 +2204,7 @@ int unregister_netdevice_notifier_dev_net(struct net_device *dev,
rtnl_net_dev_lock(dev);
list_del(&nn->list);
- err = __unregister_netdevice_notifier_net(dev_net(dev), nb, true);
+ err = __unregister_netdevice_notifier_net(dev_net(dev), nb, NULL);
rtnl_net_dev_unlock(dev);
return err;
@@ -2216,7 +2217,7 @@ static void move_netdevice_notifiers_dev_net(struct net_device *dev,
struct netdev_net_notifier *nn;
list_for_each_entry(nn, &dev->net_notifier_list, list)
- __move_netdevice_notifier_net(dev_net(dev), net, nn->nb);
+ __move_netdevice_notifier_net(dev_net(dev), net, nn->nb, dev);
}
/**
next prev parent reply other threads:[~2025-03-26 15:24 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-03-25 21:30 [PATCH net-next 0/9] net: hold instance lock during NETDEV_UP/REGISTER/UNREGISTER Stanislav Fomichev
2025-03-25 21:30 ` [PATCH net-next 1/9] net: switch to netif_disable_lro in inetdev_init Stanislav Fomichev
2025-03-25 21:30 ` [PATCH net-next 2/9] net: hold instance lock during NETDEV_REGISTER/UP/UNREGISTER Stanislav Fomichev
2025-03-26 15:03 ` Cosmin Ratiu
2025-03-26 15:23 ` Stanislav Fomichev [this message]
2025-03-26 15:37 ` Cosmin Ratiu
2025-03-26 17:49 ` Stanislav Fomichev
2025-03-26 20:37 ` Stanislav Fomichev
2025-03-26 20:57 ` Cosmin Ratiu
2025-03-26 21:18 ` Cosmin Ratiu
2025-03-26 22:02 ` Stanislav Fomichev
2025-03-26 15:24 ` Cosmin Ratiu
2025-03-26 17:43 ` Stanislav Fomichev
2025-03-25 21:30 ` [PATCH net-next 3/9] net: use netif_disable_lro in ipv6_add_dev Stanislav Fomichev
2025-03-26 7:33 ` kernel test robot
2025-03-25 21:30 ` [PATCH net-next 4/9] net: dummy: request ops lock Stanislav Fomichev
2025-03-25 21:30 ` [PATCH net-next 5/9] net: release instance lock during NETDEV_UNREGISTER for bond/team Stanislav Fomichev
2025-03-25 21:30 ` [PATCH net-next 6/9] docs: net: document netdev notifier expectations Stanislav Fomichev
2025-03-25 21:30 ` [PATCH net-next 7/9] net: designate XSK pool pointers in queues as "ops protected" Stanislav Fomichev
2025-03-25 21:30 ` [PATCH net-next 8/9] netdev: add "ops compat locking" helpers Stanislav Fomichev
2025-03-25 21:30 ` [PATCH net-next 9/9] netdev: don't hold rtnl_lock over nl queue info get when possible Stanislav Fomichev
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Z-QcD5BXD5mY3BA_@mini-arch \
--to=stfomichev@gmail.com \
--cc=cratiu@nvidia.com \
--cc=davem@davemloft.net \
--cc=edumazet@google.com \
--cc=kuba@kernel.org \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
--cc=sdf@fomichev.me \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).