[dpdk-dev] [PATCH] ethdev: avoid undefined behaviour on configuration copying

[dpdk-dev] [PATCH] ethdev: avoid undefined behaviour on configuration copying

[Andrew Rybchenko]

memcpy() source and destination areas must not overlap and equal
pointers is the case which is really met, so handle it.

Fixes: 68b931bff287 ("ethdev: eliminate interim variable")
Cc: stable@dpdk.org

Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
---
slave_configure() in drivers/net/bonding calls rte_eth_dev_configure()
with &slave_eth_dev->data->dev_conf.

Alternative solution is to fix bonding and return error if dev_conf is
equal to &dev->data->dev_conf since usecase is unclear and callers
should not use dev->data.

 lib/librte_ethdev/rte_ethdev.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index 8f48e8d659..8d2ce31a81 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -1245,7 +1245,9 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
 	 * Copy the dev_conf parameter into the dev structure.
 	 * rte_eth_dev_info_get() requires dev_conf, copy it before dev_info get
 	 */
-	memcpy(&dev->data->dev_conf, dev_conf, sizeof(dev->data->dev_conf));
+	if (dev_conf != &dev->data->dev_conf)
+		memcpy(&dev->data->dev_conf, dev_conf,
+		       sizeof(dev->data->dev_conf));
 
 	ret = rte_eth_dev_info_get(port_id, &dev_info);
 	if (ret != 0)
-- 
2.17.1

Re: [dpdk-dev] [PATCH] ethdev: avoid undefined behaviour on configuration copying

[Ferruh Yigit]

On 11/19/2019 8:22 AM, Andrew Rybchenko wrote:
> memcpy() source and destination areas must not overlap and equal
> pointers is the case which is really met, so handle it.

Agree providing same config as input can cause problem with current
implementation, but it is the limitation of the memcpy, the API doesn't request
this.

We can fix as you suggested, in this case we should document this in API
documentation I think,
we can also solve this by updating the implementation to let this, using an
interim buffer in the simplest measure, not sure which one is better.
Any practical reason to prevent this other than 'memcpy' limitation?

> 
> Fixes: 68b931bff287 ("ethdev: eliminate interim variable")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
> ---
> slave_configure() in drivers/net/bonding calls rte_eth_dev_configure()
> with &slave_eth_dev->data->dev_conf.
> 
> Alternative solution is to fix bonding and return error if dev_conf is
> equal to &dev->data->dev_conf since usecase is unclear and callers
> should not use dev->data.
> 
>  lib/librte_ethdev/rte_ethdev.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> index 8f48e8d659..8d2ce31a81 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -1245,7 +1245,9 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
>  	 * Copy the dev_conf parameter into the dev structure.
>  	 * rte_eth_dev_info_get() requires dev_conf, copy it before dev_info get
>  	 */
> -	memcpy(&dev->data->dev_conf, dev_conf, sizeof(dev->data->dev_conf));
> +	if (dev_conf != &dev->data->dev_conf)
> +		memcpy(&dev->data->dev_conf, dev_conf,
> +		       sizeof(dev->data->dev_conf));
>  
>  	ret = rte_eth_dev_info_get(port_id, &dev_info);
>  	if (ret != 0)
> 

Re: [dpdk-dev] [PATCH] ethdev: avoid undefined behaviour on configuration copying

[Andrew Rybchenko]

On 11/19/19 3:24 PM, Ferruh Yigit wrote:
> On 11/19/2019 8:22 AM, Andrew Rybchenko wrote:
>> memcpy() source and destination areas must not overlap and equal
>> pointers is the case which is really met, so handle it.
> Agree providing same config as input can cause problem with current
> implementation, but it is the limitation of the memcpy, the API doesn't request
> this.
>
> We can fix as you suggested, in this case we should document this in API
> documentation I think,

Basically the patch solves it and there is nothing to document.
If pointers are equal there is nothing to do, no copying required.

> we can also solve this by updating the implementation to let this, using an
> interim buffer in the simplest measure, not sure which one is better.

I don't think that interim buffer is required, 'if' perfectly does the job.

> Any practical reason to prevent this other than 'memcpy' limitation?

