Re: [v6 1/5] vhost: skip crypto op fetch before vring init

[Maxime Coquelin <maxime.coquelin@redhat.com>]

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: stable@dpdk.org
>>
>> Signed-off-by: Gowrishankar Muthukrishnan <gmuthukrishn@marvell.com>
>> Acked-by: Akhil Goyal <gakhil@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);
>