diff mbox series

[v2] vhost: add runtime locking check in unsafe APIs

Message ID 20220511065837.1267895-1-maxime.coquelin@redhat.com (mailing list archive)
State Accepted, archived
Delegated to: Maxime Coquelin
Headers
Series [v2] vhost: add runtime locking check in unsafe APIs |

Checks

Context Check Description
ci/checkpatch warning coding style issues
ci/Intel-compilation success Compilation OK
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-aarch64-unit-testing success Testing PASS
ci/intel-Testing success Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/github-robot: build success github build: passed
ci/iol-x86_64-unit-testing success Testing PASS
ci/iol-aarch64-compile-testing success Testing PASS
ci/iol-abi-testing success Testing PASS
ci/iol-x86_64-compile-testing success Testing PASS

Commit Message

Maxime Coquelin May 11, 2022, 6:58 a.m. UTC 
  This patch adds runtime checks in unsafe Vhost async APIs,
to ensure the access lock is taken.

The detection won't work every time, as another thread
could take the lock, but it would help to detect misuse
of these unsafe API.

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 lib/vhost/vhost.c      | 18 ++++++++++++++++++
 lib/vhost/virtio_net.c |  6 ++++++
 2 files changed, 24 insertions(+)

Comments

David Marchand May 11, 2022, 7:35 a.m. UTC | #1 
On Wed, May 11, 2022 at 8:58 AM Maxime Coquelin
<maxime.coquelin@redhat.com> wrote:
>
> This patch adds runtime checks in unsafe Vhost async APIs,
> to ensure the access lock is taken.
>
> The detection won't work every time, as another thread
> could take the lock, but it would help to detect misuse
> of these unsafe API.
>
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Reviewed-by: David Marchand <david.marchand@redhat.com>
Hu, Jiayu May 11, 2022, 7:49 a.m. UTC | #2 
> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Wednesday, May 11, 2022 3:35 PM
> To: Maxime Coquelin <maxime.coquelin@redhat.com>
> Cc: dev <dev@dpdk.org>; Xia, Chenbo <chenbo.xia@intel.com>; Ding, Xuan
> <xuan.ding@intel.com>; Hu, Jiayu <jiayu.hu@intel.com>; Jiang, Cheng1
> <cheng1.jiang@intel.com>; Pai G, Sunil <sunil.pai.g@intel.com>
> Subject: Re: [PATCH v2] vhost: add runtime locking check in unsafe APIs
> 
> On Wed, May 11, 2022 at 8:58 AM Maxime Coquelin
> <maxime.coquelin@redhat.com> wrote:
> >
> > This patch adds runtime checks in unsafe Vhost async APIs, to ensure
> > the access lock is taken.
> >
> > The detection won't work every time, as another thread could take the
> > lock, but it would help to detect misuse of these unsafe API.
> >
> > Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> 
> Reviewed-by: David Marchand <david.marchand@redhat.com>

Reviewed-by: Jiayu Hu <jiayu.hu@intel.com>

