Re: [dpdk-dev] [PATCH] lib/librte_vhost: fix vid allocation race
From: Xia, Chenbo <hidden>
Date: 2021-02-01 06:27:41
Hi Peng & Fei,
-----Original Message----- From: dev <redacted> On Behalf Of Peng He Sent: Friday, January 29, 2021 3:36 PM To: dev@dpdk.org Cc: maxime.coquelin@redhat.com Subject: [dpdk-dev] [PATCH] lib/librte_vhost: fix vid allocation race
Fix the title to 'vhost: XXXXX'
From: "chenwei.0515" <redacted>
This should not be here.. you could just delete it as the author is already in commit message.
vhost_new_devcie might be called in different threads at the same time.
s/devcie/device
thread 1(config thread)
rte_vhost_driver_start
->vhost_user_start_client
->vhost_user_add_connection
-> vhost_new_device
thread 2(vhost-events)
vhost_user_read_cb
->vhost_user_msg_handler (return value < 0)
-> vhost_user_start_client
-> vhost_new_device
So there could be a case that a same vid has been allocated twice, or
some vid might be lost in DPDK lib however still held by the upper
applications.Good catch! I checked the code and find there exists cases that different threads may allocate vhost slot. And I also find that other functions which use the global vhost_devices[] may also meet the same problem. For example, vhost_destroy_device() could be called by different thread. So I suggest to fix the problem completely in all related functions like vhost_destroy_device() and get_device(). What do you think? If you agree with above, note that the title should also be changed. Besides, please also add 'Fixes:$COMMID_ID' and cc to stable@dpdk.org so it could be fixed in LTS. You can check other commit for reference.
quoted hunk ↗ jump to hunk
Reported-by: Peng He <redacted> Signed-off-by: Fei Chen <redacted> Reviewed-by: Zhihong Wang <redacted> --- lib/librte_vhost/vhost.c | 6 ++++++ 1 file changed, 6 insertions(+)diff --git a/lib/librte_vhost/vhost.c b/lib/librte_vhost/vhost.c index efb136edd1..db11d293d2 100644 --- a/lib/librte_vhost/vhost.c +++ b/lib/librte_vhost/vhost.c@@ -26,6 +26,7 @@ #include "vhost_user.h" struct virtio_net *vhost_devices[MAX_VHOST_DEVICE]; +pthread_mutex_t vhost_dev_lock = PTHREAD_MUTEX_INITIALIZER;
There's a duplicate space between 'pthread_mutex_t' and 'vhost_dev_lock', Let's just leave one 😊 Thanks, Chenbo
quoted hunk ↗ jump to hunk
/* Called with iotlb_lock read-locked */ uint64_t@@ -645,6 +646,7 @@ vhost_new_device(void) struct virtio_net *dev; int i; + pthread_mutex_lock(&vhost_dev_lock); for (i = 0; i < MAX_VHOST_DEVICE; i++) { if (vhost_devices[i] == NULL) break;@@ -653,6 +655,7 @@ vhost_new_device(void) if (i == MAX_VHOST_DEVICE) { VHOST_LOG_CONFIG(ERR, "Failed to find a free slot for new device.\n"); + pthread_mutex_unlock(&vhost_dev_lock); return -1; }@@ -660,10 +663,13 @@ vhost_new_device(void) if (dev == NULL) { VHOST_LOG_CONFIG(ERR, "Failed to allocate memory for new dev.\n"); + pthread_mutex_unlock(&vhost_dev_lock); return -1; } vhost_devices[i] = dev; + pthread_mutex_unlock(&vhost_dev_lock); + dev->vid = i; dev->flags = VIRTIO_DEV_BUILTIN_VIRTIO_NET; dev->slave_req_fd = -1; --2.23.0