RE: [External] : Re:

[Madhuker Mythri <madhuker.mythri@oracle.com>]

Hi Gaetan and  Ferruh,

>
>-----Original Message-----
>From: Gaëtan Rivet <grive@u256.net> 
>Sent: 10 फरवरी 2022 21:39
>To: Ferruh Yigit <ferruh.yigit@intel.com>; Madhuker Mythri <madhuker.mythri@oracle.com>
>Cc: dev@dpdk.org
>Subject: [External] : Re: 
>
>On Thu, Feb 10, 2022, at 16:00, Ferruh Yigit wrote:
>> On 2/10/2022 7:10 AM, madhuker.mythri@oracle.com wrote:
>>> From: Madhuker Mythri <madhuker.mythri@oracle.com>
>>> 
>>> Failsafe pmd started crashing with global devargs syntax as devargs 
>>> is not memset to zero. Access it to in rte_devargs_parse resulted in 
>>> a crash when called from secondary process.
>>> 
>>> Bugzilla Id: 933
>>> 
>>> Signed-off-by: Madhuker Mythri <madhuker.mythri@oracle.com>
>>> ---
>>>   drivers/net/failsafe/failsafe.c | 1 +
>>>   1 file changed, 1 insertion(+)
>>> 
>>> diff --git a/drivers/net/failsafe/failsafe.c 
>>> b/drivers/net/failsafe/failsafe.c index 3c754a5f66..aa93cc6000 100644
>>> --- a/drivers/net/failsafe/failsafe.c
>>> +++ b/drivers/net/failsafe/failsafe.c
>>> @@ -360,6 +360,7 @@ rte_pmd_failsafe_probe(struct rte_vdev_device *vdev)
>>>   			if (sdev->devargs.name[0] == '\0')
>>>   				continue;
>>>   
>>> +			memset(&devargs, 0, sizeof(devargs));
>>>   			/* rebuild devargs to be able to get the bus name. */
>>>   			ret = rte_devargs_parse(&devargs,
>>>   						sdev->devargs.name);
>>
>> if 'rte_devargs_parse()' requires 'devargs' parameter to be memset, 
>> what do you think memset it in the API?
>> This prevents forgotten cases like this.
>
>Hi,
>
>I was looking at it this morning.
>Before the last release, rte_devargs_parse() was only supporting legacy syntax.
>It never read from the devargs structure, only wrote to it. So it was safe to use with a non-memset devargs.
>
>The rte_devargs_layer_parse() however is more complex. To allow rte_dev_iterator_init() to call it without doing memory allocation, it reads parts of the devargs to make decisions.
>
>Doing a first call to rte_devargs_layer_parse() as part of rte_devargs_parse() thus modified the contract it had with the users, that it would never read from devargs.
>
>It is not possible to completely avoid reading from devargs in rte_devargs_layer_parse().
>It is necessary for RTE_DEV_FOREACH() to be safe to interrupt without having to do iterator cleanup.
>
>This is my current understanding. In that context, yes I think it is preferrable to do memset() within rte_devargs_parse(). It will restore the previous part of the API saying that calling it with non-memset >devargs was safe to do.
>
>Thanks,
>--
>Gaetan Rivet

Thanks for your comments.
The rte_devargs_parse() is used in other 'netvsc' PMD also in netvsc_hotadd_callback().
In this netvsc_hotadd_callback(), it was assigning the devargs with some other instance pointer(not sure, whether its just a pointer or with data values) before calling this rte_devargs_parse(), so if we memset inside this API, then the devargs data values will be nullified right.
I'm not fully familiar with this parsing functionality. So, please let me know, doing memset() inside this rte_devargs_parse() is valid or not, as this is a generic function for all the PMD's.

Thanks,
Madhuker.