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

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

[Gowrishankar Muthukrishnan]

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);
+
 	start_idx = vq->last_used_idx;
 	count = avail_idx - start_idx;
 	count = RTE_MIN(count, VHOST_CRYPTO_MAX_BURST_SIZE);
-- 
2.25.1

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

[Maxime Coquelin]

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.

>   	start_idx = vq->last_used_idx;
>   	count = avail_idx - start_idx;
>   	count = RTE_MIN(count, VHOST_CRYPTO_MAX_BURST_SIZE);

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

[Maxime Coquelin]

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);
> 

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

[Gowrishankar Muthukrishnan]

Hi Maxime,

> >
> > 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.
> 

Ack.
Thanks,
Gowrishankar.

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

[Gowrishankar Muthukrishnan]

> >
> > 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.
> >
> 
Removing it would break compilation for thread safety flag.
http://mails.dpdk.org/archives/test-report/2025-February/857515.html

It is due to local vc_req that is passed to func that requires iotlb lock
In vc_req->vq. Even though vc_req->vq is locked vq, GCC does not allow it, as I understand.

	vc_req = &data_req;
	vc_req->desc_idx = desc_idx;
	vc_req->dev = vcrypto->dev;
	vc_req->vq = vq;

Thanks,
Gowrishankar

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

[David Marchand]

On Thu, Feb 27, 2025 at 7:07 PM Gowrishankar Muthukrishnan
<gmuthukrishn@marvell.com> wrote:
> > > 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.
> > >
> >
> Removing it would break compilation for thread safety flag.
> http://mails.dpdk.org/archives/test-report/2025-February/857515.html
>
> It is due to local vc_req that is passed to func that requires iotlb lock
> In vc_req->vq. Even though vc_req->vq is locked vq, GCC does not allow it, as I understand.

*cough* clang.

>
>         vc_req = &data_req;
>         vc_req->desc_idx = desc_idx;
>         vc_req->dev = vcrypto->dev;
>         vc_req->vq = vq;

The annotations won't handle this wrapping in the vc_req object.
Just pass a vq object rather than the vc_req (which I don't see little
point in having in the first place..) and adjust annotations.


-- 
David Marchand

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

[Maxime Coquelin]

Hi Gowri,

On 2/28/25 9:48 AM, David Marchand wrote:
> On Thu, Feb 27, 2025 at 7:07 PM Gowrishankar Muthukrishnan
> <gmuthukrishn@marvell.com> wrote:
>>>> 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.
>>>>
>>>
>> Removing it would break compilation for thread safety flag.
>> http://mails.dpdk.org/archives/test-report/2025-February/857515.html
>>
>> It is due to local vc_req that is passed to func that requires iotlb lock
>> In vc_req->vq. Even though vc_req->vq is locked vq, GCC does not allow it, as I understand.
> 
> *cough* clang.
> 
>>
>>          vc_req = &data_req;
>>          vc_req->desc_idx = desc_idx;
>>          vc_req->dev = vcrypto->dev;
>>          vc_req->vq = vq;
> 
> The annotations won't handle this wrapping in the vc_req object.
> Just pass a vq object rather than the vc_req (which I don't see little
> point in having in the first place..) and adjust annotations.
> 
> 

Before your series arrived, we were wondering if we should not deprecate
Vhost crypto as it was not really maintained and we had no identified
user.

Since it seems you are going to use it, which is great, would you commit
to make the necessary changes to make it reliable? If yes, I would agree
to take your v8 as is for v25.03 if proper rework is done for v25.07,
would that work for you?

Maxime

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

[Gowrishankar Muthukrishnan]

Hi Maxime,

> 
> Before your series arrived, we were wondering if we should not deprecate Vhost
> crypto as it was not really maintained and we had no identified user.
> 
> Since it seems you are going to use it, which is great, would you commit to make
> the necessary changes to make it reliable? If yes, I would agree to take your v8 as
> is for v25.03 if proper rework is done for v25.07, would that work for you?
> 
I have sent v9 to include one additional patch to fix vhost_crypto_process_one_req()
thread safety attribute. Please consider it.

Yes, if any other changes are required, we can assist going forward. We also believe
this software backend needs to be reliable and hence can help test required virtio
functionalities.

Thanks,
Gowrishankar

Regards,
Gowrishankar

> Maxime

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

[Maxime Coquelin]

Hi Gowri,

On 2/28/25 2:59 PM, Gowrishankar Muthukrishnan wrote:
> Hi Maxime,
> 
>>
>> Before your series arrived, we were wondering if we should not deprecate Vhost
>> crypto as it was not really maintained and we had no identified user.
>>
>> Since it seems you are going to use it, which is great, would you commit to make
>> the necessary changes to make it reliable? If yes, I would agree to take your v8 as
>> is for v25.03 if proper rework is done for v25.07, would that work for you?
>>
> I have sent v9 to include one additional patch to fix vhost_crypto_process_one_req()
> thread safety attribute. Please consider it.

Thanks! I will check it as soon as possible.

> Yes, if any other changes are required, we can assist going forward. We also believe
> this software backend needs to be reliable and hence can help test required virtio
> functionalities.

That's great news.

Best regards,
Maxime

> Thanks,
> Gowrishankar
> 
> Regards,
> Gowrishankar
> 
>> Maxime
> 

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

[Gowrishankar Muthukrishnan]

Hi David,
> >
> > It is due to local vc_req that is passed to func that requires iotlb
> > lock In vc_req->vq. Even though vc_req->vq is locked vq, GCC does not allow it,
> as I understand.
> 
> *cough* clang.
> 
> >
> >         vc_req = &data_req;
> >         vc_req->desc_idx = desc_idx;
> >         vc_req->dev = vcrypto->dev;
> >         vc_req->vq = vq;
> 
> The annotations won't handle this wrapping in the vc_req object.
> Just pass a vq object rather than the vc_req (which I don't see little point in having
> in the first place..) and adjust annotations.
> 

Yes I thought about it initially, but then I had thought vhost_crypto_process_one_req() is only static and called with locks held.
Anyway, I have come up with one additional patch in series to fix it specifically.

Thanks for your suggestion.
Regards,
Gowrishankar
> 
> --
> David Marchand

[v6 3/5] examples/vhost_crypto: fix user callbacks

[Gowrishankar Muthukrishnan]

In order to handle new vhost user connection, use new_connection
and destroy_connection callbacks.

Fixes: f5188211c721 ("examples/vhost_crypto: add sample application")
Cc: stable@dpdk.org

Signed-off-by: Gowrishankar Muthukrishnan <gmuthukrishn@marvell.com>
Acked-by: Akhil Goyal <gakhil@marvell.com>
---
 examples/vhost_crypto/main.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/examples/vhost_crypto/main.c b/examples/vhost_crypto/main.c
index 558c09a60f..b1fe4120b9 100644
--- a/examples/vhost_crypto/main.c
+++ b/examples/vhost_crypto/main.c
@@ -362,8 +362,8 @@ destroy_device(int vid)
 }
 
 static const struct rte_vhost_device_ops virtio_crypto_device_ops = {
-	.new_device =  new_device,
-	.destroy_device = destroy_device,
+	.new_connection =  new_device,
+	.destroy_connection = destroy_device,
 };
 
 static int
-- 
2.25.1