Message-ID: <d77de1d2-e931-d333-f4d7-d0087460d8d4@virtuozzo.com>
Date:   Fri, 30 Mar 2018 12:57:53 +0300
From:   Kirill Tkhai <ktkhai@...tuozzo.com>
To:     davem@...emloft.net, netdev@...r.kernel.org
Subject: Re: [PATCH net-next] net: Revert net_rwsem

On 30.03.2018 12:23, Kirill Tkhai wrote:
> This reverts:
> 
> 152f253152cc net: Remove rtnl_lock() in nf_ct_iterate_destroy()
> ec9c780925c5 ovs: Remove rtnl_lock() from ovs_exit_net()
> 350311aab4c0 security: Remove rtnl_lock() in selinux_xfrm_notify_policyload()
> 10256debb918 net: Don't take rtnl_lock() in wireless_nlevent_flush()
> f0b07bb151b0 net: Introduce net_rwsem to protect net_namespace_list
> 
> There are missed, that down_read() can't be taken recursive.
> This is because of rw_semaphore design, which prevents it
> to be occupied by only readers forever.
> 
> So, we can't take it in register_netdevice_notifier(), as it's also
> taken in wext_netdev_notifier_call()->wireless_nlevent_flush().
> 
> The solution is to protect net_namespace_list modifications
> in register_netdevice_notifier() via pernet_ops_rwsem, as it's
> made in:
> 
> https://patchwork.ozlabs.org/project/netdev/list/?series=36495
> 
> Then, we won't have to take net_rwsem in call_netevent_notifiers().
> 
> But since the patchset is not in kernel, let's just revert net_rwsem
> for now, and I'll resubmit it later (after the above patchset).
> 
> Signed-off-by: Kirill Tkhai <ktkhai@...tuozzo.com>

Or, if https://patchwork.ozlabs.org/project/netdev/list/?series=36495
is ok enough to go in kernel in a little while, I'll make another fix
(removing down_read(&net_rwsem) from {,un}register_netdevice_notifiers()).
Please, let me know if some other actions are required.

Thanks,
Kirill

