patches for DPDK stable branches
 help / color / mirror / Atom feed
* [dpdk-stable] [PATCH v2] vhost: fix vid allocation race
       [not found] <MN2PR11MB406353097EA1E6C1D051D24B9CB69@MN2PR11MB4063.namprd11.prod.outlook.com>
@ 2021-02-01  8:48 ` Peng He
  2021-02-03  2:44   ` Xia, Chenbo
  2021-02-03 17:21   ` [dpdk-stable] [dpdk-dev] " Maxime Coquelin
  0 siblings, 2 replies; 3+ messages in thread
From: Peng He @ 2021-02-01  8:48 UTC (permalink / raw)
  To: dev, chenbo.xia; +Cc: stable

vhost_new_device might be called in different threads at the same time.
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.

Another place where race would happen is at the func *vhost_destroy_device*,
but after a detailed investigation, the race does not exist as long as
no two devices have the same vid: Calling vhost_destroy_devices in
different threads with different vids is actually safe.

Fixes: a277c715987 ("vhost: refactor code structure")
Reported-by: Peng He <hepeng.0320@bytedance.com>
Signed-off-by: Fei Chen <chenwei.0515@bytedance.com>
Reviewed-by: Zhihong Wang <wangzhihong.wzh@bytedance.com>
---
 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..52ab93d1ec 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;
 
 /* 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


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [dpdk-stable] [PATCH v2] vhost: fix vid allocation race
  2021-02-01  8:48 ` [dpdk-stable] [PATCH v2] vhost: fix vid allocation race Peng He
@ 2021-02-03  2:44   ` Xia, Chenbo
  2021-02-03 17:21   ` [dpdk-stable] [dpdk-dev] " Maxime Coquelin
  1 sibling, 0 replies; 3+ messages in thread
From: Xia, Chenbo @ 2021-02-03  2:44 UTC (permalink / raw)
  To: Peng He, dev; +Cc: stable, Maxime Coquelin, chenwei.0515, Zhihong Wang

Hi Peng,

> -----Original Message-----
> From: Peng He <xnhp0320@gmail.com>
> Sent: Monday, February 1, 2021 4:49 PM
> To: dev@dpdk.org; Xia, Chenbo <chenbo.xia@intel.com>
> Cc: stable@dpdk.org
> Subject: [PATCH v2] vhost: fix vid allocation race
> 
> vhost_new_device might be called in different threads at the same time.
> 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.
> 
> Another place where race would happen is at the func *vhost_destroy_device*,
> but after a detailed investigation, the race does not exist as long as
> no two devices have the same vid: Calling vhost_destroy_devices in
> different threads with different vids is actually safe.

I want to clarify another thing, vhost_destroy_device and get_device may have
a thread-safe problem. That is: vhost_user_read_cb() destroys the device while
app thread is calling vhost API (with get_device in it) to use that device.

A good thing is before vhost_user_read_cb() destroys the device, it notifies the app
thread, so vhost app should make sure it avoids this kind of problem. Otherwise we may
need to lock all places that uses the global vhost_devices, which is not good since
that will affect data path perf a lot.

Anyway, your patch fixes the specific problem you mentioned.

For this patch:

Reviewed-by: Chenbo Xia <chenbo.xia@intel.com>

> 
> Fixes: a277c715987 ("vhost: refactor code structure")
> Reported-by: Peng He <hepeng.0320@bytedance.com>
> Signed-off-by: Fei Chen <chenwei.0515@bytedance.com>
> Reviewed-by: Zhihong Wang <wangzhihong.wzh@bytedance.com>
> ---
>  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..52ab93d1ec 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;
> 
>  /* 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


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [dpdk-stable] [dpdk-dev] [PATCH v2] vhost: fix vid allocation race
  2021-02-01  8:48 ` [dpdk-stable] [PATCH v2] vhost: fix vid allocation race Peng He
  2021-02-03  2:44   ` Xia, Chenbo
@ 2021-02-03 17:21   ` Maxime Coquelin
  1 sibling, 0 replies; 3+ messages in thread
From: Maxime Coquelin @ 2021-02-03 17:21 UTC (permalink / raw)
  To: Peng He, dev, chenbo.xia; +Cc: stable



On 2/1/21 9:48 AM, Peng He wrote:
> vhost_new_device might be called in different threads at the same time.
> 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.
> 
> Another place where race would happen is at the func *vhost_destroy_device*,
> but after a detailed investigation, the race does not exist as long as
> no two devices have the same vid: Calling vhost_destroy_devices in
> different threads with different vids is actually safe.
> 
> Fixes: a277c715987 ("vhost: refactor code structure")
> Reported-by: Peng He <hepeng.0320@bytedance.com>
> Signed-off-by: Fei Chen <chenwei.0515@bytedance.com>
> Reviewed-by: Zhihong Wang <wangzhihong.wzh@bytedance.com>
> ---
>  lib/librte_vhost/vhost.c | 6 ++++++
>  1 file changed, 6 insertions(+)

Applied to dpdk-next-virtio/main.

Thanks,
Maxime


^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2021-02-07  9:39 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <MN2PR11MB406353097EA1E6C1D051D24B9CB69@MN2PR11MB4063.namprd11.prod.outlook.com>
2021-02-01  8:48 ` [dpdk-stable] [PATCH v2] vhost: fix vid allocation race Peng He
2021-02-03  2:44   ` Xia, Chenbo
2021-02-03 17:21   ` [dpdk-stable] [dpdk-dev] " Maxime Coquelin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).