route-table: Make route-table module thread-safe.

Since the use of xcache, the netdev struct can be freed by the
revalidator threads.  This fact also makes the following race possible:

1. Consider there is a gre tunnel, and datapath flows that go through
   the tunnel.  Now, assume user deletes the tunnel port.
2. The main thread closes all of its references to the corresponding
   netdev struct.
3. If the ukey for the tunnel datapath flows hold the last reference
   to the old port's netdev, the revalidator will then close it
   and remove the netlink notifier struct (if the netdev is the last
   vport netdev).
4. However, if the main thread executes the netdev_vport_run(), and
   sees the existence of netlink notifier struct (before revalidator
   frees it), it will try polling updates from the notifier socket.
   And when it polls the socket fd, the fd has already been freed,
   and poll will keep failing with "Bad file descriptor".

The following script could be used to reproduce the race:
- assume on a VM-VM setup, with the setup below:

ovs-vsctl add-br br-int -- add-port br-int p3
ovs-vsctl add-port br-int vif1 -- set int vif1 type=internal
ifconfig vif1 11.0.0.1 up; ifconfig eth3 3.3.3.1 up

- while keeping a ping from 11.0.0.1 to 11.0.0.2, run this loop:

for i in `seq 1000000`; do
    sleep 5; ovs-vsctl del-port p3;
    ovs-vsctl add-port br-int p3 -- set int p3  type=gre \
        options:remote_ip=3.3.3.2 options:key=1;
done

- after a while, the race should be triggered.  and the main thread
  should run at 100% cpu.

This race has already been fixed on master by commit 3c27dbe6 (route
-table: Make route-table module thread-safe.).  This commit backports
it to branch-2.3.

VMware-BZ: #1287360

Signed-off-by: Ryan Wilson <wryan@nicira.com>
Acked-by: Ben Pfaff <blp@nicira.com>
Signed-off-by: Alex Wang <alexw@nicira.com>

diff --git a/lib/route-table-stub.c b/lib/route-table-stub.c
index 9acc81c..dab4fd2 100644 (file)
--- a/lib/route-table-stub.c
+++ b/lib/route-table-stub.c
@@ -24,13 +24,6 @@ route_table_get_name(ovs_be32 ip OVS_UNUSED, char name[IFNAMSIZ] OVS_UNUSED)
     return false;
 }
 
-bool
-route_table_get_ifindex(ovs_be32 ip OVS_UNUSED, int *ifindex)
-{
-    *ifindex = 0;
-    return false;
-}
-
 uint64_t
 route_table_get_change_seq(void)
 {

diff --git a/lib/route-table.c b/lib/route-table.c
index 2986d3d..9284924 100644 (file)
--- a/lib/route-table.c
+++ b/lib/route-table.c
@@ -63,6 +63,7 @@ struct name_node {
     char ifname[IFNAMSIZ]; /* Interface name. */
 };
 
+static struct ovs_mutex route_table_mutex = OVS_MUTEX_INITIALIZER;
 static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 20);
 
 /* Global change number for route-table, which should be incremented
@@ -81,6 +82,8 @@ static struct hmap route_map;
 static struct hmap name_map;
 
 static int route_table_reset(void);
+static bool route_table_get_ifindex(ovs_be32 ip, int *)
+    OVS_REQUIRES(route_table_mutex);
 static void route_table_handle_msg(const struct route_table_msg *);
 static bool route_table_parse(struct ofpbuf *, struct route_table_msg *);
 static void route_table_change(const struct route_table_msg *, void *);
@@ -102,9 +105,12 @@ static struct name_node *name_node_lookup(int ifi_index);
  * Returns true if successful, otherwise false. */
 bool
 route_table_get_name(ovs_be32 ip, char name[IFNAMSIZ])
+    OVS_EXCLUDED(route_table_mutex)
 {
     int ifindex;
 
+    ovs_mutex_lock(&route_table_mutex);
+
     if (!name_table_valid) {
         name_table_reset();
     }
@@ -115,10 +121,12 @@ route_table_get_name(ovs_be32 ip, char name[IFNAMSIZ])
         nn = name_node_lookup(ifindex);
         if (nn) {
             ovs_strlcpy(name, nn->ifname, IFNAMSIZ);
+            ovs_mutex_unlock(&route_table_mutex);
             return true;
         }
     }
 
+    ovs_mutex_unlock(&route_table_mutex);
     return false;
 }
 
@@ -128,8 +136,9 @@ route_table_get_name(ovs_be32 ip, char name[IFNAMSIZ])
  * interface which is not physical (such as a bridge port).
  *
  * Returns true if successful, otherwise false. */
-bool
+static bool
 route_table_get_ifindex(ovs_be32 ip_, int *ifindex)
+    OVS_REQUIRES(route_table_mutex)
 {
     struct route_node *rn;
     uint32_t ip = ntohl(ip_);
@@ -168,7 +177,9 @@ route_table_get_change_seq(void)
  * function before making any other route_table function calls. */
 void
 route_table_register(void)
+    OVS_EXCLUDED(route_table_mutex)
 {
+    ovs_mutex_lock(&route_table_mutex);
     if (!register_count) {
         ovs_assert(!nln);
         ovs_assert(!route_notifier);
@@ -186,6 +197,7 @@ route_table_register(void)
     }
 
     register_count++;
+    ovs_mutex_unlock(&route_table_mutex);
 }
 
 /* Users of the route_table module should unregister themselves with this
@@ -193,7 +205,9 @@ route_table_register(void)
  * calls. */
 void
 route_table_unregister(void)
+    OVS_EXCLUDED(route_table_mutex)
 {
+    ovs_mutex_lock(&route_table_mutex);
     register_count--;
 
     if (!register_count) {
@@ -206,12 +220,15 @@ route_table_unregister(void)
         hmap_destroy(&route_map);
         name_table_uninit();
     }
+    ovs_mutex_unlock(&route_table_mutex);
 }
 
 /* Run periodically to update the locally maintained routing table. */
 void
 route_table_run(void)
+    OVS_EXCLUDED(route_table_mutex)
 {
+    ovs_mutex_lock(&route_table_mutex);
     if (nln) {
         rtnetlink_link_run();
         nln_run(nln);
@@ -220,16 +237,20 @@ route_table_run(void)
             route_table_reset();
         }
     }
+    ovs_mutex_unlock(&route_table_mutex);
 }
 
 /* Causes poll_block() to wake up when route_table updates are required. */
 void
 route_table_wait(void)
+    OVS_EXCLUDED(route_table_mutex)
 {
+    ovs_mutex_lock(&route_table_mutex);
     if (nln) {
         rtnetlink_link_wait();
         nln_wait(nln);
     }
+    ovs_mutex_unlock(&route_table_mutex);
 }
 
 static int

diff --git a/lib/route-table.h b/lib/route-table.h
index 2ed4b91..2c5967e 100644 (file)
--- a/lib/route-table.h
+++ b/lib/route-table.h
@@ -25,7 +25,6 @@
 
 #include "openvswitch/types.h"
 
-bool route_table_get_ifindex(ovs_be32 ip, int *ifindex);
 bool route_table_get_name(ovs_be32 ip, char name[IFNAMSIZ]);
 uint64_t route_table_get_change_seq(void);
 void route_table_register(void);