network namespace locking rules

Current locking for network namespace list/initialization is broken. for_each_net is called under single rtnl_lock in register_netdevice_notifier.

Locking:

    net_mutex -> rtnl_lock() -> dev_base_lock

Reasoning:
- net_mutex holds serialization of the addition/removal of subsystems/modules and the creation/destruction of network namespaces as a whole
- loopback device is one of such subsystems and it takes rtnl_lock inside
- per/namespace RTNL netlink socket requires an iteration over namespace list inside rtnl_unlock, which is called inside net_mutex
Resume:
net_namespace_list is guarded by both rtnl_lock & net_mutex and can be safely iterated under any of them

Signed-off-by Denis V. Lunev <[email protected]>

--------

static struct net *net_alloc(void)
{

net/core/net_namespace.c +5 -5

 	return kmem_cache_alloc(net_cachep, GFP_KERNEL);

@@ -71,9 +62,9 @@ static void cleanup_net(struct work_struct *work)
 	mutex_lock(&net_mutex);
 
 	/* Don't let anyone else find us. */
-	net_lock();
+	rtnl_lock();
 	list_del(&net->list);
-	net_unlock();
+	rtnl_unlock();
 
 	/* Run all of the network namespace exit methods */
 	pernet_count = 0;

@@ -193,9 +184,9 @@ struct net *copy_net_ns(unsigned long flags, struct net *old_net)
 	if (err)
 		goto out_unlock;
 
-	net_lock();
+	rtnl_lock();
 	list_add_tail(&new_net->list, &net_namespace_list);
-	net_unlock();
+	rtnl_unlock();
 
 
 out_unlock:

@@ -220,14 +211,13 @@ static int __init net_ns_init(void)
 	mutex_lock(&net_mutex);
 	err = setup_net(&init_net);
 
-	net_lock();
+	rtnl_lock();
 	list_add_tail(&init_net.list, &net_namespace_list);

net/core/rtnetlink.c +1 -3

index 82ebc23..e610313 100644
@@ -73,16 +73,24 @@ void __rtnl_unlock(void)
 void rtnl_unlock(void)
 {
 	struct net *net;
-	mutex_unlock(&rtnl_mutex);
-	
-	net_lock();
+

Quoted 21 lines · Denis V. Lunev

Current locking for network namespace list/initialization is broken. for_each_net is called under single rtnl_lock in register_netdevice_notifier.

Locking:

    net_mutex -> rtnl_lock() -> dev_base_lock

Reasoning:
- net_mutex holds serialization of the addition/removal of subsystems/modules and the creation/destruction of network namespaces as a whole
- loopback device is one of such subsystems and it takes rtnl_lock inside
- per/namespace RTNL netlink socket requires an iteration over namespace list inside rtnl_unlock, which is called inside net_mutex
Resume:
net_namespace_list is guarded by both rtnl_lock & net_mutex and can be safely iterated under any of them

Signed-off-by Denis V. Lunev <[email protected]>

--------

[ cut ]

net/core/net_namespace.c static struct kmem_cache *net_cachep; · line 36

Quoted 11 lines

net/core/net_namespace.c +0 -10

-void net_lock(void)
-{
-    mutex_lock(&net_list_mutex);
-}
-
-void net_unlock(void)
-{
-    mutex_unlock(&net_list_mutex);
-}
-

net_list_mutex is no more needed, right ? The removing of its declaration is missing (already done in #netns49)

Quoted 35 lines

static struct net *net_alloc(void)

 {
     return kmem_cache_alloc(net_cachep, GFP_KERNEL);

net/core/net_namespace.c +4 -4

@@ -71,9 +62,9 @@ static void cleanup_net(struct work_struct *work)
     mutex_lock(&net_mutex);
 
     /* Don't let anyone else find us. */
-    net_lock();
+    rtnl_lock();
     list_del(&net->list);
-    net_unlock();
+    rtnl_unlock();
 
     /* Run all of the network namespace exit methods */
     pernet_count = 0;

@@ -193,9 +184,9 @@ struct net *copy_net_ns(unsigned long flags,
 struct net *old_net)
     if (err)
         goto out_unlock;
 
-    net_lock();
+    rtnl_lock();
     list_add_tail(&new_net->list, &net_namespace_list);
-    net_unlock();
+    rtnl_unlock();
 
 

out_unlock:

net/core/net_namespace.c +1 -1

@@ -220,14 +211,13 @@ static int __init net_ns_init(void)
     mutex_lock(&net_mutex);
     err = setup_net(&init_net);
 
-    net_lock();
+    rtnl_lock();
     list_add_tail(&init_net.list, &net_namespace_list);

net/core/net_namespace.c static int __init net_ns_init(void) · line 223

Quoted 10 lines

net/core/net_namespace.c +1 -2

-    net_unlock();
+    rtnl_unlock();
 
     mutex_unlock(&net_mutex);
     if (err)
         panic("Could not setup the initial network namespace");
-
     return 0;
 }
 

Eric did already these changes in #netns49.

Quoted 12 lines

net/core/rtnetlink.c +1 -3

index 82ebc23..e610313 100644
@@ -73,16 +73,24 @@ void __rtnl_unlock(void)
 void rtnl_unlock(void)
 {
     struct net *net;
-    mutex_unlock(&rtnl_mutex);
-   
-    net_lock();
+

net/core/rtnetlink.c void __rtnl_unlock(void) · line 86

Quoted 10 lines

net/core/rtnetlink.c +7 -0

+retry:
     for_each_net(net) {
         struct sock *rtnl = net->rtnl;
+
+        if (rtnl == NULL || rtnl->sk_receive_queue.qlen == 0)
+            continue;
+
+        get_net(net);
+        mutex_unlock(&rtnl_mutex);
         if (rtnl && rtnl->sk_receive_queue.qlen)

                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

this statement is always true with the previous one.

net/core/rtnetlink.c void __rtnl_unlock(void) · line 91

Quoted 7 lines

net/core/rtnetlink.c +3 -2

             rtnl->sk_data_ready(rtnl, 0);
-    }
-    net_unlock();
+        mutex_lock(&rtnl_mutex);
+        put_net(net);
 
+        goto retry;

Why do you need to return to the beginning of the list ?

net/core/rtnetlink.c void __rtnl_unlock(void) · line 95

Quoted 4 lines

net/core/rtnetlink.c +2 -0

+    }
+    mutex_unlock(&rtnl_mutex);
     netdev_run_todo();
 }

Denis, can you explain why this part must be modified ?

Current locking for network namespace list/initialization is broken. for_each_net is called under single rtnl_lock in register_netdevice_notifier.

Quoted 3 lines · Denis V. Lunev

Current locking for network namespace list/initialization is broken. for_each_net is called under single rtnl_lock in register_netdevice_notifier.

As of 984e617a3e1974022b8f671427a76ffbe886f75b this issue has been addressed in net-2.6.24

The only remaining part to address is rtnl_unlock().

My current hypothesis is that rtnl_unlock() only needs to process the packets that are queued while we had the rtnl_lock held, to maintain the current semantics.

So the retry may not be necessary as it may be possible to prove that the only extra packets that could come in come from another thread taking the rtnl_lock and they will those packets in their rtnl_unlock() if we don't.

I will review this later today, and add the retry if necessary.

One way or another I think we agree on how to get the locking correct. The other details are a bit tricky but look usable.

Eric