Nothing except application should not play with dev->data,
but I'm not sure if it is the right place to forbid it.

Alternative solution is to fix bonding and return error if dev_conf is
equal to &dev->data->dev_conf since usecase is unclear and callers
should not use dev->data.

>> Fixes: 68b931bff287 ("ethdev: eliminate interim variable")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
>> ---
>> slave_configure() in drivers/net/bonding calls rte_eth_dev_configure()
>> with &slave_eth_dev->data->dev_conf.
>>
>> Alternative solution is to fix bonding and return error if dev_conf is
>> equal to &dev->data->dev_conf since usecase is unclear and callers
>> should not use dev->data.
>>
>>  lib/librte_ethdev/rte_ethdev.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
>> index 8f48e8d659..8d2ce31a81 100644
>> --- a/lib/librte_ethdev/rte_ethdev.c
>> +++ b/lib/librte_ethdev/rte_ethdev.c
>> @@ -1245,7 +1245,9 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
>>  	 * Copy the dev_conf parameter into the dev structure.
>>  	 * rte_eth_dev_info_get() requires dev_conf, copy it before dev_info get
>>  	 */
>> -	memcpy(&dev->data->dev_conf, dev_conf, sizeof(dev->data->dev_conf));
>> +	if (dev_conf != &dev->data->dev_conf)
>> +		memcpy(&dev->data->dev_conf, dev_conf,
>> +		       sizeof(dev->data->dev_conf));
>>  
>>  	ret = rte_eth_dev_info_get(port_id, &dev_info);
>>  	if (ret != 0)
>>

Re: [dpdk-dev] [PATCH] ethdev: avoid undefined behaviour on configuration copying

[Ferruh Yigit]

On 11/19/2019 12:36 PM, Andrew Rybchenko wrote:
> On 11/19/19 3:24 PM, Ferruh Yigit wrote:
>> On 11/19/2019 8:22 AM, Andrew Rybchenko wrote:
>>> memcpy() source and destination areas must not overlap and equal
>>> pointers is the case which is really met, so handle it.
>> Agree providing same config as input can cause problem with current
>> implementation, but it is the limitation of the memcpy, the API doesn't request
>> this.
>>
>> We can fix as you suggested, in this case we should document this in API
>> documentation I think,
> 
> Basically the patch solves it and there is nothing to document.
> If pointers are equal there is nothing to do, no copying required.

You are right, scratch my comment. I overlooked as just overlapping memory issue.

> 
>> we can also solve this by updating the implementation to let this, using an
>> interim buffer in the simplest measure, not sure which one is better.
> 
> I don't think that interim buffer is required, 'if' perfectly does the job.
> 
>> Any practical reason to prevent this other than 'memcpy' limitation?
> 
> Nothing except application should not play with dev->data,

+1.
Bonding PMD though not exactly an application, not sure to let or not it to
update 'dev->data'

> but I'm not sure if it is the right place to forbid it.
> 
> Alternative solution is to fix bonding and return error if dev_conf is
> equal to &dev->data->dev_conf since usecase is unclear and callers
> should not use dev->data.
> 
>>> Fixes: 68b931bff287 ("ethdev: eliminate interim variable")
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
>>> ---
>>> slave_configure() in drivers/net/bonding calls rte_eth_dev_configure()
>>> with &slave_eth_dev->data->dev_conf.
>>>
>>> Alternative solution is to fix bonding and return error if dev_conf is
>>> equal to &dev->data->dev_conf since usecase is unclear and callers
>>> should not use dev->data.
>>>
>>>  lib/librte_ethdev/rte_ethdev.c | 4 +++-
>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
>>> index 8f48e8d659..8d2ce31a81 100644
>>> --- a/lib/librte_ethdev/rte_ethdev.c
>>> +++ b/lib/librte_ethdev/rte_ethdev.c
>>> @@ -1245,7 +1245,9 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
>>>  	 * Copy the dev_conf parameter into the dev structure.
>>>  	 * rte_eth_dev_info_get() requires dev_conf, copy it before dev_info get
>>>  	 */
>>> -	memcpy(&dev->data->dev_conf, dev_conf, sizeof(dev->data->dev_conf));
>>> +	if (dev_conf != &dev->data->dev_conf)
>>> +		memcpy(&dev->data->dev_conf, dev_conf,
>>> +		       sizeof(dev->data->dev_conf));
>>>  
>>>  	ret = rte_eth_dev_info_get(port_id, &dev_info);
>>>  	if (ret != 0)
>>>
> 

