vhost: add runtime locking check in unsafe APIs
Checks
Commit Message
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
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
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
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
>
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
>
> -----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
> >
@@ -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;