DPDK patches and discussions
 help / color / mirror / Atom feed
* [PATCH] net/failsafe: Fix crash due to global devargs syntax parsing from secondary process
@ 2022-02-10  7:10 madhuker.mythri
  2022-02-10 15:00 ` Ferruh Yigit
  2022-02-10 17:01 ` [PATCH] devargs: Fix rte_devargs_parse uninitialized calls Gaetan Rivet
  0 siblings, 2 replies; 12+ messages in thread
From: madhuker.mythri @ 2022-02-10  7:10 UTC (permalink / raw)
  To: grive; +Cc: dev, Madhuker Mythri

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);
-- 
2.32.0.windows.1


^ permalink raw reply	[flat|nested] 12+ messages in thread

* (no subject)
  2022-02-10  7:10 [PATCH] net/failsafe: Fix crash due to global devargs syntax parsing from secondary process madhuker.mythri
@ 2022-02-10 15:00 ` Ferruh Yigit
  2022-02-10 16:08   ` Gaëtan Rivet
  2022-02-10 17:01 ` [PATCH] devargs: Fix rte_devargs_parse uninitialized calls Gaetan Rivet
  1 sibling, 1 reply; 12+ messages in thread
From: Ferruh Yigit @ 2022-02-10 15:00 UTC (permalink / raw)
  To: madhuker.mythri, grive; +Cc: dev

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.


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re:
  2022-02-10 15:00 ` Ferruh Yigit
@ 2022-02-10 16:08   ` Gaëtan Rivet
  2022-02-11  9:37     ` [External] : Re: Madhuker Mythri
  0 siblings, 1 reply; 12+ messages in thread
From: Gaëtan Rivet @ 2022-02-10 16:08 UTC (permalink / raw)
  To: Ferruh Yigit, madhuker.mythri; +Cc: dev

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH] devargs: Fix rte_devargs_parse uninitialized calls
  2022-02-10  7:10 [PATCH] net/failsafe: Fix crash due to global devargs syntax parsing from secondary process madhuker.mythri
  2022-02-10 15:00 ` Ferruh Yigit
@ 2022-02-10 17:01 ` Gaetan Rivet
  2022-02-15 12:51   ` Ferruh Yigit
  2022-02-15 15:27   ` David Marchand
  1 sibling, 2 replies; 12+ messages in thread
From: Gaetan Rivet @ 2022-02-10 17:01 UTC (permalink / raw)
  To: dev; +Cc: madhuker.mythri, ferruh.yigit

The function rte_devargs_parse() previously was safe to call with
non-initialized devargs structure as parameter.

When adding the support for the global device syntax,
this assumption was broken. Restore it by forcing memset as part of
the call itself.

Bugzilla Id: 933
Fixes: b344eb5d941a ("devargs: parse global device syntax")
Signed-off-by: Gaetan Rivet <grive@u256.net>
---
 lib/eal/common/eal_common_devargs.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/eal/common/eal_common_devargs.c b/lib/eal/common/eal_common_devargs.c
index 8c7650cf6c..184fe676aa 100644
--- a/lib/eal/common/eal_common_devargs.c
+++ b/lib/eal/common/eal_common_devargs.c
@@ -191,6 +191,7 @@ rte_devargs_parse(struct rte_devargs *da, const char *dev)
 
 	if (da == NULL)
 		return -EINVAL;
