> Subject: Re: [External] : Re: > > 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
When netvsc calls rte_devargs_parse, it assumes the value in rte_devargs will be overwritten. There is an issue with netvsc dealing multiple devices, that is on my todo list. In this case, I think it's safe to do memset() within rte_devargs_parse(). Long