> ---
>  drivers/infiniband/core/roce_gid_mgmt.c |    2 --
>  include/linux/rtnetlink.h               |    1 -
>  include/net/net_namespace.h             |    1 -
>  net/core/dev.c                          |    5 -----
>  net/core/fib_notifier.c                 |    2 --
>  net/core/net_namespace.c                |   18 +++++-------------
>  net/core/rtnetlink.c                    |    5 -----
>  net/netfilter/nf_conntrack_core.c       |    4 ++--
>  net/openvswitch/datapath.c              |    4 ++--
>  net/wireless/wext-core.c                |    6 ++++--
>  security/selinux/include/xfrm.h         |    4 ++--
>  11 files changed, 15 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/infiniband/core/roce_gid_mgmt.c b/drivers/infiniband/core/roce_gid_mgmt.c
> index cc2966380c0c..5a52ec77940a 100644
> --- a/drivers/infiniband/core/roce_gid_mgmt.c
> +++ b/drivers/infiniband/core/roce_gid_mgmt.c
> @@ -403,12 +403,10 @@ static void enum_all_gids_of_dev_cb(struct ib_device *ib_dev,
>  	 * our feet
>  	 */
>  	rtnl_lock();
> -	down_read(&net_rwsem);
>  	for_each_net(net)
>  		for_each_netdev(net, ndev)
>  			if (is_eth_port_of_netdev(ib_dev, port, rdma_ndev, ndev))
>  				add_netdev_ips(ib_dev, port, rdma_ndev, ndev);
> -	up_read(&net_rwsem);
>  	rtnl_unlock();
>  }
>  
> diff --git a/include/linux/rtnetlink.h b/include/linux/rtnetlink.h
> index 5225832bd6ff..c7d1e4689325 100644
> --- a/include/linux/rtnetlink.h
> +++ b/include/linux/rtnetlink.h
> @@ -37,7 +37,6 @@ extern int rtnl_lock_killable(void);
>  
>  extern wait_queue_head_t netdev_unregistering_wq;
>  extern struct rw_semaphore pernet_ops_rwsem;
> -extern struct rw_semaphore net_rwsem;
>  
>  #ifdef CONFIG_PROVE_LOCKING
>  extern bool lockdep_rtnl_is_held(void);
> diff --git a/include/net/net_namespace.h b/include/net/net_namespace.h
> index 47e35cce3b64..1ab4f920f109 100644
> --- a/include/net/net_namespace.h
> +++ b/include/net/net_namespace.h
> @@ -291,7 +291,6 @@ static inline struct net *read_pnet(const possible_net_t *pnet)
>  #endif
>  }
>  
> -/* Protected by net_rwsem */
>  #define for_each_net(VAR)				\
>  	list_for_each_entry(VAR, &net_namespace_list, list)
>  
> diff --git a/net/core/dev.c b/net/core/dev.c
> index eca5458b2753..e13807b5c84d 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -1629,7 +1629,6 @@ int register_netdevice_notifier(struct notifier_block *nb)
>  		goto unlock;
>  	if (dev_boot_phase)
>  		goto unlock;
> -	down_read(&net_rwsem);
>  	for_each_net(net) {
>  		for_each_netdev(net, dev) {
>  			err = call_netdevice_notifier(nb, NETDEV_REGISTER, dev);
> @@ -1643,7 +1642,6 @@ int register_netdevice_notifier(struct notifier_block *nb)
>  			call_netdevice_notifier(nb, NETDEV_UP, dev);
>  		}
>  	}
> -	up_read(&net_rwsem);
>  
>  unlock:
>  	rtnl_unlock();
> @@ -1666,7 +1664,6 @@ int register_netdevice_notifier(struct notifier_block *nb)
>  	}
>  
>  outroll:
> -	up_read(&net_rwsem);
>  	raw_notifier_chain_unregister(&netdev_chain, nb);
>  	goto unlock;
>  }
> @@ -1697,7 +1694,6 @@ int unregister_netdevice_notifier(struct notifier_block *nb)
>  	if (err)
>  		goto unlock;
>  
> -	down_read(&net_rwsem);
>  	for_each_net(net) {
>  		for_each_netdev(net, dev) {
>  			if (dev->flags & IFF_UP) {
> @@ -1708,7 +1704,6 @@ int unregister_netdevice_notifier(struct notifier_block *nb)
>  			call_netdevice_notifier(nb, NETDEV_UNREGISTER, dev);
>  		}
>  	}
> -	up_read(&net_rwsem);
>  unlock:
>  	rtnl_unlock();
>  	return err;
> diff --git a/net/core/fib_notifier.c b/net/core/fib_notifier.c
> index 13a40b831d6d..b793b523aba3 100644
> --- a/net/core/fib_notifier.c
> +++ b/net/core/fib_notifier.c
> @@ -39,7 +39,6 @@ static unsigned int fib_seq_sum(void)
>  	struct net *net;
>  
>  	rtnl_lock();
> -	down_read(&net_rwsem);
>  	for_each_net(net) {
>  		rcu_read_lock();
>  		list_for_each_entry_rcu(ops, &net->fib_notifier_ops, list) {
> @@ -50,7 +49,6 @@ static unsigned int fib_seq_sum(void)
>  		}
>  		rcu_read_unlock();
>  	}
> -	up_read(&net_rwsem);
>  	rtnl_unlock();
>  
>  	return fib_seq;
> diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c
> index 7fdf321d4997..b5796d17a302 100644
> --- a/net/core/net_namespace.c
> +++ b/net/core/net_namespace.c
> @@ -33,10 +33,6 @@ static struct list_head *first_device = &pernet_list;
>  LIST_HEAD(net_namespace_list);
>  EXPORT_SYMBOL_GPL(net_namespace_list);
>  
> -/* Protects net_namespace_list. Nests iside rtnl_lock() */
> -DECLARE_RWSEM(net_rwsem);
> -EXPORT_SYMBOL_GPL(net_rwsem);
> -
>  struct net init_net = {
>  	.count		= REFCOUNT_INIT(1),
>  	.dev_base_head	= LIST_HEAD_INIT(init_net.dev_base_head),
> @@ -313,9 +309,9 @@ static __net_init int setup_net(struct net *net, struct user_namespace *user_ns)
>  		if (error < 0)
>  			goto out_undo;
>  	}
> -	down_write(&net_rwsem);
> +	rtnl_lock();
>  	list_add_tail_rcu(&net->list, &net_namespace_list);
> -	up_write(&net_rwsem);
> +	rtnl_unlock();
>  out:
>  	return error;
>  
> @@ -454,7 +450,7 @@ static void unhash_nsid(struct net *net, struct net *last)
>  	 * and this work is the only process, that may delete
>  	 * a net from net_namespace_list. So, when the below
>  	 * is executing, the list may only grow. Thus, we do not
> -	 * use for_each_net_rcu() or net_rwsem.
> +	 * use for_each_net_rcu() or rtnl_lock().
>  	 */
>  	for_each_net(tmp) {
>  		int id;
> @@ -489,7 +485,7 @@ static void cleanup_net(struct work_struct *work)
>  	down_read(&pernet_ops_rwsem);
>  
>  	/* Don't let anyone else find us. */
> -	down_write(&net_rwsem);
> +	rtnl_lock();
>  	llist_for_each_entry(net, net_kill_list, cleanup_list)
>  		list_del_rcu(&net->list);
>  	/* Cache last net. After we unlock rtnl, no one new net
> @@ -503,7 +499,7 @@ static void cleanup_net(struct work_struct *work)
>  	 * useless anyway, as netns_ids are destroyed there.
>  	 */
>  	last = list_last_entry(&net_namespace_list, struct net, list);
> -	up_write(&net_rwsem);
> +	rtnl_unlock();
>  
>  	llist_for_each_entry(net, net_kill_list, cleanup_list) {
>  		unhash_nsid(net, last);
> @@ -904,9 +900,6 @@ static int __register_pernet_operations(struct list_head *list,
>  
>  	list_add_tail(&ops->list, list);
>  	if (ops->init || (ops->id && ops->size)) {
> -		/* We held write locked pernet_ops_rwsem, and parallel
> -		 * setup_net() and cleanup_net() are not possible.
> -		 */
>  		for_each_net(net) {
>  			error = ops_init(ops, net);
>  			if (error)
> @@ -930,7 +923,6 @@ static void __unregister_pernet_operations(struct pernet_operations *ops)
>  	LIST_HEAD(net_exit_list);
>  
>  	list_del(&ops->list);
> -	/* See comment in __register_pernet_operations() */
>  	for_each_net(net)
>  		list_add_tail(&net->exit_list, &net_exit_list);
>  	ops_exit_list(ops, &net_exit_list);
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index e86b28482ca7..2d3949789cef 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -418,11 +418,9 @@ void __rtnl_link_unregister(struct rtnl_link_ops *ops)
>  {
>  	struct net *net;
>  
> -	down_read(&net_rwsem);
>  	for_each_net(net) {
>  		__rtnl_kill_links(net, ops);
>  	}
> -	up_read(&net_rwsem);
>  	list_del(&ops->list);
>  }
>  EXPORT_SYMBOL_GPL(__rtnl_link_unregister);
> @@ -440,9 +438,6 @@ static void rtnl_lock_unregistering_all(void)
>  	for (;;) {
>  		unregistering = false;
>  		rtnl_lock();
> -		/* We held write locked pernet_ops_rwsem, and parallel
> -		 * setup_net() and cleanup_net() are not possible.
> -		 */
>  		for_each_net(net) {
>  			if (net->dev_unreg_count > 0) {
>  				unregistering = true;
> diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
> index 41ff04ee2554..705198de671d 100644
> --- a/net/netfilter/nf_conntrack_core.c
> +++ b/net/netfilter/nf_conntrack_core.c
> @@ -1763,14 +1763,14 @@ nf_ct_iterate_destroy(int (*iter)(struct nf_conn *i, void *data), void *data)
>  {
>  	struct net *net;
>  
> -	down_read(&net_rwsem);
> +	rtnl_lock();
>  	for_each_net(net) {
>  		if (atomic_read(&net->ct.count) == 0)
>  			continue;
>  		__nf_ct_unconfirmed_destroy(net);
>  		nf_queue_nf_hook_drop(net);
>  	}
> -	up_read(&net_rwsem);
> +	rtnl_unlock();
>  
>  	/* Need to wait for netns cleanup worker to finish, if its
>  	 * running -- it might have deleted a net namespace from
> diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
> index 015e24e08909..ef38e5aecd28 100644
> --- a/net/openvswitch/datapath.c
> +++ b/net/openvswitch/datapath.c
> @@ -2363,10 +2363,10 @@ static void __net_exit ovs_exit_net(struct net *dnet)
>  	list_for_each_entry_safe(dp, dp_next, &ovs_net->dps, list_node)
>  		__dp_destroy(dp);
>  
> -	down_read(&net_rwsem);
> +	rtnl_lock();
>  	for_each_net(net)
>  		list_vports_from_net(net, dnet, &head);
> -	up_read(&net_rwsem);
> +	rtnl_unlock();
>  
>  	/* Detach all vports from given namespace. */
>  	list_for_each_entry_safe(vport, vport_next, &head, detach_list) {
> diff --git a/net/wireless/wext-core.c b/net/wireless/wext-core.c
> index 5e677dac2a0c..9efbfc753347 100644
> --- a/net/wireless/wext-core.c
> +++ b/net/wireless/wext-core.c
> @@ -347,13 +347,13 @@ void wireless_nlevent_flush(void)
>  	struct sk_buff *skb;
>  	struct net *net;
>  
> -	down_read(&net_rwsem);
> +	ASSERT_RTNL();
> +
>  	for_each_net(net) {
>  		while ((skb = skb_dequeue(&net->wext_nlevents)))
>  			rtnl_notify(skb, net, 0, RTNLGRP_LINK, NULL,
>  				    GFP_KERNEL);
>  	}
> -	up_read(&net_rwsem);
>  }
>  EXPORT_SYMBOL_GPL(wireless_nlevent_flush);
>  
> @@ -410,7 +410,9 @@ subsys_initcall(wireless_nlevent_init);
>  /* Process events generated by the wireless layer or the driver. */
>  static void wireless_nlevent_process(struct work_struct *work)
>  {
> +	rtnl_lock();
>  	wireless_nlevent_flush();
> +	rtnl_unlock();
>  }
>  
>  static DECLARE_WORK(wireless_nlevent_work, wireless_nlevent_process);
> diff --git a/security/selinux/include/xfrm.h b/security/selinux/include/xfrm.h
> index a0b465316292..1f173a7a4daa 100644
> --- a/security/selinux/include/xfrm.h
> +++ b/security/selinux/include/xfrm.h
> @@ -47,10 +47,10 @@ static inline void selinux_xfrm_notify_policyload(void)
>  {
>  	struct net *net;
>  
> -	down_read(&net_rwsem);
> +	rtnl_lock();
>  	for_each_net(net)
>  		rt_genid_bump_all(net);
> -	up_read(&net_rwsem);
> +	rtnl_unlock();
>  }
>  #else
>  static inline int selinux_xfrm_enabled(void)
> 

Powered by blists - more mailing lists