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

[Kavanagh, Mark B]

>
>Minor comment inline.
>
>Acked-by: Ilya Maximets <i.maximets at samsung.com>
>

Other than the comment mentioned by Ilya, this LGTM also - thanks again for resolving, Daniele.

Acked-by: mark.b.kavanagh at intel.com

>On 05.08.2016 23:57, Daniele Di Proietto wrote:
>> 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",
>
>Space between arguments of 'CONST_CAST()' and parenthesized operand of 'sizeof'.
>
>>               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
>>
>_______________________________________________
>dev mailing list
>dev at openvswitch.org
>http://openvswitch.org/mailman/listinfo/dev