Re: [External] : Re: [PATCH] net/failsafe: Fix crash due to global devargs syntax parsing from secondary process

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 2/11/2022 9:37 AM, Madhuker Mythri wrote:
> 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.
> 

Hi Madhuker,

Gaetan already sent a patch for it:
https://patches.dpdk.org/project/dpdk/patch/20220210170131.2199922-1-grive@u256.net/