On 2/11/2022 9:37 AM, 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.
Hi Madhuker,
Gaetan already sent a patch for it:
https://patches.dpdk.org/project/dpdk/patch/20220210170131.2199922-1-gr...@u256.net/