Re: [dpdk-dev] [PATCH] ethdev: avoid undefined behaviour on configuration copying

[Andrew Rybchenko]

On 11/19/19 4:19 PM, Ferruh Yigit wrote:
> On 11/19/2019 12:36 PM, Andrew Rybchenko wrote:
>> On 11/19/19 3:24 PM, Ferruh Yigit wrote:
>>> On 11/19/2019 8:22 AM, Andrew Rybchenko wrote:
>>>> memcpy() source and destination areas must not overlap and equal
>>>> pointers is the case which is really met, so handle it.
>>> Agree providing same config as input can cause problem with current
>>> implementation, but it is the limitation of the memcpy, the API doesn't request
>>> this.
>>>
>>> We can fix as you suggested, in this case we should document this in API
>>> documentation I think,
>>
>> Basically the patch solves it and there is nothing to document.
>> If pointers are equal there is nothing to do, no copying required.
> 
> You are right, scratch my comment. I overlooked as just overlapping memory issue.
> 
>>
>>> we can also solve this by updating the implementation to let this, using an
>>> interim buffer in the simplest measure, not sure which one is better.
>>
>> I don't think that interim buffer is required, 'if' perfectly does the job.
>>
>>> Any practical reason to prevent this other than 'memcpy' limitation?
>>
>> Nothing except application should not play with dev->data,
> 
> +1.
> Bonding PMD though not exactly an application, not sure to let or not it to
> update 'dev->data'

I think it is an application for its slaves, so it is better to have
no exceptions.

>> but I'm not sure if it is the right place to forbid it.
>>
>> Alternative solution is to fix bonding and return error if dev_conf is
>> equal to &dev->data->dev_conf since usecase is unclear and callers
>> should not use dev->data.
>>
>>>> Fixes: 68b931bff287 ("ethdev: eliminate interim variable")
>>>> Cc: stable@dpdk.org
>>>>
>>>> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
>>>> ---
>>>> slave_configure() in drivers/net/bonding calls rte_eth_dev_configure()
>>>> with &slave_eth_dev->data->dev_conf.
>>>>
>>>> Alternative solution is to fix bonding and return error if dev_conf is
>>>> equal to &dev->data->dev_conf since usecase is unclear and callers
>>>> should not use dev->data.
>>>>
>>>>  lib/librte_ethdev/rte_ethdev.c | 4 +++-
>>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
>>>> index 8f48e8d659..8d2ce31a81 100644
>>>> --- a/lib/librte_ethdev/rte_ethdev.c
>>>> +++ b/lib/librte_ethdev/rte_ethdev.c
>>>> @@ -1245,7 +1245,9 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
>>>>  	 * Copy the dev_conf parameter into the dev structure.
>>>>  	 * rte_eth_dev_info_get() requires dev_conf, copy it before dev_info get
>>>>  	 */
>>>> -	memcpy(&dev->data->dev_conf, dev_conf, sizeof(dev->data->dev_conf));
>>>> +	if (dev_conf != &dev->data->dev_conf)
>>>> +		memcpy(&dev->data->dev_conf, dev_conf,
>>>> +		       sizeof(dev->data->dev_conf));
>>>>  
>>>>  	ret = rte_eth_dev_info_get(port_id, &dev_info);
>>>>  	if (ret != 0)
>>>>
>>
> 

Re: [dpdk-dev] [dpdk-stable] [PATCH] ethdev: avoid undefined behaviour on configuration copying

[Ferruh Yigit]

