[ovs-dev] [PATCH] netdev-dpdk: Fix deadlock in destroy_device().

[Daniele Di Proietto]

netdev_dpdk_vhost_destruct() calls rte_vhost_driver_unregister(), which
can trigger the destroy_device() callback.  destroy_device() will try to
take two mutexes already held by netdev_dpdk_vhost_destruct(), causing a
deadlock.

This problem can be solved by dropping the mutexes before calling
rte_vhost_driver_unregister().  The netdev_dpdk_vhost_destruct() and
construct() call are already serialized by netdev_mutex.

This commit also makes clear that dev->vhost_id is constant and can be
accessed without taking any mutexes in the lifetime of the devices.

Fixes: 8d38823bdf8b("netdev-dpdk: fix memory leak")
Reported-by: Ilya Maximets <i.maximets at samsung.com>
Signed-off-by: Daniele Di Proietto <diproiettod at vmware.com>
---
 lib/netdev-dpdk.c | 34 ++++++++++++++++++++++++----------
 1 file changed, 24 insertions(+), 10 deletions(-)

diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index f37ec1c..98bff62 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -355,8 +355,10 @@ struct netdev_dpdk {
     /* True if vHost device is 'up' and has been reconfigured at least once */
     bool vhost_reconfigured;
 
-    /* Identifier used to distinguish vhost devices from each other */
-    char vhost_id[PATH_MAX];
+    /* Identifier used to distinguish vhost devices from each other.  It does
+     * not change during the lifetime of a struct netdev_dpdk.  It can be read
+     * without holding any mutex. */
+    const char vhost_id[PATH_MAX];
 
     /* In dpdk_list. */
     struct ovs_list list_node OVS_GUARDED_BY(dpdk_mutex);
@@ -846,7 +848,8 @@ netdev_dpdk_vhost_cuse_construct(struct netdev *netdev)
     }
 
     ovs_mutex_lock(&dpdk_mutex);
-    strncpy(dev->vhost_id, netdev->name, sizeof(dev->vhost_id));
+    strncpy(CONST_CAST(char *, dev->vhost_id), netdev->name,
+            sizeof dev->vhost_id);
     err = vhost_construct_helper(netdev);
     ovs_mutex_unlock(&dpdk_mutex);
     return err;
@@ -878,7 +881,7 @@ netdev_dpdk_vhost_user_construct(struct netdev *netdev)
     /* Take the name of the vhost-user port and append it to the location where
      * the socket is to be created, then register the socket.
      */
-    snprintf(dev->vhost_id, sizeof(dev->vhost_id), "%s/%s",
+    snprintf(CONST_CAST(char *,dev->vhost_id), sizeof(dev->vhost_id), "%s/%s",
              vhost_sock_dir, name);
 
     err = rte_vhost_driver_register(dev->vhost_id, flags);
@@ -938,6 +941,17 @@ netdev_dpdk_destruct(struct netdev *netdev)
     ovs_mutex_unlock(&dpdk_mutex);
 }
 
+/* rte_vhost_driver_unregister() can call back destroy_device(), which will
+ * try to acquire 'dpdk_mutex' and possibly 'dev->mutex'.  To avoid a
+ * deadlock, none of the mutexes must be held while calling this function. */
+static int
+dpdk_vhost_driver_unregister(struct netdev_dpdk *dev)
+    OVS_EXCLUDED(dpdk_mutex)
+    OVS_EXCLUDED(dev->mutex)
+{
+    return rte_vhost_driver_unregister(dev->vhost_id);
+}
+
 static void
 netdev_dpdk_vhost_destruct(struct netdev *netdev)
 {
@@ -955,12 +969,6 @@ netdev_dpdk_vhost_destruct(struct netdev *netdev)
                  dev->vhost_id);
     }
 
-    if (rte_vhost_driver_unregister(dev->vhost_id)) {
-        VLOG_ERR("Unable to remove vhost-user socket %s", dev->vhost_id);
-    } else {
-        fatal_signal_remove_file_to_unlink(dev->vhost_id);
-    }
-
     free(ovsrcu_get_protected(struct ingress_policer *,
                               &dev->ingress_policer));
 
@@ -970,6 +978,12 @@ netdev_dpdk_vhost_destruct(struct netdev *netdev)
 
     ovs_mutex_unlock(&dev->mutex);
     ovs_mutex_unlock(&dpdk_mutex);
+
+    if (dpdk_vhost_driver_unregister(dev)) {
+        VLOG_ERR("Unable to remove vhost-user socket %s", dev->vhost_id);
+    } else {
+        fatal_signal_remove_file_to_unlink(dev->vhost_id);
+    }
 }
 
 static void
-- 
2.8.1