On Fri, Feb 11, 2022, at 10:37, Madhuker Mythri wrote: > Hi Gaetan and Ferruh, > >> >>-----Original Message----- >>From: Gaëtan Rivet <gr...@u256.net> >>Sent: 10 फरवरी 2022 21:39 >>To: Ferruh Yigit <ferruh.yi...@intel.com>; Madhuker Mythri >><madhuker.myt...@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.myt...@oracle.com wrote: >>>> From: Madhuker Mythri <madhuker.myt...@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.myt...@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.
Hi Madhuker, I have just checked the code, it does not seem that netvsc relies on values being kept in the devargs structure across calls, so it seems that it would be safe. It would be better to check with the maintainers however, I'm adding them to this thread. -- Gaetan Rivet
