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

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

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
>