Thanks,
Jiayu
> 
> 
> --
> David Marchand
Chenbo Xia May 11, 2022, 1:58 p.m. UTC | #3 
> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Wednesday, May 11, 2022 2:59 PM
> To: dev@dpdk.org; Xia, Chenbo <chenbo.xia@intel.com>; Ding, Xuan
> <xuan.ding@intel.com>; Hu, Jiayu <jiayu.hu@intel.com>; Jiang, Cheng1
> <cheng1.jiang@intel.com>; Pai G, Sunil <sunil.pai.g@intel.com>;
> david.marchand@redhat.com
> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>
> Subject: [PATCH v2] vhost: add runtime locking check in unsafe APIs
> 
> This patch adds runtime checks in unsafe Vhost async APIs,
> to ensure the access lock is taken.
> 
> The detection won't work every time, as another thread
> could take the lock, but it would help to detect misuse
> of these unsafe API.
> 
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
>  lib/vhost/vhost.c      | 18 ++++++++++++++++++
>  lib/vhost/virtio_net.c |  6 ++++++
>  2 files changed, 24 insertions(+)
> 
> diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c
> index df0bb9d043..39cbeb415c 100644
> --- a/lib/vhost/vhost.c
> +++ b/lib/vhost/vhost.c
> @@ -1732,6 +1732,12 @@ rte_vhost_async_channel_register_thread_unsafe(int
> vid, uint16_t queue_id)
>  	if (unlikely(vq == NULL || !dev->async_copy))
>  		return -1;
> 
> +	if (unlikely(!rte_spinlock_is_locked(&vq->access_lock))) {
> +		VHOST_LOG_CONFIG(ERR, "(%s) %s() called without access lock
> taken.\n",
> +				dev->ifname, __func__);
> +		return -1;
> +	}
> +
>  	return async_channel_register(vid, queue_id);
>  }
> 
> @@ -1796,6 +1802,12 @@
> rte_vhost_async_channel_unregister_thread_unsafe(int vid, uint16_t
> queue_id)
>  	if (vq == NULL)
>  		return -1;
> 
> +	if (unlikely(!rte_spinlock_is_locked(&vq->access_lock))) {
> +		VHOST_LOG_CONFIG(ERR, "(%s) %s() called without access lock
> taken.\n",
> +				dev->ifname, __func__);
> +		return -1;
> +	}
> +
>  	if (!vq->async)
>  		return 0;
> 
> @@ -1925,6 +1937,12 @@ rte_vhost_async_get_inflight_thread_unsafe(int vid,
> uint16_t queue_id)
>  	if (vq == NULL)
>  		return ret;
> 
> +	if (unlikely(!rte_spinlock_is_locked(&vq->access_lock))) {
> +		VHOST_LOG_CONFIG(ERR, "(%s) %s() called without access lock
> taken.\n",
> +				dev->ifname, __func__);
> +		return -1;
> +	}
> +
>  	if (!vq->async)
>  		return ret;
> 
> diff --git a/lib/vhost/virtio_net.c b/lib/vhost/virtio_net.c
> index 5f432b0d77..59163bc165 100644
> --- a/lib/vhost/virtio_net.c
> +++ b/lib/vhost/virtio_net.c
> @@ -2091,6 +2091,12 @@ rte_vhost_clear_queue_thread_unsafe(int vid,
> uint16_t queue_id,
> 
>  	vq = dev->virtqueue[queue_id];
> 
> +	if (unlikely(!rte_spinlock_is_locked(&vq->access_lock))) {
> +		VHOST_LOG_DATA(ERR, "(%s) %s() called without access lock
> taken.\n",
> +				dev->ifname, __func__);
> +		return -1;
> +	}
> +
>  	if (unlikely(!vq->async)) {
>  		VHOST_LOG_DATA(ERR, "(%s) %s: async not registered for queue
> id %d.\n",
>  			dev->ifname, __func__, queue_id);
> --
> 2.35.1

Reviewed-by: Chenbo Xia <chenbo.xia@intel.com>
Maxime Coquelin May 17, 2022, 1:23 p.m. UTC | #4 
On 5/11/22 08:58, Maxime Coquelin wrote:
> This patch adds runtime checks in unsafe Vhost async APIs,
> to ensure the access lock is taken.
> 
> The detection won't work every time, as another thread
> could take the lock, but it would help to detect misuse
> of these unsafe API.
> 
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
>   lib/vhost/vhost.c      | 18 ++++++++++++++++++
>   lib/vhost/virtio_net.c |  6 ++++++
>   2 files changed, 24 insertions(+)
> 

Applied to dpdk-next-virtio/main.

Thanks,
Maxime
diff mbox series

Patch

diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c
index df0bb9d043..39cbeb415c 100644
--- a/lib/vhost/vhost.c
+++ b/lib/vhost/vhost.c
@@ -1732,6 +1732,12 @@  rte_vhost_async_channel_register_thread_unsafe(int vid, uint16_t queue_id)
 	if (unlikely(vq == NULL || !dev->async_copy))
 		return -1;
 
+	if (unlikely(!rte_spinlock_is_locked(&vq->access_lock))) {
+		VHOST_LOG_CONFIG(ERR, "(%s) %s() called without access lock taken.\n",
+				dev->ifname, __func__);
+		return -1;
+	}
+
 	return async_channel_register(vid, queue_id);
 }
 
@@ -1796,6 +1802,12 @@  rte_vhost_async_channel_unregister_thread_unsafe(int vid, uint16_t queue_id)
 	if (vq == NULL)
 		return -1;
 
+	if (unlikely(!rte_spinlock_is_locked(&vq->access_lock))) {
+		VHOST_LOG_CONFIG(ERR, "(%s) %s() called without access lock taken.\n",
+				dev->ifname, __func__);
+		return -1;
+	}
+
 	if (!vq->async)
 		return 0;
 
@@ -1925,6 +1937,12 @@  rte_vhost_async_get_inflight_thread_unsafe(int vid, uint16_t queue_id)
 	if (vq == NULL)
 		return ret;
 
+	if (unlikely(!rte_spinlock_is_locked(&vq->access_lock))) {
+		VHOST_LOG_CONFIG(ERR, "(%s) %s() called without access lock taken.\n",
+				dev->ifname, __func__);
+		return -1;
+	}
+
 	if (!vq->async)
 		return ret;
 
diff --git a/lib/vhost/virtio_net.c b/lib/vhost/virtio_net.c
index 5f432b0d77..59163bc165 100644
--- a/lib/vhost/virtio_net.c
+++ b/lib/vhost/virtio_net.c
@@ -2091,6 +2091,12 @@  rte_vhost_clear_queue_thread_unsafe(int vid, uint16_t queue_id,
 
 	vq = dev->virtqueue[queue_id];
 
+	if (unlikely(!rte_spinlock_is_locked(&vq->access_lock))) {
+		VHOST_LOG_DATA(ERR, "(%s) %s() called without access lock taken.\n",
+				dev->ifname, __func__);
+		return -1;
+	}
+
 	if (unlikely(!vq->async)) {
 		VHOST_LOG_DATA(ERR, "(%s) %s: async not registered for queue id %d.\n",
 			dev->ifname, __func__, queue_id);