[PATCH net-next 0/2] rtnetlink: move rtnl_lock handling out of af_netlink

[PATCH net-next 0/2] rtnetlink: move rtnl_lock handling out of af_netlink

[Jakub Kicinski]

With the changes done in commit 5b4b62a169e1 ("rtnetlink: make
the "split" NLM_DONE handling generic") we can also move the
rtnl locking out of af_netlink.

Jakub Kicinski (2):
  rtnetlink: move rtnl_lock handling out of af_netlink
  net: netlink: remove the cb_mutex "injection" from netlink core

 include/linux/netlink.h  |  1 -
 net/core/rtnetlink.c     |  9 +++++++--
 net/netlink/af_netlink.c | 20 +++-----------------
 3 files changed, 10 insertions(+), 20 deletions(-)

-- 
2.45.2

[PATCH net-next 1/2] rtnetlink: move rtnl_lock handling out of af_netlink

[Jakub Kicinski]

Now that we have an intermediate layer of code for handling
rtnl-level netlink dump quirks, we can move the rtnl_lock
taking there.

For dump handlers with RTNL_FLAG_DUMP_SPLIT_NLM_DONE we can
avoid taking rtnl_lock just to generate NLM_DONE, once again.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 net/core/rtnetlink.c     | 9 +++++++--
 net/netlink/af_netlink.c | 2 --
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 4668d6718040..eabfc8290f5e 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -6486,6 +6486,7 @@ static int rtnl_mdb_del(struct sk_buff *skb, struct nlmsghdr *nlh,
 
 static int rtnl_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
 {
+	const bool needs_lock = !(cb->flags & RTNL_FLAG_DUMP_UNLOCKED);
 	rtnl_dumpit_func dumpit = cb->data;
 	int err;
 
@@ -6495,7 +6496,11 @@ static int rtnl_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
 	if (!dumpit)
 		return 0;
 
+	if (needs_lock)
+		rtnl_lock();
 	err = dumpit(skb, cb);
+	if (needs_lock)
+		rtnl_unlock();
 
 	/* Old dump handlers used to send NLM_DONE as in a separate recvmsg().
 	 * Some applications which parse netlink manually depend on this.
@@ -6515,7 +6520,8 @@ static int rtnetlink_dump_start(struct sock *ssk, struct sk_buff *skb,
 				const struct nlmsghdr *nlh,
 				struct netlink_dump_control *control)
 {
-	if (control->flags & RTNL_FLAG_DUMP_SPLIT_NLM_DONE) {
+	if (control->flags & RTNL_FLAG_DUMP_SPLIT_NLM_DONE ||
+	    !(control->flags & RTNL_FLAG_DUMP_UNLOCKED)) {
 		WARN_ON(control->data);
 		control->data = control->dump;
 		control->dump = rtnl_dumpit;
@@ -6703,7 +6709,6 @@ static int __net_init rtnetlink_net_init(struct net *net)
 	struct netlink_kernel_cfg cfg = {
 		.groups		= RTNLGRP_MAX,
 		.input		= rtnetlink_rcv,
-		.cb_mutex	= &rtnl_mutex,
 		.flags		= NL_CFG_F_NONROOT_RECV,
 		.bind		= rtnetlink_bind,
 	};
diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index fa9c090cf629..8bbbe75e75db 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -2330,8 +2330,6 @@ static int netlink_dump(struct sock *sk, bool lock_taken)
 
 		cb->extack = &extack;
 
-		if (cb->flags & RTNL_FLAG_DUMP_UNLOCKED)
-			extra_mutex = NULL;
 		if (extra_mutex)
 			mutex_lock(extra_mutex);
 		nlk->dump_done_errno = cb->dump(skb, cb);
-- 
2.45.2

Re: [PATCH net-next 1/2] rtnetlink: move rtnl_lock handling out of af_netlink

[Kuniyuki Iwashima]

From: Jakub Kicinski <kuba@kernel.org>
Date: Thu,  6 Jun 2024 12:29:05 -0700
> Now that we have an intermediate layer of code for handling
> rtnl-level netlink dump quirks, we can move the rtnl_lock
> taking there.
> 
> For dump handlers with RTNL_FLAG_DUMP_SPLIT_NLM_DONE we can
> avoid taking rtnl_lock just to generate NLM_DONE, once again.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
>  net/core/rtnetlink.c     | 9 +++++++--
>  net/netlink/af_netlink.c | 2 --
>  2 files changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index 4668d6718040..eabfc8290f5e 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -6486,6 +6486,7 @@ static int rtnl_mdb_del(struct sk_buff *skb, struct nlmsghdr *nlh,
>  
>  static int rtnl_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
>  {
> +	const bool needs_lock = !(cb->flags & RTNL_FLAG_DUMP_UNLOCKED);
>  	rtnl_dumpit_func dumpit = cb->data;
>  	int err;
>  
> @@ -6495,7 +6496,11 @@ static int rtnl_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
>  	if (!dumpit)
>  		return 0;
>  
> +	if (needs_lock)
> +		rtnl_lock();
>  	err = dumpit(skb, cb);
> +	if (needs_lock)
> +		rtnl_unlock();

This calls netdev_run_todo() now, is this change intended ?

Other parts look good to me.


>  
>  	/* Old dump handlers used to send NLM_DONE as in a separate recvmsg().
>  	 * Some applications which parse netlink manually depend on this.
> @@ -6515,7 +6520,8 @@ static int rtnetlink_dump_start(struct sock *ssk, struct sk_buff *skb,
>  				const struct nlmsghdr *nlh,
>  				struct netlink_dump_control *control)
>  {
> -	if (control->flags & RTNL_FLAG_DUMP_SPLIT_NLM_DONE) {
> +	if (control->flags & RTNL_FLAG_DUMP_SPLIT_NLM_DONE ||
> +	    !(control->flags & RTNL_FLAG_DUMP_UNLOCKED)) {
>  		WARN_ON(control->data);
>  		control->data = control->dump;
>  		control->dump = rtnl_dumpit;
> @@ -6703,7 +6709,6 @@ static int __net_init rtnetlink_net_init(struct net *net)
>  	struct netlink_kernel_cfg cfg = {
>  		.groups		= RTNLGRP_MAX,
>  		.input		= rtnetlink_rcv,
> -		.cb_mutex	= &rtnl_mutex,
>  		.flags		= NL_CFG_F_NONROOT_RECV,
>  		.bind		= rtnetlink_bind,
>  	};
> diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
> index fa9c090cf629..8bbbe75e75db 100644
> --- a/net/netlink/af_netlink.c
> +++ b/net/netlink/af_netlink.c
> @@ -2330,8 +2330,6 @@ static int netlink_dump(struct sock *sk, bool lock_taken)
>  
>  		cb->extack = &extack;
>  
> -		if (cb->flags & RTNL_FLAG_DUMP_UNLOCKED)
> -			extra_mutex = NULL;
>  		if (extra_mutex)
>  			mutex_lock(extra_mutex);
>  		nlk->dump_done_errno = cb->dump(skb, cb);
> -- 
> 2.45.2

Re: [PATCH net-next 1/2] rtnetlink: move rtnl_lock handling out of af_netlink

[Jakub Kicinski]

On Thu, 6 Jun 2024 16:33:03 -0700 Kuniyuki Iwashima wrote:
> > +	if (needs_lock)
> > +		rtnl_lock();
> >  	err = dumpit(skb, cb);
> > +	if (needs_lock)
> > +		rtnl_unlock();  
> 
> This calls netdev_run_todo() now, is this change intended ?

Nice catch / careful thinking, indeed we're moving from pure unlock to
run_todo. I don't really recall if I thought of this when writing the
change (it was few days back). My guess is that the fact we weren't
calling full rtnl_unlock() was unintentional / out of laziness in the
first place. It didn't matter since dumps are unlikely to changes /
unregister / free things. But still, someone may get caught off guard
as some point that we're holding rtnl but won't go via the usual unlock
path.

Would you like me to add a note to the commit message?

Re: [PATCH net-next 1/2] rtnetlink: move rtnl_lock handling out of af_netlink

[Kuniyuki Iwashima]

From: Jakub Kicinski <kuba@kernel.org>
Date: Thu, 6 Jun 2024 17:04:53 -0700
> On Thu, 6 Jun 2024 16:33:03 -0700 Kuniyuki Iwashima wrote:
> > > +	if (needs_lock)
> > > +		rtnl_lock();
> > >  	err = dumpit(skb, cb);
> > > +	if (needs_lock)
> > > +		rtnl_unlock();  
> > 
> > This calls netdev_run_todo() now, is this change intended ?
> 
> Nice catch / careful thinking, indeed we're moving from pure unlock to
> run_todo. I don't really recall if I thought of this when writing the
> change (it was few days back). My guess is that the fact we weren't
> calling full rtnl_unlock() was unintentional / out of laziness in the
> first place. It didn't matter since dumps are unlikely to changes /
> unregister / free things.

This makes sense.  Probably due to cb_mutex interface constraint.


> But still, someone may get caught off guard
> as some point that we're holding rtnl but won't go via the usual unlock
> path.
> 
> Would you like me to add a note to the commit message?

That would be nice, but I'm fine with this version :)

Reviewed-by: Kuniyuki Iwashima <kuniyu@amazon.com>

Thanks!

Re: [PATCH net-next 1/2] rtnetlink: move rtnl_lock handling out of af_netlink

[Eric Dumazet]

On Thu, Jun 6, 2024 at 9:29 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> Now that we have an intermediate layer of code for handling
> rtnl-level netlink dump quirks, we can move the rtnl_lock
> taking there.
>
> For dump handlers with RTNL_FLAG_DUMP_SPLIT_NLM_DONE we can
> avoid taking rtnl_lock just to generate NLM_DONE, once again.
>
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Reviewed-by: Eric Dumazet <edumazet@google.com>

[PATCH net-next 2/2] net: netlink: remove the cb_mutex "injection" from netlink core

[Jakub Kicinski]

Back in 2007, in commit af65bdfce98d ("[NETLINK]: Switch cb_lock spinlock
to mutex and allow to override it") netlink core was extended to allow
subsystems to replace the dump mutex lock with its own lock.

The mechanism was used by rtnetlink to take rtnl_lock but it isn't
sufficiently flexible for other users. Over the 17 years since
it was added no other user appeared. Since rtnetlink needs conditional
locking now, and doesn't use it either, axe this feature complete.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: anjali.k.kulkarni@oracle.com
CC: Liam.Howlett@oracle.com
CC: jiri@resnulli.us
---
 include/linux/netlink.h  |  1 -
 net/netlink/af_netlink.c | 18 +++---------------
 2 files changed, 3 insertions(+), 16 deletions(-)

diff --git a/include/linux/netlink.h b/include/linux/netlink.h
index 5df7340d4dab..b332c2048c75 100644
--- a/include/linux/netlink.h
+++ b/include/linux/netlink.h
@@ -47,7 +47,6 @@ struct netlink_kernel_cfg {
 	unsigned int	groups;
 	unsigned int	flags;
 	void		(*input)(struct sk_buff *skb);
-	struct mutex	*cb_mutex;
 	int		(*bind)(struct net *net, int group);
 	void		(*unbind)(struct net *net, int group);
 	void            (*release) (struct sock *sk, unsigned long *groups);
diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index 8bbbe75e75db..0b7a89db3ab7 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -636,8 +636,7 @@ static struct proto netlink_proto = {
 };
 
 static int __netlink_create(struct net *net, struct socket *sock,
-			    struct mutex *dump_cb_mutex, int protocol,
-			    int kern)
+			    int protocol, int kern)
 {
 	struct sock *sk;
 	struct netlink_sock *nlk;
@@ -655,7 +654,6 @@ static int __netlink_create(struct net *net, struct socket *sock,
 	lockdep_set_class_and_name(&nlk->nl_cb_mutex,
 					   nlk_cb_mutex_keys + protocol,
 					   nlk_cb_mutex_key_strings[protocol]);
-	nlk->dump_cb_mutex = dump_cb_mutex;
 	init_waitqueue_head(&nlk->wait);
 
 	sk->sk_destruct = netlink_sock_destruct;
@@ -667,7 +665,6 @@ static int netlink_create(struct net *net, struct socket *sock, int protocol,
 			  int kern)
 {
 	struct module *module = NULL;
-	struct mutex *cb_mutex;
 	struct netlink_sock *nlk;
 	int (*bind)(struct net *net, int group);
 	void (*unbind)(struct net *net, int group);
@@ -696,7 +693,6 @@ static int netlink_create(struct net *net, struct socket *sock, int protocol,
 		module = nl_table[protocol].module;
 	else
 		err = -EPROTONOSUPPORT;
-	cb_mutex = nl_table[protocol].cb_mutex;
 	bind = nl_table[protocol].bind;
 	unbind = nl_table[protocol].unbind;
 	release = nl_table[protocol].release;
@@ -705,7 +701,7 @@ static int netlink_create(struct net *net, struct socket *sock, int protocol,
 	if (err < 0)
 		goto out;
 
-	err = __netlink_create(net, sock, cb_mutex, protocol, kern);
+	err = __netlink_create(net, sock, protocol, kern);
 	if (err < 0)
 		goto out_module;
 
@@ -2016,7 +2012,6 @@ __netlink_kernel_create(struct net *net, int unit, struct module *module,
 	struct sock *sk;
 	struct netlink_sock *nlk;
 	struct listeners *listeners = NULL;
-	struct mutex *cb_mutex = cfg ? cfg->cb_mutex : NULL;
 	unsigned int groups;
 
 	BUG_ON(!nl_table);
@@ -2027,7 +2022,7 @@ __netlink_kernel_create(struct net *net, int unit, struct module *module,
 	if (sock_create_lite(PF_NETLINK, SOCK_DGRAM, unit, &sock))
 		return NULL;
 
-	if (__netlink_create(net, sock, cb_mutex, unit, 1) < 0)
+	if (__netlink_create(net, sock, unit, 1) < 0)
 		goto out_sock_release_nosk;
 
 	sk = sock->sk;
@@ -2055,7 +2050,6 @@ __netlink_kernel_create(struct net *net, int unit, struct module *module,
 	if (!nl_table[unit].registered) {
 		nl_table[unit].groups = groups;
 		rcu_assign_pointer(nl_table[unit].listeners, listeners);
-		nl_table[unit].cb_mutex = cb_mutex;
 		nl_table[unit].module = module;
 		if (cfg) {
 			nl_table[unit].bind = cfg->bind;
@@ -2326,15 +2320,9 @@ static int netlink_dump(struct sock *sk, bool lock_taken)
 	netlink_skb_set_owner_r(skb, sk);
 
 	if (nlk->dump_done_errno > 0) {
-		struct mutex *extra_mutex = nlk->dump_cb_mutex;
-
 		cb->extack = &extack;
 
-		if (extra_mutex)
-			mutex_lock(extra_mutex);
 		nlk->dump_done_errno = cb->dump(skb, cb);
-		if (extra_mutex)
-			mutex_unlock(extra_mutex);
 
 		/* EMSGSIZE plus something already in the skb means
 		 * that there's more to dump but current skb has filled up.
-- 
2.45.2

Re: [PATCH net-next 2/2] net: netlink: remove the cb_mutex "injection" from netlink core

[Kuniyuki Iwashima]

From: Jakub Kicinski <kuba@kernel.org>
Date: Thu,  6 Jun 2024 12:29:06 -0700
> Back in 2007, in commit af65bdfce98d ("[NETLINK]: Switch cb_lock spinlock
> to mutex and allow to override it") netlink core was extended to allow
> subsystems to replace the dump mutex lock with its own lock.
> 
> The mechanism was used by rtnetlink to take rtnl_lock but it isn't
> sufficiently flexible for other users. Over the 17 years since
> it was added no other user appeared. Since rtnetlink needs conditional
> locking now, and doesn't use it either, axe this feature complete.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Reviewed-by: Kuniyuki Iwashima <kuniyu@amazon.com>

Thanks!


> ---
> CC: anjali.k.kulkarni@oracle.com
> CC: Liam.Howlett@oracle.com
> CC: jiri@resnulli.us
> ---
>  include/linux/netlink.h  |  1 -
>  net/netlink/af_netlink.c | 18 +++---------------
>  2 files changed, 3 insertions(+), 16 deletions(-)
> 
> diff --git a/include/linux/netlink.h b/include/linux/netlink.h
> index 5df7340d4dab..b332c2048c75 100644
> --- a/include/linux/netlink.h
> +++ b/include/linux/netlink.h
> @@ -47,7 +47,6 @@ struct netlink_kernel_cfg {
>  	unsigned int	groups;
>  	unsigned int	flags;
>  	void		(*input)(struct sk_buff *skb);
> -	struct mutex	*cb_mutex;
>  	int		(*bind)(struct net *net, int group);
>  	void		(*unbind)(struct net *net, int group);
>  	void            (*release) (struct sock *sk, unsigned long *groups);
> diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
> index 8bbbe75e75db..0b7a89db3ab7 100644
> --- a/net/netlink/af_netlink.c
> +++ b/net/netlink/af_netlink.c
> @@ -636,8 +636,7 @@ static struct proto netlink_proto = {
>  };
>  
>  static int __netlink_create(struct net *net, struct socket *sock,
> -			    struct mutex *dump_cb_mutex, int protocol,
> -			    int kern)
> +			    int protocol, int kern)
>  {
>  	struct sock *sk;
>  	struct netlink_sock *nlk;
> @@ -655,7 +654,6 @@ static int __netlink_create(struct net *net, struct socket *sock,
>  	lockdep_set_class_and_name(&nlk->nl_cb_mutex,
>  					   nlk_cb_mutex_keys + protocol,
>  					   nlk_cb_mutex_key_strings[protocol]);
> -	nlk->dump_cb_mutex = dump_cb_mutex;
>  	init_waitqueue_head(&nlk->wait);
>  
>  	sk->sk_destruct = netlink_sock_destruct;
> @@ -667,7 +665,6 @@ static int netlink_create(struct net *net, struct socket *sock, int protocol,
>  			  int kern)
>  {
>  	struct module *module = NULL;
> -	struct mutex *cb_mutex;
>  	struct netlink_sock *nlk;
>  	int (*bind)(struct net *net, int group);
>  	void (*unbind)(struct net *net, int group);
> @@ -696,7 +693,6 @@ static int netlink_create(struct net *net, struct socket *sock, int protocol,
>  		module = nl_table[protocol].module;
>  	else
>  		err = -EPROTONOSUPPORT;
> -	cb_mutex = nl_table[protocol].cb_mutex;
>  	bind = nl_table[protocol].bind;
>  	unbind = nl_table[protocol].unbind;
>  	release = nl_table[protocol].release;
> @@ -705,7 +701,7 @@ static int netlink_create(struct net *net, struct socket *sock, int protocol,
>  	if (err < 0)
>  		goto out;
>  
> -	err = __netlink_create(net, sock, cb_mutex, protocol, kern);
> +	err = __netlink_create(net, sock, protocol, kern);
>  	if (err < 0)
>  		goto out_module;
>  
> @@ -2016,7 +2012,6 @@ __netlink_kernel_create(struct net *net, int unit, struct module *module,
>  	struct sock *sk;
>  	struct netlink_sock *nlk;
>  	struct listeners *listeners = NULL;
> -	struct mutex *cb_mutex = cfg ? cfg->cb_mutex : NULL;
>  	unsigned int groups;
>  
>  	BUG_ON(!nl_table);
> @@ -2027,7 +2022,7 @@ __netlink_kernel_create(struct net *net, int unit, struct module *module,
>  	if (sock_create_lite(PF_NETLINK, SOCK_DGRAM, unit, &sock))
>  		return NULL;
>  
> -	if (__netlink_create(net, sock, cb_mutex, unit, 1) < 0)
> +	if (__netlink_create(net, sock, unit, 1) < 0)
>  		goto out_sock_release_nosk;
>  
>  	sk = sock->sk;
> @@ -2055,7 +2050,6 @@ __netlink_kernel_create(struct net *net, int unit, struct module *module,
>  	if (!nl_table[unit].registered) {
>  		nl_table[unit].groups = groups;
>  		rcu_assign_pointer(nl_table[unit].listeners, listeners);
> -		nl_table[unit].cb_mutex = cb_mutex;
>  		nl_table[unit].module = module;
>  		if (cfg) {
>  			nl_table[unit].bind = cfg->bind;
> @@ -2326,15 +2320,9 @@ static int netlink_dump(struct sock *sk, bool lock_taken)
>  	netlink_skb_set_owner_r(skb, sk);
>  
>  	if (nlk->dump_done_errno > 0) {
> -		struct mutex *extra_mutex = nlk->dump_cb_mutex;
> -
>  		cb->extack = &extack;
>  
> -		if (extra_mutex)
> -			mutex_lock(extra_mutex);
>  		nlk->dump_done_errno = cb->dump(skb, cb);
> -		if (extra_mutex)
> -			mutex_unlock(extra_mutex);
>  
>  		/* EMSGSIZE plus something already in the skb means
>  		 * that there's more to dump but current skb has filled up.
> -- 
> 2.45.2

Re: [PATCH net-next 0/2] rtnetlink: move rtnl_lock handling out of af_netlink

[David Ahern]

On 6/6/24 1:29 PM, Jakub Kicinski wrote:
> With the changes done in commit 5b4b62a169e1 ("rtnetlink: make
> the "split" NLM_DONE handling generic") we can also move the
> rtnl locking out of af_netlink.
> 
> Jakub Kicinski (2):
>   rtnetlink: move rtnl_lock handling out of af_netlink
>   net: netlink: remove the cb_mutex "injection" from netlink core
> 
>  include/linux/netlink.h  |  1 -
>  net/core/rtnetlink.c     |  9 +++++++--
>  net/netlink/af_netlink.c | 20 +++-----------------
>  3 files changed, 10 insertions(+), 20 deletions(-)
> 

both look good to me
Reviewed-by: David Ahern <dsahern@kernel.org>

Re: [PATCH net-next 0/2] rtnetlink: move rtnl_lock handling out of af_netlink

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by David S. Miller <davem@davemloft.net>:

On Thu,  6 Jun 2024 12:29:04 -0700 you wrote:
> With the changes done in commit 5b4b62a169e1 ("rtnetlink: make
> the "split" NLM_DONE handling generic") we can also move the
> rtnl locking out of af_netlink.
> 
> Jakub Kicinski (2):
>   rtnetlink: move rtnl_lock handling out of af_netlink
>   net: netlink: remove the cb_mutex "injection" from netlink core
> 
> [...]

Here is the summary with links:
  - [net-next,1/2] rtnetlink: move rtnl_lock handling out of af_netlink
    https://git.kernel.org/netdev/net-next/c/5380d64f8d76
  - [net-next,2/2] net: netlink: remove the cb_mutex "injection" from netlink core
    https://git.kernel.org/netdev/net-next/c/5fbf57a937f4

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html