[External] : Re:

[Long Li]

> 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 <grive at u256.net>
> >>Sent: 10 फरवरी 2022 21:39
> >>To: Ferruh Yigit <ferruh.yigit at intel.com>; Madhuker Mythri
> >><madhuker.mythri at oracle.com>
> >>Cc: dev at 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 at oracle.com wrote:
> >>>> From: Madhuker Mythri <madhuker.mythri at 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 at 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