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

[Kuniyuki Iwashima <kuniyu@amazon.com>]

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