* [PATCH] vhost: add runtime locking check in unsafe APIs @ 2022-05-10 8:25 Maxime Coquelin 2022-05-10 9:00 ` Ding, Xuan 2022-05-10 9:00 ` Hu, Jiayu 0 siblings, 2 replies; 6+ messages in thread From: Maxime Coquelin @ 2022-05-10 8:25 UTC (permalink / raw) To: dev, chenbo.xia, xuan.ding, jiayu.hu, cheng1.jiang, sunil.pai.g, david.marchand Cc: Maxime Coquelin 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH] vhost: add runtime locking check in unsafe APIs 2022-05-10 8:25 [PATCH] vhost: add runtime locking check in unsafe APIs Maxime Coquelin @ 2022-05-10 9:00 ` Ding, Xuan 2022-05-10 9:09 ` Maxime Coquelin 2022-05-10 9:00 ` Hu, Jiayu 1 sibling, 1 reply; 6+ messages in thread From: Ding, Xuan @ 2022-05-10 9:00 UTC (permalink / raw) To: Maxime Coquelin, dev, Xia, Chenbo, Hu, Jiayu, Jiang, Cheng1, Pai G, Sunil, david.marchand 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] vhost: add runtime locking check in unsafe APIs 2022-05-10 9:00 ` Ding, Xuan @ 2022-05-10 9:09 ` Maxime Coquelin 0 siblings, 0 replies; 6+ messages in thread From: Maxime Coquelin @ 2022-05-10 9:09 UTC (permalink / raw) To: Ding, Xuan, dev, Xia, Chenbo, Hu, Jiayu, Jiang, Cheng1, Pai G, Sunil, david.marchand 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 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH] vhost: add runtime locking check in unsafe APIs 2022-05-10 8:25 [PATCH] vhost: add runtime locking check in unsafe APIs Maxime Coquelin 2022-05-10 9:00 ` Ding, Xuan @ 2022-05-10 9:00 ` Hu, Jiayu 2022-05-10 9:14 ` Maxime Coquelin 1 sibling, 1 reply; 6+ messages in thread From: Hu, Jiayu @ 2022-05-10 9:00 UTC (permalink / raw) To: Maxime Coquelin, dev, Xia, Chenbo, Ding, Xuan, Jiang, Cheng1, Pai G, Sunil, david.marchand 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] vhost: add runtime locking check in unsafe APIs 2022-05-10 9:00 ` Hu, Jiayu @ 2022-05-10 9:14 ` Maxime Coquelin 2022-05-11 1:51 ` Hu, Jiayu 0 siblings, 1 reply; 6+ messages in thread From: Maxime Coquelin @ 2022-05-10 9:14 UTC (permalink / raw) To: Hu, Jiayu, dev, Xia, Chenbo, Ding, Xuan, Jiang, Cheng1, Pai G, Sunil, david.marchand 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 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH] vhost: add runtime locking check in unsafe APIs 2022-05-10 9:14 ` Maxime Coquelin @ 2022-05-11 1:51 ` Hu, Jiayu 0 siblings, 0 replies; 6+ messages in thread From: Hu, Jiayu @ 2022-05-11 1:51 UTC (permalink / raw) To: Maxime Coquelin, dev, Xia, Chenbo, Ding, Xuan, Jiang, Cheng1, Pai G, Sunil, david.marchand > -----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 > > ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-05-11 1:52 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-05-10 8:25 [PATCH] vhost: add runtime locking check in unsafe APIs Maxime Coquelin 2022-05-10 9:00 ` Ding, Xuan 2022-05-10 9:09 ` Maxime Coquelin 2022-05-10 9:00 ` Hu, Jiayu 2022-05-10 9:14 ` Maxime Coquelin 2022-05-11 1:51 ` Hu, Jiayu
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).