+	memset(da, 0, sizeof(*da));
 
 	/* First parse according global device syntax. */
 	if (rte_devargs_layers_parse(da, dev) == 0) {
-- 
2.31.1


^ permalink raw reply	[flat|nested] 12+ messages in thread

* RE: [External] : Re:
  2022-02-10 16:08   ` Gaëtan Rivet
@ 2022-02-11  9:37     ` Madhuker Mythri
  2022-02-11  9:58       ` [External] : Re: [PATCH] net/failsafe: Fix crash due to global devargs syntax parsing from secondary process Ferruh Yigit
  2022-02-11  9:58       ` [External] : Gaëtan Rivet
  0 siblings, 2 replies; 12+ messages in thread
From: Madhuker Mythri @ 2022-02-11  9:37 UTC (permalink / raw)
  To: Gaëtan Rivet, Ferruh Yigit; +Cc: dev

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.

Thanks,
Madhuker.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [External] : Re: [PATCH] net/failsafe: Fix crash due to global devargs syntax parsing from secondary process
  2022-02-11  9:37     ` [External] : Re: Madhuker Mythri
@ 2022-02-11  9:58       ` Ferruh Yigit
  2022-02-11  9:58       ` [External] : Gaëtan Rivet
  1 sibling, 0 replies; 12+ messages in thread
From: Ferruh Yigit @ 2022-02-11  9:58 UTC (permalink / raw)
  To: Madhuker Mythri, Gaëtan Rivet; +Cc: dev

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/

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [External] : Re:
  2022-02-11  9:37     ` [External] : Re: Madhuker Mythri
  2022-02-11  9:58       ` [External] : Re: [PATCH] net/failsafe: Fix crash due to global devargs syntax parsing from secondary process Ferruh Yigit
@ 2022-02-11  9:58       ` Gaëtan Rivet
  2022-02-16  8:27         ` Long Li
  1 sibling, 1 reply; 12+ messages in thread
From: Gaëtan Rivet @ 2022-02-11  9:58 UTC (permalink / raw)
  To: madhuker.mythri, Ferruh Yigit, Stephen Hemminger, Long Li; +Cc: dev

On Fri, Feb 11, 2022, at 10:37, 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.
>
> 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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] devargs: Fix rte_devargs_parse uninitialized calls
  2022-02-10 17:01 ` [PATCH] devargs: Fix rte_devargs_parse uninitialized calls Gaetan Rivet
@ 2022-02-15 12:51   ` Ferruh Yigit
  2022-02-15 13:45     ` Gaëtan Rivet
  2022-02-15 15:27   ` David Marchand
  1 sibling, 1 reply; 12+ messages in thread
From: Ferruh Yigit @ 2022-02-15 12:51 UTC (permalink / raw)
  To: Gaetan Rivet, dev; +Cc: madhuker.mythri

On 2/10/2022 5:01 PM, Gaetan Rivet wrote:
> The function rte_devargs_parse() previously was safe to call with
> non-initialized devargs structure as parameter.
> 
> When adding the support for the global device syntax,
> this assumption was broken. Restore it by forcing memset as part of
> the call itself.
> 
> Bugzilla Id: 933
> Fixes: b344eb5d941a ("devargs: parse global device syntax")
> Signed-off-by: Gaetan Rivet <grive@u256.net>

Reported-by: Madhuker Mythri <madhuker.mythri@oracle.com>

Since Madhuker did the initial analysis and the patch
I think fair to give credit for the work.

Thanks,
ferruh

> ---
>   lib/eal/common/eal_common_devargs.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/lib/eal/common/eal_common_devargs.c b/lib/eal/common/eal_common_devargs.c
> index 8c7650cf6c..184fe676aa 100644
> --- a/lib/eal/common/eal_common_devargs.c
> +++ b/lib/eal/common/eal_common_devargs.c
> @@ -191,6 +191,7 @@ rte_devargs_parse(struct rte_devargs *da, const char *dev)
>   
>   	if (da == NULL)
>   		return -EINVAL;
> +	memset(da, 0, sizeof(*da));
>   
>   	/* First parse according global device syntax. */
>   	if (rte_devargs_layers_parse(da, dev) == 0) {


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] devargs: Fix rte_devargs_parse uninitialized calls
  2022-02-15 12:51   ` Ferruh Yigit
@ 2022-02-15 13:45     ` Gaëtan Rivet
  0 siblings, 0 replies; 12+ messages in thread
From: Gaëtan Rivet @ 2022-02-15 13:45 UTC (permalink / raw)
  To: Ferruh Yigit, dev; +Cc: madhuker.mythri

On Tue, Feb 15, 2022, at 13:51, Ferruh Yigit wrote:
> On 2/10/2022 5:01 PM, Gaetan Rivet wrote:
>> The function rte_devargs_parse() previously was safe to call with
>> non-initialized devargs structure as parameter.
>> 
>> When adding the support for the global device syntax,
>> this assumption was broken. Restore it by forcing memset as part of
>> the call itself.
>> 
>> Bugzilla Id: 933
>> Fixes: b344eb5d941a ("devargs: parse global device syntax")
>> Signed-off-by: Gaetan Rivet <grive@u256.net>
>
> Reported-by: Madhuker Mythri <madhuker.mythri@oracle.com>
>
> Since Madhuker did the initial analysis and the patch
> I think fair to give credit for the work.
>
> Thanks,
> ferruh
>

Sure, thanks Ferruh. Whichever patch is fine by me.

-- 
Gaetan Rivet

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] devargs: Fix rte_devargs_parse uninitialized calls
  2022-02-10 17:01 ` [PATCH] devargs: Fix rte_devargs_parse uninitialized calls Gaetan Rivet
  2022-02-15 12:51   ` Ferruh Yigit
@ 2022-02-15 15:27   ` David Marchand
  2022-02-16 17:12     ` Gaëtan Rivet
  1 sibling, 1 reply; 12+ messages in thread
From: David Marchand @ 2022-02-15 15:27 UTC (permalink / raw)
  To: Gaetan Rivet; +Cc: dev, madhuker.mythri, Yigit, Ferruh

On Thu, Feb 10, 2022 at 6:01 PM Gaetan Rivet <grive@u256.net> wrote:
>
> The function rte_devargs_parse() previously was safe to call with
> non-initialized devargs structure as parameter.
>
> When adding the support for the global device syntax,
> this assumption was broken. Restore it by forcing memset as part of
> the call itself.
>
> Bugzilla Id: 933

Nit: Bugzilla ID*

> Fixes: b344eb5d941a ("devargs: parse global device syntax")

We need a backport in 21.11, right?


> Signed-off-by: Gaetan Rivet <grive@u256.net>
> ---
>  lib/eal/common/eal_common_devargs.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/lib/eal/common/eal_common_devargs.c b/lib/eal/common/eal_common_devargs.c
> index 8c7650cf6c..184fe676aa 100644
> --- a/lib/eal/common/eal_common_devargs.c
> +++ b/lib/eal/common/eal_common_devargs.c
> @@ -191,6 +191,7 @@ rte_devargs_parse(struct rte_devargs *da, const char *dev)
>
>         if (da == NULL)
>                 return -EINVAL;
> +       memset(da, 0, sizeof(*da));
>
>         /* First parse according global device syntax. */
>         if (rte_devargs_layers_parse(da, dev) == 0) {
> --
> 2.31.1
>

Otherwise lgtm.


-- 
David Marchand


^ permalink raw reply	[flat|nested] 12+ messages in thread

* RE: [External] : Re:
  2022-02-11  9:58       ` [External] : Gaëtan Rivet
@ 2022-02-16  8:27         ` Long Li
  0 siblings, 0 replies; 12+ messages in thread
From: Long Li @ 2022-02-16  8:27 UTC (permalink / raw)
  To: Gaëtan Rivet, madhuker.mythri, Ferruh Yigit, Stephen Hemminger; +Cc: dev

> 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@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.
> >
> > 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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] devargs: Fix rte_devargs_parse uninitialized calls
  2022-02-15 15:27   ` David Marchand
@ 2022-02-16 17:12     ` Gaëtan Rivet
  0 siblings, 0 replies; 12+ messages in thread
From: Gaëtan Rivet @ 2022-02-16 17:12 UTC (permalink / raw)
  To: David Marchand; +Cc: dev, madhuker.mythri, Ferruh Yigit

On Tue, Feb 15, 2022, at 16:27, David Marchand wrote:
> On Thu, Feb 10, 2022 at 6:01 PM Gaetan Rivet <grive@u256.net> wrote:
>>
>> The function rte_devargs_parse() previously was safe to call with
>> non-initialized devargs structure as parameter.
>>
>> When adding the support for the global device syntax,
>> this assumption was broken. Restore it by forcing memset as part of
>> the call itself.
>>
>> Bugzilla Id: 933
>
> Nit: Bugzilla ID*
>
>> Fixes: b344eb5d941a ("devargs: parse global device syntax")
>
> We need a backport in 21.11, right?

Hi David, yes it should be in 21.11 as well.

-- 
Gaetan Rivet

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2022-02-16 17:13 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-02-10  7:10 [PATCH] net/failsafe: Fix crash due to global devargs syntax parsing from secondary process madhuker.mythri
2022-02-10 15:00 ` Ferruh Yigit
2022-02-10 16:08   ` Gaëtan Rivet
2022-02-11  9:37     ` [External] : Re: Madhuker Mythri
2022-02-11  9:58       ` [External] : Re: [PATCH] net/failsafe: Fix crash due to global devargs syntax parsing from secondary process Ferruh Yigit
2022-02-11  9:58       ` [External] : Gaëtan Rivet
2022-02-16  8:27         ` Long Li
2022-02-10 17:01 ` [PATCH] devargs: Fix rte_devargs_parse uninitialized calls Gaetan Rivet
2022-02-15 12:51   ` Ferruh Yigit
2022-02-15 13:45     ` Gaëtan Rivet
2022-02-15 15:27   ` David Marchand
2022-02-16 17:12     ` Gaëtan Rivet

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).