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

[Ferruh Yigit <ferruh.yigit@intel.com>]

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)
>