On 11/19/2019 1:19 PM, Ferruh Yigit wrote:
> On 11/19/2019 12:36 PM, Andrew Rybchenko wrote:
>> On 11/19/19 3:24 PM, Ferruh Yigit wrote:
>>> On 11/19/2019 8:22 AM, Andrew Rybchenko wrote:
>>>> memcpy() source and destination areas must not overlap and equal
>>>> pointers is the case which is really met, so handle it.
>>> Agree providing same config as input can cause problem with current
>>> implementation, but it is the limitation of the memcpy, the API doesn't request
>>> this.
>>>
>>> We can fix as you suggested, in this case we should document this in API
>>> documentation I think,
>>
>> Basically the patch solves it and there is nothing to document.
>> If pointers are equal there is nothing to do, no copying required.
> 
> You are right, scratch my comment. I overlooked as just overlapping memory issue.
> 
>>
>>> we can also solve this by updating the implementation to let this, using an
>>> interim buffer in the simplest measure, not sure which one is better.
>>
>> I don't think that interim buffer is required, 'if' perfectly does the job.
>>
>>> Any practical reason to prevent this other than 'memcpy' limitation?
>>
>> Nothing except application should not play with dev->data,
> 
> +1.
> Bonding PMD though not exactly an application, not sure to let or not it to
> update 'dev->data'
> 
>> but I'm not sure if it is the right place to forbid it.
>>
>> Alternative solution is to fix bonding and return error if dev_conf is
>> equal to &dev->data->dev_conf since usecase is unclear and callers
>> should not use dev->data.
>>
>>>> Fixes: 68b931bff287 ("ethdev: eliminate interim variable")
>>>> Cc: stable@dpdk.org
>>>>
>>>> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
>>>> ---
>>>> slave_configure() in drivers/net/bonding calls rte_eth_dev_configure()
>>>> with &slave_eth_dev->data->dev_conf.
>>>>
>>>> Alternative solution is to fix bonding and return error if dev_conf is
>>>> equal to &dev->data->dev_conf since usecase is unclear and callers
>>>> should not use dev->data.

Right now each application should have a copy of each port config in
application, and as we discussed before there is a chance that the application
copy and the PMD copy can diverge here or there.

What do you think having an API to get a copy/clone of the config from the PMD,
later we can introduce the check you mentioned above, use case becomes:
- Get the config from PMD
- Update it
- Configure PMD back with it

This can prevent both application keeping copy and application updating config
directly. Does it make sense?

>>>>
>>>>  lib/librte_ethdev/rte_ethdev.c | 4 +++-
>>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
>>>> index 8f48e8d659..8d2ce31a81 100644
>>>> --- a/lib/librte_ethdev/rte_ethdev.c
>>>> +++ b/lib/librte_ethdev/rte_ethdev.c
>>>> @@ -1245,7 +1245,9 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
>>>>  	 * Copy the dev_conf parameter into the dev structure.
>>>>  	 * rte_eth_dev_info_get() requires dev_conf, copy it before dev_info get
>>>>  	 */
>>>> -	memcpy(&dev->data->dev_conf, dev_conf, sizeof(dev->data->dev_conf));
>>>> +	if (dev_conf != &dev->data->dev_conf)
>>>> +		memcpy(&dev->data->dev_conf, dev_conf,
>>>> +		       sizeof(dev->data->dev_conf));
>>>>  
>>>>  	ret = rte_eth_dev_info_get(port_id, &dev_info);
>>>>  	if (ret != 0)
>>>>
>>
> 

Re: [dpdk-dev] [dpdk-stable] [PATCH] ethdev: avoid undefined behaviour on configuration copying

[Andrew Rybchenko]

