netdev - Re: [PATCH net-next 2/2] net: netlink: remove the cb

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Thu, 6 Jun 2024 16:35:12 -0700
From: Kuniyuki Iwashima <kuniyu@...zon.com>
To: <kuba@...nel.org>
CC: <Liam.Howlett@...cle.com>, <anjali.k.kulkarni@...cle.com>,
	<davem@...emloft.net>, <dsahern@...il.com>, <edumazet@...gle.com>,
	<jiri@...nulli.us>, <netdev@...r.kernel.org>, <pabeni@...hat.com>,
	<kuniyu@...zon.com>
Subject: Re: [PATCH net-next 2/2] net: netlink: remove the cb_mutex "injection" from netlink core

From: Jakub Kicinski <kuba@...nel.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@...nel.org>

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

Thanks!


> ---
> CC: anjali.k.kulkarni@...cle.com
> CC: Liam.Howlett@...cle.com
> CC: jiri@...nulli.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