diff mbox series

[v2,net-next,6/7] rtnetlink: Convert RTM_NEWLINK to per-netns RTNL.

Message ID 20241106022432.13065-7-kuniyu@amazon.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series rtnetlink: Convert rtnl_newlink() to per-netns RTNL. | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 3 this patch: 3
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers success CCed 6 of 6 maintainers
netdev/build_clang success Errors and warnings before: 3 this patch: 3
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 8 this patch: 8
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 53 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 6 this patch: 6
netdev/source_inline success Was 0 now: 0
netdev/contest fail net-next-2024-11-06--09-00 (tests: 783)

Commit Message

Kuniyuki Iwashima Nov. 6, 2024, 2:24 a.m. UTC 
Now, we are ready to convert rtnl_newlink() to per-netns RTNL;
rtnl_link_ops is protected by SRCU and netns is prefetched in
rtnl_newlink().

Let's register rtnl_newlink() with RTNL_FLAG_DOIT_PERNET and
push RTNL down as rtnl_nets_lock().

Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
Reviewed-by: Eric Dumazet <edumazet@google.com>
---
v2: Remove __rtnl_unlock() dance in rtnl_newlink().
---
 net/core/rtnetlink.c | 27 ++++++++++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)

Comments

Paolo Abeni Nov. 6, 2024, 9 a.m. UTC | #1 
On 11/6/24 03:24, Kuniyuki Iwashima wrote:
> @@ -7001,7 +7021,8 @@ static struct pernet_operations rtnetlink_net_ops = {
>  };
>  
>  static const struct rtnl_msg_handler rtnetlink_rtnl_msg_handlers[] __initconst = {
> -	{.msgtype = RTM_NEWLINK, .doit = rtnl_newlink},
> +	{.msgtype = RTM_NEWLINK, .doit = rtnl_newlink,
> +	 .flags = RTNL_FLAG_DOIT_PERNET},
>  	{.msgtype = RTM_DELLINK, .doit = rtnl_dellink},
>  	{.msgtype = RTM_GETLINK, .doit = rtnl_getlink,
>  	 .dumpit = rtnl_dump_ifinfo, .flags = RTNL_FLAG_DUMP_SPLIT_NLM_DONE},

It looks like this still causes look problems - srcu/rtnl acquired in
both orders:

https://netdev-3.bots.linux.dev/vmksft-net-dbg/results/847881/12-rtnetlink-sh/stderr

It looks like __rtnl_link_unregister() should release the rtnl lock
around synchronize_srcu(). I'm unsure if would cause other problems, too.

Please have a self-tests run with lockdep enabled before the next iteration:

https://github.com/linux-netdev/nipa/wiki/How-to-run-netdev-selftests-CI-style

The whole test suite could be quite cumbersome, but the rtnetlink.sh
test should give a good coverage.

Thanks.

Paolo

p.s. kudos to Jakub for the extra miles to create and maintain the CI
infra: it's really catching up non trivial things.
Nikolay Aleksandrov Nov. 6, 2024, 10:40 a.m. UTC | #2 
On Tue, Nov 05, 2024 at 06:24:31PM -0800, Kuniyuki Iwashima wrote:
> Now, we are ready to convert rtnl_newlink() to per-netns RTNL;
> rtnl_link_ops is protected by SRCU and netns is prefetched in
> rtnl_newlink().
> 
> Let's register rtnl_newlink() with RTNL_FLAG_DOIT_PERNET and
> push RTNL down as rtnl_nets_lock().
> 
> Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
> Reviewed-by: Eric Dumazet <edumazet@google.com>
> ---
> v2: Remove __rtnl_unlock() dance in rtnl_newlink().
> ---
>  net/core/rtnetlink.c | 27 ++++++++++++++++++++++++---
>  1 file changed, 24 insertions(+), 3 deletions(-)
> 
 
Reviewed-by: Nikolay Aleksandrov <razor@blackwall.org>
Kuniyuki Iwashima Nov. 6, 2024, 4:32 p.m. UTC | #3 
From: Paolo Abeni <pabeni@redhat.com>
Date: Wed, 6 Nov 2024 10:00:26 +0100
> On 11/6/24 03:24, Kuniyuki Iwashima wrote:
> > @@ -7001,7 +7021,8 @@ static struct pernet_operations rtnetlink_net_ops = {
> >  };
> >  
> >  static const struct rtnl_msg_handler rtnetlink_rtnl_msg_handlers[] __initconst = {
> > -	{.msgtype = RTM_NEWLINK, .doit = rtnl_newlink},
> > +	{.msgtype = RTM_NEWLINK, .doit = rtnl_newlink,
> > +	 .flags = RTNL_FLAG_DOIT_PERNET},
> >  	{.msgtype = RTM_DELLINK, .doit = rtnl_dellink},
> >  	{.msgtype = RTM_GETLINK, .doit = rtnl_getlink,
> >  	 .dumpit = rtnl_dump_ifinfo, .flags = RTNL_FLAG_DUMP_SPLIT_NLM_DONE},
> 
> It looks like this still causes look problems - srcu/rtnl acquired in
> both orders:
> 
> https://netdev-3.bots.linux.dev/vmksft-net-dbg/results/847881/12-rtnetlink-sh/stderr
> 
> It looks like __rtnl_link_unregister() should release the rtnl lock
> around synchronize_srcu(). I'm unsure if would cause other problems, too.

It seems I need to unexport __rtnl_link_unregister(), add mutex for
rtnl_link_ops, and move ops deletion before down_write(&pernet_ops_rwsem)
in rtnl_link_unregister().

It would be better than releasing RTNL after rtnl_lock_unregistering_all().

> 
> Please have a self-tests run with lockdep enabled before the next iteration:
> 
> https://github.com/linux-netdev/nipa/wiki/How-to-run-netdev-selftests-CI-style
> 
> The whole test suite could be quite cumbersome, but the rtnetlink.sh
> test should give a good coverage.

Sure, sorry for the churn for other patches..

I noticed rmmod wasn't tested on my QEMU because I enabled drivers
as =y so that I need not copy .ko.  I'll run test with netdevsim
as =m at least.


> 
> Thanks.
> 
> Paolo
> 
> p.s. kudos to Jakub for the extra miles to create and maintain the CI
> infra: it's really catching up non trivial things.

+100

Thank you!
diff mbox series

Patch

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index d5557a621099..1b58a7c4c912 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -319,6 +319,26 @@  static void rtnl_nets_add(struct rtnl_nets *rtnl_nets, struct net *net)
 	rtnl_nets->len++;
 }
 
+static void rtnl_nets_lock(struct rtnl_nets *rtnl_nets)
+{
+	int i;
+
+	rtnl_lock();
+
+	for (i = 0; i < rtnl_nets->len; i++)
+		__rtnl_net_lock(rtnl_nets->net[i]);
+}
+
+static void rtnl_nets_unlock(struct rtnl_nets *rtnl_nets)
+{
+	int i;
+
+	for (i = 0; i < rtnl_nets->len; i++)
+		__rtnl_net_unlock(rtnl_nets->net[i]);
+
+	rtnl_unlock();
+}
+
 static struct rtnl_link __rcu *__rcu *rtnl_msg_handlers[RTNL_FAMILY_MAX + 1];
 
 static inline int rtm_msgindex(int msgtype)
@@ -3932,9 +3952,7 @@  static int rtnl_newlink(struct sk_buff *skb, struct nlmsghdr *nlh,
 		ops = rtnl_link_ops_get(kind, &ops_srcu_index);
 #ifdef CONFIG_MODULES
 		if (!ops) {
-			__rtnl_unlock();
 			request_module("rtnl-link-%s", kind);
-			rtnl_lock();
 			ops = rtnl_link_ops_get(kind, &ops_srcu_index);
 		}
 #endif
@@ -3997,7 +4015,9 @@  static int rtnl_newlink(struct sk_buff *skb, struct nlmsghdr *nlh,
 		}
 	}
 
+	rtnl_nets_lock(&rtnl_nets);
 	ret = __rtnl_newlink(skb, nlh, ops, tgt_net, link_net, tbs, data, extack);
+	rtnl_nets_unlock(&rtnl_nets);
 
 put_net:
 	rtnl_nets_destroy(&rtnl_nets);
@@ -7001,7 +7021,8 @@  static struct pernet_operations rtnetlink_net_ops = {
 };
 
 static const struct rtnl_msg_handler rtnetlink_rtnl_msg_handlers[] __initconst = {
-	{.msgtype = RTM_NEWLINK, .doit = rtnl_newlink},
+	{.msgtype = RTM_NEWLINK, .doit = rtnl_newlink,
+	 .flags = RTNL_FLAG_DOIT_PERNET},
 	{.msgtype = RTM_DELLINK, .doit = rtnl_dellink},
 	{.msgtype = RTM_GETLINK, .doit = rtnl_getlink,
 	 .dumpit = rtnl_dump_ifinfo, .flags = RTNL_FLAG_DUMP_SPLIT_NLM_DONE},