diff mbox series

vhost: add runtime locking check in unsafe APIs

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

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-abi-testing success Testing PASS
ci/iol-aarch64-unit-testing success 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/Intel-compilation success Compilation OK
ci/intel-Testing success Testing PASS

Commit Message

Maxime Coquelin May 10, 2022, 8:25 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 ++++++++++++++++++
 1 file changed, 18 insertions(+)

Comments

Ding, Xuan May 10, 2022, 9 a.m. UTC | #1 
Hi Maxime,

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Tuesday, May 10, 2022 4:25 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] 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 ++++++++++++++++++
>  1 file changed, 18 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;

Just to confirm, is the rte_vhost_clear_queue_thread_unsafe() API missed the check?

Thanks,
Xuan

> 
> --
> 2.35.1
Hu, Jiayu May 10, 2022, 9 a.m. UTC | #2 
Hi Maxime,

This is a good idea to add the lock check below. But I have two
questions:
First, rte_vhost_clear_queue_thread_unsafe() is thread unsafe.
Why doesn't add the check for it?
Second, dev->notify_ops->destroy_device() is called without taking
the lock. If vhost applications try to clear inflight packets or even
unregister asynchronous data-path in this callback, rather than in
dev->notify_ops->vring_state_changed(), asynchronous APIs below
will return -1. How to handle this situation?

Thanks,
Jiayu

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Tuesday, May 10, 2022 4:25 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] 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 ++++++++++++++++++
>  1 file changed, 18 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;
> 
> --
> 2.35.1
Maxime Coquelin May 10, 2022, 9:09 a.m. UTC | #3 
On 5/10/22 11:00, Ding, Xuan wrote:
> Hi Maxime,
> 
>> -----Original Message-----
>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>> Sent: Tuesday, May 10, 2022 4:25 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] 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 ++++++++++++++++++
>>   1 file changed, 18 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;
> 
> Just to confirm, is the rte_vhost_clear_queue_thread_unsafe() API missed the check?

I missed it, I thought they were all in vhost.c.
I'll send a v2.

Regards,
Maxime

> Thanks,
> Xuan
> 
>>
>> --
>> 2.35.1
>
Maxime Coquelin May 10, 2022, 9:14 a.m. UTC | #4 
Hi Jiayu,

On 5/10/22 11:00, Hu, Jiayu wrote:
> Hi Maxime,
> 
> This is a good idea to add the lock check below. But I have two
> questions:
> First, rte_vhost_clear_queue_thread_unsafe() is thread unsafe.
> Why doesn't add the check for it?

Yes, I missed it. Thanks for spotting that!

> Second, dev->notify_ops->destroy_device() is called without taking
> the lock. If vhost applications try to clear inflight packets or even
> unregister asynchronous data-path in this callback, rather than in
> dev->notify_ops->vring_state_changed(), asynchronous APIs below
> will return -1. How to handle this situation?

You should call the proper safe/unsafe API depending on the context.
The idea of this patch was to catch corner cases like this one, so good
it helped to identify this issue.

Thanks,
Maxime

> Thanks,
> Jiayu
> 
>> -----Original Message-----
>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>> Sent: Tuesday, May 10, 2022 4:25 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] 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 ++++++++++++++++++
>>   1 file changed, 18 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;
>>
>> --
>> 2.35.1
>
Hu, Jiayu May 11, 2022, 1:51 a.m. UTC | #5 
> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Tuesday, May 10, 2022 5:14 PM
> To: Hu, Jiayu <jiayu.hu@intel.com>; dev@dpdk.org; Xia, Chenbo
> <chenbo.xia@intel.com>; Ding, Xuan <xuan.ding@intel.com>; Jiang, Cheng1
> <cheng1.jiang@intel.com>; Pai G, Sunil <sunil.pai.g@intel.com>;
> david.marchand@redhat.com
> Subject: Re: [PATCH] vhost: add runtime locking check in unsafe APIs
> 
> Hi Jiayu,
> 
> On 5/10/22 11:00, Hu, Jiayu wrote:
> > Hi Maxime,
> >
> > This is a good idea to add the lock check below. But I have two
> > questions:
> > First, rte_vhost_clear_queue_thread_unsafe() is thread unsafe.
> > Why doesn't add the check for it?
> 
> Yes, I missed it. Thanks for spotting that!
> 
> > Second, dev->notify_ops->destroy_device() is called without taking the
> > lock. If vhost applications try to clear inflight packets or even
> > unregister asynchronous data-path in this callback, rather than in
> > dev->notify_ops->vring_state_changed(), asynchronous APIs below
> > will return -1. How to handle this situation?
> 
> You should call the proper safe/unsafe API depending on the context.
> The idea of this patch was to catch corner cases like this one, so good it
> helped to identify this issue.

Sure. More documentation about these APIs will be helpful, and I can add
later. In addition, for v2, add Reviewed-by: Jiayu Hu <jiayu.hu@intel.com>

Thanks,
Jiayu
> 
> Thanks,
> Maxime
> 
> > Thanks,
> > Jiayu
> >
> >> -----Original Message-----
> >> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> >> Sent: Tuesday, May 10, 2022 4:25 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] 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 ++++++++++++++++++
> >>   1 file changed, 18 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;
> >>
> >> --
> >> 2.35.1
> >
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;