On 2/27/25 10:15 AM, Maxime Coquelin wrote:
Hi Gowri, Thanks for the change, but I think there is an issue with the locking, more below: On 2/26/25 7:43 PM, Gowrishankar Muthukrishnan wrote:Until virtio avail ring is initialized (by VHOST_USER_SET_VRING_ADDR), worker thread should not try to fetch crypto op, which would lead to memory fault. Fixes: 939066d96563 ("vhost/crypto: add public function implementation") Cc: sta...@dpdk.org Signed-off-by: Gowrishankar Muthukrishnan <gmuthukri...@marvell.com> Acked-by: Akhil Goyal <gak...@marvell.com> --- v6: - added lock checks. --- lib/vhost/vhost_crypto.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/lib/vhost/vhost_crypto.c b/lib/vhost/vhost_crypto.c index 3dc41a3bd5..d3d13eff07 100644 --- a/lib/vhost/vhost_crypto.c +++ b/lib/vhost/vhost_crypto.c @@ -8,6 +8,7 @@ #include <rte_mbuf.h> #include <rte_cryptodev.h> +#include "iotlb.h" #include "rte_vhost_crypto.h" #include "vhost.h" #include "vhost_user.h"@@ -1580,7 +1581,26 @@ rte_vhost_crypto_fetch_requests(int vid, uint32_t qid,vq = dev->virtqueue[qid]; + if (unlikely(vq == NULL)) { + VC_LOG_ERR("Invalid virtqueue %u", qid); + return 0; + } + + if (unlikely(rte_rwlock_read_trylock(&vq->access_lock) != 0)) + return 0; + + vhost_user_iotlb_rd_lock(vq); + if (unlikely(!vq->access_ok)) { + VC_LOG_DBG("Virtqueue %u vrings not yet initialized", qid); + vhost_user_iotlb_rd_unlock(vq); + rte_rwlock_read_unlock(&vq->access_lock); + return 0; + } + avail_idx = *((volatile uint16_t *)&vq->avail->idx); + vhost_user_iotlb_rd_unlock(vq); + rte_rwlock_read_unlock(&vq->access_lock); +You should only unlock at the end of the function, otherwise there is not much protection.
Ha, and also you should be able to remove: __rte_no_thread_safety_analysis /* FIXME: requires iotlb_lock? */
in vhost_crypto_process_one_req() once implemented. Regards, Maxime
start_idx = vq->last_used_idx; count = avail_idx - start_idx; count = RTE_MIN(count, VHOST_CRYPTO_MAX_BURST_SIZE);