On 11/19/19 4:37 PM, Ferruh Yigit wrote:
> On 11/19/2019 1:19 PM, Ferruh Yigit wrote:
>> On 11/19/2019 12:36 PM, Andrew Rybchenko wrote:
>>> On 11/19/19 3:24 PM, Ferruh Yigit wrote:
>>>> On 11/19/2019 8:22 AM, Andrew Rybchenko wrote:
>>>>> memcpy() source and destination areas must not overlap and equal
>>>>> pointers is the case which is really met, so handle it.
>>>> Agree providing same config as input can cause problem with current
>>>> implementation, but it is the limitation of the memcpy, the API doesn't request
>>>> this.
>>>>
>>>> We can fix as you suggested, in this case we should document this in API
>>>> documentation I think,
>>> Basically the patch solves it and there is nothing to document.
>>> If pointers are equal there is nothing to do, no copying required.
>> You are right, scratch my comment. I overlooked as just overlapping memory issue.
>>
>>>> we can also solve this by updating the implementation to let this, using an
>>>> interim buffer in the simplest measure, not sure which one is better.
>>> I don't think that interim buffer is required, 'if' perfectly does the job.
>>>
>>>> Any practical reason to prevent this other than 'memcpy' limitation?
>>> Nothing except application should not play with dev->data,
>> +1.
>> Bonding PMD though not exactly an application, not sure to let or not it to
>> update 'dev->data'
>>
>>> but I'm not sure if it is the right place to forbid it.
>>>
>>> Alternative solution is to fix bonding and return error if dev_conf is
>>> equal to &dev->data->dev_conf since usecase is unclear and callers
>>> should not use dev->data.
>>>
>>>>> Fixes: 68b931bff287 ("ethdev: eliminate interim variable")
>>>>> Cc: stable@dpdk.org
>>>>>
>>>>> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
>>>>> ---
>>>>> slave_configure() in drivers/net/bonding calls rte_eth_dev_configure()
>>>>> with &slave_eth_dev->data->dev_conf.
>>>>>
>>>>> Alternative solution is to fix bonding and return error if dev_conf is
>>>>> equal to &dev->data->dev_conf since usecase is unclear and callers
>>>>> should not use dev->data.
> Right now each application should have a copy of each port config in
> application, and as we discussed before there is a chance that the application
> copy and the PMD copy can diverge here or there.
>
> What do you think having an API to get a copy/clone of the config from the PMD,
> later we can introduce the check you mentioned above, use case becomes:
> - Get the config from PMD
> - Update it
> - Configure PMD back with it
>
> This can prevent both application keeping copy and application updating config
> directly. Does it make sense?

Yes, it makes sense for me. It will allow testpmd to show actual
configuration from PMD point of view.

Update and configure back approach has obvious problems with
configuration items interdependencies, but it should be the
application headache if it chooses the way since nothing forces
application to do so.

>>>>>  lib/librte_ethdev/rte_ethdev.c | 4 +++-
>>>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
>>>>> index 8f48e8d659..8d2ce31a81 100644
>>>>> --- a/lib/librte_ethdev/rte_ethdev.c
>>>>> +++ b/lib/librte_ethdev/rte_ethdev.c
>>>>> @@ -1245,7 +1245,9 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
>>>>>  	 * Copy the dev_conf parameter into the dev structure.
>>>>>  	 * rte_eth_dev_info_get() requires dev_conf, copy it before dev_info get
>>>>>  	 */
>>>>> -	memcpy(&dev->data->dev_conf, dev_conf, sizeof(dev->data->dev_conf));
>>>>> +	if (dev_conf != &dev->data->dev_conf)
>>>>> +		memcpy(&dev->data->dev_conf, dev_conf,
>>>>> +		       sizeof(dev->data->dev_conf));
>>>>>  
>>>>>  	ret = rte_eth_dev_info_get(port_id, &dev_info);
>>>>>  	if (ret != 0)
>>>>>

Re: [dpdk-dev] [PATCH] ethdev: avoid undefined behaviour on configuration copying

[Ferruh Yigit]

On 11/19/2019 8:22 AM, Andrew Rybchenko wrote:
> memcpy() source and destination areas must not overlap and equal
> pointers is the case which is really met, so handle it.
> 
> Fixes: 68b931bff287 ("ethdev: eliminate interim variable")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>

Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>

Re: [dpdk-dev] [dpdk-stable] [PATCH] ethdev: avoid undefined behaviour on configuration copying

[Ferruh Yigit]

On 11/19/2019 2:57 PM, Ferruh Yigit wrote:
> On 11/19/2019 8:22 AM, Andrew Rybchenko wrote:
>> memcpy() source and destination areas must not overlap and equal
>> pointers is the case which is really met, so handle it.
>>
>> Fixes: 68b931bff287 ("ethdev: eliminate interim variable")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
> 
> Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
> 

Applied to dpdk-next-net/master, thanks.