Re: [PATCH] vhost: add runtime locking check in unsafe APIs

[Maxime Coquelin]

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.coque...@redhat.com>
Sent: Tuesday, May 10, 2022 4:25 PM
To: dev@dpdk.org; Xia, Chenbo <chenbo....@intel.com>; Ding, Xuan
<xuan.d...@intel.com>; Hu, Jiayu <jiayu...@intel.com>; Jiang, Cheng1
<cheng1.ji...@intel.com>; Pai G, Sunil <sunil.pa...@intel.com>;
david.march...@redhat.com
Cc: Maxime Coquelin <maxime.coque...@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.coque...@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