[PATCH v2] vhost: fix unchecked return value

[PATCH v2] vhost: fix unchecked return value

[Jiayu Hu]

This patch checks the return value of rte_dma_info_get()
called in rte_vhost_async_dma_configure().

Coverity issue: 379066
Fixes: 53d3f4778c1d ("vhost: integrate dmadev in asynchronous data-path")
Cc: stable@dpdk.org

Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
Reviewed-by: Chenbo Xia <chenbo.xia@intel.com>
---
v2:
- add cc stable tag
---
 lib/vhost/vhost.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c
index b14521e4d1..70c04c036e 100644
--- a/lib/vhost/vhost.c
+++ b/lib/vhost/vhost.c
@@ -1868,7 +1868,11 @@ rte_vhost_async_dma_configure(int16_t dma_id, uint16_t vchan_id)
 		return -1;
 	}
 
-	rte_dma_info_get(dma_id, &info);
+	if (rte_dma_info_get(dma_id, &info) != 0) {
+		VHOST_LOG_CONFIG(ERR, "Fail to get DMA %d information.\n", dma_id);
+		return -1;
+	}
+
 	if (vchan_id >= info.max_vchans) {
 		VHOST_LOG_CONFIG(ERR, "Invalid DMA %d vChannel %u.\n", dma_id, vchan_id);
 		return -1;
-- 
2.25.1

Re: [PATCH v2] vhost: fix unchecked return value

[lihuisong (C)]

在 2022/6/29 17:07, Jiayu Hu 写道:
> This patch checks the return value of rte_dma_info_get()
> called in rte_vhost_async_dma_configure().
>
> Coverity issue: 379066
> Fixes: 53d3f4778c1d ("vhost: integrate dmadev in asynchronous data-path")
> Cc: stable@dpdk.org
>
> Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
> Reviewed-by: Chenbo Xia <chenbo.xia@intel.com>
> ---
> v2:
> - add cc stable tag
> ---
>   lib/vhost/vhost.c | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c
> index b14521e4d1..70c04c036e 100644
> --- a/lib/vhost/vhost.c
> +++ b/lib/vhost/vhost.c
> @@ -1868,7 +1868,11 @@ rte_vhost_async_dma_configure(int16_t dma_id, uint16_t vchan_id)
>   		return -1;
>   	}
>   
> -	rte_dma_info_get(dma_id, &info);
> +	if (rte_dma_info_get(dma_id, &info) != 0) {
> +		VHOST_LOG_CONFIG(ERR, "Fail to get DMA %d information.\n", dma_id);
> +		return -1;
> +	}
> +
>   	if (vchan_id >= info.max_vchans) {
>   		VHOST_LOG_CONFIG(ERR, "Invalid DMA %d vChannel %u.\n", dma_id, vchan_id);
>   		return -1;
Reviewed-by: Huisong Li <lihuisong@huawei.com>

Re: [PATCH v2] vhost: fix unchecked return value

[Maxime Coquelin]

On 6/29/22 11:07, Jiayu Hu wrote:
> This patch checks the return value of rte_dma_info_get()
> called in rte_vhost_async_dma_configure().
> 
> Coverity issue: 379066
> Fixes: 53d3f4778c1d ("vhost: integrate dmadev in asynchronous data-path")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
> Reviewed-by: Chenbo Xia <chenbo.xia@intel.com>
> ---
> v2:
> - add cc stable tag
> ---
>   lib/vhost/vhost.c | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c
> index b14521e4d1..70c04c036e 100644
> --- a/lib/vhost/vhost.c
> +++ b/lib/vhost/vhost.c
> @@ -1868,7 +1868,11 @@ rte_vhost_async_dma_configure(int16_t dma_id, uint16_t vchan_id)
>   		return -1;
>   	}
>   
> -	rte_dma_info_get(dma_id, &info);
> +	if (rte_dma_info_get(dma_id, &info) != 0) {
> +		VHOST_LOG_CONFIG(ERR, "Fail to get DMA %d information.\n", dma_id);
> +		return -1;
> +	}
> +
>   	if (vchan_id >= info.max_vchans) {
>   		VHOST_LOG_CONFIG(ERR, "Invalid DMA %d vChannel %u.\n", dma_id, vchan_id);
>   		return -1;

The patch itself looks good, but rte_vhost_async_dma_configure() should
be protected by a lock, as concurrent calls of this function would lead
to undefined behavior.

Can you cook something?

David, is that the issue you mentioned me this week or was it another
one?

Thanks,
Maxime

RE: [PATCH v2] vhost: fix unchecked return value

[Hu, Jiayu]

Hi Maxime,

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Thursday, June 30, 2022 5:57 PM
> To: Hu, Jiayu <jiayu.hu@intel.com>; dev@dpdk.org
> Cc: Xia, Chenbo <chenbo.xia@intel.com>; stable@dpdk.org; David Marchand
> <david.marchand@redhat.com>
> Subject: Re: [PATCH v2] vhost: fix unchecked return value
> 
> 
> 
> On 6/29/22 11:07, Jiayu Hu wrote:
> > This patch checks the return value of rte_dma_info_get() called in
> > rte_vhost_async_dma_configure().
> >
> > Coverity issue: 379066
> > Fixes: 53d3f4778c1d ("vhost: integrate dmadev in asynchronous
> > data-path")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
> > Reviewed-by: Chenbo Xia <chenbo.xia@intel.com>
> > ---
> > v2:
> > - add cc stable tag
> > ---
> >   lib/vhost/vhost.c | 6 +++++-
> >   1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c index
> > b14521e4d1..70c04c036e 100644
> > --- a/lib/vhost/vhost.c
> > +++ b/lib/vhost/vhost.c
> > @@ -1868,7 +1868,11 @@ rte_vhost_async_dma_configure(int16_t
> dma_id, uint16_t vchan_id)
> >   		return -1;
> >   	}
> >
> > -	rte_dma_info_get(dma_id, &info);
> > +	if (rte_dma_info_get(dma_id, &info) != 0) {
> > +		VHOST_LOG_CONFIG(ERR, "Fail to get DMA %d
> information.\n", dma_id);
> > +		return -1;
> > +	}
> > +
> >   	if (vchan_id >= info.max_vchans) {
> >   		VHOST_LOG_CONFIG(ERR, "Invalid DMA %d vChannel %u.\n",
> dma_id, vchan_id);
> >   		return -1;
> 
> The patch itself looks good, but rte_vhost_async_dma_configure() should be
> protected by a lock, as concurrent calls of this function would lead to
> undefined behavior.

This function is expected to be called only once. Is there any use case to cause it
called concurrently?

Thanks,
Jiayu
> 
> Can you cook something?
> 
> David, is that the issue you mentioned me this week or was it another one?
> 
> Thanks,
> Maxime

Re: [PATCH v2] vhost: fix unchecked return value

[Maxime Coquelin]

On 7/1/22 09:11, Hu, Jiayu wrote:
> Hi Maxime,
> 
>> -----Original Message-----
>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>> Sent: Thursday, June 30, 2022 5:57 PM
>> To: Hu, Jiayu <jiayu.hu@intel.com>; dev@dpdk.org
>> Cc: Xia, Chenbo <chenbo.xia@intel.com>; stable@dpdk.org; David Marchand
>> <david.marchand@redhat.com>
>> Subject: Re: [PATCH v2] vhost: fix unchecked return value
>>
>>
>>
>> On 6/29/22 11:07, Jiayu Hu wrote:
>>> This patch checks the return value of rte_dma_info_get() called in
>>> rte_vhost_async_dma_configure().
>>>
>>> Coverity issue: 379066
>>> Fixes: 53d3f4778c1d ("vhost: integrate dmadev in asynchronous
>>> data-path")
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
>>> Reviewed-by: Chenbo Xia <chenbo.xia@intel.com>
>>> ---
>>> v2:
>>> - add cc stable tag
>>> ---
>>>    lib/vhost/vhost.c | 6 +++++-
>>>    1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c index
>>> b14521e4d1..70c04c036e 100644
>>> --- a/lib/vhost/vhost.c
>>> +++ b/lib/vhost/vhost.c
>>> @@ -1868,7 +1868,11 @@ rte_vhost_async_dma_configure(int16_t
>> dma_id, uint16_t vchan_id)
>>>    		return -1;
>>>    	}
>>>
>>> -	rte_dma_info_get(dma_id, &info);
>>> +	if (rte_dma_info_get(dma_id, &info) != 0) {
>>> +		VHOST_LOG_CONFIG(ERR, "Fail to get DMA %d
>> information.\n", dma_id);
>>> +		return -1;
>>> +	}
>>> +
>>>    	if (vchan_id >= info.max_vchans) {
>>>    		VHOST_LOG_CONFIG(ERR, "Invalid DMA %d vChannel %u.\n",
>> dma_id, vchan_id);
>>>    		return -1;
>>
>> The patch itself looks good, but rte_vhost_async_dma_configure() should be
>> protected by a lock, as concurrent calls of this function would lead to
>> undefined behavior.
> 
> This function is expected to be called only once. Is there any use case to cause it
> called concurrently?

Ok, so what about:

================================================================
static bool dma_configured:

int
rte_vhost_async_dma_configure(int16_t dma_id, uint16_t vchan_id)
{
	struct rte_dma_info info;
	void *pkts_cmpl_flag_addr;
	uint16_t max_desc;

	if (dma_configured)
		return -1;

	dma_configured = true;

================================================================

If this is called only once, this should be OK. ;)


Maxime

> Thanks,
> Jiayu
>>
>> Can you cook something?
>>
>> David, is that the issue you mentioned me this week or was it another one?
>>
>> Thanks,
>> Maxime
> 

Re: [PATCH v2] vhost: fix unchecked return value

[Maxime Coquelin]

On 6/29/22 11:07, Jiayu Hu wrote:
> This patch checks the return value of rte_dma_info_get()
> called in rte_vhost_async_dma_configure().
> 
> Coverity issue: 379066
> Fixes: 53d3f4778c1d ("vhost: integrate dmadev in asynchronous data-path")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
> Reviewed-by: Chenbo Xia <chenbo.xia@intel.com>
> ---
> v2:
> - add cc stable tag
> ---
>   lib/vhost/vhost.c | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
> 


Applied to dpdk-next-virtio/main.

Please propose something to protect DMA channels registration & also
consider introducing a function to unregister.

Thanks,
Maxime

RE: [PATCH v2] vhost: fix unchecked return value

[Hu, Jiayu]

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Friday, July 1, 2022 10:00 PM
> To: Hu, Jiayu <jiayu.hu@intel.com>; dev@dpdk.org
> Cc: Xia, Chenbo <chenbo.xia@intel.com>; stable@dpdk.org
> Subject: Re: [PATCH v2] vhost: fix unchecked return value
> 
> 
> 
> On 6/29/22 11:07, Jiayu Hu wrote:
> > This patch checks the return value of rte_dma_info_get() called in
> > rte_vhost_async_dma_configure().
> >
> > Coverity issue: 379066
> > Fixes: 53d3f4778c1d ("vhost: integrate dmadev in asynchronous
> > data-path")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
> > Reviewed-by: Chenbo Xia <chenbo.xia@intel.com>
> > ---
> > v2:
> > - add cc stable tag
> > ---
> >   lib/vhost/vhost.c | 6 +++++-
> >   1 file changed, 5 insertions(+), 1 deletion(-)
> >
> 
> 
> Applied to dpdk-next-virtio/main.
> 
> Please propose something to protect DMA channels registration & also
> consider introducing a function to unregister.

Thanks Maxime. Will do.

Regards,
Jiayu
> 
> Thanks,
> Maxime