Re: [PATCH net-next] net: Revert net_rwsem

[David Miller]

From: Kirill Tkhai 
Date: Fri, 30 Mar 2018 12:57:53 +0300

> 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(_rwsem) from {,un}register_netdevice_notifiers()).
> Please, let me know if some other actions are required.

I applied that series.

Re: [PATCH net-next] net: Revert net_rwsem

[Kirill Tkhai]

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 

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(_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(_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(_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, _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(_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(_rwsem);
>  
>  unlock:
>   rtnl_unlock();
> @@ -1666,7 +1664,6 @@ int register_netdevice_notifier(struct notifier_block 
> *nb)
>   }
>  
>  outroll:
> - up_read(_rwsem);
>   raw_notifier_chain_unregister(_chain, nb);
>   goto unlock;
>  }
> @@ -1697,7 +1694,6 @@ int unregister_netdevice_notifier(struct notifier_block 
> *nb)
>   if (err)
>   goto unlock;
>  
> - down_read(_rwsem);
>   for_each_net(net) {
>   for_each_netdev(net, dev) {
>   if (dev->flags & IFF_UP) {
> @@ 

[PATCH net-next] net: Revert net_rwsem

[Kirill Tkhai]

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 
---
 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(_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(_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, _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(_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(_rwsem);
 
 unlock:
rtnl_unlock();
@@ -1666,7 +1664,6 @@ int register_netdevice_notifier(struct notifier_block *nb)
}
 
 outroll:
-   up_read(_rwsem);
raw_notifier_chain_unregister(_chain, nb);
goto unlock;
 }
@@ -1697,7 +1694,6 @@ int unregister_netdevice_notifier(struct notifier_block 
*nb)
if (err)
goto unlock;
 
-   down_read(_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(_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;